[runtime] Fix a race condition when switching between weak and strong GCHandles. Fixes #24702. (#24841)

## Summary

Fix a race condition in `xamarin_switch_gchandle` where one thread switching a
GCHandle between weak and strong could free a handle that another thread was
simultaneously reading. This caused intermittent crashes and "Could not find
an existing managed instance" errors at runtime.

Fixes #24702.

## Problem

`xamarin_switch_gchandle` is called from retain/release to toggle an object's
GCHandle between weak (so the GC can collect the managed peer) and strong (so
the managed peer stays alive while Objective-C holds a reference).

The old implementation performed a non-atomic free-then-replace on the
object's `gc_handle` field:

1. Read the old GCHandle and its flags.
2. Allocate a new GCHandle (weak or strong).
3. Free the old GCHandle.
4. Store the new GCHandle on the object.

Between steps 3 and 4, a concurrent thread invoking an exported Objective-C
method would read the `gc_handle` field, obtain the already-freed handle, and
either crash or fail to resolve the managed object — producing errors like:

> Failed to marshal the Objective-C object 0x… Could not find an existing
> managed instance for this object, nor was it possible to create a new
> managed instance.

## Solution: dual-GCHandle scheme

Instead of replacing the object's GCHandle in place, the fix introduces a
**dual-GCHandle** design:

- **The object's `gc_handle` is always a weak handle** and is never modified
  by `xamarin_switch_gchandle`. Concurrent readers always see a valid handle.
- **A separate strong GCHandle** is stored in a global mutex-protected
  `CFMutableDictionary` (`strong_gchandle_hash`), keyed by the native object
  pointer.
- **Switching to strong**: allocate a strong GCHandle for the managed object
  and insert it into the hash table. The weak handle remains on the object
  unchanged.
- **Switching to weak**: remove and free the strong GCHandle from the hash
  table. The weak handle remains on the object unchanged.

Because the `gc_handle` field is never written by `xamarin_switch_gchandle`,
the race is eliminated: concurrent readers always see a valid weak GCHandle
that can resolve the managed object (the strong GCHandle in the hash table
keeps it alive and prevents collection).

## Changes

### `runtime/runtime.m`

- Add `strong_gchandle_hash` (`CFMutableDictionaryRef`) and its
  `pthread_mutex_t` lock.
- Add `free_strong_gchandle()` — removes and frees the strong handle for a
  native object from the hash table.
- Add `create_strong_gchandle()` — creates a strong handle and inserts it into
  the hash table (no-op if one already exists, preventing duplicates from
  concurrent callers).
- Rewrite `xamarin_switch_gchandle()` to use the dual-GCHandle scheme
  instead of free-then-replace.
- Update `xamarin_free_gchandle()` to also clear any strong GCHandle from
  the hash table (`xamarin_free_gchandle()` is called when a native object
  is about to be freed).
- Clean up `strong_gchandle_hash` during runtime shutdown.
- Remove `get_gchandle_safe()` (no longer needed; replaced by
  `get_gchandle_without_flags()`).

### `runtime/xamarin/trampolines.h`

- Remove `XamarinGCHandleFlags_WeakGCHandle = 1` (the flag is no longer
  needed since `gc_handle` is always weak).

### `src/Foundation/NSObject2.cs`

- Remove `WeakGCHandle` from the managed `XamarinGCHandleFlags` enum.
- Stop setting `WeakGCHandle` in `CreateManagedRef()`.

### `tests/monotouch-test/ObjCRuntime/GCHandleSwitchRaceTest.cs` (new)

- Regression test that reproduces the race: one thread rapidly
  retains/releases an object (toggling the GCHandle) while another thread
  simultaneously invokes an exported Objective-C method (reading the
  GCHandle). Without the fix this crashes; with the fix it completes
  cleanly.

### `tests/dotnet/UnitTests/expected/iOS-MonoVM-*.txt`

- Remove `WeakGCHandle` from preserved API expectations.

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Author: rolfbjarne

runtime/runtime.m

@@ -90,6 +90,13 @@
 static pthread_mutex_t framework_peer_release_lock;
 static MonoGHashTable *xamarin_wrapper_hash;
 
+// Hash table mapping native object pointers (id) to strong GCHandles.
+// Used by the dual-gchandle scheme: the object's gc_handle is always a weak
+// handle used for lookups, while this table holds an optional strong handle
+// that keeps the managed object alive. See xamarin_switch_gchandle.
+static CFMutableDictionaryRef strong_gchandle_hash = NULL;
+static pthread_mutex_t strong_gchandle_hash_lock = PTHREAD_MUTEX_INITIALIZER;
+
 static bool initialize_started = FALSE;
 
 #include "delegates.inc"
@@ -412,16 +419,6 @@ -(void) xamarinSetGCHandleFlags: (enum XamarinGCHandleFlags) gchandle_flags;
 -(struct NSObjectData*) xamarinGetNSObjectData;
 @end
 
-static inline GCHandle
-get_gchandle_safe (id self, enum XamarinGCHandleFlags *gchandle_flags)
-{
-	id<XamarinExtendedObject> xself = self;
-	GCHandle rv = [xself xamarinGetGCHandle];
-	if (gchandle_flags)
-		*gchandle_flags = [xself xamarinGetGCHandleFlags];
-	return rv;
-}
-
 static inline bool
 set_gchandle (id self, GCHandle gc_handle, enum XamarinGCHandleFlags flags, struct NSObjectData *data)
 {
@@ -1567,26 +1564,69 @@ -(struct NSObjectData*) xamarinGetNSObjectData;
  */
 //#define DEBUG_REF_COUNTING
 
+// Free the strong gchandle for a given native object.
+static void
+free_strong_gchandle (id self)
+{
+	GCHandle strong_gchandle = INVALID_GCHANDLE;
+	pthread_mutex_lock (&strong_gchandle_hash_lock);
+	if (strong_gchandle_hash != NULL) {
+		const void *value;
+		if (CFDictionaryGetValueIfPresent (strong_gchandle_hash, self, &value)) {
+			strong_gchandle = (GCHandle) value;
+			CFDictionaryRemoveValue (strong_gchandle_hash, self);
+		}
+	}
+	pthread_mutex_unlock (&strong_gchandle_hash_lock);
+	if (strong_gchandle != INVALID_GCHANDLE) {
+#if defined(DEBUG_REF_COUNTING)
+		PRINT ("Cleared strong gchandle %d for %p\n", strong_gchandle, self);
+#endif
+		xamarin_gchandle_free (strong_gchandle);
+	} else {
+#if defined(DEBUG_REF_COUNTING)
+		PRINT ("Did not clear a strong gchandle for %p, because none was found\n", self);
+#endif
+	}
+}
+
+// Creates a strong gchandle for the given managed object and associates it
+// with the native object. If a strong gchandle already exists for this
+// native object, the newly created one is freed and the existing one is kept.
+// Returns true if a new strong gchandle was set, false if one already existed.
+static bool
+create_strong_gchandle (id self, MonoObject *managed_object)
+{
+	GCHandle new_gchandle = xamarin_gchandle_new (managed_object, FALSE);
+
+	GCHandle existing = INVALID_GCHANDLE;
+	pthread_mutex_lock (&strong_gchandle_hash_lock);
+	if (strong_gchandle_hash == NULL)
+		strong_gchandle_hash = CFDictionaryCreateMutable (kCFAllocatorDefault, 0, NULL, NULL);
+	const void *value;
+	if (CFDictionaryGetValueIfPresent (strong_gchandle_hash, self, &value))
+		existing = (GCHandle) value;
+	if (existing == INVALID_GCHANDLE)
+		CFDictionarySetValue (strong_gchandle_hash, self, (const void *) new_gchandle);
+	pthread_mutex_unlock (&strong_gchandle_hash_lock);
+
+	if (existing != INVALID_GCHANDLE) {
+		// Another thread already set a strong gchandle, free ours.
+		xamarin_gchandle_free (new_gchandle);
+		return false;
+	}
+	return true;
+}
+
 void
 xamarin_switch_gchandle (id self, bool to_weak)
 {
-	GCHandle new_gchandle;
 	GCHandle old_gchandle;
-	MonoObject *managed_object;
-	enum XamarinGCHandleFlags flags = XamarinGCHandleFlags_None;
-
-	old_gchandle = get_gchandle_safe (self, &flags);
-	if (old_gchandle) {
-		bool is_weak = (flags & XamarinGCHandleFlags_WeakGCHandle) == XamarinGCHandleFlags_WeakGCHandle;
-		if (to_weak == is_weak) {
-			// we already have the GCHandle we need
-#if defined(DEBUG_REF_COUNTING)
-			PRINT ("Object %p already has a %s GCHandle = %d\n", self, to_weak ? "weak" : "strong", old_gchandle);
-#endif
-			return;
-		}
-	} else {
-		// We don't have a GCHandle. This means there's no managed instance for this 
+	MonoObject *managed_object = NULL;
+
+	old_gchandle = get_gchandle_without_flags (self);
+	if (!old_gchandle) {
+		// We don't have a GCHandle. This means there's no managed instance for this
 		// native object.
 		// If to_weak is true, then there's obviously nothing to do
 		// (why create a managed object which can immediately be freed by the GC?).
@@ -1600,40 +1640,31 @@ -(struct NSObjectData*) xamarinGetNSObjectData;
 		return;
 	}
 
-	MONO_THREAD_ATTACH;
-
-	managed_object = xamarin_gchandle_get_target (old_gchandle);
+	// The object's gc_handle is always a weak handle used for lookups. It is
+	// never modified here. A separate strong gchandle (stored in a global hash
+	// table) is created/freed as needed to keep the managed object alive.
+	// Since gc_handle is never modified, there is no race with concurrent
+	// readers (fixing https://github.com/dotnet/macios/issues/24702).
 
 	if (to_weak) {
-		new_gchandle = xamarin_gchandle_new_weakref (managed_object, TRUE);
-		flags = (enum XamarinGCHandleFlags) (flags | XamarinGCHandleFlags_WeakGCHandle);
+		free_strong_gchandle (self);
 	} else {
-		new_gchandle = xamarin_gchandle_new (managed_object, FALSE);
-		flags = (enum XamarinGCHandleFlags) (flags & ~XamarinGCHandleFlags_WeakGCHandle);
-	}
+		MONO_THREAD_ATTACH;
 
-	xamarin_gchandle_free (old_gchandle);
-	
-	if (managed_object) {
-		// It's possible to not have a managed object if:
-		// 1. Objective-C holds a weak reference to the native object (and no other strong references)
-		//    - in which case the original (old) gchandle would be a weak one.
-		// 2. Managed code does not reference the managed object.
-		// 3. The GC ran and collected the managed object, but the main thread has not gotten
-		//    around to release the native object yet.
-		// If all these conditions hold, then the original gchandle will point to
-		// null, because the target would be collected.
-		xamarin_set_nsobject_flags (managed_object, xamarin_get_nsobject_flags (managed_object) | NSObjectFlagsHasManagedRef);
-	}
-	set_gchandle (self, new_gchandle, flags, NULL);
-
-	MONO_THREAD_DETACH;
+		managed_object = xamarin_gchandle_get_target (old_gchandle);
+		if (managed_object) {
+			if (create_strong_gchandle (self, managed_object)) {
+				xamarin_set_nsobject_flags (managed_object, xamarin_get_nsobject_flags (managed_object) | NSObjectFlagsHasManagedRef);
+			}
+		}
 
-	xamarin_mono_object_release (&managed_object);
+		MONO_THREAD_DETACH;
+		xamarin_mono_object_release (&managed_object);
 
 #if defined(DEBUG_REF_COUNTING)
-	PRINT ("Switched object %p to %s GCHandle = %d managed object = %p\n", self, to_weak ? "weak" : "strong", new_gchandle, managed_object);
+		PRINT ("Object %p switched to strong mode\n", self);
 #endif
+	}
 }
 
 void
@@ -1651,6 +1682,9 @@ -(struct NSObjectData*) xamarinGetNSObjectData;
 		PRINT ("\tNo GCHandle for the object %p\n", self);
 #endif
 	}
+
+	// Also free any strong gchandle from the dual-gchandle scheme.
+	free_strong_gchandle (self);
 }
 
 void
@@ -1952,6 +1986,13 @@ -(struct NSObjectData*) xamarinGetNSObjectData;
 	xamarin_mono_object_release (&xamarin_wrapper_hash);
 	pthread_mutex_unlock (&wrapper_hash_lock);
 #endif
+
+	pthread_mutex_lock (&strong_gchandle_hash_lock);
+	if (strong_gchandle_hash != NULL) {
+		CFRelease (strong_gchandle_hash);
+		strong_gchandle_hash = NULL;
+	}
+	pthread_mutex_unlock (&strong_gchandle_hash_lock);
 }
 
 void

runtime/xamarin/trampolines.h

@@ -16,7 +16,7 @@ extern "C" {
 // Must be kept in sync with the same enum in NSObject2.cs
 enum XamarinGCHandleFlags : uint32_t {
 	XamarinGCHandleFlags_None = 0,
-	XamarinGCHandleFlags_WeakGCHandle = 1,
+	// unused = 1
 	XamarinGCHandleFlags_HasManagedRef = 2,
 	XamarinGCHandleFlags_InitialSet = 4,
 };

src/Foundation/NSObject2.cs

@@ -263,7 +263,7 @@ internal enum Flags : uint {
 		// Must be kept in sync with the same enum in trampolines.h
 		enum XamarinGCHandleFlags : uint {
 			None = 0,
-			WeakGCHandle = 1,
+			// unused = 1
 			HasManagedRef = 2,
 			InitialSet = 4,
 		}
@@ -500,7 +500,7 @@ void CreateManagedRef (bool retain)
 				throw new InvalidOperationException ($"Unable to create a managed reference for the pointer {handle} whose managed type is {GetType ().FullName} because it wasn't possible to get the class of the pointer: {error_message}");
 
 			if (isUserType) {
-				var gchandle_flags = XamarinGCHandleFlags.HasManagedRef | XamarinGCHandleFlags.InitialSet | XamarinGCHandleFlags.WeakGCHandle;
+				var gchandle_flags = XamarinGCHandleFlags.HasManagedRef | XamarinGCHandleFlags.InitialSet;
 				var gchandle = GCHandle.Alloc (this, GCHandleType.WeakTrackResurrection);
 				var h = GCHandle.ToIntPtr (gchandle);
 				byte rv;

tests/dotnet/UnitTests/expected/iOS-MonoVM-interpreter-preservedapis.txt

@@ -189,7 +189,6 @@ Microsoft.iOS.dll:Foundation.NSObject/XamarinGCHandleFlags
 Microsoft.iOS.dll:Foundation.NSObject/XamarinGCHandleFlags Foundation.NSObject/XamarinGCHandleFlags::HasManagedRef
 Microsoft.iOS.dll:Foundation.NSObject/XamarinGCHandleFlags Foundation.NSObject/XamarinGCHandleFlags::InitialSet
 Microsoft.iOS.dll:Foundation.NSObject/XamarinGCHandleFlags Foundation.NSObject/XamarinGCHandleFlags::None
-Microsoft.iOS.dll:Foundation.NSObject/XamarinGCHandleFlags Foundation.NSObject/XamarinGCHandleFlags::WeakGCHandle
 Microsoft.iOS.dll:Foundation.NSObjectData
 Microsoft.iOS.dll:Foundation.NSObjectData* Foundation.NSObject::__data_for_mono
 Microsoft.iOS.dll:Foundation.NSObjectData* Foundation.NSObjectDataHandle::Data()

tests/dotnet/UnitTests/expected/iOS-MonoVM-preservedapis.txt

@@ -173,7 +173,6 @@ Microsoft.iOS.dll:Foundation.NSObject/XamarinGCHandleFlags
 Microsoft.iOS.dll:Foundation.NSObject/XamarinGCHandleFlags Foundation.NSObject/XamarinGCHandleFlags::HasManagedRef
 Microsoft.iOS.dll:Foundation.NSObject/XamarinGCHandleFlags Foundation.NSObject/XamarinGCHandleFlags::InitialSet
 Microsoft.iOS.dll:Foundation.NSObject/XamarinGCHandleFlags Foundation.NSObject/XamarinGCHandleFlags::None
-Microsoft.iOS.dll:Foundation.NSObject/XamarinGCHandleFlags Foundation.NSObject/XamarinGCHandleFlags::WeakGCHandle
 Microsoft.iOS.dll:Foundation.NSObjectData
 Microsoft.iOS.dll:Foundation.NSObjectData* Foundation.NSObject::__data_for_mono
 Microsoft.iOS.dll:Foundation.NSObjectData* Foundation.NSObjectDataHandle::Data()

tests/monotouch-test/ObjCRuntime/GCHandleSwitchRaceTest.cs

@@ -0,0 +1,80 @@
+using System.Diagnostics;
+using System.Threading;
+
+namespace MonoTouchFixtures.ObjCRuntime {
+	[TestFixture]
+	[Preserve (AllMembers = true)]
+	public class GCHandleSwitchRaceTest {
+
+		class CustomObject : NSObject {
+			[Export ("getCounter")]
+			public int GetCounter ()
+			{
+				return 42;
+			}
+		}
+
+		[Test]
+		public void RunTest ()
+		{
+			// This test verifies a race condition in xamarin_switch_gchandle:
+			// Thread A switches between weak/strong gchandles (via retain/release),
+			// while Thread B fetches the gchandle (by calling an exported ObjC method).
+			// The race is that Thread B can read the old gchandle after Thread A has
+			// freed it but before Thread A has set the new one.
+			// Ref: https://github.com/dotnet/macios/issues/24702
+			var done = new ManualResetEvent (false);
+			Exception? switchException = null;
+			Exception? fetchException = null;
+
+			var obj = new CustomObject ();
+
+			// Thread that switches gchandle between weak and strong repeatedly.
+			var switchThread = new Thread (() => {
+				try {
+					while (!done.WaitOne (0)) {
+						obj.DangerousRetain ();
+						obj.DangerousRelease ();
+					}
+				} catch (Exception ex) {
+					switchException = ex;
+					done.Set ();
+				}
+			}) {
+				IsBackground = true,
+				Name = "GCHandle Switch Thread",
+			};
+			switchThread.Start ();
+
+			// Thread that fetches the managed object (reads the gchandle) via an ObjC call.
+			var fetchThread = new Thread (() => {
+				try {
+					while (!done.WaitOne (0)) {
+						Messaging.int_objc_msgSend (obj.Handle, Selector.GetHandle ("getCounter"));
+					}
+				} catch (Exception ex) {
+					fetchException = ex;
+					done.Set ();
+				}
+			}) {
+				IsBackground = true,
+				Name = "GCHandle Fetch Thread",
+			};
+			fetchThread.Start ();
+
+			// Let the threads race for a few seconds.
+			Thread.Sleep (TimeSpan.FromSeconds (3));
+
+			done.Set ();
+
+			// If there's a hang, the bug is there.
+			if (!switchThread.Join (TimeSpan.FromSeconds (30)))
+				Assert.Fail ("Switch thread is hung.");
+			if (!fetchThread.Join (TimeSpan.FromSeconds (30)))
+				Assert.Fail ("Fetch thread is hung.");
+
+			Assert.IsNull (switchException, $"Switch thread failed: {switchException}");
+			Assert.IsNull (fetchException, $"Fetch thread failed: {fetchException}");
+		}
+	}
+}