Some constructors cause crashes with objects created in different threads

[cwensley]

When calling an init method that returns a different handle than the one created with alloc, a race condition occurs where for a small period of time a dealloc'd handle is incorrectly mapped to that object until it is reassigned with the one returned from the init method. I am not sure of all the API's that cause this issue, but we are running into this one a LOT: NSBitmapImageRep(NSData). This is causing many random crashes in our application.

Calling the constructor in Xamarin.Mac appears to do this:

1. Calls [NSBitmapImageRep alloc], which returns a handle
2. Registers that handle with Runtime.object_map
3. Calls [NSBitmapImageRep initWithData:], which returns a different handle
4. Sets the object's Handle property to the new handle, unregistering the alloc handle and registering the new handle.

Between 3 and 4 there is a possibility that a different thread may try to allocate an object and incorrectly return the NSBitmapImageRep instead of the returned object. In this case I'm just calling Copy() on a NSMutableParagraphStyle which was part of our specific example, but any API that returns an object could also cause the same issue.

To give some background, we are calling this API to load images from a web service in a background thread while the UI stays responsive.

Steps to Reproduce

1. Load the attached project in VS for Mac and start it without debugging*
2. Click the "Click Me" button
3. Wait a (usually short while) and it will show a message box.

*it takes longer to reproduce in debug mode.

Expected Behavior

No messages should ever occur, and NSMutableParagraphStyle.Copy() should always return an NSParagraphStyle

Actual Behavior

NSMutableParagraphStyle.Copy() returns an NSBitmapImageRep sometimes.

Environment

=== Visual Studio Community 2019 for Mac ===

Version 8.7.3 (build 13)
Installation UUID: 79acd3c6-4b3a-4fa7-bd23-165037ef1cf1
    GTK+ 2.24.23 (Raleigh theme)
    Xamarin.Mac 6.18.0.23 (d16-6 / 088c73638)

    Package version: 612000090

=== Mono Framework MDK ===

Runtime:
    Mono 6.12.0.90 (2020-02/d3daacdaa80) (64-bit)
    Package version: 612000090

=== Roslyn (Language Service) ===

3.7.0-6.20375.2+34202cc2f3e869fd70a26d8237f4552cf9e192cf

=== NuGet ===

Version: 5.7.0.6702

=== .NET Core SDK ===

SDK: /usr/local/share/dotnet/sdk/5.0.100-preview.6.20318.15/Sdks
SDK Versions:
    5.0.100-preview.6.20318.15
    3.1.302
    3.1.301
    3.1.300
    3.1.200
    3.1.102
    3.1.101
    3.1.100
    3.0.101
    2.1.500
MSBuild SDKs: /Library/Frameworks/Mono.framework/Versions/6.12.0/lib/mono/msbuild/Current/bin/Sdks

=== .NET Core Runtime ===

Runtime: /usr/local/share/dotnet/dotnet
Runtime Versions:
    5.0.0-preview.6.20305.6
    3.1.6
    3.1.5
    3.1.4
    3.1.2
    3.1.1
    3.1.0
    3.0.1
    2.1.21
    2.1.20
    2.1.19
    2.1.18
    2.1.17
    2.1.16
    2.1.15
    2.1.14
    2.1.6

=== Xamarin.Profiler ===

Version: 1.6.12.29
Location: /Applications/Xamarin Profiler.app/Contents/MacOS/Xamarin Profiler

=== Updater ===

Version: 11

=== Xamarin.Android ===

Version: 11.0.2.0 (Visual Studio Community)
Commit: xamarin-android/d16-7/025fde9
Android SDK: /Users/curtis/Library/Developer/Xamarin/android-sdk-macosx
    Supported Android versions:
        None installed

SDK Tools Version: 26.1.1
SDK Platform Tools Version: 28.0.2
SDK Build Tools Version: 28.0.3

Build Information: 
Mono: 83105ba
Java.Interop: xamarin/java.interop/d16-7@1f3388a
ProGuard: Guardsquare/proguard/proguard6.2.2@ebe9000
SQLite: xamarin/sqlite/3.32.1@1a3276b
Xamarin.Android Tools: xamarin/xamarin-android-tools/d16-7@017078f

=== Microsoft OpenJDK for Mobile ===

Java SDK: /Users/curtis/Library/Developer/Xamarin/jdk/microsoft_dist_openjdk_1.8.0.25
1.8.0-25
Android Designer EPL code available here:
https://github.com/xamarin/AndroidDesigner.EPL

=== Android SDK Manager ===

Version: 16.7.0.13
Hash: 8380518
Branch: remotes/origin/d16-7~2
Build date: 2020-08-13 18:19:24 UTC

=== Android Device Manager ===

Version: 16.7.0.22
Hash: 576004d
Branch: remotes/origin/d16-7
Build date: 2020-08-13 18:19:46 UTC

=== Apple Developer Tools ===

Xcode 11.6 (16141)
Build 11E708

=== Xamarin.Mac ===

Version: 6.20.2.2 (Visual Studio Community)
Hash: 817b6f72a
Branch: d16-7
Build date: 2020-07-18 18:44:59-0400

=== Xamarin.iOS ===

Version: 13.20.2.2 (Visual Studio Community)
Hash: 817b6f72a
Branch: d16-7
Build date: 2020-07-18 18:45:00-0400

=== Xamarin Designer ===

Version: 16.7.0.492
Hash: f5afe667d
Branch: remotes/origin/d16-7-vsmac
Build date: 2020-07-10 18:42:54 UTC

=== Build Information ===

Release ID: 807030013
Git revision: 793cad2e5999f9797bc6bc71759f31e97f57d94a
Build date: 2020-08-13 12:22:13-04
Build branch: release-8.7
Xamarin extensions: 793cad2e5999f9797bc6bc71759f31e97f57d94a

=== Operating System ===

Mac OS X 10.15.6
Darwin 19.6.0 Darwin Kernel Version 19.6.0
    Sun Jul  5 00:43:10 PDT 2020
    root:xnu-6153.141.1~9/RELEASE_X86_64 x86_64

=== Enabled user installed extensions ===

Eto.Forms Support Addin 2.5.0
.NET Core support for Mono.Debugging 8.0.5
AddinMaker 1.5.0
RhinoCommon Plugin Support 8.4.4.0
MSBuild Editor 2.4.0

Example Project (If Possible)

TestXamMacConstructorIssue.zip

[cwensley]

Note that this also has the opposite effect where if you use this API on the main thread, and another object is created on a background thread thread between steps 2 and 4 above (e.g. for example an object used for NSApplication.SharedApplication.InvokeOnMainThread), it can possibly unregister that new object, which then would try to re-create said object which does not have a constructor with an IntPtr handle (and adding the constructor would not solve the problem).

[rolfbjarne]

I can reproduce the problem with the test project, it also makes sense from your description (great debugging btw!)

We might be able to fix this by not registering handles until init has been called, but this is a rather invasive change, so it will likely take some time until we're able to release a fix (at the very least because a lot of thinking and testing would be required first).

[cwensley]

Thanks @rolfbjarne, doing as you suggest (not registering until after init is called) was my thinking as well to avoid the race condition.

I understand that it'll take a while to get that fix in, but one thing that would help greatly in the meantime is to have some debug output or some way to detect if other objects do the same so we can work around them, perhaps a simple test in InitializeHandle to see if the handle is different than the one created in AllocIfNeeded.

This case is the only one we have found so far after many years of trying to find the cause of these crashes and issues, but they are usually impossible to find. We just fluked out with one specific set of code that pinpointed the issue.