From 9498459697d951c77f5876ee5f2cf3c3db562e05 Mon Sep 17 00:00:00 2001 From: Jonathan Pryor Date: Fri, 4 Sep 2020 19:25:47 -0400 Subject: [PATCH] [Java.Interop] Dispose, *then* Remove Fixes: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1193891 A bug was found in Xamarin.Android 11.1.0.2, candidate for Visual Studio 16.8 Preview 3: 1. Create a new "Shell Forms App" 2. Build & Run the app 3. Tap the "Browse" button. Expected results: it works! Actual results: A rather abrupt crash: Unhandled Exception: System.NotSupportedException: Unable to activate instance of type Xamarin.Forms.Platform.Android.PageRenderer from native handle 0x69 (key_handle 0x33efaa1). ---> System.MissingMethodException: No constructor found for Xamarin.Forms.Platform.Android.PageRenderer::.ctor(System.IntPtr, Android.Runtime.JniHandleOwnership) ---> Java.Interop.JavaLocationException: Exception of type 'Java.Interop.JavaLocationException' was thrown. --- End of inner exception stack trace --- at Java.Interop.TypeManager.CreateProxy (System.Type type, System.IntPtr handle, Android.Runtime.JniHandleOwnership transfer) at Java.Interop.TypeManager.CreateInstance (System.IntPtr handle, Android.Runtime.JniHandleOwnership transfer, System.Type targetType) --- End of inner exception stack trace --- at (wrapper dynamic-method) Android.Runtime.DynamicMethodNameCounter.64(intptr,intptr) at (wrapper native-to-managed) Android.Runtime.DynamicMethodNameCounter.64(intptr,intptr) The crash was caused by commit 007b35b4, which changed `JniRuntime.JniValueManager.DisposePeer()` semantics from: value.Disposed (); RemovePeer (value); and reversed this to: RemovePeer (value); value.Disposed (); (Why? Unmentioned in 007b35b4 -- doh! -- but as @jonpryor is the one who did this, "it seemed like a good idea at the time!") (Very clearly, this wasn't a good idea.) The crash was caused by commit 007b35b4 interacting with Xamarin.Forms' [`VisualElementPackager.Dispose(bool)`][0], which can call back into Java code, potentially causing `this` to need to be returned. When `RemovePeer()` is done *first*, as was done in 007b35b4, then there may be no available instance to return, causing the `NotSupportedException` to be thrown. The new `JavaObjectTest.DisposeAccessesThis()` test and associated `Java.InteropTests.GetThis` type & Java peer trigger this scenario. Without the fix, `DisposeAccessesThis()` fails with: System.NotSupportedException : Could not find an appropriate constructable wrapper type for Java type 'com/xamarin/interop/GetThis', targetType='Java.InteropTests.GetThis'. at Java.Interop.JniRuntime+JniValueManager.CreatePeer (Java.Interop.JniObjectReference& reference, Java.Interop.JniObjectReferenceOptions transfer, System.Type targetType) at Java.Interop.JavaPeerableValueMarshaler.CreateGenericValue (Java.Interop.JniObjectReference& reference, Java.Interop.JniObjectReferenceOptions options, System.Type targetType) at Java.Interop.JniRuntime+JniValueManager.GetValue[T] (Java.Interop.JniObjectReference& reference, Java.Interop.JniObjectReferenceOptions options, System.Type targetType) at Java.InteropTests.GetThis.get_This () at Java.InteropTests.GetThis.Dispose (System.Boolean disposing) at Java.Interop.JavaObject.Java.Interop.IJavaPeerable.Disposed () at Java.Interop.JniRuntime+JniValueManager.DisposePeer (Java.Interop.IJavaPeerable value) at Java.Interop.JavaObject.Dispose () demonstrating how `RemovePeer(value)` *must not* happen before `value.Disposed()`. [0]: https://github.com/xamarin/Xamarin.Forms/blob/17881ec93d6b3fb0ee5e1a2be46d7eeadef23529/Xamarin.Forms.Platform.Android/VisualElementPackager.cs#L65-L108 --- .../JniRuntime.JniValueManager.cs | 3 +- .../Java.Interop-Tests.csproj | 1 + .../Java.Interop/GetThis.cs | 47 +++++++++++++++++++ .../Java.Interop/JavaObjectTest.cs | 8 ++++ .../java/com/xamarin/interop/GetThis.java | 42 +++++++++++++++++ 5 files changed, 99 insertions(+), 2 deletions(-) create mode 100644 tests/Java.Interop-Tests/Java.Interop/GetThis.cs create mode 100644 tests/Java.Interop-Tests/java/com/xamarin/interop/GetThis.java diff --git a/src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs b/src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs index 1e8d4dbae..95b907ea4 100644 --- a/src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs +++ b/src/Java.Interop/Java.Interop/JniRuntime.JniValueManager.cs @@ -146,9 +146,8 @@ public virtual void DisposePeer (IJavaPeerable value) if (!value.PeerReference.IsValid) return; - RemovePeer (value); - value.Disposed (); + RemovePeer (value); var h = value.PeerReference; if (!h.IsValid) diff --git a/tests/Java.Interop-Tests/Java.Interop-Tests.csproj b/tests/Java.Interop-Tests/Java.Interop-Tests.csproj index 24fb0a335..2537a2f4a 100644 --- a/tests/Java.Interop-Tests/Java.Interop-Tests.csproj +++ b/tests/Java.Interop-Tests/Java.Interop-Tests.csproj @@ -40,6 +40,7 @@ + diff --git a/tests/Java.Interop-Tests/Java.Interop/GetThis.cs b/tests/Java.Interop-Tests/Java.Interop/GetThis.cs new file mode 100644 index 000000000..227d2b50e --- /dev/null +++ b/tests/Java.Interop-Tests/Java.Interop/GetThis.cs @@ -0,0 +1,47 @@ +using System; + +using Java.Interop; + +namespace Java.InteropTests +{ + [JniTypeSignature (GetThis.JniTypeName)] + public class GetThis : JavaObject + { + internal const string JniTypeName = "com/xamarin/interop/GetThis"; + + bool _isDisposed; + + readonly static JniPeerMembers _members = new JniPeerMembers (JniTypeName, typeof (GetThis)); + + public override JniPeerMembers JniPeerMembers { + get {return _members;} + } + + public GetThis () + { + } + + public unsafe GetThis This { + get { + var o = _members.InstanceMethods.InvokeNonvirtualObjectMethod ("getThis.()Lcom/xamarin/interop/GetThis;", this, null); + return JniEnvironment.Runtime.ValueManager.GetValue (ref o, JniObjectReferenceOptions.CopyAndDispose); + } + } + + protected override void Dispose (bool disposing) + { + if (_isDisposed) { + return; + } + _isDisposed = true; + if (disposing) { + var t = This; + if (t != this) { + throw new InvalidOperationException ("SHOULD NOT BE REACHED"); + } + } + base.Dispose (disposing); + } + } +} + diff --git a/tests/Java.Interop-Tests/Java.Interop/JavaObjectTest.cs b/tests/Java.Interop-Tests/Java.Interop/JavaObjectTest.cs index bf65b4b84..6ce8935f0 100644 --- a/tests/Java.Interop-Tests/Java.Interop/JavaObjectTest.cs +++ b/tests/Java.Interop-Tests/Java.Interop/JavaObjectTest.cs @@ -193,6 +193,14 @@ public void NestedDisposeInvocations () value.Dispose (); value.Dispose (); } + + [Test] + public void DisposeAccessesThis () + { + var value = new GetThis (); + value.Dispose (); + value.Dispose (); + } } class JavaObjectWithNoJavaPeer : JavaObject { diff --git a/tests/Java.Interop-Tests/java/com/xamarin/interop/GetThis.java b/tests/Java.Interop-Tests/java/com/xamarin/interop/GetThis.java new file mode 100644 index 000000000..89cdc53f9 --- /dev/null +++ b/tests/Java.Interop-Tests/java/com/xamarin/interop/GetThis.java @@ -0,0 +1,42 @@ +package com.xamarin.interop; + +import java.util.ArrayList; + +import com.xamarin.java_interop.GCUserPeerable; + +public class GetThis implements GCUserPeerable { + + static final String assemblyQualifiedName = "Java.InteropTests.GetThis, Java.Interop-Tests, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null"; + static { + com.xamarin.java_interop.ManagedPeer.registerNativeMembers ( + GetThis.class, + assemblyQualifiedName, + ""); + } + + ArrayList managedReferences = new ArrayList(); + + public GetThis () { + if (GetThis.class == getClass ()) { + com.xamarin.java_interop.ManagedPeer.construct ( + this, + "Java.InteropTests.GetThis, Java.Interop-Tests, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null", + "" + ); + } + } + + public final GetThis getThis() { + return this; + } + + public void jiAddManagedReference (java.lang.Object obj) + { + managedReferences.add (obj); + } + + public void jiClearManagedReferences () + { + managedReferences.clear (); + } +}