This is Xamarin's bug tracking system. For product support, please use the support links listed in your Xamarin Account.
Bug 1553 - NullReferenceExceptions in WebConnectionStream & WebConnection
: NullReferenceExceptions in WebConnectionStream & WebConnection
Status: RESOLVED FIXED
Product: iOS
Classification: Xamarin
Component: Class Libraries
: 4.x
: Macintosh Mac OS
: --- normal
: 6.4 (async)
Assigned To: Bugzilla
:
:
:
:
  Show dependency treegraph
 
Reported: 2011-10-19 01:03 EDT by rumination.xamarin
Modified: 2013-07-15 13:16 EDT (History)
9 users (show)

See Also:
Tags:
Test Case URL:
External Submit: ---


Attachments
Theoretical fix. Untested. (1.02 KB, patch)
2012-06-25 14:41 EDT, Matt Crocker
Details | Diff

Description rumination.xamarin 2011-10-19 01:03:34 EDT
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 EDT
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)
http://bugzilla.xamarin.com/show_bug.cgi?id=241#c11

others
http://bugzilla.xamarin.com/show_bug.cgi?id=12
http://bugzilla.xamarin.com/show_bug.cgi?id=60
Comment 2 Gonzalo Paniagua Javier 2011-10-20 21:14:49 EDT
Yes, #421 is the last fix that I remember.
Comment 3 Matt Crocker 2012-06-25 14:41:56 EDT
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();"
https://github.com/mono/mono/blob/mono-2-10/mcs/class/System/System.Net/WebConnection.cs#L1078
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.
https://github.com/mono/mono/blob/mono-2-10/mcs/class/System/System.Net/WebConnection.cs#L718
Then the app crashes, because there is nothing to catch the exception before it
gets back to the ThreadPool.

Plausible?

Attached is a patch that theoretically fixes the problem. I have no way of
actually testing it though.
Comment 4 Miguel de Icaza 2012-06-27 17:45:45 EDT
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 EDT
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 EDT
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 EDT
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 EDT
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 EDT
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
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

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 EDT
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 EDT
Fix was reported in comment 8, marking as RESOLVED FIXED.

Note You need to log in before you can comment on or make changes to this bug.