Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Java.Interop] Dispose, *then* Remove #708

Merged
merged 1 commit into from
Sep 5, 2020

Conversation

jonpryor
Copy link
Member

@jonpryor jonpryor commented Sep 4, 2020

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 007b35b, which changed
JniRuntime.JniValueManager.DisposePeer() semantics from:

value.Disposed ();
RemovePeer (value);

and reversed this to:

RemovePeer (value);
value.Disposed ();

(Why? Unmentioned in 007b35b -- 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 007b35b interacting with Xamarin.Forms'
VisualElementPackager.Dispose(bool), which can call back into
Java code, potentially causing this to need to be returned.
When RemovePeer() is done first, as was done in 007b35b, 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().

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 007b35b, which changed
`JniRuntime.JniValueManager.DisposePeer()` semantics from:

	value.Disposed ();
	RemovePeer (value);

and reversed this to:

	RemovePeer (value);
	value.Disposed ();

(Why?  Unmentioned in 007b35b -- 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 007b35b 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 007b35b, 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
@jonpryor jonpryor force-pushed the jonp-fix-disposepeer-semantics branch from bacc25f to 9498459 Compare September 5, 2020 00:06
@jonpryor jonpryor merged commit 75354b9 into dotnet:master Sep 5, 2020
jonpryor added a commit that referenced this pull request Sep 5, 2020
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 007b35b, which changed
`JniRuntime.JniValueManager.DisposePeer()` semantics from:

	value.Disposed ();
	RemovePeer (value);

and reversed this to:

	RemovePeer (value);
	value.Disposed ();

(Why this reversal?  Unmentioned in 007b35b -- 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 007b35b 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 007b35b, 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
@jpobst jpobst added this to the 11.0 (16.8 / 8.8) milestone Sep 8, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Apr 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants