Address code review feedback on GC bridge implementation

- Remove explicit Cdecl calling convention from UnmanagedCallersOnly and
  function pointers (Cdecl is default on Linux/Android)
- Restore bridgeProcessingSemaphore for WaitForGCBridgeProcessing
- Convert BridgeProcessing.Initialize to static constructor using RuntimeFeature
- Replace CrossReferenceTarget struct with ValueTuple
- Rename peer variable to peerToDispose for clarity
- Simplify AddReference/ClearReferences by removing outer try-catch blocks

Co-authored-by: simonrozsival <374616+simonrozsival@users.noreply.github.com>

Author: Copilot

src/Mono.Android/Android.Runtime/RuntimeNativeMethods.cs

@@ -92,7 +92,7 @@ internal unsafe static class RuntimeNativeMethods
 		/// </summary>
 		[DllImport (RuntimeConstants.InternalDllName, CallingConvention = CallingConvention.Cdecl)]
 		internal static extern delegate* unmanaged<void*, void> clr_gc_bridge_initialize_for_managed_processing (
-			delegate* unmanaged[Cdecl]<void*, void> onMarkCrossReferencesCallback);
+			delegate* unmanaged<void*, void> onMarkCrossReferencesCallback);
 
 		[MethodImplAttribute(MethodImplOptions.InternalCall)]
 		internal static extern void monodroid_unhandled_exception (Exception javaException);

src/Mono.Android/Microsoft.Android.Runtime/BridgeProcessing.cs

@@ -34,41 +34,17 @@ unsafe class BridgeProcessing
 	static JniMethodInfo? s_Runtime_gc;
 
 	// Is NativeAOT mode (uses GCUserPeerable instead of IGCUserPeer)
-	static bool s_initialized;
-	static bool s_isNativeAOT;
+	static readonly bool s_isNativeAOT;
 
-	public BridgeProcessing (MarkCrossReferencesArgs* args)
-	{
-		if (args == null) {
-			throw new ArgumentNullException (nameof (args), "Cross references argument is a NULL pointer");
-		}
-
-		if (args->ComponentCount > 0 && args->Components == null) {
-			throw new InvalidOperationException ("Components member of the cross references arguments structure is NULL");
-		}
-
-		if (args->CrossReferenceCount > 0 && args->CrossReferences == null) {
-			throw new InvalidOperationException ("CrossReferences member of the cross references arguments structure is NULL");
-		}
-
-		crossRefs = args;
-	}
-
-	/// <summary>
-	/// Initialize cached Java references. Must be called once during runtime initialization.
-	/// </summary>
-	public static void Initialize (bool isNativeAOT)
+	static BridgeProcessing ()
 	{
-		if (s_initialized)
-			return;
-
-		s_isNativeAOT = isNativeAOT;
+		s_isNativeAOT = !RuntimeFeature.IsMonoRuntime;
 
 		// Initialize GCUserPeer class for creating temporary peers
 		s_GCUserPeerClass = new JniType ("mono/android/GCUserPeer");
 		s_GCUserPeerCtor = s_GCUserPeerClass.GetConstructor ("()V");
 
-		if (isNativeAOT) {
+		if (s_isNativeAOT) {
 			// For NativeAOT, we also use GCUserPeerable interface for optimized method calls
 			s_GCUserPeerableClass = new JniType ("net/dot/jni/GCUserPeerable");
 			s_GCUserPeerable_jiAddManagedReference = s_GCUserPeerableClass.GetInstanceMethod ("jiAddManagedReference", "(Ljava/lang/Object;)V");
@@ -82,8 +58,23 @@ public static void Initialize (bool isNativeAOT)
 		var runtimeLocal = JniEnvironment.StaticMethods.CallStaticObjectMethod (runtimeClass.PeerReference, getRuntimeMethod, null);
 		s_RuntimeInstance = runtimeLocal.NewGlobalRef ();
 		JniObjectReference.Dispose (ref runtimeLocal);
+	}
 
-		s_initialized = true;
+	public BridgeProcessing (MarkCrossReferencesArgs* args)
+	{
+		if (args == null) {
+			throw new ArgumentNullException (nameof (args), "Cross references argument is a NULL pointer");
+		}
+
+		if (args->ComponentCount > 0 && args->Components == null) {
+			throw new InvalidOperationException ("Components member of the cross references arguments structure is NULL");
+		}
+
+		if (args->CrossReferenceCount > 0 && args->CrossReferences == null) {
+			throw new InvalidOperationException ("CrossReferences member of the cross references arguments structure is NULL");
+		}
+
+		crossRefs = args;
 	}
 
 	/// <summary>
@@ -117,8 +108,8 @@ void PrepareForJavaCollection ()
 
 		// With cross references processed, the temporary peer list can be released
 		foreach (var (_, temporaryPeer) in temporaryPeers) {
-			var peer = temporaryPeer;
-			JniObjectReference.Dispose (ref peer);
+			var peerToDispose = temporaryPeer;
+			JniObjectReference.Dispose (ref peerToDispose);
 		}
 
 		// Switch global to weak references
@@ -177,52 +168,35 @@ void AddCircularReferences (ref StronglyConnectedComponent scc)
 
 	void AddCrossReference (nuint sourceIndex, nuint destIndex)
 	{
-		CrossReferenceTarget from = SelectCrossReferenceTarget (sourceIndex);
-		CrossReferenceTarget to = SelectCrossReferenceTarget (destIndex);
+		var (fromRef, fromContextPtr) = SelectCrossReferenceTarget (sourceIndex);
+		var (toRef, _) = SelectCrossReferenceTarget (destIndex);
 
-		if (AddReference (from.Reference, to.Reference)) {
-			from.MarkRefsAddedIfNeeded ();
+		if (AddReference (fromRef, toRef) && fromContextPtr != IntPtr.Zero) {
+			HandleContext* fromContext = (HandleContext*)fromContextPtr;
+			fromContext->ControlBlock->RefsAdded = 1;
 		}
 	}
 
-	readonly struct CrossReferenceTarget
-	{
-		public readonly JniObjectReference Reference;
-		public readonly HandleContext* Context;
-		public readonly bool IsTemporaryPeer;
-
-		public CrossReferenceTarget (JniObjectReference reference, HandleContext* context, bool isTemporaryPeer)
-		{
-			Reference = reference;
-			Context = context;
-			IsTemporaryPeer = isTemporaryPeer;
-		}
-
-		public void MarkRefsAddedIfNeeded ()
-		{
-			if (IsTemporaryPeer || Context == null) {
-				return;
-			}
-			Context->ControlBlock->RefsAdded = 1;
-		}
-	}
-
-	CrossReferenceTarget SelectCrossReferenceTarget (nuint sccIndex)
+	/// <summary>
+	/// Selects the target for a cross-reference from an SCC.
+	/// Returns the JNI reference and the handle context pointer (IntPtr.Zero for temporary peers).
+	/// </summary>
+	(JniObjectReference Reference, IntPtr ContextPtr) SelectCrossReferenceTarget (nuint sccIndex)
 	{
 		ref StronglyConnectedComponent scc = ref crossRefs->Components[sccIndex];
 
 		if (scc.Count == 0) {
 			if (!temporaryPeers.TryGetValue (sccIndex, out var tempPeer)) {
 				throw new InvalidOperationException ("Temporary peer must be found in the map");
 			}
-			return new CrossReferenceTarget (tempPeer, null, isTemporaryPeer: true);
+			return (tempPeer, IntPtr.Zero);
 		}
 
 		HandleContext* context = (HandleContext*)scc.Contexts[0];
 		Debug.Assert (context != null && context->ControlBlock != null, "SCC must have at least one valid context");
 
 		var reference = new JniObjectReference (context->ControlBlock->Handle, JniObjectReferenceType.Global);
-		return new CrossReferenceTarget (reference, context, isTemporaryPeer: false);
+		return (reference, (IntPtr)context);
 	}
 
 	bool AddReference (JniObjectReference from, JniObjectReference to)
@@ -238,23 +212,20 @@ bool AddReference (JniObjectReference from, JniObjectReference to)
 
 		// Fall back to reflection-based approach
 		var fromClassRef = JniEnvironment.Types.GetObjectClass (from);
-		try {
-			using var fromClass = new JniType (ref fromClassRef, JniObjectReferenceOptions.CopyAndDispose);
-			JniMethodInfo addMethod;
-			try {
-				addMethod = fromClass.GetInstanceMethod ("monodroidAddReference", "(Ljava/lang/Object;)V");
-			} catch (Java.Lang.NoSuchMethodError) {
-				Logger.Log (LogLevel.Error, "monodroid-gc", "Failed to find monodroidAddReference method");
-				return false;
-			}
+		using var fromClass = new JniType (ref fromClassRef, JniObjectReferenceOptions.CopyAndDispose);
 
-			JniArgumentValue* args = stackalloc JniArgumentValue[1];
-			args[0] = new JniArgumentValue (to);
-			JniEnvironment.InstanceMethods.CallVoidMethod (from, addMethod, args);
-			return true;
-		} catch {
+		JniMethodInfo addMethod;
+		try {
+			addMethod = fromClass.GetInstanceMethod ("monodroidAddReference", "(Ljava/lang/Object;)V");
+		} catch (Java.Lang.NoSuchMethodError) {
+			Logger.Log (LogLevel.Error, "monodroid-gc", "Failed to find monodroidAddReference method");
 			return false;
 		}
+
+		JniArgumentValue* args = stackalloc JniArgumentValue[1];
+		args[0] = new JniArgumentValue (to);
+		JniEnvironment.InstanceMethods.CallVoidMethod (from, addMethod, args);
+		return true;
 	}
 
 	bool TryCallGCUserPeerableAddManagedReference (JniObjectReference from, JniObjectReference to)
@@ -279,7 +250,7 @@ bool TryCallGCUserPeerableAddManagedReference (JniObjectReference from, JniObjec
 	static void TriggerJavaGC ()
 	{
 		if (s_Runtime_gc == null) {
-			throw new InvalidOperationException ("BridgeProcessing.Initialize must be called before TriggerJavaGC");
+			throw new InvalidOperationException ("BridgeProcessing static constructor must run before TriggerJavaGC");
 		}
 
 		try {
@@ -370,20 +341,17 @@ void ClearReferences (JniObjectReference handle)
 
 		// Fall back to reflection-based approach
 		var javaClassRef = JniEnvironment.Types.GetObjectClass (handle);
-		try {
-			using var javaClass = new JniType (ref javaClassRef, JniObjectReferenceOptions.CopyAndDispose);
-			JniMethodInfo clearMethod;
-			try {
-				clearMethod = javaClass.GetInstanceMethod ("monodroidClearReferences", "()V");
-			} catch (Java.Lang.NoSuchMethodError) {
-				Logger.Log (LogLevel.Error, "monodroid-gc", "Failed to find monodroidClearReferences method");
-				return;
-			}
+		using var javaClass = new JniType (ref javaClassRef, JniObjectReferenceOptions.CopyAndDispose);
 
-			JniEnvironment.InstanceMethods.CallVoidMethod (handle, clearMethod, null);
-		} catch {
-			// Ignore exceptions during cleanup
+		JniMethodInfo clearMethod;
+		try {
+			clearMethod = javaClass.GetInstanceMethod ("monodroidClearReferences", "()V");
+		} catch (Java.Lang.NoSuchMethodError) {
+			Logger.Log (LogLevel.Error, "monodroid-gc", "Failed to find monodroidClearReferences method");
+			return;
 		}
+
+		JniEnvironment.InstanceMethods.CallVoidMethod (handle, clearMethod, null);
 	}
 
 	bool TryCallGCUserPeerableClearManagedReferences (JniObjectReference handle)

src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs

@@ -28,34 +28,41 @@ class ManagedValueManager : JniRuntime.JniValueManager
 
 	bool disposed;
 
+	static readonly SemaphoreSlim bridgeProcessingSemaphore = new (1, 1);
+
 	static Lazy<ManagedValueManager> s_instance = new (() => new ManagedValueManager ());
 	public static ManagedValueManager GetOrCreateInstance () => s_instance.Value;
 
 	/// <summary>
 	/// Callback invoked by native background thread to process bridge args.
 	/// This runs on the native bridge processing thread, not a managed thread pool thread.
 	/// </summary>
-	[UnmanagedCallersOnly (CallConvs = [typeof (CallConvCdecl)])]
+	[UnmanagedCallersOnly]
 	static unsafe void OnBridgeProcessing (void* argsPtr)
 	{
 		MarkCrossReferencesArgs* args = (MarkCrossReferencesArgs*)argsPtr;
 
-		// Validate contexts
-		HandleContext.EnsureAllContextsAreOurs (args);
-
-		// Do the actual bridge processing in managed code
-		var processing = new BridgeProcessing (args);
-		processing.Process ();
-
-		// Process collected contexts and finish
-		ReadOnlySpan<GCHandle> handlesToFree = ProcessCollectedContexts (args);
-		JavaMarshal.FinishCrossReferenceProcessing (args, handlesToFree);
+		bridgeProcessingSemaphore.Wait ();
+		try {
+			// Validate contexts
+			HandleContext.EnsureAllContextsAreOurs (args);
+
+			// Do the actual bridge processing in managed code
+			var processing = new BridgeProcessing (args);
+			processing.Process ();
+
+			// Process collected contexts and finish
+			ReadOnlySpan<GCHandle> handlesToFree = ProcessCollectedContexts (args);
+			JavaMarshal.FinishCrossReferenceProcessing (args, handlesToFree);
+		} finally {
+			bridgeProcessingSemaphore.Release ();
+		}
 	}
 
 	unsafe ManagedValueManager ()
 	{
 		// Initialize GC bridge with our callback that will be invoked from the native background thread
-		delegate* unmanaged[Cdecl]<void*, void> callback = &OnBridgeProcessing;
+		delegate* unmanaged<void*, void> callback = &OnBridgeProcessing;
 		var mark_cross_references_ftn = RuntimeNativeMethods.clr_gc_bridge_initialize_for_managed_processing (callback);
 		JavaMarshal.Initialize (mark_cross_references_ftn);
 	}
@@ -74,8 +81,8 @@ void ThrowIfDisposed ()
 
 	public override void WaitForGCBridgeProcessing ()
 	{
-		// No-op: The native code guarantees mark_cross_references won't be called again
-		// until FinishCrossReferenceProcessing returns, so there's no need to wait.
+		bridgeProcessingSemaphore.Wait ();
+		bridgeProcessingSemaphore.Release ();
 	}
 
 	public unsafe override void CollectPeers ()