xamarin / Xamarin.Social

Xamarin.Social
Apache License 2.0
124 stars 74 forks source link

Crash in FoundationResponse.GetResponseText() #23

Closed gaearon closed 10 years ago

gaearon commented 10 years ago

I can't isolate the issue yet, but I have FoundationResponse.GetResponseText() crashing my process in some cases:

mono-rt:   at <unknown> <0xffffffff>
mono-rt:   at (wrapper managed-to-native) System.Runtime.InteropServices.Marshal.copy_from_unmanaged (intptr,int,System.Array,int) <IL 0x00024, 0xffffffff>
mono-rt:   at System.Runtime.InteropServices.Marshal.Copy (intptr,byte[],int,int) [0x00000] in /Developer/MonoTouch/Source/mono/mcs/class/corlib/System.Runtime.InteropServices/Marshal.cs:146
mono-rt:   at System.IO.UnmanagedMemoryStream.Read (byte[],int,int) [0x0010b] in /Developer/MonoTouch/Source/mono/mcs/class/corlib/System.IO/UnmanagedMemoryStream.cs:206
mono-rt:   at System.IO.StreamReader.ReadBuffer () [0x0000e] in /Developer/MonoTouch/Source/mono/mcs/class/corlib/System.IO/StreamReader.cs:392
mono-rt:   at System.IO.StreamReader.ReadToEnd () [0x0002c] in /Developer/MonoTouch/Source/mono/mcs/class/corlib/System.IO/StreamReader.cs:580
mono-rt:   at Xamarin.Auth.Response.GetResponseText () [0x00046] in /Users/dan/Documents/Projects/stampsy-ipad/external/Xamarin.Social/Xamarin.Auth/src/Xamarin.Auth/Response.cs:93
mono-rt:   at Stampsy.Social.ServiceManager.GetResponseJson (System.Threading.Tasks.Task`1<Xamarin.Auth.Response>) [0x00074] in /Users/dan/Documents/Projects/stampsy-ipad/external/sociopath/libs/Stampsy.Social/Services/ServiceManager.cs:101
mono-rt:   at Stampsy.Social.ServiceManager/<ParsePageAsync>c__AnonStorey15`1.<>m__2C (System.Threading.Tasks.Task`1<Xamarin.Auth.Response>) [0x00008] in /Users/dan/Documents/Projects/stampsy-ipad/external/sociopath/libs/Stampsy.Social/Services/ServiceManager.cs:129
gaearon commented 10 years ago

This seems to happen when large NSData is passed to FoundationResponse constructor.
In my case, its length was 498430.

I'll see why the data is so large, but still the crash is weird, is the memory being freed too soon?

ermau commented 10 years ago

I'll see why the data is so large, but still the crash is weird, is the memory being freed too soon?

Looks like a possibility, I don't see anything holding on to a reference of the NSData.

gaearon commented 10 years ago

Would it be better to hold on to NSData or to pin it and store GCHandle? I suppose without pinning, its address could change, no? Or does it not matter because its Bytes point to native memory GC can't move anyway? Thanks.

ermau commented 10 years ago

Probably simpler to just hold on to the NSData, as you said the Bytes is going to be unmanaged memory. Really the construction of the new stream could be moved to GetResponseStream if we hold on to the NSData as well.

gaearon commented 10 years ago

I see. Should I dispose of both stream and NSData in Dispose, or just NSData? Do I need to put Dispose calls inside if (disposing)?

ermau commented 10 years ago

Do I need to put Dispose calls inside if (disposing)?

Yes, NSObject has its own finalizer and the execution order of finalizers is not guaranteed.

ermau commented 10 years ago

The real question is why @praeclarum special cases NSMutableData? AsStream() automatically keeps a reference to the NSData.

gaearon commented 10 years ago

@ermau

But FoundationResponse's base class is Response, not NSObject. It just follows the same convention. It also frees its own field (HttpWebResponse response) without doing this check.

ermau commented 10 years ago

But FoundationResponse's base class is Response, not NSObject.

NSData's base class is NSObject.

It also frees its own field (HttpWebResponse response) without doing this check.

True, that doesn't make it correct unfortunately. I'll have to fix that.