Chasing down a browser detection bug

A colleague of mine recently hit upon an odd issue with the Sitecore integration for the 51Degrees browser detection service. It worked fine for most of his testing, but raised an exception in some circumstances. Trying to dig into this and create a test to demonstrate the bug kept us amused for a few hours – maybe it will help you to?

The bug we saw...

A set of changes were deployed to our QA servers for a particular client, which added code making use of a particular Sitecore wrapper for the 51Degrees browser detection service. At a certain point our new code was making a decision based on ASP.Net's property for the requesting device's screen pixel-width.

QA reported that for mobile browsers this was all fine, and the new code worked as expected, but for desktop browsers the following exception was occurring when the browser property was accessed for the first time:

System.FormatException: Input string was not in a correct format.
   at System.Number.StringToNumber(String str, NumberStyles options, NumberBuffer& number, NumberFormatInfo info, Boolean parseDecimal)
   at System.Number.ParseInt32(String s, NumberStyles style, NumberFormatInfo info)
   at System.Web.Configuration.HttpCapabilitiesBase.get_ScreenPixelsWidth()

					

Why was it happening?

Looking at the ScreenPixelWidth property with ILSpy, the crash was happening here:

public virtual int ScreenPixelsWidth
{
	get
	{
		if (!this._haveScreenPixelsWidth)
		{
			if (this["screenPixelsWidth"] == null)
			{
				int num = 80;
				int num2 = 8;
				if (this["screenCharactersWidth"] != null && this["characterWidth"] != null)
				{
					num = Convert.ToInt32(this["screenCharactersWidth"], CultureInfo.InvariantCulture);
					num2 = Convert.ToInt32(this["characterWidth"], CultureInfo.InvariantCulture);
				}
				else if (this["screenCharactersWidth"] != null)
				{
					num = Convert.ToInt32(this["screenCharactersWidth"], CultureInfo.InvariantCulture);
					num2 = Convert.ToInt32(this["defaultCharacterWidth"], CultureInfo.InvariantCulture);
				}
				else if (this["characterWidth"] != null)
				{
					num = Convert.ToInt32(this["defaultScreenCharactersWidth"], CultureInfo.InvariantCulture);
					num2 = Convert.ToInt32(this["characterWidth"], CultureInfo.InvariantCulture);
				}
				else if (this["defaultScreenPixelsWidth"] != null)
				{
					num = Convert.ToInt32(this["defaultScreenPixelsWidth"], CultureInfo.InvariantCulture);
					num2 = 1;
				}
				this._screenPixelsWidth = num * num2;
			}
			else
			{
				this._screenPixelsWidth = Convert.ToInt32(this["screenPixelsWidth"], CultureInfo.InvariantCulture);
			}
			this._haveScreenPixelsWidth = true;
		}
		return this._screenPixelsWidth;
	}
}

					

Basically, if this property has not already cached a value, and the raw browser capability property "screenPixelsWidth" has a value, then the code tries to parse that value as an integer. So if we're getting an exception here, that value can't be something that can be cast...

So what is the value appearing there?

Delving into the code for the Sitecore integration module, it asks the 51 Degrees service for capabilities data for the current browser, and then calls this code to move that data into the raw data used by the .Net capabilities object:

public void SetBrowserCapabilities()
{
    if (_httpContextWrapper.Items.Contains("FiftyOneDegreesService.SetBrowserCapabilities"))
    {
        return;
    }

    var detectedDevice = GetDetectedDevice();

    if (detectedDevice != null)
    {
        var browserCapabilities = _httpContextWrapper.Request.Browser;

        foreach (var deviceProperty in detectedDevice.DeviceProperties)
        {
            var value = detectedDevice[deviceProperty];

            browserCapabilities.Capabilities[deviceProperty] = value;
        }

        browserCapabilities.Capabilities["isMobileDevice"] = IsMobileDevice(detectedDevice);
        browserCapabilities.Capabilities["isTabletDevice"] = IsTabletDevice(detectedDevice);
    }

    _httpContextWrapper.Items.Add("FiftyOneDegreesService.SetBrowserCapabilities", true);
}

					

So, it seems logical that the issue would be with what the 51 Degrees service returns. What is the data that comes back for a desktop browser?

The value that comes back from the service is the string "Unknown" – clearly something we can't cast to an Integer...

So the fix is fairly obvious – we need to make sure the value "Unknown" gets replaced with some sort of sensible alternative. But to do that properly, we need a test to prove the bug and then our fix.

Building a test to reproduce the issue in dev

The integration with Sitecore registers a pipeline component which calls FiftyOneDegreesService class's SetBrowserCapabilities() method to look at the current request and update the browser detection data with 51Degrees' data. So that's the class we need to write a test for. The constructor for that class requires a set of dependencies passed in – so we need to provide those objects.

The first object we need is an ISitecoreSettingsWrapper, which is used to read some settings from Sitecore's config files, via the GetSetting() method. So initially we can mock that to return nothing. If we need to add any specific behaviour to it we can do so later:

var setting = new Mock<ISitecoreSettingsWrapper>();
setting.Setup(i => i.GetSetting(It.IsAny<string>())).Returns(string.Empty);

					

Next up is an instance of 51Degrees' IHttpContextWrapper. That has to wrap around the current HttpContext‘s request, to provide access to things like the browser detection data, the Browser's agent string. Experience tells me that trying to mock this will be challenging, but that Microsoft's code is usually flexible enough to create a request using their objects. So the first thing to do is to create a bogus request and assign it to the current context:

var request = new HttpRequest("", "http://test.com", "");
HttpContext.Current = new HttpContext(request, new HttpResponse(new StringWriter()));

					

A check with the debugger shows that when you create a request like this, the Browser Capabilities data is all null – so we need to initialise that:

var capsdictionary = new Dictionary<string, object>(StringComparer.OrdinalIgnoreCase);
HttpContext.Current.Request.Browser = new HttpBrowserCapabilities() { Capabilities = capsdictionary };

					

Note that the dictionary created here has a case-insensitive string comparison – that's important. In creating all of this I spent some time wondering why the test didn't work as expected, all because initially I created a case-sensitive dictionary...

Then we can wrap up our context ready to give it to the 51Degrees class:

var httpcontextWrapper = new Sitecore.FiftyOneDegrees.CloudDeviceDetection.System.Wrappers.HttpContextWrapper(HttpContext.Current);

					

Next up, the 51Degrees class needs an IHttpRuntimeCacheWrapper. Caching isn't important to the test, so lets just provide a mock for that:

var httpruntimeCache = new Mock<IHttpRuntimeCacheWrapper>();

					

So finally we need an IWebRequestWrapper object. Digging through the code, this is used to issue the web service request to the 51Degrees service. We don't really want to issue that request and deal with processing and deserialising the response. There's only one method, so let's just make a fake:

public class FakeWebRequestWrapper : IWebRequestWrapper
{
    public T GetJson<T>(string requestUrl)
    {
        var serialiser = new JsonSerializer();
        var device = serialiser.Deserialize<T>("{\"MatchMethod\":\"Exact\",\"Difference\":0,\"DetectionTime\":0.0,\"Values\":{\"BrowserName\":[\"Chrome\"],\"BrowserVersion\":[\"57\"],\"DeviceType\":[\"Desktop\"],\"IsConsole\":[\"False\"],\"IsEReader\":[\"False\"],\"IsMediaHub\":[\"False\"],\"IsMobile\":[\"False\"],\"IsSmallScreen\":[\"False\"],\"IsSmartPhone\":[\"False\"],\"IsTablet\":[\"False\"],\"IsTv\":[\"False\"],\"PlatformName\":[\"Windows\"],\"PlatformVersion\":[\"8.1\"],\"ScreenPixelsHeight\":[\"Unknown\"],\"ScreenPixelsWidth\":[\"Unknown\"]},\"DataSetName\":\"PremiumV3\",\"Published\":\"2017-04-19T00:00:00Z\",\"SignaturesCompared\":0,\"ProfileIds\":{\"1\":15364,\"2\":21460,\"3\":69850,\"4\":18092},\"Useragent\":\"Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko  Chrome/57            Safari/537\",\"TargetUseragent\":\"Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/57.0.2987.133 Safari/537.36\"}");
        return device;
    }
}

					

The Json data there was captured from a real request (which threw the exception) using Fiddler to intercept the traffic. The class just ignores the input and returns the deserialised object for the captured data. And it's trivial to create an instance:

var httpWebRequest = new FakeWebRequestWrapper();

					

So finally, we can create the object to test and we can test what happens when we access the ScreenPixelsWidth:

var sut = new FiftyOneDegreesService(setting.Object, httpcontextWrapper, httpruntimeCache.Object, httpWebRequest);
sut.SetBrowserCapabilities();

Assert.AreEqual(0, request.Browser.ScreenPixelsWidth);

					

The complete code for the test is available in a Gist. And when we run it:

Bingo - a failed test, with the same exception message we saw on our QA site.

A bit of a detour...

While I was working through the construction of the test with my colleague, I went down a bit of a rabbit hole about how we could fake the UserAgent for our test request. It turns out that because of the FakeWebRequestWrapper fake, this isn't actually necessary. But to help my future self (who will inevitably need to do this somewhere else) I figured I should include it...

If you try to write to the UserAgent property of the request, you'll realise it doesn't have a setter. And if you try to add a header for the user agent, you'll get a PlatformNotSupportedException for your trouble.

So what can you do?

Diving through the code for how a request is initialised, it starts off as an HttpWorkerRequest object, which implements a GetKnownRequestHeader(). That gets called with the value "39" when the runtime tries to retrieve the user agent string.

So we can just mock up one of those? Sadly, not that simple. The constructor for HttpRequest which initialises it with a HttpWorkerRequest isn't public. But we can cheat a bit for a test scenario and use reflection to poke that object into the HttpRequest after it's been initialised the normal way. A bit of decompilation tells us that the HttpWorkerRequest object is stored in a field called "_wr". So:

var workerrequest = new Mock<HttpWorkerRequest>();
workerrequest.Setup(w => w.GetKnownRequestHeader(39)).Returns("Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/57.0.2987.133 Safari/537.36");

var type = request.GetType();
var field = type.GetField("_wr", System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance);
field.SetValue(request, workerrequest.Object);

					

Not pretty – but it does work.

But getting back to the important stuff...

The fix

Fixing the issue is pretty simple. We just need to ensure that if the code is copying a 51Degrees property into the .Net browser capabilities, it replaces any "Unknown" values with a value that will parse as an integer:

public void SetBrowserCapabilities()
{
    if (_httpContextWrapper.Items.Contains("FiftyOneDegreesService.SetBrowserCapabilities"))
    {
        return;
    }

    var detectedDevice = GetDetectedDevice();

    if (detectedDevice != null)
    {
        var browserCapabilities = _httpContextWrapper.Request.Browser;

        foreach (var deviceProperty in detectedDevice.DeviceProperties)
        {
            var value = detectedDevice[deviceProperty];
            if(value.ToLower() == "unknown")
            {
                value = "0";
            }
            browserCapabilities.Capabilities[deviceProperty] = value;
        }

        browserCapabilities.Capabilities["isMobileDevice"] = IsMobileDevice(detectedDevice);
        browserCapabilities.Capabilities["isTabletDevice"] = IsTabletDevice(detectedDevice);
    }

    _httpContextWrapper.Items.Add("FiftyOneDegreesService.SetBrowserCapabilities", true);
}

					

And if we re-run the test:

A win!

I'm not sure whether zero is the right default value to use here – but it's a good starting point.

My colleague will be putting in a PR to fix this, so hopefully the underlying issue won't be in the code for long, but it was interesting getting to this fix.

↑ Back to top