Bug 1553 - NullReferenceExceptions in WebConnectionStream & WebConnection
Summary: NullReferenceExceptions in WebConnectionStream & WebConnection
Alias: None
Product: iOS
Classification: Xamarin
Component: Xamarin.iOS.dll ()
Version: 4.x
Hardware: Macintosh Mac OS
: --- normal
Target Milestone: 6.4 (async)
Assignee: Bugzilla
Depends on:
Reported: 2011-10-19 01:03 UTC by rumination.xamarin
Modified: 2013-07-15 13:16 UTC (History)
9 users (show)

Is this bug a regression?: ---
Last known good build:

Theoretical fix. Untested. (1.02 KB, patch)
2012-06-25 14:41 UTC, Matt Crocker

Notice (2018-05-24): bugzilla.xamarin.com is now in read-only mode.

Please join us on Visual Studio Developer Community and in the Xamarin and Mono organizations on GitHub to continue tracking issues. Bugzilla will remain available for reference in read-only mode. We will continue to work on open Bugzilla bugs, copy them to the new locations as needed for follow-up, and add the new items under Related Links.

Our sincere thanks to everyone who has contributed on this bug tracker over the years. Thanks also for your understanding as we make these adjustments and improvements for the future.

Please create a new report on Developer Community or GitHub with your current version information, steps to reproduce, and relevant error messages or log files if you are hitting an issue that looks similar to this resolved bug and you do not yet see a matching new report.

Related Links:

Description rumination.xamarin 2011-10-19 01:03:34 UTC
We have received a few exception reports from the field indicating some kind of issue(s) in WebConnection/WebConnectionStream.

System.NullReferenceException: Object reference not set to an instance of an object
	System.Net.WebConnectionStream..ctor (System.Net.WebConnection cnc) [0x00000] in <filename unknown>:0
	System.Net.WebConnection.ReadDone (IAsyncResult result) [0x00000] in <filename unknown>:0

System.NullReferenceException: Object reference not set to an instance of an object
	System.Net.WebConnection.NextRead () [0x00000] in <filename unknown>:0
	System.Net.WebConnectionStream.ReadAll () [0x00000] in <filename unknown>:0
	System.Net.WebConnectionStream.CheckResponseInBuffer () [0x00000] in <filename unknown>:0
	System.Net.WebConnection.ReadDone (IAsyncResult result) [0x00000] in <filename unknown>:0

These are all of the System.Net calls we make:
	- WebRequest.Create(string)
	- HttpWebRequest.GetRequestStream()
	- HttpWebRequest.GetResponse()
	- HttpWebResponse.GetResponseStream()
	- Override GetWebRequest(Uri) in a WebClient subclass

Perhaps relevant details:
	- All web requests happen via ThreadPool.QueueUserWorkItem().
	- Requests are mainly HTTP GET, and occassionally HTTP POST.
	- We are not using WCF.
	- The reports have come from builds made with MonoTouch versions 4.0.5 and 4.0.6.

I'm sorry I can't provide additional info at this time. I have no test case as we haven't encountered these problems in our own testing/devices. The stack traces I've included are all we have to go on.
Comment 1 Sebastien Pouliot 2011-10-19 08:11:29 UTC
There were a few HTTP/ThreadPool bugs in 4.0.x. 

IIRC the last one was fixed in 4.2.1. c.c. Gonzalo for confirmation (about being the last one)

Comment 2 Gonzalo Paniagua Javier 2011-10-20 21:14:49 UTC
Yes, #421 is the last fix that I remember.
Comment 3 Matt Crocker 2012-06-25 14:41:56 UTC
Created attachment 2114 [details]
Theoretical fix. Untested.

We're seeing ~100 WebConnection.NextRead crashes a day from our MonoDroid 4.2 app. 

Exception: Object reference not set to an instance of an object
at System.Net.WebConnection.NextRead () [0x00000] in <filename unknown>:0
at System.Net.WebConnectionStream.ReadAll () [0x00000] in <filename unknown>:0
at System.Net.WebConnectionStream.CheckResponseInBuffer () [0x00000] in <filename unknown>:0
at System.Net.WebConnection.ReadDone (IAsyncResult result) [0x00000] in <filename unknown>:0

This is during HTTPS POST requests run via ThreadPool.QueueUserWorkItem, with a manual timeout system that calls HttpWebRequest.Abort. I have a test app that does nothing but connect/transfer/abort, but so far haven't had it crash. There doesn't appear to be any hardware or OS version pattern in the user crash logs. 

I spent some time reading through WebConnection trying to find a workaround or a way to catch the exception, and in the process came up with a theory: 

Thread 1 is the connection thread
Thread 2 is the timeout monitor thread
Thread 1 just entered WebConnection.NextRead() and is about to lock(this) https://github.com/mono/mono/blob/mono-2-10/mcs/class/System/System.Net/WebConnection.cs#L717
Thread 2 has finished waiting, and calls HttpWebRequest.Abort() which eventually calls WebConnection.Abort()
Thread 2 enters WebConnection.Abort() and succeeds in locking(this) https://github.com/mono/mono/blob/mono-2-10/mcs/class/System/System.Net/WebConnection.cs#L1086
Thread 2 calls WebConnection.Close(false), which sets "Data = new WebConnectionData();"
Thread 1 finishes locking(this) and continues in WebConnection.NextRead() 
Thread 1 attempts to do "Data.request.FinishedReading = true;" which throws a null reference exception because Data is now an empty WebConnectionData object.
Then the app crashes, because there is nothing to catch the exception before it gets back to the ThreadPool.


Attached is a patch that theoretically fixes the problem. I have no way of actually testing it though.
Comment 4 Miguel de Icaza [MSFT] 2012-06-27 17:45:45 UTC
Update on this bug:  we fixed this bug recently, as part of the fix for #5710.

The fix is in mono master as patch 5c5be866e2e5a860368771287e6ef3dca7f349a7

The correct fix is to not let the call get as far as it got there.

We are backporting now.
Comment 5 Matt Crocker 2012-06-28 14:09:10 UTC
Thanks Miguel. If this alleviates the problem, great, but I think the race condition remains. I know next to nothing about this class, but bear with me of a moment:

Even in the latest version of Mono there doesn't appear to be anything preventing Data.request from going null just as NextRead is about to run.  Thread A is about to enter the NextRead lock, Thread B calls Abort and nulls Data.request, Thread A is free to continue and immediately crashes since there is no state check after it acquires the lock.
Comment 6 Gonzalo Paniagua Javier 2012-06-28 14:19:44 UTC
Matt, you did not look in the right place. There is a recent patch (June 18th) that avoids that call to NextRead when Data.request is null.

Once a package with the fix is ready, you should try it and let us know how it goes.
Comment 7 Matt Crocker 2012-06-28 14:51:18 UTC
This patch? https://github.com/mono/mono/commit/271524d471b3291567f397dfc82cd851f2b9850e#mcs/class/System/System.Net/WebConnection.cs

Sorry, my point is that Data.request is not made null until NextRead has ALREADY been called, at which point there is nothing to stop it from crashing. 

Thread A is here https://github.com/mono/mono/blob/master/mcs/class/System/System.Net/WebConnection.cs#L728 but has not yet acquired the lock

Thread B is here https://github.com/mono/mono/blob/master/mcs/class/System/System.Net/WebConnection.cs#L1089 with the lock, making Data.request = null

Thread A will then get the lock and crash the app

Am I missing something? (very possible)
Comment 8 Gonzalo Paniagua Javier 2012-06-28 15:50:25 UTC
Mmm. That case that you describe might be possible. The patch that you mentioned fixed a bug with the same stack trace that happened when calls to HttpWebRequest.Abort() were used frequently.

Anyway, I think that just checkint for(Data.request != null) should work and have no collateral effects. I am going to make that change and request a new release of MT.

Mig, master/69dad66 has the fix (hopefully)
Comment 10 Tammo Hinrichs 2013-05-06 08:51:58 UTC
The issue is still there.

Mono 3.0.8 running on Linux (Debian Squeeze, compiled manually w/ llvm and sgen), running a server app that is communicating with other web services quite often, still yields the first of the two exceptions described:

System.NullReferenceException: Object reference not set to an instance of an
    System.Net.WebConnectionStream..ctor (System.Net.WebConnection cnc)
[0x00000] in <filename unknown>:0
    System.Net.WebConnection.ReadDone (IAsyncResult result) [0x00000] in
<filename unknown>:0

I checked the code and it's quite obvious it can't work :) If you look at WebConnection.ReadDone the Data member is copied to a local variable and used thereafter to avoid the race conditions described above. But then somewhere along the function a new WebConnectionStream is constructed - and its ctor doesn't use the local copy of Data but the connection's Data member again, and the race condition still applies.

Solution: Pass the local WebConnectionData variable to the WebConnectionStream ctor and use that one instead. We're currently testing if this works (happens every few days or so, so we just wait) and if yes, I shall submit a pull request for the fix.
Comment 11 Tammo Hinrichs 2013-05-08 10:39:34 UTC
At least our issue seems fixed with the change described above - see https://github.com/mono/mono/pull/631
Comment 13 PJ 2013-07-15 13:16:11 UTC
Fix was reported in comment 8, marking as RESOLVED FIXED.