diff --git a/src/Microsoft.Android.Runtime.NativeAOT/Android.Runtime.NativeAOT/JavaInteropRuntime.cs b/src/Microsoft.Android.Runtime.NativeAOT/Android.Runtime.NativeAOT/JavaInteropRuntime.cs index 45a82899bba..dbdc1eb4327 100644 --- a/src/Microsoft.Android.Runtime.NativeAOT/Android.Runtime.NativeAOT/JavaInteropRuntime.cs +++ b/src/Microsoft.Android.Runtime.NativeAOT/Android.Runtime.NativeAOT/JavaInteropRuntime.cs @@ -62,7 +62,7 @@ static void init (IntPtr jnienv, IntPtr klass, IntPtr classLoader, IntPtr langua EnvironmentPointer = jnienv, ClassLoader = new JniObjectReference (classLoader, JniObjectReferenceType.Global), TypeManager = new ManagedTypeManager (), - ValueManager = ManagedValueManager.GetOrCreateInstance (), + ValueManager = ManagedValueManager.Instance, UseMarshalMemberBuilder = false, JniGlobalReferenceLogWriter = settings.GrefLog, JniLocalReferenceLogWriter = settings.LrefLog, diff --git a/src/Microsoft.Android.Runtime.NativeAOT/Java.Interop/JreRuntime.cs b/src/Microsoft.Android.Runtime.NativeAOT/Java.Interop/JreRuntime.cs index 4e70da25a0e..afe50d3ea24 100644 --- a/src/Microsoft.Android.Runtime.NativeAOT/Java.Interop/JreRuntime.cs +++ b/src/Microsoft.Android.Runtime.NativeAOT/Java.Interop/JreRuntime.cs @@ -61,7 +61,7 @@ static NativeAotRuntimeOptions CreateJreVM (NativeAotRuntimeOptions builder) builder.TypeManager ??= new ManagedTypeManager (); #endif // NET - builder.ValueManager ??= ManagedValueManager.GetOrCreateInstance(); + builder.ValueManager ??= ManagedValueManager.Instance; builder.ObjectReferenceManager ??= new Android.Runtime.AndroidObjectReferenceManager (); if (builder.InvocationPointer != IntPtr.Zero || builder.EnvironmentPointer != IntPtr.Zero) diff --git a/src/Mono.Android/Android.Runtime/JNIEnvInit.cs b/src/Mono.Android/Android.Runtime/JNIEnvInit.cs index 71b370a3cee..65741304b35 100644 --- a/src/Mono.Android/Android.Runtime/JNIEnvInit.cs +++ b/src/Mono.Android/Android.Runtime/JNIEnvInit.cs @@ -139,7 +139,7 @@ internal static unsafe void Initialize (JnienvInitializeArgs* args) if (RuntimeFeature.IsMonoRuntime) { valueManager = new AndroidValueManager (); } else if (RuntimeFeature.IsCoreClrRuntime) { - valueManager = ManagedValueManager.GetOrCreateInstance (); + valueManager = ManagedValueManager.Instance; } else { throw new NotSupportedException ("Internal error: unknown runtime not supported"); } diff --git a/src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs b/src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs index e84a518971b..27ceb09d019 100644 --- a/src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs +++ b/src/Mono.Android/Microsoft.Android.Runtime/ManagedValueManager.cs @@ -13,6 +13,7 @@ using System.Runtime.InteropServices; using System.Runtime.InteropServices.Java; using System.Threading; +using System.Threading.Tasks; using Android.Runtime; using Java.Interop; @@ -22,15 +23,15 @@ class ManagedValueManager : JniRuntime.JniValueManager { const DynamicallyAccessedMemberTypes Constructors = DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors; - readonly Dictionary> RegisteredInstances = new (); - readonly ConcurrentQueue CollectedContexts = new (); + readonly Lock _registeredInstancesLock = new (); + readonly Dictionary> _registeredInstances = new (); + readonly ConcurrentQueue _collectedContexts = new (); - bool disposed; - - static readonly SemaphoreSlim bridgeProcessingSemaphore = new (1, 1); + bool _disposed; static Lazy s_instance = new (() => new ManagedValueManager ()); - public static ManagedValueManager GetOrCreateInstance () => s_instance.Value; + + public static ManagedValueManager Instance => s_instance.Value; unsafe ManagedValueManager () { @@ -41,31 +42,29 @@ unsafe ManagedValueManager () protected override void Dispose (bool disposing) { - disposed = true; + _disposed = true; base.Dispose (disposing); } void ThrowIfDisposed () { - if (disposed) + if (_disposed) throw new ObjectDisposedException (nameof (ManagedValueManager)); } public override void WaitForGCBridgeProcessing () { - bridgeProcessingSemaphore.Wait (); - bridgeProcessingSemaphore.Release (); } public unsafe override void CollectPeers () { ThrowIfDisposed (); - while (CollectedContexts.TryDequeue (out IntPtr contextPtr)) { - Debug.Assert (contextPtr != IntPtr.Zero, "CollectedContexts should not contain null pointers."); + while (_collectedContexts.TryDequeue (out IntPtr contextPtr)) { + Debug.Assert (contextPtr != IntPtr.Zero, "_collectedContexts should not contain null pointers."); HandleContext* context = (HandleContext*)contextPtr; - lock (RegisteredInstances) { + lock (_registeredInstancesLock) { Remove (context); } @@ -75,7 +74,7 @@ public unsafe override void CollectPeers () void Remove (HandleContext* context) { int key = context->PeerIdentityHashCode; - if (!RegisteredInstances.TryGetValue (key, out List? peers)) + if (!_registeredInstances.TryGetValue (key, out List? peers)) return; for (int i = peers.Count - 1; i >= 0; i--) { @@ -86,7 +85,7 @@ void Remove (HandleContext* context) } if (peers.Count == 0) { - RegisteredInstances.Remove (key); + _registeredInstances.Remove (key); } } } @@ -95,9 +94,6 @@ public override void AddPeer (IJavaPeerable value) { ThrowIfDisposed (); - // Remove any collected contexts before adding a new peer. - CollectPeers (); - var r = value.PeerReference; if (!r.IsValid) throw new ObjectDisposedException (value.GetType ().FullName); @@ -107,11 +103,11 @@ public override void AddPeer (IJavaPeerable value) JniObjectReference.Dispose (ref r, JniObjectReferenceOptions.CopyAndDispose); } int key = value.JniIdentityHashCode; - lock (RegisteredInstances) { + lock (_registeredInstancesLock) { List? peers; - if (!RegisteredInstances.TryGetValue (key, out peers)) { + if (!_registeredInstances.TryGetValue (key, out peers)) { peers = [new ReferenceTrackingHandle (value)]; - RegisteredInstances.Add (key, peers); + _registeredInstances.Add (key, peers); return; } @@ -160,8 +156,8 @@ void WarnNotReplacing (int key, IJavaPeerable ignoreValue, IJavaPeerable keepVal int key = GetJniIdentityHashCode (reference); - lock (RegisteredInstances) { - if (!RegisteredInstances.TryGetValue (key, out List? peers)) + lock (_registeredInstancesLock) { + if (!_registeredInstances.TryGetValue (key, out List? peers)) return null; for (int i = peers.Count - 1; i >= 0; i--) { @@ -173,7 +169,7 @@ void WarnNotReplacing (int key, IJavaPeerable ignoreValue, IJavaPeerable keepVal } if (peers.Count == 0) - RegisteredInstances.Remove (key); + _registeredInstances.Remove (key); } return null; } @@ -182,20 +178,20 @@ public override void RemovePeer (IJavaPeerable value) { ThrowIfDisposed (); - // Remove any collected contexts before modifying RegisteredInstances + // Remove any collected contexts before modifying _registeredInstances CollectPeers (); if (value == null) throw new ArgumentNullException (nameof (value)); - lock (RegisteredInstances) { + lock (_registeredInstancesLock) { int key = value.JniIdentityHashCode; - if (!RegisteredInstances.TryGetValue (key, out List? peers)) + if (!_registeredInstances.TryGetValue (key, out List? peers)) return; for (int i = peers.Count - 1; i >= 0; i--) { ReferenceTrackingHandle peer = peers [i]; - IJavaPeerable target = peer.Target; + IJavaPeerable? target = peer.Target; if (ReferenceEquals (value, target)) { peers.RemoveAt (i); peer.Dispose (); @@ -203,7 +199,7 @@ public override void RemovePeer (IJavaPeerable value) GC.KeepAlive (target); } if (peers.Count == 0) - RegisteredInstances.Remove (key); + _registeredInstances.Remove (key); } } @@ -284,9 +280,9 @@ public override List GetSurfacedPeers () // Remove any collected contexts before iterating over all the registered instances CollectPeers (); - lock (RegisteredInstances) { - var peers = new List (RegisteredInstances.Count); - foreach (var (identityHashCode, referenceTrackingHandles) in RegisteredInstances) { + lock (_registeredInstancesLock) { + var peers = new List (_registeredInstances.Count); + foreach (var (identityHashCode, referenceTrackingHandles) in _registeredInstances) { foreach (var peer in referenceTrackingHandles) { if (peer.Target is IJavaPeerable target) { peers.Add (new JniSurfacedPeerInfo (identityHashCode, new WeakReference (target))); @@ -299,7 +295,7 @@ public override List GetSurfacedPeers () unsafe struct ReferenceTrackingHandle : IDisposable { - WeakReference _weakReference; + WeakReference _weakReference; HandleContext* _context; public bool BelongsToContext (HandleContext* context) @@ -308,7 +304,7 @@ public bool BelongsToContext (HandleContext* context) public ReferenceTrackingHandle (IJavaPeerable peer) { _context = HandleContext.Alloc (peer); - _weakReference = new WeakReference (peer); + _weakReference = new (peer); } public IJavaPeerable? Target @@ -321,7 +317,7 @@ public void Dispose () IJavaPeerable? target = Target; - GCHandle handle = HandleContext.GetAssociatedGCHandle (_context); + GCHandle handle = _context->AssociatedGCHandle; HandleContext.Free (ref _context); _weakReference.SetTarget (null); if (handle.IsAllocated) { @@ -337,20 +333,20 @@ public void Dispose () unsafe struct HandleContext { static readonly nuint Size = (nuint)Marshal.SizeOf (); - static readonly Dictionary referenceTrackingHandles = new (); - int identityHashCode; - IntPtr controlBlock; + int _identityHashCode; + IntPtr _controlBlock; + IntPtr _gcHandle; // GCHandle stored as IntPtr to keep blittable layout - public int PeerIdentityHashCode => identityHashCode; + public int PeerIdentityHashCode => _identityHashCode; public bool IsCollected { get { - if (controlBlock == IntPtr.Zero) + if (_controlBlock == IntPtr.Zero) throw new InvalidOperationException ("HandleContext control block is not initialized."); - return ((JniObjectReferenceControlBlock*) controlBlock)->handle == IntPtr.Zero; + return ((JniObjectReferenceControlBlock*) _controlBlock)->handle == IntPtr.Zero; } } @@ -363,40 +359,25 @@ private struct JniObjectReferenceControlBlock public int refs_added; } - public static GCHandle GetAssociatedGCHandle (HandleContext* context) - { - lock (referenceTrackingHandles) { - if (!referenceTrackingHandles.TryGetValue ((IntPtr) context, out GCHandle handle)) { - throw new InvalidOperationException ("Unknown reference tracking handle."); - } + public GCHandle AssociatedGCHandle => GCHandle.FromIntPtr (_gcHandle); - return handle; - } - } +#if DEBUG + static readonly HashSet s_knownContexts = new (); public static unsafe void EnsureAllContextsAreOurs (MarkCrossReferencesArgs* mcr) { - lock (referenceTrackingHandles) { + lock (s_knownContexts) { for (nuint i = 0; i < mcr->ComponentCount; i++) { StronglyConnectedComponent component = mcr->Components [i]; - EnsureAllContextsInComponentAreOurs (component); - } - } - - static void EnsureAllContextsInComponentAreOurs (StronglyConnectedComponent component) - { - for (nuint i = 0; i < component.Count; i++) { - EnsureContextIsOurs ((IntPtr)component.Contexts [i]); + for (nuint j = 0; j < component.Count; j++) { + if (!s_knownContexts.Contains ((IntPtr)component.Contexts [j])) { + throw new InvalidOperationException ("Unknown reference tracking handle."); + } + } } } - - static void EnsureContextIsOurs (IntPtr context) - { - if (!referenceTrackingHandles.ContainsKey (context)) { - throw new InvalidOperationException ("Unknown reference tracking handle."); - } - } } +#endif public static HandleContext* Alloc (IJavaPeerable peer) { @@ -405,13 +386,17 @@ static void EnsureContextIsOurs (IntPtr context) throw new OutOfMemoryException ("Failed to allocate memory for HandleContext."); } - context->identityHashCode = peer.JniIdentityHashCode; - context->controlBlock = peer.JniObjectReferenceControlBlock; + context->_identityHashCode = peer.JniIdentityHashCode; + context->_controlBlock = peer.JniObjectReferenceControlBlock; GCHandle handle = JavaMarshal.CreateReferenceTrackingHandle (peer, context); - lock (referenceTrackingHandles) { - referenceTrackingHandles [(IntPtr) context] = handle; + context->_gcHandle = GCHandle.ToIntPtr (handle); + +#if DEBUG + lock (s_knownContexts) { + s_knownContexts.Add ((IntPtr)context); } +#endif return context; } @@ -422,9 +407,11 @@ public static void Free (ref HandleContext* context) return; } - lock (referenceTrackingHandles) { - referenceTrackingHandles.Remove ((IntPtr)context); +#if DEBUG + lock (s_knownContexts) { + s_knownContexts.Remove ((IntPtr)context); } +#endif NativeMemory.Free (context); context = null; @@ -438,8 +425,9 @@ static unsafe void BridgeProcessingStarted (MarkCrossReferencesArgs* mcr) throw new ArgumentNullException (nameof (mcr), "MarkCrossReferencesArgs should never be null."); } +#if DEBUG HandleContext.EnsureAllContextsAreOurs (mcr); - bridgeProcessingSemaphore.Wait (); +#endif } [UnmanagedCallersOnly] @@ -452,13 +440,15 @@ static unsafe void BridgeProcessingFinished (MarkCrossReferencesArgs* mcr) ReadOnlySpan handlesToFree = ProcessCollectedContexts (mcr); JavaMarshal.FinishCrossReferenceProcessing (mcr, handlesToFree); - bridgeProcessingSemaphore.Release (); + // Schedule cleanup of _registeredInstances on a thread pool thread. + // The bridge thread must not take lock(_registeredInstances) — see deadlock notes. + Task.Run (Instance.CollectPeers); } static unsafe ReadOnlySpan ProcessCollectedContexts (MarkCrossReferencesArgs* mcr) { List handlesToFree = []; - ManagedValueManager instance = GetOrCreateInstance (); + ManagedValueManager instance = Instance; for (int i = 0; (nuint)i < mcr->ComponentCount; i++) { StronglyConnectedComponent component = mcr->Components [i]; @@ -478,12 +468,12 @@ void ProcessContext (HandleContext* context) return; } - GCHandle handle = HandleContext.GetAssociatedGCHandle (context); + GCHandle handle = context->AssociatedGCHandle; - // Note: modifying the RegisteredInstances dictionary while processing the collected contexts + // Note: modifying the _registeredInstances dictionary while processing the collected contexts // is tricky and can lead to deadlocks, so we remember which contexts were collected and we will free // them later outside of the bridge processing loop. - instance.CollectedContexts.Enqueue ((IntPtr)context); + instance._collectedContexts.Enqueue ((IntPtr)context); // important: we must not free the handle before passing it to JavaMarshal.FinishCrossReferenceProcessing handlesToFree.Add (handle);