diff --git a/external/Java.Interop/src/Java.Interop/Java.Interop/JniPeerMembers.JniInstanceMethods.cs b/external/Java.Interop/src/Java.Interop/Java.Interop/JniPeerMembers.JniInstanceMethods.cs index f9e3092ddfa..af0215acb07 100644 --- a/external/Java.Interop/src/Java.Interop/Java.Interop/JniPeerMembers.JniInstanceMethods.cs +++ b/external/Java.Interop/src/Java.Interop/Java.Interop/JniPeerMembers.JniInstanceMethods.cs @@ -1,7 +1,6 @@ #nullable enable using System; -using System.Collections.Concurrent; namespace Java.Interop { @@ -39,16 +38,16 @@ internal JniType JniPeerType { readonly Type DeclaringType; - ConcurrentDictionary? instanceMethods; - ConcurrentDictionary? subclassConstructors; + JniValueCache? instanceMethods; + JniValueCache? subclassConstructors; - ConcurrentDictionary InstanceMethods => GetOrCreate (ref instanceMethods, 3); - ConcurrentDictionary SubclassConstructors => GetOrCreate (ref subclassConstructors, 1); + JniValueCache InstanceMethods => JniValueCache.GetOrCreate (ref instanceMethods, 1, 3, static value => value.StaticRedirect?.Dispose ()); + JniValueCache SubclassConstructors => JniValueCache.GetOrCreate (ref subclassConstructors, 1, 1, static value => value.Dispose ()); internal void Dispose () { - Clear (ref instanceMethods); - Clear (ref subclassConstructors, static value => value.Dispose ()); + JniValueCache.Dispose (ref instanceMethods); + JniValueCache.Dispose (ref subclassConstructors); if (jniPeerType != null) jniPeerType.Dispose (); @@ -111,15 +110,20 @@ JniMethodInfo GetMethodInfo (ReadOnlySpan method, ReadOnlySpan signa var methodName = newMethod.Value.TargetJniMethodName is string name ? name.AsSpan () : method; var methodSig = newMethod.Value.TargetJniMethodSignature is string sig ? sig.AsSpan () : signature; - using var t = new JniType (typeName); - if (newMethod.Value.TargetJniMethodInstanceToStatic && - t.TryGetStaticMethod (methodName, methodSig, out m)) { - m.ParameterCount = newMethod.Value.TargetJniMethodParameterCount; - m.StaticRedirect = new JniType (typeName); - return m; - } - if (t.TryGetInstanceMethod (methodName, methodSig, out m)) { - return m; + JniType? t = new JniType (typeName); + try { + if (newMethod.Value.TargetJniMethodInstanceToStatic && + t.TryGetStaticMethod (methodName, methodSig, out m)) { + m.ParameterCount = newMethod.Value.TargetJniMethodParameterCount; + m.StaticRedirect = t; + t = null; + return m; + } + if (t.TryGetInstanceMethod (methodName, methodSig, out m)) { + return m; + } + } finally { + t?.Dispose (); } Console.Error.WriteLine ($"warning: For declared method `{Members.JniPeerTypeName}.{method}.{signature}`, could not find requested method `{typeName}.{methodName}.{methodSig}`!"); } diff --git a/external/Java.Interop/src/Java.Interop/Java.Interop/JniPeerMembers.JniStaticMethods.cs b/external/Java.Interop/src/Java.Interop/Java.Interop/JniPeerMembers.JniStaticMethods.cs index a7f8ce9a096..db5d7ff72af 100644 --- a/external/Java.Interop/src/Java.Interop/Java.Interop/JniPeerMembers.JniStaticMethods.cs +++ b/external/Java.Interop/src/Java.Interop/Java.Interop/JniPeerMembers.JniStaticMethods.cs @@ -1,7 +1,6 @@ #nullable enable using System; -using System.Collections.Concurrent; namespace Java.Interop { @@ -15,13 +14,13 @@ internal JniStaticMethods (JniPeerMembers members) internal readonly JniPeerMembers Members; - ConcurrentDictionary? staticMethods; + JniValueCache? staticMethods; - ConcurrentDictionary StaticMethods => GetOrCreate (ref staticMethods, 3); + JniValueCache StaticMethods => JniValueCache.GetOrCreate (ref staticMethods, 1, 3, static value => value.StaticRedirect?.Dispose ()); internal void Dispose () { - Clear (ref staticMethods); + JniValueCache.Dispose (ref staticMethods); } public JniMethodInfo GetMethodInfo (string encodedMember) @@ -38,12 +37,20 @@ JniMethodInfo GetMethodInfo (ReadOnlySpan method, ReadOnlySpan signa var m = (JniMethodInfo?) null; var newMethod = JniEnvironment.Runtime.TypeManager.GetReplacementMethodInfo (Members.JniPeerTypeName, method, signature); if (newMethod.HasValue) { - using var t = new JniType (newMethod.Value.TargetJniType ?? Members.JniPeerTypeName); - if (t.TryGetStaticMethod ( - newMethod.Value.TargetJniMethodName is string name ? name.AsSpan () : method, - newMethod.Value.TargetJniMethodSignature is string sig ? sig.AsSpan () : signature, - out m)) { - return m; + JniType? t = new JniType (newMethod.Value.TargetJniType ?? Members.JniPeerTypeName); + try { + if (t.TryGetStaticMethod ( + newMethod.Value.TargetJniMethodName is string name ? name.AsSpan () : method, + newMethod.Value.TargetJniMethodSignature is string sig ? sig.AsSpan () : signature, + out m)) { + if (!JniEnvironment.Types.IsSameObject (t.PeerReference, Members.JniPeerType.PeerReference)) { + m.StaticRedirect = t; + t = null; + } + return m; + } + } finally { + t?.Dispose (); } } if (Members.JniPeerType.TryGetStaticMethod (method, signature, out m)) { @@ -72,23 +79,28 @@ JniType GetMethodDeclaringType (JniMethodInfo method) if (fallbackTypes == null) { return null; } - foreach (var ft in fallbackTypes) { - JniType? t = null; - try { + JniType? t = null; + try { + JniMethodInfo? m = null; + foreach (var ft in fallbackTypes) { if (!JniType.TryParse (ft, out t)) { continue; } - if (t.TryGetStaticMethod (method, signature, out var m)) { - m.StaticRedirect = t; - t = null; - return m; + if (t.TryGetStaticMethod (method, signature, out m)) { + break; } + t.Dispose (); + t = null; } - finally { - t?.Dispose (); + if (m != null) { + // Transfer ownership only after the fallback enumerator has been disposed. + m.StaticRedirect = t; + t = null; } + return m; + } finally { + t?.Dispose (); } - return null; } public unsafe void InvokeVoidMethod (string encodedMember, JniArgumentValue* parameters) diff --git a/external/Java.Interop/src/Java.Interop/Java.Interop/JniPeerMembers.JniValueCache.cs b/external/Java.Interop/src/Java.Interop/Java.Interop/JniPeerMembers.JniValueCache.cs new file mode 100644 index 00000000000..24cf259db88 --- /dev/null +++ b/external/Java.Interop/src/Java.Interop/Java.Interop/JniPeerMembers.JniValueCache.cs @@ -0,0 +1,92 @@ +#nullable enable + +using System; +using System.Collections; +using System.Collections.Concurrent; +using System.Collections.Generic; +using System.Diagnostics.CodeAnalysis; +using System.Threading; + +namespace Java.Interop { + + partial class JniPeerMembers { + + private sealed class JniValueCache : IDisposable, IReadOnlyDictionary + where TKey : notnull + where TValue : class + { + + readonly ConcurrentDictionary values; + readonly Action dispose; + + public JniValueCache (int concurrencyLevel, int capacity, Action dispose) + { + values = new ConcurrentDictionary (concurrencyLevel, capacity); + this.dispose = dispose; + } + + public int Count => values.Count; + public IEnumerable Keys => values.Keys; + public IEnumerable Values => values.Values; + public TValue this [TKey key] => values [key]; + + public bool ContainsKey (TKey key) => values.ContainsKey (key); + public bool TryGetValue (TKey key, [MaybeNullWhen (false)] out TValue value) => values.TryGetValue (key, out value); + + public IEnumerator> GetEnumerator () => values.GetEnumerator (); + IEnumerator IEnumerable.GetEnumerator () => GetEnumerator (); + + internal static JniValueCache GetOrCreate (ref JniValueCache? cache, int concurrencyLevel, int capacity, Action dispose) + { + var value = Volatile.Read (ref cache); + if (value != null) + return value; + + var candidate = new JniValueCache (concurrencyLevel, capacity, dispose); + var existing = Interlocked.CompareExchange (ref cache, candidate, null); + if (existing == null) + return candidate; + + candidate.Dispose (); + return existing; + } + + internal static void Dispose (ref JniValueCache? cache) + { + Interlocked.Exchange (ref cache, null)?.Dispose (); + } + + public TValue GetOrAdd (TKey key, Func factory) + { + return GetOrAdd (key, static (key, factory) => factory (key), factory); + } + + public TValue GetOrAdd (TKey key, Func factory, TArg argument) + { + if (values.TryGetValue (key, out var value)) + return value; + + // ConcurrentDictionary may invoke a GetOrAdd factory multiple times and discard + // losing values. Construct explicitly so an unpublished owner can be disposed. + // JNI lookup can also reenter this cache, so do not lock construction. + TValue? candidate = factory (key, argument); + try { + value = values.GetOrAdd (key, candidate); + if (ReferenceEquals (value, candidate)) + candidate = null; + return value; + } finally { + if (candidate != null) + dispose (candidate); + } + } + + public void Dispose () + { + foreach (var value in values.Values) + dispose (value); + values.Clear (); + } + } + } +} diff --git a/external/Java.Interop/src/Java.Interop/Java.Interop/JniPeerMembers.cs b/external/Java.Interop/src/Java.Interop/Java.Interop/JniPeerMembers.cs index 1b2242181d2..3cb5818202f 100644 --- a/external/Java.Interop/src/Java.Interop/Java.Interop/JniPeerMembers.cs +++ b/external/Java.Interop/src/Java.Interop/Java.Interop/JniPeerMembers.cs @@ -136,14 +136,14 @@ static void Clear (ref ConcurrentDictionary? diction protected virtual void Dispose (bool disposing) { - if (!disposing || jniPeerType == null) + if (!disposing) return; instanceMethods.Dispose (); instanceFields.Dispose (); staticMethods.Dispose (); staticFields.Dispose (); - jniPeerType.Dispose (); + jniPeerType?.Dispose (); jniPeerType = null; } diff --git a/external/Java.Interop/src/Java.Interop/Java.Interop/JniType.cs b/external/Java.Interop/src/Java.Interop/Java.Interop/JniType.cs index d23487846e3..27e7733940a 100644 --- a/external/Java.Interop/src/Java.Interop/Java.Interop/JniType.cs +++ b/external/Java.Interop/src/Java.Interop/Java.Interop/JniType.cs @@ -5,7 +5,6 @@ using System.Diagnostics; using System.Diagnostics.CodeAnalysis; using System.Collections.Generic; -using System.Linq; using System.Reflection; using System.Runtime.CompilerServices; using System.Runtime.InteropServices; @@ -122,6 +121,12 @@ public void Dispose () JniObjectReference.Dispose (ref peerReference); } + internal void DisposeUnlessRegisteredWithRuntime () + { + if (!registered) + Dispose (); + } + public JniType? GetSuperclass () { AssertValid (); @@ -151,11 +156,25 @@ public bool IsInstanceOfType (JniObjectReference value) return JniEnvironment.Types.IsInstanceOf (value, PeerReference); } -#pragma warning disable 0414 - // This isn't used anywhere; it's just present so that the GC won't collect the referenced delegates. - JniNativeMethodRegistration[]? methods; -#pragma warning restore 0414 + object? nativeMethodsLock; + // Retains delegates from every batch JNI may have partially registered. + List? methods; + + object GetNativeMethodsLock () + { + var value = Volatile.Read (ref nativeMethodsLock); + if (value != null) + return value; + + var candidate = new object (); + return Interlocked.CompareExchange (ref nativeMethodsLock, candidate, null) ?? candidate; + } + /// + /// Once a non-empty registration is requested, the runtime retains this type and its + /// delegates until unregistration or disposal, even if registration throws: JNI may + /// have registered part of the batch. + /// [RequiresDynamicCode ("Native method registration via JniNativeMethodRegistration[] requires dynamic code generation. Use the blittable RegisterNatives(JniObjectReference, ReadOnlySpan) overload with statically-compiled function pointers for Native AOT compatibility.")] public void RegisterNativeMethods (params JniNativeMethodRegistration[] methods) { @@ -163,18 +182,29 @@ public void RegisterNativeMethods (params JniNativeMethodRegistration[] methods) if (methods == null) throw new ArgumentNullException (nameof (methods)); + if (methods.Length == 0) + return; - JniEnvironment.Types.RegisterNatives (PeerReference, methods, checked ((int)methods.Length)); - // Prevents method delegates from being GC'd so long as this type remains - this.methods = methods; - RegisterWithRuntime (); + lock (GetNativeMethodsLock ()) { + // Retain each batch before calling RegisterNatives: JNI stores only the + // unmanaged function pointers and may publish part of a batch before throwing. + // Storing it afterward could therefore leave callable pointers to collected + // delegates. + this.methods ??= new List (); + this.methods.Add (methods); + RegisterWithRuntime (); + JniEnvironment.Types.RegisterNatives (PeerReference, methods, methods.Length); + } } public void UnregisterNativeMethods () { AssertValid (); - JniEnvironment.Types.UnregisterNatives (PeerReference); + lock (GetNativeMethodsLock ()) { + JniEnvironment.Types.UnregisterNatives (PeerReference); + methods = null; + } } public JniMethodInfo GetConstructor (string signature) diff --git a/external/Java.Interop/src/Java.Interop/Java.Interop/ManagedPeer.cs b/external/Java.Interop/src/Java.Interop/Java.Interop/ManagedPeer.cs index d026789a180..4ae7a346de0 100644 --- a/external/Java.Interop/src/Java.Interop/Java.Interop/ManagedPeer.cs +++ b/external/Java.Interop/src/Java.Interop/Java.Interop/ManagedPeer.cs @@ -282,30 +282,32 @@ static unsafe void RegisterNativeMembers ( try { var r_nativeClass = new JniObjectReference (n_nativeClass); -#pragma warning disable CA2000 +#pragma warning disable CA2000 // Disposed below unless native registration transfers ownership to the runtime. var nativeClass = new JniType (ref r_nativeClass, JniObjectReferenceOptions.Copy); #pragma warning restore CA2000 + try { + var methodsRef = new JniObjectReference (n_methods); - var methodsRef = new JniObjectReference (n_methods); - - var typeSig = new JniTypeSignature (nativeClass.Name); - var type = GetTypeFromSignature (JniEnvironment.Runtime.TypeManager, typeSig); + var typeSig = new JniTypeSignature (nativeClass.Name); + var type = GetTypeFromSignature (JniEnvironment.Runtime.TypeManager, typeSig); - int methodsLength = JniEnvironment.Strings.GetStringLength (methodsRef); - var methodsChars = JniEnvironment.Strings.GetStringChars (methodsRef, null); - var methods = new ReadOnlySpan(methodsChars, methodsLength); - try { - JniEnvironment.Runtime.TypeManager.RegisterNativeMembers (nativeClass, type, methods); - } - catch (Exception e) { - throw new NotSupportedException ( - $"Unable to register native members for Java type `{nativeClass.Name}` <=> managed type `{type?.AssemblyQualifiedName}`.", - e); - } - finally { - JniEnvironment.Strings.ReleaseStringChars (methodsRef, methodsChars); + int methodsLength = JniEnvironment.Strings.GetStringLength (methodsRef); + var methodsChars = JniEnvironment.Strings.GetStringChars (methodsRef, null); + var methods = new ReadOnlySpan(methodsChars, methodsLength); + try { + JniEnvironment.Runtime.TypeManager.RegisterNativeMembers (nativeClass, type, methods); + } + catch (Exception e) { + throw new NotSupportedException ( + $"Unable to register native members for Java type `{nativeClass.Name}` <=> managed type `{type?.AssemblyQualifiedName}`.", + e); + } + finally { + JniEnvironment.Strings.ReleaseStringChars (methodsRef, methodsChars); + } + } finally { + nativeClass.DisposeUnlessRegisteredWithRuntime (); } - } catch (Exception e) { __r?.OnUserUnhandledException (ref envp, e); diff --git a/external/Java.Interop/tests/Java.Interop-Tests/Java.Interop-Tests.csproj b/external/Java.Interop/tests/Java.Interop-Tests/Java.Interop-Tests.csproj index 1d43a2ca427..b046594a5f5 100644 --- a/external/Java.Interop/tests/Java.Interop-Tests/Java.Interop-Tests.csproj +++ b/external/Java.Interop/tests/Java.Interop-Tests/Java.Interop-Tests.csproj @@ -39,6 +39,7 @@ + diff --git a/external/Java.Interop/tests/Java.Interop-Tests/Java.Interop/JavaVMFixture.cs b/external/Java.Interop/tests/Java.Interop-Tests/Java.Interop/JavaVMFixture.cs index 99004f98c2b..00dc145951f 100644 --- a/external/Java.Interop/tests/Java.Interop-Tests/Java.Interop/JavaVMFixture.cs +++ b/external/Java.Interop/tests/Java.Interop-Tests/Java.Interop/JavaVMFixture.cs @@ -126,6 +126,7 @@ IEnumerable CreateSimpleReferencesEnumerator (Type type) [("java/lang/Object", "remappedToToString", "()Ljava/lang/String;")] = (null, "toString", null, null, false), [("java/lang/Object", "remappedToStaticHashCode", null)] = ("net/dot/jni/test/ObjectHelper", "getHashCodeHelper", null, null, true), [("java/lang/Runtime", "remappedToGetRuntime", null)] = (null, "getRuntime", null, null, false), + [("java/lang/Runtime", "remappedToCurrentTimeMillis", "()J")] = ("java/lang/System", "currentTimeMillis", null, null, false), // NOTE: key must use *post-renamed* value, not pre-renamed value // NOTE: SourceSignature lacking return type; "closer in spirit" to what `remapping-config.json` allows diff --git a/external/Java.Interop/tests/Java.Interop-Tests/Java.Interop/JniPeerMembersDisposalTests.cs b/external/Java.Interop/tests/Java.Interop-Tests/Java.Interop/JniPeerMembersDisposalTests.cs new file mode 100644 index 00000000000..ee051661c73 --- /dev/null +++ b/external/Java.Interop/tests/Java.Interop-Tests/Java.Interop/JniPeerMembersDisposalTests.cs @@ -0,0 +1,110 @@ +using System; +using System.Collections.Generic; +using System.Reflection; + +using Java.Interop; +using NUnit.Framework; + +namespace Java.InteropTests +{ + [TestFixture] + public class JniPeerMembersDisposalTests : JavaVMFixture + { + const BindingFlags PrivateInstance = BindingFlags.NonPublic | BindingFlags.Instance; + + [TestCase (false)] + [TestCase (true)] + public unsafe void Dispose_ReleasesSubclassCache (bool initializeOwner) + { + var members = new JniPeerMembers (CallNonvirtualBase.JniTypeName, typeof (CallNonvirtualBase)); + try { + for (int cycle = 0; cycle < 3; ++cycle) { + using var ownerType = initializeOwner ? members.JniPeerType : null; + var peer = members.InstanceMethods.StartCreateInstance ("()V", typeof (CallNonvirtualDerived), null); + try { + Assert.IsTrue (peer.IsValid); + } finally { + JniObjectReference.Dispose (ref peer); + } + + var constructors = members.InstanceMethods.GetConstructorsForType (typeof (CallNonvirtualDerived)); + using var subclassType = constructors.JniPeerType; + Assert.IsTrue (subclassType.PeerReference.IsValid); + Assert.AreEqual (JniObjectReferenceType.Global, subclassType.PeerReference.Type); + Assert.IsTrue (IsTracked (subclassType)); + Assert.AreSame (ownerType, GetOwnerType (members)); + + JniPeerMembers.Dispose (members); + + Assert.IsFalse (subclassType.PeerReference.IsValid, "Subclass class reference must be released before runtime shutdown."); + Assert.IsFalse (IsTracked (subclassType)); + if (ownerType != null) { + Assert.IsFalse (ownerType.PeerReference.IsValid); + Assert.IsFalse (IsTracked (ownerType)); + } + Assert.Throws (() => { + var type = constructors.JniPeerType; + }); + AssertUninitialized (members); + + JniPeerMembers.Dispose (members); + AssertUninitialized (members); + } + } finally { + JniPeerMembers.Dispose (members); + } + } + + [Test] + public void Dispose_UninitializedMembers_RemainsLazy () + { + var members = new JniPeerMembers (JavaObjectWithMissingJavaPeer.JniTypeName, typeof (JavaObjectWithMissingJavaPeer)); + try { + AssertUninitialized (members); + for (int i = 0; i < 3; ++i) { + // Resolving the owning class here would throw because it does not exist. + JniPeerMembers.Dispose (members); + AssertUninitialized (members); + } + } finally { + JniPeerMembers.Dispose (members); + } + } + + static JniType GetOwnerType (JniPeerMembers members) + { + return (JniType) GetFieldValue (typeof (JniPeerMembers).GetField ("jniPeerType", PrivateInstance), members); + } + + static bool IsTracked (JniType type) + { + var tracked = (Dictionary) GetFieldValue ( + typeof (JniRuntime).GetField ("TrackedInstances", PrivateInstance), JniEnvironment.Runtime); + lock (tracked) { + return tracked.ContainsValue (type); + } + } + + static void AssertUninitialized (JniPeerMembers members) + { + Assert.IsNull (GetOwnerType (members)); + Assert.IsNull (GetFieldValue ( + typeof (JniPeerMembers.JniInstanceMethods).GetField ("instanceMethods", PrivateInstance), members.InstanceMethods)); + Assert.IsNull (GetFieldValue ( + typeof (JniPeerMembers.JniInstanceMethods).GetField ("subclassConstructors", PrivateInstance), members.InstanceMethods)); + Assert.IsNull (GetFieldValue ( + typeof (JniPeerMembers.JniInstanceFields).GetField ("instanceFields", PrivateInstance), members.InstanceFields)); + Assert.IsNull (GetFieldValue ( + typeof (JniPeerMembers.JniStaticMethods).GetField ("staticMethods", PrivateInstance), members.StaticMethods)); + Assert.IsNull (GetFieldValue ( + typeof (JniPeerMembers.JniStaticFields).GetField ("staticFields", PrivateInstance), members.StaticFields)); + } + + static object GetFieldValue (FieldInfo field, object owner) + { + if (field == null) + throw new InvalidOperationException ("Expected private cache field was not found."); + return field.GetValue (owner); + } + } +} diff --git a/external/Java.Interop/tests/Java.Interop-Tests/Java.Interop/JniPeerMembersTests.cs b/external/Java.Interop/tests/Java.Interop-Tests/Java.Interop/JniPeerMembersTests.cs index 51a11362b45..7ffbb724846 100644 --- a/external/Java.Interop/tests/Java.Interop-Tests/Java.Interop/JniPeerMembersTests.cs +++ b/external/Java.Interop/tests/Java.Interop-Tests/Java.Interop/JniPeerMembersTests.cs @@ -1,5 +1,5 @@ using System; -using System.Collections.Concurrent; +using System.Collections.Generic; using System.Reflection; using System.Threading.Tasks; @@ -122,7 +122,7 @@ public void ConcurrentFirstUsePublishesSingleFieldAndStaticMethodCaches () } } - static void AssertSingleCachedValue (ConcurrentDictionary cache, string key, T [] values) + static void AssertSingleCachedValue (IReadOnlyDictionary cache, string key, T [] values) where T : class { Assert.AreEqual (1, cache.Count); @@ -131,39 +131,39 @@ static void AssertSingleCachedValue (ConcurrentDictionary cache, s Assert.AreSame (cache [key], values [0]); } - static ConcurrentDictionary GetInstanceFields (JniPeerMembers.JniInstanceFields fields) + static IReadOnlyDictionary GetInstanceFields (JniPeerMembers.JniInstanceFields fields) { var field = typeof (JniPeerMembers.JniInstanceFields).GetField ("instanceFields", BindingFlags.NonPublic | BindingFlags.Instance); return GetCache (field, fields); } - static ConcurrentDictionary GetInstanceMethods (JniPeerMembers.JniInstanceMethods methods) + static IReadOnlyDictionary GetInstanceMethods (JniPeerMembers.JniInstanceMethods methods) { var field = typeof (JniPeerMembers.JniInstanceMethods).GetField ("instanceMethods", BindingFlags.NonPublic | BindingFlags.Instance); return GetCache (field, methods); } - static ConcurrentDictionary GetSubclassConstructors (JniPeerMembers.JniInstanceMethods methods) + static IReadOnlyDictionary GetSubclassConstructors (JniPeerMembers.JniInstanceMethods methods) { var field = typeof (JniPeerMembers.JniInstanceMethods).GetField ("subclassConstructors", BindingFlags.NonPublic | BindingFlags.Instance); return GetCache (field, methods); } - static ConcurrentDictionary GetStaticFields (JniPeerMembers.JniStaticFields fields) + static IReadOnlyDictionary GetStaticFields (JniPeerMembers.JniStaticFields fields) { var field = typeof (JniPeerMembers.JniStaticFields).GetField ("staticFields", BindingFlags.NonPublic | BindingFlags.Instance); return GetCache (field, fields); } - static ConcurrentDictionary GetStaticMethods (JniPeerMembers.JniStaticMethods methods) + static IReadOnlyDictionary GetStaticMethods (JniPeerMembers.JniStaticMethods methods) { var field = typeof (JniPeerMembers.JniStaticMethods).GetField ("staticMethods", BindingFlags.NonPublic | BindingFlags.Instance); return GetCache (field, methods); } - static ConcurrentDictionary GetCache (FieldInfo field, object owner) + static IReadOnlyDictionary GetCache (FieldInfo field, object owner) { - return (ConcurrentDictionary) field.GetValue (owner); + return (IReadOnlyDictionary) field.GetValue (owner); } [Test] diff --git a/external/Java.Interop/tests/Java.Interop-Tests/Java.Interop/JniRedirectCacheOwnershipTests.cs b/external/Java.Interop/tests/Java.Interop-Tests/Java.Interop/JniRedirectCacheOwnershipTests.cs new file mode 100644 index 00000000000..f19136efe99 --- /dev/null +++ b/external/Java.Interop/tests/Java.Interop-Tests/Java.Interop/JniRedirectCacheOwnershipTests.cs @@ -0,0 +1,52 @@ +using Java.Interop; +using NUnit.Framework; + +namespace Java.InteropTests +{ + [TestFixture] + [Category ("NativeAOTIgnore")] + public class JniRedirectCacheOwnershipTests : JavaVMFixture + { + [TestCase (false)] + [TestCase (true)] + public void DisposingPeerMembersReleasesRedirect (bool isStatic) + { + var members = isStatic + ? new JniPeerMembers (JavaLangRemappingTestRuntime.JniTypeName, typeof (JavaLangRemappingTestRuntime)) + : new JniPeerMembers (JavaLangRemappingTestObject.JniTypeName, typeof (JavaLangRemappingTestObject)); + JniType redirect = null; + try { + var method = GetRedirectedMethod (members, isStatic); + redirect = method.StaticRedirect; + Assert.IsNotNull (redirect); + Assert.IsTrue (redirect.PeerReference.IsValid); + Assert.AreSame (method, GetRedirectedMethod (members, isStatic)); + AssertRedirectIsCallable (members, isStatic); + + JniPeerMembers.Dispose (members); + Assert.IsFalse (redirect.PeerReference.IsValid, "Disposing the peer members must release the redirect's global reference."); + } finally { + JniPeerMembers.Dispose (members); + redirect?.Dispose (); + } + } + + static unsafe void AssertRedirectIsCallable (JniPeerMembers members, bool isStatic) + { + if (isStatic) { + Assert.Greater (members.StaticMethods.InvokeInt64Method ("remappedToCurrentTimeMillis.()J", null), 0); + } else { + using var value = new JavaLangRemappingTestObject (); + Assert.AreEqual (value.GetHashCode (), members.InstanceMethods.InvokeNonvirtualInt32Method ("remappedToStaticHashCode.()I", value, null)); + } + } + + static JniMethodInfo GetRedirectedMethod (JniPeerMembers members, bool isStatic) + { + return isStatic + ? members.StaticMethods.GetMethodInfo ("remappedToCurrentTimeMillis.()J") + : members.InstanceMethods.GetMethodInfo ("remappedToStaticHashCode.()I"); + } + + } +} diff --git a/external/Java.Interop/tests/Java.Interop-Tests/Java.Interop/JniSubclassConstructorCacheTests.cs b/external/Java.Interop/tests/Java.Interop-Tests/Java.Interop/JniSubclassConstructorCacheTests.cs new file mode 100644 index 00000000000..b9e3a55f1e8 --- /dev/null +++ b/external/Java.Interop/tests/Java.Interop-Tests/Java.Interop/JniSubclassConstructorCacheTests.cs @@ -0,0 +1,221 @@ +#if !__ANDROID__ +using System; +using System.Collections.Concurrent; +using System.Collections.Generic; +using System.Linq; +using System.Reflection; +using System.Threading; +using System.Threading.Tasks; + +using Java.Interop; +using NUnit.Framework; + +namespace Java.InteropTests +{ + [TestFixture] + [NonParallelizable] + public class JniSubclassConstructorCacheTests : JavaVMFixture + { + [Test] + public void ConcurrentCreationDisposesUnpublishedConstructors () + { + RunWithReferenceTracking ((members, references, runtime) => { + const int count = 4; + using var barrier = new Barrier (count); + references.OnCreate = () => Assert.IsTrue (barrier.SignalAndWait (TimeSpan.FromSeconds (30)), "Constructor creation did not overlap."); + + var calls = Enumerable.Range (0, count) + .Select (_ => Task.Factory.StartNew ( + () => { + runtime.AttachCurrentThread (); + Assert.AreSame (runtime, JniEnvironment.Runtime); + return members.InstanceMethods.GetConstructorsForType (typeof (MyString)); + }, + CancellationToken.None, TaskCreationOptions.LongRunning, TaskScheduler.Default)) + .ToArray (); + var constructors = Task.WhenAll (calls).GetAwaiter ().GetResult (); + references.OnCreate = null; + + foreach (var constructor in constructors) + Assert.AreSame (constructors [0], constructor); + AssertWinnerAndCleanup (members, references, runtime, constructors [0], count); + }); + } + + [Test] + public void RecursiveCreationDisposesOuterConstructor () + { + RunWithReferenceTracking ((members, references, runtime) => { + JniPeerMembers.JniInstanceMethods recursive = null; + references.OnCreate = () => { + references.OnCreate = null; + recursive = members.InstanceMethods.GetConstructorsForType (typeof (MyString)); + }; + + var constructor = members.InstanceMethods.GetConstructorsForType (typeof (MyString)); + + Assert.AreSame (recursive, constructor, "The recursive lookup should publish the winner."); + AssertWinnerAndCleanup (members, references, runtime, constructor, 2); + }); + } + + static unsafe void AssertWinnerAndCleanup (JniPeerMembers members, TrackingReferenceManager references, JniRuntime runtime, JniPeerMembers.JniInstanceMethods winner, int created) + { + var type = winner.JniPeerType; + var handle = type.PeerReference.Handle; + Assert.AreSame (winner, members.InstanceMethods.GetConstructorsForType (typeof (MyString))); + Assert.IsTrue (winner.GetConstructor ("()V").IsValid); + + var instance = members.InstanceMethods.NewObject ("()V", typeof (MyString), null); + try { + Assert.IsTrue (type.IsInstanceOfType (instance), "The winning constructor must remain usable."); + } finally { + JniObjectReference.Dispose (ref instance); + } + + Assert.Multiple (() => { + Assert.AreEqual (created, references.Created.Count, "Cache hits must not create more class globals."); + CollectionAssert.AreEquivalent (new [] { handle }, references.Created.Where (references.IsLive).Distinct ()); + var tracked = GetTrackedInstances (runtime); + lock (tracked) { + CollectionAssert.AreEquivalent (new [] { handle }, references.Created.Where (tracked.ContainsKey).Distinct ()); + Assert.AreSame (type, tracked [handle], "The winner must stay registered until disposal."); + } + + JniPeerMembers.Dispose (members); + Assert.IsFalse (type.PeerReference.IsValid); + AssertReleased (references, runtime); + }); + } + + static void AssertReleased (TrackingReferenceManager references, JniRuntime runtime) + { + Assert.IsFalse (references.Created.Any (references.IsLive), "Unpublished constructor class globals must be deleted."); + var tracked = GetTrackedInstances (runtime); + lock (tracked) + Assert.IsFalse (references.Created.Any (tracked.ContainsKey), "Constructor classes must not be retained until runtime shutdown."); + } + + static Dictionary GetTrackedInstances (JniRuntime runtime) + { + var field = typeof (JniRuntime).GetField ("TrackedInstances", BindingFlags.NonPublic | BindingFlags.Instance); + Assert.IsNotNull (field); + return (Dictionary) field.GetValue (runtime); + } + + static void RunWithReferenceTracking (Action test) + { + var runtime = JniEnvironment.Runtime; + var original = runtime.ObjectReferenceManager; + var references = new TrackingReferenceManager (original); + references.OnSetRuntime (runtime); + var property = typeof (JniRuntime).GetProperty (nameof (JniRuntime.ObjectReferenceManager)); + Assert.IsNotNull (property); + var members = new JniPeerMembers ("java/lang/Object", typeof (JavaObject)); + try { + // Exclude class initialization and the parent's separately-owned class global. + Assert.IsTrue (members.JniPeerType.PeerReference.IsValid); + using (var type = new JniType (MyString.JniTypeName)) { + var instance = type.AllocObject (); + JniObjectReference.Dispose (ref instance); + } + // Decorate the fixture's manager without creating another runtime or changing JNI environments. + property.SetValue (runtime, references); + references.TrackCreation = true; + test (members, references, runtime); + } finally { + references.TrackCreation = false; + references.OnCreate = null; + try { + JniPeerMembers.Dispose (members); + // Also clean up leaked candidates when running this regression against broken code. + var tracked = GetTrackedInstances (runtime); + List remaining; + lock (tracked) + remaining = references.LiveHandles.Where (tracked.ContainsKey).Select (handle => tracked [handle]).ToList (); + foreach (var value in remaining) + value.Dispose (); + } finally { + property.SetValue (runtime, original); + } + } + } + + sealed class TrackingReferenceManager : JniRuntime.JniObjectReferenceManager + { + readonly JniRuntime.JniObjectReferenceManager inner; + readonly ConcurrentDictionary live = new ConcurrentDictionary (); + readonly AsyncLocal trackCreation = new AsyncLocal (); + + public readonly ConcurrentBag Created = new ConcurrentBag (); + public Action OnCreate; + + public TrackingReferenceManager (JniRuntime.JniObjectReferenceManager inner) + { + this.inner = inner; + } + + public bool TrackCreation { + get => trackCreation.Value; + set => trackCreation.Value = value; + } + + public ICollection LiveHandles => live.Keys; + public override int GlobalReferenceCount => inner.GlobalReferenceCount; + public override int WeakGlobalReferenceCount => inner.WeakGlobalReferenceCount; + public override bool LogLocalReferenceMessages => inner.LogLocalReferenceMessages; + public override bool LogGlobalReferenceMessages => inner.LogGlobalReferenceMessages; + + public bool IsLive (IntPtr handle) => live.ContainsKey (handle); + + public override JniObjectReference CreateGlobalReference (JniObjectReference reference) + { + var result = inner.CreateGlobalReference (reference); + if (TrackCreation) { + live.TryAdd (result.Handle, 0); + Created.Add (result.Handle); + try { + OnCreate?.Invoke (); + } catch { + DeleteGlobalReference (ref result); + throw; + } + } + return result; + } + + public override void DeleteGlobalReference (ref JniObjectReference reference) + { + var handle = reference.Handle; + inner.DeleteGlobalReference (ref reference); + live.TryRemove (handle, out _); + } + + public override JniObjectReference CreateLocalReference (JniObjectReference reference, ref int localReferenceCount) => + inner.CreateLocalReference (reference, ref localReferenceCount); + + public override void DeleteLocalReference (ref JniObjectReference reference, ref int localReferenceCount) => + inner.DeleteLocalReference (ref reference, ref localReferenceCount); + + public override void CreatedLocalReference (JniObjectReference reference, ref int localReferenceCount) => + inner.CreatedLocalReference (reference, ref localReferenceCount); + + public override IntPtr ReleaseLocalReference (ref JniObjectReference reference, ref int localReferenceCount) => + inner.ReleaseLocalReference (ref reference, ref localReferenceCount); + + public override JniObjectReference CreateWeakGlobalReference (JniObjectReference reference) => + inner.CreateWeakGlobalReference (reference); + + public override void DeleteWeakGlobalReference (ref JniObjectReference reference) => + inner.DeleteWeakGlobalReference (ref reference); + + public override void WriteLocalReferenceLine (string format, params object [] args) => + inner.WriteLocalReferenceLine (format, args); + + public override void WriteGlobalReferenceLine (string format, params object [] args) => + inner.WriteGlobalReferenceLine (format, args); + } + + } +} +#endif diff --git a/external/Java.Interop/tests/Java.Interop-Tests/Java.Interop/ManagedPeerRegistrationTests.cs b/external/Java.Interop/tests/Java.Interop-Tests/Java.Interop/ManagedPeerRegistrationTests.cs new file mode 100644 index 00000000000..0a87d17dd1e --- /dev/null +++ b/external/Java.Interop/tests/Java.Interop-Tests/Java.Interop/ManagedPeerRegistrationTests.cs @@ -0,0 +1,166 @@ +#nullable enable +#if !__ANDROID__ + +using System; +using System.Diagnostics.CodeAnalysis; +using System.Runtime.CompilerServices; +using System.Runtime.InteropServices; +using System.Threading.Tasks; + +using Java.Interop; + +using NUnit.Framework; + +namespace Java.InteropTests { + + [TestFixture] + [NonParallelizable] + [UnconditionalSuppressMessage ("AOT", "IL3050", Justification = "Tests exercise standalone delegate-based native registration.")] + public class ManagedPeerRegistrationTests : JavaVMFixture { + + const string JniTypeName = "net/dot/jni/test/ManagedPeerRegistration"; + + [UnmanagedFunctionPointer (CallingConvention.Winapi)] + delegate int GetValue (IntPtr env, IntPtr klass); + + sealed class NativeTarget { + public int Value (IntPtr env, IntPtr klass) => 42; + } + + [Test] + public void EmptyRegistration_AllowsNonEmptyRegistration () + { + using var owner = new JniType (JniTypeName); + owner.RegisterNativeMethods (); + owner.RegisterNativeMethods (new JniNativeMethodRegistration ("value", "()I", new GetValue (static (env, klass) => 42))); + Assert.AreEqual (42, Call (owner, "value")); + } + + [TestCase (false)] + [TestCase (true)] + public void RegistrationAttempt_RetainsOwnerAndDelegate (bool fail) + { + var retained = RegisterAndReleaseOwner (fail); + Collect (); + Assert.IsTrue (retained.Owner.TryGetTarget (out var owner), "The runtime must retain the registration owner."); + Assert.IsTrue (retained.Target.IsAlive, "The runtime must retain the registered delegate."); + using var retainedOwner = owner ?? throw new InvalidOperationException ("The registration owner was collected."); + Assert.IsTrue (retainedOwner.PeerReference.IsValid); + Assert.AreEqual (42, Call (retainedOwner, "value")); + } + + [MethodImpl (MethodImplOptions.NoInlining)] + static (WeakReference Owner, WeakReference Target) RegisterAndReleaseOwner (bool fail) + { + var owner = new JniType (JniTypeName); + var target = new NativeTarget (); + if (fail) { + using var error = Assert.Throws (() => owner.RegisterNativeMethods ( + new JniNativeMethodRegistration ("value", "()I", new GetValue (target.Value)), + new JniNativeMethodRegistration ("missing", "()I", new GetValue (target.Value)))); + } else { + owner.RegisterNativeMethods (new JniNativeMethodRegistration ("value", "()I", new GetValue (target.Value))); + } + return (new WeakReference (owner), new WeakReference (target)); + } + + [TestCase (false)] + [TestCase (true)] + public void RepeatedRegistration_RetainsEveryDelegateBatch (bool firstAttemptFails) + { + var retained = RegisterRepeatedly (firstAttemptFails); + Collect (); + Assert.IsTrue (retained.First.IsAlive, "The first registration delegate must remain retained."); + Assert.IsTrue (retained.Second.IsAlive, "The second registration delegate must remain retained."); + using var owner = retained.Owner; + Assert.AreEqual (42, Call (owner, "value")); + Assert.AreEqual (42, Call (owner, "existing")); + } + + [MethodImpl (MethodImplOptions.NoInlining)] + static (JniType Owner, WeakReference First, WeakReference Second) RegisterRepeatedly (bool firstAttemptFails) + { + var owner = new JniType (JniTypeName); + var first = new NativeTarget (); + var second = new NativeTarget (); + if (firstAttemptFails) { + using var error = Assert.Throws (() => owner.RegisterNativeMethods ( + new JniNativeMethodRegistration ("value", "()I", new GetValue (first.Value)), + new JniNativeMethodRegistration ("missing", "()I", new GetValue (first.Value)))); + } else { + owner.RegisterNativeMethods (new JniNativeMethodRegistration ("value", "()I", new GetValue (first.Value))); + } + owner.RegisterNativeMethods (new JniNativeMethodRegistration ("existing", "()I", new GetValue (second.Value))); + return (owner, new WeakReference (first), new WeakReference (second)); + } + + [Test] + public void ConcurrentRegistration_SerializesAndRetainsEveryDelegateBatch () + { + var retained = RegisterConcurrently (); + Collect (); + Assert.IsTrue (retained.First.IsAlive, "The first registration delegate must remain retained."); + Assert.IsTrue (retained.Second.IsAlive, "The second registration delegate must remain retained."); + using var owner = retained.Owner; + Assert.AreEqual (42, Call (owner, "value")); + Assert.AreEqual (42, Call (owner, "existing")); + } + + [MethodImpl (MethodImplOptions.NoInlining)] + static (JniType Owner, WeakReference First, WeakReference Second) RegisterConcurrently () + { + var owner = new JniType (JniTypeName); + var first = new NativeTarget (); + var second = new NativeTarget (); + Parallel.Invoke ( + () => owner.RegisterNativeMethods (new JniNativeMethodRegistration ("value", "()I", new GetValue (first.Value))), + () => owner.RegisterNativeMethods (new JniNativeMethodRegistration ("existing", "()I", new GetValue (second.Value)))); + return (owner, new WeakReference (first), new WeakReference (second)); + } + + [Test] + public void Unregister_AllowsRegistrationAgain () + { + using var owner = new JniType (JniTypeName); + owner.RegisterNativeMethods (new JniNativeMethodRegistration ("value", "()I", new GetValue (static (env, klass) => 41))); + Assert.AreEqual (41, Call (owner, "value")); + + owner.UnregisterNativeMethods (); + owner.RegisterNativeMethods (new JniNativeMethodRegistration ("value", "()I", new GetValue (static (env, klass) => 42))); + Assert.AreEqual (42, Call (owner, "value")); + } + + [Test] + public void Unregister_ReleasesRegisteredDelegates () + { + var retained = RegisterThenUnregister (); + Collect (); + Assert.IsFalse (retained.Target.IsAlive, "Unregistering must release retained delegates."); + retained.Owner.Dispose (); + } + + [MethodImpl (MethodImplOptions.NoInlining)] + static (JniType Owner, WeakReference Target) RegisterThenUnregister () + { + var owner = new JniType (JniTypeName); + var target = new NativeTarget (); + owner.RegisterNativeMethods (new JniNativeMethodRegistration ("value", "()I", new GetValue (target.Value))); + owner.UnregisterNativeMethods (); + return (owner, new WeakReference (target)); + } + + static int Call (JniType owner, string name) + { + var method = owner.GetStaticMethod (name, "()I"); + return JniEnvironment.StaticMethods.CallStaticIntMethod (owner.PeerReference, method); + } + + static void Collect () + { + GC.Collect (); + GC.WaitForPendingFinalizers (); + GC.Collect (); + } + } +} +#endif // !__ANDROID__ diff --git a/external/Java.Interop/tests/Java.Interop-Tests/java/net/dot/jni/test/ManagedPeerRegistration.java b/external/Java.Interop/tests/Java.Interop-Tests/java/net/dot/jni/test/ManagedPeerRegistration.java new file mode 100644 index 00000000000..4e19abf85c8 --- /dev/null +++ b/external/Java.Interop/tests/Java.Interop-Tests/java/net/dot/jni/test/ManagedPeerRegistration.java @@ -0,0 +1,6 @@ +package net.dot.jni.test; + +public class ManagedPeerRegistration { + public static native int existing (); + public static native int value (); +} diff --git a/external/Java.Interop/tests/Xamarin.Android.Tools.Bytecode-Tests/ExpectedTypeDeclaration.cs b/external/Java.Interop/tests/Xamarin.Android.Tools.Bytecode-Tests/ExpectedTypeDeclaration.cs index d45cf6d4bf9..b8f51492712 100644 --- a/external/Java.Interop/tests/Xamarin.Android.Tools.Bytecode-Tests/ExpectedTypeDeclaration.cs +++ b/external/Java.Interop/tests/Xamarin.Android.Tools.Bytecode-Tests/ExpectedTypeDeclaration.cs @@ -11,7 +11,7 @@ class ExpectedTypeDeclaration { public ushort MajorVersion; public ushort MinorVersion; - public int ConstantPoolCount; + public int? ConstantPoolCount; public ClassAccessFlags AccessFlags; public string FullName; public TypeInfo Superclass; @@ -26,7 +26,8 @@ public void Assert (ClassFile classDeclaration) { NAssert.AreEqual (MajorVersion, classDeclaration.MajorVersion, FullName + " Major Version"); NAssert.AreEqual (MinorVersion, classDeclaration.MinorVersion, FullName + " Minor Version"); - NAssert.AreEqual (ConstantPoolCount, classDeclaration.ConstantPool.Count, FullName + " ConstantPool Count"); + if (ConstantPoolCount.HasValue) + NAssert.AreEqual (ConstantPoolCount.Value, classDeclaration.ConstantPool.Count, FullName + " ConstantPool Count"); NAssert.AreEqual (AccessFlags, classDeclaration.AccessFlags, FullName + " AccessFlags"); NAssert.AreEqual (FullName, classDeclaration.ThisClass.Name.Value, FullName + " Name"); NAssert.AreEqual (Superclass?.BinaryName, classDeclaration?.SuperClass?.Name?.Value, FullName + " SuperClass Name"); @@ -70,4 +71,3 @@ public void Assert (ClassFile classDeclaration) } } } - diff --git a/external/Java.Interop/tests/Xamarin.Android.Tools.Bytecode-Tests/ModuleInfoTests.cs b/external/Java.Interop/tests/Xamarin.Android.Tools.Bytecode-Tests/ModuleInfoTests.cs index 035cc708352..9e7f6a59e16 100644 --- a/external/Java.Interop/tests/Xamarin.Android.Tools.Bytecode-Tests/ModuleInfoTests.cs +++ b/external/Java.Interop/tests/Xamarin.Android.Tools.Bytecode-Tests/ModuleInfoTests.cs @@ -18,7 +18,6 @@ public void ClassFile () new ExpectedTypeDeclaration { MajorVersion = 0x37, MinorVersion = 0, - ConstantPoolCount = 12, AccessFlags = ClassAccessFlags.Module, FullName = "module-info", }.Assert (c); @@ -42,4 +41,3 @@ public void ClassFile () } } } - diff --git a/src/Mono.Android/Android.Graphics.Drawables/ClipDrawable.cs b/src/Mono.Android/Android.Graphics.Drawables/ClipDrawable.cs index e9fa6869fd6..2b5d26fbcf5 100644 --- a/src/Mono.Android/Android.Graphics.Drawables/ClipDrawable.cs +++ b/src/Mono.Android/Android.Graphics.Drawables/ClipDrawable.cs @@ -12,12 +12,7 @@ public void ScheduleDrawable (Android.Graphics.Drawables.Drawable who, Action wh public void UnscheduleDrawable (Android.Graphics.Drawables.Drawable who, Action what) { - var runnable = Java.Lang.Thread.RunnableImplementor.Remove (what); - if (runnable == null) - return; - UnscheduleDrawable (who, runnable); - runnable.Dispose (); + Java.Lang.Thread.RunnableImplementor.Remove (what, this, who, static (runnable, drawable, who) => drawable.UnscheduleDrawable (who, runnable)); } } } - diff --git a/src/Mono.Android/Android.Graphics.Drawables/Drawable.cs b/src/Mono.Android/Android.Graphics.Drawables/Drawable.cs index 18eca3a1c60..2d359d46967 100644 --- a/src/Mono.Android/Android.Graphics.Drawables/Drawable.cs +++ b/src/Mono.Android/Android.Graphics.Drawables/Drawable.cs @@ -12,12 +12,7 @@ public void ScheduleSelf (Action what, long when) public void UnscheduleSelf (Action what) { - var runnable = Java.Lang.Thread.RunnableImplementor.Remove (what); - if (runnable == null) - return; - UnscheduleSelf (runnable); - runnable.Dispose (); + Java.Lang.Thread.RunnableImplementor.Remove (what, this, static (runnable, drawable) => drawable.UnscheduleSelf (runnable)); } } } - diff --git a/src/Mono.Android/Android.Graphics.Drawables/DrawableContainer.cs b/src/Mono.Android/Android.Graphics.Drawables/DrawableContainer.cs index 623712785f3..b4c3f984d23 100644 --- a/src/Mono.Android/Android.Graphics.Drawables/DrawableContainer.cs +++ b/src/Mono.Android/Android.Graphics.Drawables/DrawableContainer.cs @@ -12,12 +12,7 @@ public void ScheduleDrawable (Android.Graphics.Drawables.Drawable who, Action wh public void UnscheduleDrawable (Android.Graphics.Drawables.Drawable who, Action what) { - var runnable = Java.Lang.Thread.RunnableImplementor.Remove (what); - if (runnable == null) - return; - UnscheduleDrawable (who, runnable); - runnable.Dispose (); + Java.Lang.Thread.RunnableImplementor.Remove (what, this, who, static (runnable, drawable, who) => drawable.UnscheduleDrawable (who, runnable)); } } } - diff --git a/src/Mono.Android/Android.Graphics.Drawables/InsetDrawable.cs b/src/Mono.Android/Android.Graphics.Drawables/InsetDrawable.cs index ec5b2cc3a7b..a14f2a76ae6 100644 --- a/src/Mono.Android/Android.Graphics.Drawables/InsetDrawable.cs +++ b/src/Mono.Android/Android.Graphics.Drawables/InsetDrawable.cs @@ -12,12 +12,7 @@ public void ScheduleDrawable (Android.Graphics.Drawables.Drawable who, Action wh public void UnscheduleDrawable (Android.Graphics.Drawables.Drawable who, Action what) { - var runnable = Java.Lang.Thread.RunnableImplementor.Remove (what); - if (runnable == null) - return; - UnscheduleDrawable (who, runnable); - runnable.Dispose (); + Java.Lang.Thread.RunnableImplementor.Remove (what, this, who, static (runnable, drawable, who) => drawable.UnscheduleDrawable (who, runnable)); } } } - diff --git a/src/Mono.Android/Android.Graphics.Drawables/LayerDrawable.cs b/src/Mono.Android/Android.Graphics.Drawables/LayerDrawable.cs index 50256dbfd0b..2e1209b46e9 100644 --- a/src/Mono.Android/Android.Graphics.Drawables/LayerDrawable.cs +++ b/src/Mono.Android/Android.Graphics.Drawables/LayerDrawable.cs @@ -12,12 +12,7 @@ public void ScheduleDrawable (Android.Graphics.Drawables.Drawable who, Action wh public void UnscheduleDrawable (Android.Graphics.Drawables.Drawable who, Action what) { - var runnable = Java.Lang.Thread.RunnableImplementor.Remove (what); - if (runnable == null) - return; - UnscheduleDrawable (who, runnable); - runnable.Dispose (); + Java.Lang.Thread.RunnableImplementor.Remove (what, this, who, static (runnable, drawable, who) => drawable.UnscheduleDrawable (who, runnable)); } } } - diff --git a/src/Mono.Android/Android.Graphics.Drawables/RotateDrawable.cs b/src/Mono.Android/Android.Graphics.Drawables/RotateDrawable.cs index 6d9812e7aed..c4adf94e1c6 100644 --- a/src/Mono.Android/Android.Graphics.Drawables/RotateDrawable.cs +++ b/src/Mono.Android/Android.Graphics.Drawables/RotateDrawable.cs @@ -12,12 +12,7 @@ public void ScheduleDrawable (Android.Graphics.Drawables.Drawable who, Action wh public void UnscheduleDrawable (Android.Graphics.Drawables.Drawable who, Action what) { - var runnable = Java.Lang.Thread.RunnableImplementor.Remove (what); - if (runnable == null) - return; - UnscheduleDrawable (who, runnable); - runnable.Dispose (); + Java.Lang.Thread.RunnableImplementor.Remove (what, this, who, static (runnable, drawable, who) => drawable.UnscheduleDrawable (who, runnable)); } } } - diff --git a/src/Mono.Android/Android.Graphics.Drawables/ScaleDrawable.cs b/src/Mono.Android/Android.Graphics.Drawables/ScaleDrawable.cs index 0011689854d..0b32dbeea80 100644 --- a/src/Mono.Android/Android.Graphics.Drawables/ScaleDrawable.cs +++ b/src/Mono.Android/Android.Graphics.Drawables/ScaleDrawable.cs @@ -12,12 +12,7 @@ public void ScheduleDrawable (Android.Graphics.Drawables.Drawable who, Action wh public void UnscheduleDrawable (Android.Graphics.Drawables.Drawable who, Action what) { - var runnable = Java.Lang.Thread.RunnableImplementor.Remove (what); - if (runnable == null) - return; - UnscheduleDrawable (who, runnable); - runnable.Dispose (); + Java.Lang.Thread.RunnableImplementor.Remove (what, this, who, static (runnable, drawable, who) => drawable.UnscheduleDrawable (who, runnable)); } } } - diff --git a/src/Mono.Android/Android.OS/Handler.cs b/src/Mono.Android/Android.OS/Handler.cs index 40634af756f..d4cba0a67d8 100644 --- a/src/Mono.Android/Android.OS/Handler.cs +++ b/src/Mono.Android/Android.OS/Handler.cs @@ -63,20 +63,12 @@ public bool PostDelayed (Action action, long delayMillis) public void RemoveCallbacks (Action action) { - var runnable = Java.Lang.Thread.RunnableImplementor.Remove (action); - if (runnable == null) - return; - RemoveCallbacks (runnable); - runnable.Dispose (); + Java.Lang.Thread.RunnableImplementor.Remove (action, this, static (runnable, handler) => handler.RemoveCallbacks (runnable)); } public void RemoveCallbacks (Action action, Java.Lang.Object token) { - var runnable = Java.Lang.Thread.RunnableImplementor.Remove (action); - if (runnable == null) - return; - RemoveCallbacks (runnable, token); - runnable.Dispose (); + Java.Lang.Thread.RunnableImplementor.Remove (action, this, token, static (runnable, handler, token) => handler.RemoveCallbacks (runnable, token)); } } @@ -103,4 +95,3 @@ public bool HandleMessage (Message m) } } } - diff --git a/src/Mono.Android/Android.Runtime/JNIEnvInit.cs b/src/Mono.Android/Android.Runtime/JNIEnvInit.cs index 9e6c6fa599d..bfb61dab91d 100644 --- a/src/Mono.Android/Android.Runtime/JNIEnvInit.cs +++ b/src/Mono.Android/Android.Runtime/JNIEnvInit.cs @@ -22,7 +22,7 @@ internal struct JnienvInitializeArgs { public IntPtr env; public IntPtr grefLoader; public IntPtr Loader_loadClass; - public IntPtr grefClass; // TODO: remove, not needed anymore + public IntPtr grefClass; // Unused; reserved to preserve the shared native/managed layout. public uint logCategories; public int version; // TODO: remove, not needed anymore public int grefGcThreshold; diff --git a/src/Mono.Android/Android.Views/View.cs b/src/Mono.Android/Android.Views/View.cs index b6c48d9d781..9d5f5024648 100644 --- a/src/Mono.Android/Android.Views/View.cs +++ b/src/Mono.Android/Android.Views/View.cs @@ -66,12 +66,10 @@ public bool PostDelayed (Action action, long delayMillis) public bool RemoveCallbacks (Action action) { - var runnable = Java.Lang.Thread.RunnableImplementor.Remove (action); - if (runnable == null) - return false; - bool result = RemoveCallbacks (runnable); - runnable.Dispose (); - return result; + return Java.Lang.Thread.RunnableImplementor.Remove ( + action, + this, + static (view, runnable) => view.RemoveCallbacks (runnable)); } public void ScheduleDrawable (Android.Graphics.Drawables.Drawable who, Action what, long when) @@ -81,11 +79,7 @@ public void ScheduleDrawable (Android.Graphics.Drawables.Drawable who, Action wh public void UnscheduleDrawable (Android.Graphics.Drawables.Drawable who, Action what) { - var runnable = Java.Lang.Thread.RunnableImplementor.Remove (what); - if (runnable == null) - return; - UnscheduleDrawable (who, runnable); - runnable.Dispose (); + Java.Lang.Thread.RunnableImplementor.Remove (what, this, who, static (runnable, view, who) => view.UnscheduleDrawable (who, runnable)); } #if ANDROID_11 diff --git a/src/Mono.Android/Java.Lang/Object.cs b/src/Mono.Android/Java.Lang/Object.cs index e065c63d27a..087ab4836e2 100644 --- a/src/Mono.Android/Java.Lang/Object.cs +++ b/src/Mono.Android/Java.Lang/Object.cs @@ -111,11 +111,14 @@ protected void SetHandle (IntPtr value, JniHandleOwnership transfer) { var reference = new JniObjectReference (value); var options = JNIEnv.ToJniObjectReferenceOptions (transfer); - JniEnvironment.Runtime.ValueManager.ConstructPeer ( - this, - ref reference, - value == IntPtr.Zero ? JniObjectReferenceOptions.None : options); - JNIEnv.DeleteRef (value, transfer); + try { + JniEnvironment.Runtime.ValueManager.ConstructPeer ( + this, + ref reference, + value == IntPtr.Zero ? JniObjectReferenceOptions.None : options); + } finally { + JNIEnv.DeleteRef (value, transfer); + } } internal static IJavaPeerable? PeekObject (IntPtr handle, Type? requiredType = null) @@ -166,9 +169,11 @@ protected void SetHandle (IntPtr value, JniHandleOwnership transfer) if (handle == IntPtr.Zero) return null; - var r = JniEnvironment.Runtime.ValueManager.GetPeer (new JniObjectReference (handle), type); - JNIEnv.DeleteRef (handle, transfer); - return r; + try { + return JniEnvironment.Runtime.ValueManager.GetPeer (new JniObjectReference (handle), type); + } finally { + JNIEnv.DeleteRef (handle, transfer); + } } [EditorBrowsable (EditorBrowsableState.Never)] diff --git a/src/Mono.Android/Java.Lang/Thread.cs b/src/Mono.Android/Java.Lang/Thread.cs index 787bebf4759..7389e4815ea 100644 --- a/src/Mono.Android/Java.Lang/Thread.cs +++ b/src/Mono.Android/Java.Lang/Thread.cs @@ -26,31 +26,105 @@ public RunnableImplementor (Action handler, bool removable) Handler = handler; this.removable = removable; - if (removable) - lock (instances) - instances.AddOrUpdate (handler, this); + if (removable) { + lock (instances) { + var runnables = instances.GetOrCreateValue (handler); + Prune (runnables); + runnables.Add (new WeakReference (this, trackResurrection: true)); + } + } } public void Run () { - if (Handler != null) - Handler (); - if (removable) - lock (instances) - if (Handler != null) - instances.Remove (Handler); - Dispose (); + try { + Handler?.Invoke (); + } finally { + Dispose (); + } + } + + public new void Dispose () + { + lock (this) + base.Dispose (); + } + + protected override void Dispose (bool disposing) + { + if (removable && Handler != null) { + lock (instances) { + if (instances.TryGetValue (Handler, out var runnables)) { + Prune (runnables, this); + if (runnables.Count == 0) + instances.Remove (Handler); + } + } + } + base.Dispose (disposing); + } + + // Java owns queued callbacks. Neither a rooted Action nor native cancellation + // should keep a runnable alive through this lookup table. + static readonly ConditionalWeakTable>> instances = new (); + + static void Prune (List> runnables, RunnableImplementor? completed = null) + { + for (int i = runnables.Count - 1; i >= 0; i--) { + if (!runnables [i].TryGetTarget (out var runnable) || + ReferenceEquals (runnable, completed) || runnable.Handle == IntPtr.Zero) + runnables.RemoveAt (i); + } + } + + public static void Remove (Action handler, Action remove) + { + Remove (handler, remove, static (callback, runnable) => { + callback (runnable); + return false; + }); + } + + public static void Remove (Action handler, TState state, Action remove) + { + Remove (handler, (state, remove), static (context, runnable) => { + context.remove (runnable, context.state); + return false; + }); } - static ConditionalWeakTable instances = new (); + public static void Remove (Action handler, TState1 state1, TState2 state2, Action remove) + { + Remove (handler, (state1, state2, remove), static (context, runnable) => { + context.remove (runnable, context.state1, context.state2); + return false; + }); + } - public static RunnableImplementor Remove (Action handler) + public static bool Remove (Action handler, TState state, Func remove) { - RunnableImplementor result; + List pending = new (); lock (instances) { - instances.TryGetValue (handler, out result!); - instances.Remove (handler); + if (!instances.TryGetValue (handler, out var runnables)) + return false; + Prune (runnables); + foreach (var reference in runnables) { + if (reference.TryGetTarget (out var runnable)) + pending.Add (runnable); + } + if (runnables.Count == 0) + instances.Remove (handler); + } + + bool result = false; + foreach (var runnable in pending) { + lock (runnable) { + if (runnable.Handle != IntPtr.Zero) + result |= remove (state, runnable); + } } + // Native removal may not match the handler, token or drawable. Keep the + // weak mapping and let Java reachability determine when disposal is safe. return result; } } diff --git a/src/Mono.Android/Java.Lang/Throwable.cs b/src/Mono.Android/Java.Lang/Throwable.cs index a6dcdb4422f..58188ba3a3d 100644 --- a/src/Mono.Android/Java.Lang/Throwable.cs +++ b/src/Mono.Android/Java.Lang/Throwable.cs @@ -100,13 +100,16 @@ protected void SetHandle (IntPtr value, JniHandleOwnership transfer) { var reference = new JniObjectReference (value); - Construct ( - ref reference, - value == IntPtr.Zero ? JniObjectReferenceOptions.None : JniObjectReferenceOptions.Copy); - if (value != IntPtr.Zero) { - SetJavaStackTrace (new JniObjectReference (value)); + try { + Construct ( + ref reference, + value == IntPtr.Zero ? JniObjectReferenceOptions.None : JniObjectReferenceOptions.Copy); + if (value != IntPtr.Zero) { + SetJavaStackTrace (new JniObjectReference (value)); + } + } finally { + JNIEnv.DeleteRef (value, transfer); } - JNIEnv.DeleteRef (value, transfer); } public static Throwable FromException (System.Exception e) diff --git a/src/native/clr/host/host.cc b/src/native/clr/host/host.cc index f82d71f37a2..76a376c83d5 100644 --- a/src/native/clr/host/host.cc +++ b/src/native/clr/host/host.cc @@ -492,8 +492,11 @@ void Host::Java_mono_android_Runtime_initInternal ( // GC threshold is 90% of the max GREF count init.grefGcThreshold = static_cast(AndroidSystem::get_gref_gc_threshold ()); - init.grefClass = RuntimeUtil::get_class_from_runtime_field (env, runtimeClass, "java_lang_Class"sv, true); - Class_getName = env->GetMethodID (init.grefClass, "getName", "()Ljava/lang/String;"); + + // java.lang.Class is a bootstrap class, so the cached method ID outlives this local reference. + jclass lrefClass = RuntimeUtil::get_class_from_runtime_field (env, runtimeClass, "java_lang_Class"sv, false); + Class_getName = env->GetMethodID (lrefClass, "getName", "()Ljava/lang/String;"); + env->DeleteLocalRef (lrefClass); jclass lrefLoaderClass = env->GetObjectClass (loader); init.Loader_loadClass = env->GetMethodID (lrefLoaderClass, "loadClass", "(Ljava/lang/String;)Ljava/lang/Class;"); diff --git a/src/native/common/include/managed-interface.hh b/src/native/common/include/managed-interface.hh index ca29461dc05..421cff849e7 100644 --- a/src/native/common/include/managed-interface.hh +++ b/src/native/common/include/managed-interface.hh @@ -23,7 +23,7 @@ namespace xamarin::android { JNIEnv *env; jobject grefLoader; jmethodID Loader_loadClass; - jclass grefClass; + jclass grefClass; // Unused; reserved to preserve the shared native/managed layout. unsigned int logCategories; int version; int grefGcThreshold; diff --git a/src/native/mono/monodroid/monodroid-glue.cc b/src/native/mono/monodroid/monodroid-glue.cc index 4a5f9e38c40..a01862f9ff7 100644 --- a/src/native/mono/monodroid/monodroid-glue.cc +++ b/src/native/mono/monodroid/monodroid-glue.cc @@ -836,8 +836,10 @@ MonodroidRuntime::init_android_runtime (JNIEnv *env, jclass runtimeClass, jobjec log_info (LOG_GC, "GREF GC Threshold: {}", init.grefGcThreshold); - init.grefClass = RuntimeUtil::get_class_from_runtime_field (env, runtimeClass, "java_lang_Class", true); - Class_getName = env->GetMethodID (init.grefClass, "getName", "()Ljava/lang/String;"); + // java.lang.Class is a bootstrap class, so the cached method ID outlives this local reference. + jclass lrefClass = RuntimeUtil::get_class_from_runtime_field (env, runtimeClass, "java_lang_Class", false); + Class_getName = env->GetMethodID (lrefClass, "getName", "()Ljava/lang/String;"); + env->DeleteLocalRef (lrefClass); MonoAssembly *mono_android_assembly; diff --git a/tests/Mono.Android-Tests/Mono.Android-Tests/Android.OS/CallbackLifetimeTests.cs b/tests/Mono.Android-Tests/Mono.Android-Tests/Android.OS/CallbackLifetimeTests.cs new file mode 100644 index 00000000000..e7f72e89b4a --- /dev/null +++ b/tests/Mono.Android-Tests/Mono.Android-Tests/Android.OS/CallbackLifetimeTests.cs @@ -0,0 +1,295 @@ +using System; +using System.Diagnostics; +using System.Runtime.CompilerServices; +using System.Runtime.ExceptionServices; +using System.Threading; + +using Android.OS; +using Android.Runtime; +using Java.Interop; + +using NUnit.Framework; + +using RunnableImplementor = Java.Lang.Thread.RunnableImplementor; + +namespace Xamarin.Android.RuntimeTests { + + [TestFixture] + [Category ("CallbackLifetime")] + public class CallbackLifetimeTests { + [TestCase (false)] + [TestCase (true)] + public void NativeCancellationReleasesRunnableWithRootedAction (bool removeAll) + { + using var queue = new CallbackQueue (); + using var token = new Java.Lang.String ("token"); + int calls = 0; + Action action = () => Interlocked.Increment (ref calls); + WeakReference weak = null; + JniObjectReference javaWeak = default; + + try { + OnFreshThread (() => { + var runnable = new RunnableImplementor (action, removable: true); + weak = new WeakReference (runnable, trackResurrection: true); + javaWeak = runnable.PeerReference.NewWeakGlobalRef (); + Assert.IsTrue (queue.Handler.PostAtTime (runnable, token, SystemClock.UptimeMillis ())); + }); + + CollectPeers (); + Assert.IsTrue (IsAlive (weak), "The Java queue must retain the original managed callback."); + Assert.IsFalse (JNIEnv.IsSameObject (javaWeak.Handle, IntPtr.Zero)); + + // This is the generated native binding, not the Action-specific removal helper. + queue.Handler.RemoveCallbacksAndMessages (removeAll ? null : token); + WaitForCollection (() => !IsAlive (weak) && JNIEnv.IsSameObject (javaWeak.Handle, IntPtr.Zero)); + queue.Drain (); + Assert.AreEqual (0, calls); + } finally { + JniObjectReference.Dispose (ref javaWeak); + GC.KeepAlive (action); + } + } + + [Test] + public void JavaQueueKeepsCallbackCallableAcrossCollection () + { + using var queue = new CallbackQueue (); + int calls = 0; + WeakReference weakAction = null; + OnFreshThread (() => { + Action action = () => Interlocked.Increment (ref calls); + weakAction = new WeakReference (action); + Assert.IsTrue (queue.Handler.Post (action)); + }); + + CollectPeers (); + Assert.IsTrue (IsAlive (weakAction), "The queue, not a managed Action root, owns the callback."); + queue.Drain (); + Assert.AreEqual (1, calls); + } + + [Test] + public void CacheDoesNotRootActionOrUnqueuedRunnable () + { + WeakReference weakAction = null; + WeakReference weakRunnable = null; + OnFreshThread (() => { + var target = new object (); + Action action = () => GC.KeepAlive (target); + var runnable = new RunnableImplementor (action, removable: true); + weakAction = new WeakReference (action); + weakRunnable = new WeakReference (runnable, trackResurrection: true); + }); + + WaitForCollection (() => !IsAlive (weakAction) && !IsAlive (weakRunnable)); + } + + [TestCase (false)] + [TestCase (true)] + public void RemoveCallbacksRemovesEveryPost (bool useToken) + { + using var queue = new CallbackQueue (); + using var token = new Java.Lang.String ("token"); + int calls = 0; + int otherCalls = 0; + Action action = () => calls++; + for (int i = 0; i < 3; i++) + Assert.IsTrue (queue.Handler.PostAtTime (action, token, SystemClock.UptimeMillis ())); + Assert.IsTrue (queue.Handler.Post (() => otherCalls++)); + + if (useToken) + queue.Handler.RemoveCallbacks (action, token); + else + queue.Handler.RemoveCallbacks (action); + queue.Drain (); + + Assert.AreEqual (0, calls); + Assert.AreEqual (1, otherCalls); + } + + [Test] + public void WrongTokenDoesNotDisposeOrForgetQueuedCallback () + { + using var queue = new CallbackQueue (); + using var token = new Java.Lang.String ("token"); + using var otherToken = new Java.Lang.String ("other token"); + int calls = 0; + Action action = () => calls++; + using var runnable = new RunnableImplementor (action, removable: true); + Assert.IsTrue (queue.Handler.PostAtTime (runnable, token, SystemClock.UptimeMillis ())); + + queue.Handler.RemoveCallbacks (action, otherToken); + Assert.AreNotEqual (IntPtr.Zero, runnable.Handle, "A token mismatch must not dispose queued work."); + queue.Handler.RemoveCallbacks (action, token); + queue.Drain (); + Assert.AreEqual (0, calls, "A token mismatch must leave the callback removable."); + } + + [TestCase (false)] + [TestCase (true)] + public void RemovingOneTokenPreservesOtherTokenCallbacks (bool removeOther) + { + using var queue = new CallbackQueue (); + using var token = new Java.Lang.String ("token"); + using var otherToken = new Java.Lang.String ("other token"); + int calls = 0; + Action action = () => calls++; + Assert.IsTrue (queue.Handler.PostAtTime (action, token, SystemClock.UptimeMillis ())); + Assert.IsTrue (queue.Handler.PostAtTime (action, otherToken, SystemClock.UptimeMillis ())); + + queue.Handler.RemoveCallbacks (action, token); + if (removeOther) + queue.Handler.RemoveCallbacks (action, otherToken); + queue.Drain (); + Assert.AreEqual (removeOther ? 0 : 1, calls); + } + + [TestCase (false)] + [TestCase (true)] + public void RemovingFromOneHandlerPreservesOtherHandlerCallbacks (bool removeOther) + { + using var queue = new CallbackQueue (); + using var other = new Handler (queue.Handler.Looper); + int calls = 0; + Action action = () => calls++; + Assert.IsTrue (queue.Handler.Post (action)); + Assert.IsTrue (other.Post (action)); + + queue.Handler.RemoveCallbacks (action); + if (removeOther) + other.RemoveCallbacks (action); + queue.Drain (); + Assert.AreEqual (removeOther ? 0 : 1, calls); + } + + [Test] + public void OlderCompletionDoesNotForgetNewerPost () + { + using var queue = new CallbackQueue (); + int calls = 0; + Action action = () => calls++; + using var remove = new Java.Lang.Runnable (() => queue.Handler.RemoveCallbacks (action)); + Assert.IsTrue (queue.Handler.Post (action)); + Assert.IsTrue (queue.Handler.Post (remove)); + Assert.IsTrue (queue.Handler.Post (action)); + + queue.Drain (); + Assert.AreEqual (1, calls); + } + + [Test] + public void SelfRepostingCallbackRemainsRemovable () + { + using var queue = new CallbackQueue (); + int calls = 0; + Action action = null; + using var remove = new Java.Lang.Runnable (() => queue.Handler.RemoveCallbacks (action)); + action = () => { + if (++calls == 1) { + queue.Handler.Post (remove); + queue.Handler.Post (action); + } + }; + Assert.IsTrue (queue.Handler.Post (action)); + + queue.Drain (); + // The first callback queues its removal and repost after the first drain marker. + queue.Drain (); + Assert.AreEqual (1, calls); + } + + static void CollectPeers () + { + int generation = JNIEnv.BridgeProcessingGeneration; + var timeout = Stopwatch.StartNew (); + do { + GC.Collect (); + GC.WaitForPendingFinalizers (); + JNIEnv.WaitForBridgeProcessing (); + Java.Lang.JavaSystem.Gc (); + JniEnvironment.Runtime.ValueManager.CollectPeers (); + JNIEnv.WaitForBridgeProcessing (); + if (Microsoft.Android.Runtime.RuntimeFeature.IsMonoRuntime || + JNIEnv.BridgeProcessingGeneration != generation) + return; + Thread.Sleep (10); + } while (timeout.ElapsedMilliseconds < 5000); + Assert.Fail ("No JNI bridge-processing cycle completed."); + } + + static void WaitForCollection (Func collected) + { + var timeout = Stopwatch.StartNew (); + do { + CollectPeers (); + if (collected ()) + return; + Thread.Sleep (10); + } while (timeout.ElapsedMilliseconds < 10000); + Assert.Fail ("The callback ownership chain was not released."); + } + + [MethodImpl (MethodImplOptions.NoInlining)] + static bool IsAlive (WeakReference weak) where T : class + { + bool alive = false; + OnFreshThread (() => alive = weak.TryGetTarget (out _)); + return alive; + } + + static void OnFreshThread (Action action) + { + // Do not leave callback references on a conservatively scanned Mono test stack. + Exception error = null; + var thread = new Thread (() => { + try { + action (); + } catch (Exception e) { + error = e; + } + }); + thread.Start (); + Assert.IsTrue (thread.Join (TimeSpan.FromSeconds (10)), "The callback operation did not finish."); + if (error != null) + ExceptionDispatchInfo.Capture (error).Throw (); + } + + sealed class CallbackQueue : IDisposable { + readonly HandlerThread thread = new HandlerThread ("CallbackLifetimeTests"); + readonly ManualResetEventSlim release = new ManualResetEventSlim (); + readonly Java.Lang.Runnable blocker; + + public Handler Handler { get; } + + public CallbackQueue () + { + thread.Start (); + Handler = new Handler (thread.Looper); + blocker = new Java.Lang.Runnable (() => release.Wait ()); + Assert.IsTrue (Handler.Post (blocker)); + } + + public void Drain () + { + using var done = new ManualResetEventSlim (); + using var marker = new Java.Lang.Runnable (() => done.Set ()); + Assert.IsTrue (Handler.Post (marker)); + release.Set (); + Assert.IsTrue (done.Wait (TimeSpan.FromSeconds (10)), "The callback queue did not drain."); + } + + public void Dispose () + { + Handler.RemoveCallbacksAndMessages (null); + release.Set (); + thread.Quit (); + thread.Join (); + blocker.Dispose (); + Handler.Dispose (); + thread.Dispose (); + release.Dispose (); + } + } + } +} diff --git a/tests/Mono.Android-Tests/Mono.Android-Tests/Java.Interop/TransferredReferenceTests.cs b/tests/Mono.Android-Tests/Mono.Android-Tests/Java.Interop/TransferredReferenceTests.cs new file mode 100644 index 00000000000..2b3814fe701 --- /dev/null +++ b/tests/Mono.Android-Tests/Mono.Android-Tests/Java.Interop/TransferredReferenceTests.cs @@ -0,0 +1,293 @@ +using System; +using System.Reflection; + +using Android.Runtime; + +using Java.Interop; + +using NUnit.Framework; + +namespace Java.InteropTests +{ + [TestFixture] + [NonParallelizable] + [Category ("TransferredReferences")] + public class TransferredReferenceTests + { + [Test] + public void GetObject_Success ( + [Values (JniHandleOwnership.DoNotTransfer, JniHandleOwnership.TransferLocalRef, JniHandleOwnership.TransferGlobalRef)] JniHandleOwnership ownership, + [Values (false, true)] bool doNotRegister) + { + var transfer = WithRegistration (ownership, doNotRegister); + using var input = new InputReferenceTracker (JNIEnv.AllocObject ("java/lang/Object"), transfer); + Assert.IsNull (JniEnvironment.Runtime.ValueManager.PeekPeer (new JniObjectReference (input.Handle))); + + using var peer = Java.Lang.Object.GetObject (input.Handle, transfer); + input.AssertOwnership (transfer); + Assert.IsNotNull (peer); + Assert.IsTrue (peer.PeerReference.IsValid); + Assert.AreNotEqual (input.Handle, peer.Handle, "The peer must own a separate reference."); + } + + [Test] + public void GetObject_CachedPeer ( + [Values (JniHandleOwnership.DoNotTransfer, JniHandleOwnership.TransferLocalRef, JniHandleOwnership.TransferGlobalRef)] JniHandleOwnership ownership) + { + using var peer = new Java.Lang.Object (JNIEnv.CreateInstance ("java/lang/Object", "()V"), JniHandleOwnership.TransferLocalRef); + using var input = new InputReferenceTracker (JNIEnv.NewLocalRef (peer.Handle), ownership); + + Assert.AreSame (peer, Java.Lang.Object.GetObject (input.Handle, ownership)); + input.AssertOwnership (ownership); + Assert.IsTrue (peer.PeerReference.IsValid); + } + + [Test] + public void GetObject_MissingActivationConstructor ( + [Values (JniHandleOwnership.DoNotTransfer, JniHandleOwnership.TransferLocalRef, JniHandleOwnership.TransferGlobalRef)] JniHandleOwnership ownership, + [Values (false, true)] bool doNotRegister) + { + if (Microsoft.Android.Runtime.RuntimeFeature.TrimmableTypeMap) { + Assert.Ignore ("The trimmable typemap supports inherited activation constructors."); + } + + var transfer = WithRegistration (ownership, doNotRegister); + using var input = new InputReferenceTracker (JNIEnv.AllocObject (typeof (MissingTransferredReferencePeer)), transfer); + Assert.IsNull (JniEnvironment.Runtime.ValueManager.PeekPeer (new JniObjectReference (input.Handle))); + + var exception = Assert.Throws (() => + Java.Lang.Object.GetObject (input.Handle, transfer)); + StringAssert.Contains (typeof (MissingTransferredReferencePeer).FullName, exception.Message); + input.AssertOwnership (transfer); + } + + [Test] + public void GetObject_ThrowingActivationConstructor ( + [Values (JniHandleOwnership.DoNotTransfer, JniHandleOwnership.TransferLocalRef, JniHandleOwnership.TransferGlobalRef)] JniHandleOwnership ownership, + [Values (false, true)] bool doNotRegister) + { + var transfer = WithRegistration (ownership, doNotRegister); + using var input = new InputReferenceTracker (JNIEnv.AllocObject (typeof (ThrowingTransferredReferencePeer)), transfer); + Assert.IsNull (JniEnvironment.Runtime.ValueManager.PeekPeer (new JniObjectReference (input.Handle))); + + var exception = Assert.Catch (() => + Java.Lang.Object.GetObject (input.Handle, transfer)); + if (exception is TargetInvocationException invocation) { + exception = invocation.InnerException; + } + Assert.IsInstanceOf (exception); + Assert.AreEqual (ThrowingTransferredReferencePeer.ExceptionMessage, exception.Message); + input.AssertOwnership (transfer); + } + + [Test] + public void ObjectSetHandle ( + [Values (JniHandleOwnership.DoNotTransfer, JniHandleOwnership.TransferLocalRef, JniHandleOwnership.TransferGlobalRef)] JniHandleOwnership ownership, + [Values (false, true)] bool doNotRegister, + [Values (false, true)] bool failCopy) + { + var transfer = WithRegistration (ownership, doNotRegister); + using var input = new InputReferenceTracker (JNIEnv.AllocObject ("java/lang/Object"), transfer) { + FailCopy = failCopy, + }; + using var peer = new SetHandleObject (); + if (failCopy) { + Assert.Throws (() => peer.Assign (input.Handle, transfer)); + Assert.IsFalse (peer.PeerReference.IsValid); + } else { + peer.Assign (input.Handle, transfer); + Assert.IsTrue (peer.PeerReference.IsValid); + var registered = JniEnvironment.Runtime.ValueManager.PeekPeer (peer.PeerReference); + if (doNotRegister) { + Assert.IsNull (registered); + } else { + Assert.AreSame (peer, registered); + } + } + input.AssertOwnership (transfer); + } + + [Test] + public void ThrowableSetHandle ( + [Values (JniHandleOwnership.DoNotTransfer, JniHandleOwnership.TransferLocalRef, JniHandleOwnership.TransferGlobalRef)] JniHandleOwnership ownership, + [Values (false, true)] bool doNotRegister, + [Values (false, true)] bool failCopy) + { + var transfer = WithRegistration (ownership, doNotRegister); + using var input = new InputReferenceTracker (JNIEnv.CreateInstance ("java/lang/Throwable", "()V"), transfer) { + FailCopy = failCopy, + }; + using var peer = new SetHandleThrowable (); + if (failCopy) { + Assert.Throws (() => peer.Assign (input.Handle, transfer)); + Assert.IsFalse (peer.PeerReference.IsValid); + } else { + peer.Assign (input.Handle, transfer); + Assert.IsTrue (peer.PeerReference.IsValid); + Assert.IsNotNull (peer.JavaStackTrace); + } + input.AssertOwnership (transfer); + } + + static JniHandleOwnership WithRegistration (JniHandleOwnership ownership, bool doNotRegister) + { + return ownership | (doNotRegister ? JniHandleOwnership.DoNotRegister : JniHandleOwnership.DoNotTransfer); + } + + // Observe only the input handle, forwarding all JNI operations to the real manager. + // This avoids global-count races and never asks JNI about an already deleted handle. + sealed class InputReferenceTracker : JniRuntime.JniObjectReferenceManager + { + static readonly PropertyInfo managerProperty = typeof (JniRuntime).GetProperty (nameof (JniRuntime.ObjectReferenceManager)) + ?? throw new InvalidOperationException ("Could not find the JNI object reference manager property."); + + readonly JniRuntime.JniObjectReferenceManager original; + readonly JniObjectReferenceType referenceType; + int deletions; + + public IntPtr Handle { get; } + public bool FailCopy { get; set; } + + public InputReferenceTracker (IntPtr local, JniHandleOwnership ownership) + { + OnSetRuntime (JniEnvironment.Runtime); + original = Runtime.ObjectReferenceManager; + referenceType = (ownership & JniHandleOwnership.TransferLocalRef) != 0 + ? JniObjectReferenceType.Local + : JniObjectReferenceType.Global; + if (referenceType == JniObjectReferenceType.Local) { + Handle = local; + } else { + try { + Handle = JNIEnv.NewGlobalRef (local); + } finally { + JNIEnv.DeleteLocalRef (local); + } + } + bool installed = false; + try { + managerProperty.SetValue (Runtime, this); + installed = true; + } finally { + if (!installed) { + DeleteInput (); + } + } + } + + public void AssertOwnership (JniHandleOwnership ownership) + { + bool transferred = (ownership & (JniHandleOwnership.TransferLocalRef | JniHandleOwnership.TransferGlobalRef)) != 0; + Assert.AreEqual (transferred ? 1 : 0, deletions, "Input reference deletion count."); + } + + protected override void Dispose (bool disposing) + { + try { + // Also clean up when a regression leaves the transferred input behind. + if (deletions == 0) { + DeleteInput (); + } + } finally { + managerProperty.SetValue (Runtime, original); + } + } + + void DeleteInput () + { + if (referenceType == JniObjectReferenceType.Local) { + JNIEnv.DeleteLocalRef (Handle); + } else { + JNIEnv.DeleteGlobalRef (Handle); + } + } + + void Deleting (JniObjectReference reference) + { + if (reference.Handle != Handle) { + return; + } + Assert.AreEqual (referenceType, reference.Type, "Wrong deletion API for the input reference."); + // Fail before entering JNI if a regression attempts a double delete. + Assert.AreEqual (0, deletions, "Input reference was deleted more than once."); + deletions++; + } + + public override int GlobalReferenceCount => original.GlobalReferenceCount; + public override int WeakGlobalReferenceCount => original.WeakGlobalReferenceCount; + public override bool LogGlobalReferenceMessages => original.LogGlobalReferenceMessages; + public override bool LogLocalReferenceMessages => original.LogLocalReferenceMessages; + + public override void WriteGlobalReferenceLine (string format, params object [] args) => original.WriteGlobalReferenceLine (format, args); + public override void WriteLocalReferenceLine (string format, params object [] args) => original.WriteLocalReferenceLine (format, args); + public override JniObjectReference CreateLocalReference (JniObjectReference reference, ref int count) => original.CreateLocalReference (reference, ref count); + public override void CreatedLocalReference (JniObjectReference reference, ref int count) => original.CreatedLocalReference (reference, ref count); + public override IntPtr ReleaseLocalReference (ref JniObjectReference reference, ref int count) => original.ReleaseLocalReference (ref reference, ref count); + public override JniObjectReference CreateWeakGlobalReference (JniObjectReference reference) => original.CreateWeakGlobalReference (reference); + public override void DeleteWeakGlobalReference (ref JniObjectReference reference) => original.DeleteWeakGlobalReference (ref reference); + + public override JniObjectReference CreateGlobalReference (JniObjectReference reference) + { + if (FailCopy && reference.Handle == Handle) { + throw new InvalidOperationException ("Injected input reference copy failure."); + } + return original.CreateGlobalReference (reference); + } + + public override void DeleteLocalReference (ref JniObjectReference reference, ref int count) + { + Deleting (reference); + original.DeleteLocalReference (ref reference, ref count); + } + + public override void DeleteGlobalReference (ref JniObjectReference reference) + { + Deleting (reference); + original.DeleteGlobalReference (ref reference); + } + } + + sealed class SetHandleObject : Java.Lang.Object + { + public SetHandleObject () : base (IntPtr.Zero, JniHandleOwnership.DoNotTransfer) + { + } + + public void Assign (IntPtr handle, JniHandleOwnership transfer) => SetHandle (handle, transfer); + } + + sealed class SetHandleThrowable : Java.Lang.Throwable + { + public SetHandleThrowable () : base (IntPtr.Zero, JniHandleOwnership.DoNotTransfer) + { + } + + public void Assign (IntPtr handle, JniHandleOwnership transfer) => SetHandle (handle, transfer); + } + } + + [Register ("net/dot/android/test/MissingTransferredReferencePeer")] + public sealed class MissingTransferredReferencePeer : Java.Lang.Object + { + public MissingTransferredReferencePeer () + { + } + } + + [Register ("net/dot/android/test/ThrowingTransferredReferencePeer")] + public sealed class ThrowingTransferredReferencePeer : Java.Lang.Object + { + public const string ExceptionMessage = "transferred reference activation failure"; + + public ThrowingTransferredReferencePeer () + { + } + + public ThrowingTransferredReferencePeer (IntPtr handle, JniHandleOwnership transfer) + : base (IntPtr.Zero, JniHandleOwnership.DoNotTransfer) + { + // Fail before creating a peer reference so this tests only input ownership. + throw new InvalidOperationException (ExceptionMessage); + } + } +} diff --git a/tests/Mono.Android-Tests/Mono.Android-Tests/Java.Lang/RunnableCacheTests.cs b/tests/Mono.Android-Tests/Mono.Android-Tests/Java.Lang/RunnableCacheTests.cs new file mode 100644 index 00000000000..de21313f7ee --- /dev/null +++ b/tests/Mono.Android-Tests/Mono.Android-Tests/Java.Lang/RunnableCacheTests.cs @@ -0,0 +1,133 @@ +using System; +using System.Collections.Generic; +using System.Threading; +using System.Threading.Tasks; + +using NUnit.Framework; + +using RunnableImplementor = Java.Lang.Thread.RunnableImplementor; + +namespace Xamarin.Android.RuntimeTests { + + [TestFixture] + [Category ("CallbackLifetime")] + public class RunnableCacheTests { + [TestCase (false)] + [TestCase (true)] + public void HandledExceptionStillDisposesRunnable (bool removable) + { + var expected = new InvalidOperationException ("callback failure"); + Action action = () => throw expected; + using var runnable = new RunnableImplementor (action, removable); + + Assert.AreSame (expected, Assert.Throws (() => runnable.Run ())); + Assert.AreEqual (IntPtr.Zero, runnable.Handle, "Run must clean up even when its exception is handled."); + GC.KeepAlive (action); + } + + [Test] + public void DisposedCallbacksAreNotRemovalCandidates () + { + Action action = () => {}; + using var disposed = new RunnableImplementor (action, removable: true); + disposed.Dispose (); + using var pending = new RunnableImplementor (action, removable: true); + var candidates = new List (); + + RunnableImplementor.Remove (action, candidates.Add); + Assert.AreEqual (1, candidates.Count); + Assert.AreSame (pending, candidates [0]); + Assert.AreNotEqual (IntPtr.Zero, pending.Handle); + } + + [TestCase (false)] + [TestCase (true)] + public void TerminalCleanupOnlyRemovesItsOwnInstance (bool throws) + { + Action action = () => { + if (throws) + throw new InvalidOperationException (); + }; + using var first = new RunnableImplementor (action, removable: true); + using var second = new RunnableImplementor (action, removable: true); + if (throws) + Assert.Throws (() => first.Run ()); + else + first.Run (); + + var candidates = new List (); + RunnableImplementor.Remove (action, candidates.Add); + Assert.AreEqual (1, candidates.Count); + Assert.AreSame (second, candidates [0]); + Assert.AreEqual (IntPtr.Zero, first.Handle); + Assert.AreNotEqual (IntPtr.Zero, second.Handle); + } + + [Test] + public void RemovalAggregatesCallbackResults () + { + Action action = () => {}; + using var first = new RunnableImplementor (action, removable: true); + using var second = new RunnableImplementor (action, removable: true); + var callbacks = new List (); + + bool result = RunnableImplementor.Remove ( + action, + callbacks, + static (items, runnable) => { + items.Add (runnable); + return items.Count == 2; + }); + + Assert.IsTrue (result); + Assert.AreEqual (2, callbacks.Count); + } + + [Test] + public void RemovalUsesSnapshotWithoutHoldingCacheLock () + { + Action action = () => {}; + using var first = new RunnableImplementor (action, removable: true); + RunnableImplementor second = null; + int removals = 0; + try { + RunnableImplementor.Remove (action, runnable => { + Assert.AreSame (first, runnable); + removals++; + var post = Task.Run (() => second = new RunnableImplementor (action, removable: true)); + Assert.IsTrue (post.Wait (TimeSpan.FromSeconds (10)), "Removal must not hold the cache lock."); + }); + Assert.AreEqual (1, removals, "A reentrant post must not be added to an in-progress removal."); + var candidates = new List (); + RunnableImplementor.Remove (action, candidates.Add); + Assert.AreEqual (2, candidates.Count); + Assert.AreSame (first, candidates [0]); + Assert.AreSame (second, candidates [1]); + } finally { + second?.Dispose (); + } + } + + [Test] + public void CompletionDoesNotDisposeDuringRemoval () + { + using var running = new ManualResetEventSlim (); + Action action = () => running.Set (); + using var runnable = new RunnableImplementor (action, removable: true); + Task execution = null; + try { + RunnableImplementor.Remove (action, candidate => { + execution = Task.Run (() => runnable.Run ()); + Assert.IsTrue (running.Wait (TimeSpan.FromSeconds (10))); + Assert.IsFalse (execution.Wait (TimeSpan.FromMilliseconds (100)), + "Completion must wait until the native removal finishes using the peer."); + Assert.AreNotEqual (IntPtr.Zero, candidate.Handle); + }); + } finally { + if (execution != null) + Assert.IsTrue (execution.Wait (TimeSpan.FromSeconds (10))); + } + Assert.AreEqual (IntPtr.Zero, runnable.Handle); + } + } +} diff --git a/tests/Mono.Android-Tests/Mono.Android-Tests/Mono.Android.NET-Tests.csproj b/tests/Mono.Android-Tests/Mono.Android-Tests/Mono.Android.NET-Tests.csproj index 8c817a19434..b6b7ee73cd0 100644 --- a/tests/Mono.Android-Tests/Mono.Android-Tests/Mono.Android.NET-Tests.csproj +++ b/tests/Mono.Android-Tests/Mono.Android-Tests/Mono.Android.NET-Tests.csproj @@ -127,6 +127,7 @@ + @@ -147,11 +148,13 @@ + + diff --git a/tests/Mono.Android-Tests/Mono.Android-Tests/Remaps.xml b/tests/Mono.Android-Tests/Mono.Android-Tests/Remaps.xml index 53a299d9d49..09e93c89fef 100644 --- a/tests/Mono.Android-Tests/Mono.Android-Tests/Remaps.xml +++ b/tests/Mono.Android-Tests/Mono.Android-Tests/Remaps.xml @@ -21,6 +21,12 @@ source-method-name="remappedToGetRuntime" target-type="java/lang/Runtime" target-method-name="getRuntime" target-method-instance-to-static="false" /> +