[Bug] [Android] Crash when calling Platform.ClearRenderer(...)

[velocitysystems]

Description

A crash occurs when calling Platform.ClearRenderer(..) from the Dispose method block in a custom renderer.

Previously custom renderers which needed to cleanup and detach the renderer from the Element would call Platform.ClearRenderer() in the dispose method block. This worked for several years up until recently.

Steps to Reproduce

Create a custom renderer.
In the Dispose method block, call Platform.ClearRenderer(..) to detach the renderer from the Element.
App crash with stack trace below.

Expected Behavior

The app should not crash.

Actual Behavior

The app crashes with the following exception and stack trace:
System.ArgumentException: Must have a valid JNI object reference!

06-12 12:30:42.757 I/MonoDroid( 8577): UNHANDLED EXCEPTION:
06-12 12:30:42.763 I/MonoDroid( 8577): System.ArgumentException: Must have a valid JNI object reference!
06-12 12:30:42.763 I/MonoDroid( 8577): Parameter name: value
06-12 12:30:42.763 I/MonoDroid( 8577):   at Android.Runtime.AndroidValueManager.RemovePeer (Java.Interop.IJavaPeerable value) [0x0001e] in <55654ebe9f2a48e6bade2862bb243f94>:0 
06-12 12:30:42.763 I/MonoDroid( 8577):   at Java.Interop.JniRuntime+JniValueManager.DisposePeer (Java.Interop.JniObjectReference h, Java.Interop.IJavaPeerable value) [0x0001f] in <dac4c5a4b77f4e61a5e6d9d3050dfb9f>:0 
06-12 12:30:42.763 I/MonoDroid( 8577):   at Java.Interop.JniRuntime+JniValueManager.DisposePeer (Java.Interop.IJavaPeerable value) [0x00038] in <dac4c5a4b77f4e61a5e6d9d3050dfb9f>:0 
06-12 12:30:42.763 I/MonoDroid( 8577):   at Java.Lang.Object.Dispose () [0x00000] in <55654ebe9f2a48e6bade2862bb243f94>:0 
06-12 12:30:42.763 I/MonoDroid( 8577):   at Xamarin.Forms.Platform.Android.VisualElementPackager.RemoveChild (Xamarin.Forms.VisualElement view) [0x0007f] in D:\a\1\s\Xamarin.Forms.Platform.Android\VisualElementPackager.cs:275 
06-12 12:30:42.763 I/MonoDroid( 8577):   at Xamarin.Forms.Platform.Android.VisualElementPackager.SetElement (Xamarin.Forms.VisualElement oldElement, Xamarin.Forms.VisualElement newElement) [0x000ab] in D:\a\1\s\Xamarin.Forms.Platform.Android\VisualElementPackager.cs:321 
06-12 12:30:42.763 I/MonoDroid( 8577):   at Xamarin.Forms.Platform.Android.VisualElementPackager.Dispose (System.Boolean disposing) [0x000ad] in D:\a\1\s\Xamarin.Forms.Platform.Android\VisualElementPackager.cs:88 
06-12 12:30:42.764 I/MonoDroid( 8577):   at Xamarin.Forms.Platform.Android.VisualElementPackager.Dispose () [0x00000] in D:\a\1\s\Xamarin.Forms.Platform.Android\VisualElementPackager.cs:61 
06-12 12:30:42.764 I/MonoDroid( 8577):   at Xamarin.Forms.Platform.Android.VisualElementRenderer`1[TElement].Dispose (System.Boolean disposing) [0x00081] in D:\a\1\s\Xamarin.Forms.Platform.Android\VisualElementRenderer.cs:306 
06-12 12:30:42.764 I/MonoDroid( 8577):   at Xamarin.Forms.Platform.Android.Platform+DefaultRenderer.Dispose (System.Boolean disposing) [0x0001a] in D:\a\1\s\Xamarin.Forms.Platform.Android\Platform.cs:1306 
06-12 12:30:42.764 I/MonoDroid( 8577):   at Java.Lang.Object.Java.Interop.IJavaPeerable.Disposed () [0x00000] in <55654ebe9f2a48e6bade2862bb243f94>:0 
06-12 12:30:42.764 I/MonoDroid( 8577):   at Java.Interop.JniRuntime+JniValueManager.DisposePeer (Java.Interop.JniObjectReference h, Java.Interop.IJavaPeerable value) [0x00019] in <dac4c5a4b77f4e61a5e6d9d3050dfb9f>:0 
06-12 12:30:42.764 I/MonoDroid( 8577):   at Java.Interop.JniRuntime+JniValueManager.DisposePeer (Java.Interop.IJavaPeerable value) [0x00038] in <dac4c5a4b77f4e61a5e6d9d3050dfb9f>:0 
06-12 12:30:42.764 I/MonoDroid( 8577):   at Java.Lang.Object.Dispose () [0x00000] in <55654ebe9f2a48e6bade2862bb243f94>:0 
06-12 12:30:42.764 I/MonoDroid( 8577):   at Xamarin.Forms.Platform.Android.VisualElementPackager.RemoveChild (Xamarin.Forms.VisualElement view) [0x0007f] in D:\a\1\s\Xamarin.Forms.Platform.Android\VisualElementPackager.cs:275 
06-12 12:30:42.764 I/MonoDroid( 8577):   at Xamarin.Forms.Platform.Android.VisualElementPackager.SetElement (Xamarin.Forms.VisualElement oldElement, Xamarin.Forms.VisualElement newElement) [0x000ab] in D:\a\1\s\Xamarin.Forms.Platform.Android\VisualElementPackager.cs:321 
06-12 12:30:42.764 I/MonoDroid( 8577):   at Xamarin.Forms.Platform.Android.VisualElementPackager.Dispose (System.Boolean disposing) [0x000ad] in D:\a\1\s\Xamarin.Forms.Platform.Android\VisualElementPackager.cs:88 
06-12 12:30:42.765 I/MonoDroid( 8577):   at Xamarin.Forms.Platform.Android.VisualElementPackager.Dispose () [0x00000] in D:\a\1\s\Xamarin.Forms.Platform.Android\VisualElementPackager.cs:61 
06-12 12:30:42.765 I/MonoDroid( 8577):   at Xamarin.Forms.Platform.Android.VisualElementRenderer`1[TElement].Dispose (System.Boolean disposing) [0x00081] in D:\a\1\s\Xamarin.Forms.Platform.Android\VisualElementRenderer.cs:306 
06-12 12:30:42.765 I/MonoDroid( 8577):   at Xamarin.Forms.Platform.Android.PageRenderer.Dispose (System.Boolean disposing) [0x00024] in D:\a\1\s\Xamarin.Forms.Platform.Android\Renderers\PageRenderer.cs:59 
06-12 12:30:42.765 I/MonoDroid( 8577):   at Java.Lang.Object.Java.Interop.IJavaPeerable.Disposed () [0x00000] in <55654ebe9f2a48e6bade2862bb243f94>:0 
06-12 12:30:42.765 I/MonoDroid( 8577):   at Java.Interop.JniRuntime+JniValueManager.DisposePeer (Java.Interop.JniObjectReference h, Java.Interop.IJavaPeerable value) [0x00019] in <dac4c5a4b77f4e61a5e6d9d3050dfb9f>:0 
06-12 12:30:42.766 I/MonoDroid( 8577):   at Java.Interop.JniRuntime+JniValueManager.DisposePeer (Java.Interop.IJavaPeerable value) [0x00038] in <dac4c5a4b77f4e61a5e6d9d3050dfb9f>:0 
06-12 12:30:42.766 I/MonoDroid( 8577):   at Java.Lang.Object.Dispose () [0x00000] in <55654ebe9f2a48e6bade2862bb243f94>:0 
06-12 12:30:42.766 I/MonoDroid( 8577):   at Xamarin.Forms.Platform.Android.AppCompat.FragmentContainer.OnDestroyView () [0x0003c] in D:\a\1\s\Xamarin.Forms.Platform.Android\AppCompat\FragmentContainer.cs:88 
06-12 12:30:42.766 I/MonoDroid( 8577):   at Android.Support.V4.App.Fragment.n_OnDestroyView (System.IntPtr jnienv, System.IntPtr native__this) [0x00008] in <fefee6c2c695459088a9df092723e052>:0 
06-12 12:30:42.766 I/MonoDroid( 8577):   at (wrapper dynamic-method) Android.Runtime.DynamicMethodNameCounter.68(intptr,intptr) in D:\a\1\s\Xamarin.Forms.Platform.And

Basic Information

• Version with issue: 4.6.0-sr2 and above
• Last known good version: N/A - Seems related to Xamarin.Android/Mono itself.
• IDE: Microsoft Visual Studio 2019 Enterprise 16.6.2
• Platform Target Frameworks:
  • Android SDK: 10.3.1.4
• Android Support Library Version: 28.0.0.3

Reproduction Link

To be supplied.

Workaround

See workaround documented below.

[velocitysystems]

We are experiencing this in a custom fast renderer we have implemented.
Testing demonstrates this may be related to this commit and/or commit.

@samhouts @hartez Follow the changes made in the commit(s), do you have any recommendations for changes that need to be made to custom renderers to make them compatible with recent changes made?

[velocitysystems]

@samhouts @hartez Upon further examining the changes, we have solved this issue in our custom renderer(s) by adding the following lines to the OnElementPropertyChanged handler:

1. Add IsDisposed() check.

if (this.IsDisposed())
{
    return;
}

2. Don't use Platform.ClearRenderer(this), but just clear the RendererProperty.

Original

if (Platform.GetRenderer(Element) == this)
{
    Platform.ClearRenderer(this);
}

Modified

if (Platform.GetRenderer(Element) == this)
{
    Element.ClearRenderer();
}

Since RendererProperty is internal, we have implemented this way:

    /// <summary>
    /// Extensions for <see cref="VisualElement"/>.
    /// </summary>
    internal static class VisualElementExtensions
    {
        /// <summary>
        /// Gets the renderer property on <see cref="Platform"/>.
        /// </summary>
        public static BindableProperty RendererProperty
        {
            get
            {
                var prop = typeof(Platform).GetField(nameof(RendererProperty), BindingFlags.Static | BindingFlags.NonPublic);
                return (BindableProperty)prop.GetValue(null);
            }
        }

        /// <summary>
        /// Clear the renderer property on the <see cref="VisualElement"/>.
        /// This is needed as workaround, since the bindable property is internal within <see cref="Platform"/>.
        /// <para>This method is the equivalent of: Element.ClearValue(Platform.RendererProperty).</para>
        /// </summary>
        /// <param name="element">The element.</param>
        public static void ClearRenderer(this VisualElement element)
        {
            element.ClearValue(RendererProperty);
        }
    }

Can you confirm this is now a requirement for custom (fast) renderers?
Unfortunately this change caught us out by surprise as it wasn't called out in the release notes.

[hartez]

We are experiencing this in a custom fast renderer we have implemented.
Testing demonstrates this may be related to this commit and/or commit.

@samhouts @hartez Follow the changes made in the commit(s), do you have any recommendations for changes that need to be made to custom renderers to make them compatible with recent changes made?

The changes in e87323b don't create any new requirements for a custom renderer, they are just a guard against an issue with renderers in Android which has always been there. I suppose I'd say that it's a good idea to add that check to any override of OnElementPropertyChanged you implement in a custom renderer. Hopefully in a future version we can find a way to make those checks redundant.

As for 8fb8ea3, that's just correcting an improper implementation of the DIspose pattern in one class; it should have no effect on how anyone uses the class or writes any custom renderers. If that commit does turn out to be the one that caused your issue, I'd be very keen to understand why.

Why did you have to stop using Platform.ClearRenderer()? I'm not sure I understand that change.

[velocitysystems]

Thanks @hartez.

The reason we stopped using Platform.ClearRenderer() was that this was the source of the JNI crash reported in the ticket description. We noticed that none of the stock XF fast renderers use this method, but rather simply clear the value of the attached Renderer property.

Looking at the implementation in the Platform class, there is additional behaviour in this method as opposed to simply clearing out the attached property. Possibly this is conflicting with the changes made in 8fb8ea3.

Xamarin.Forms/Xamarin.Forms.Platform.Android/Platform.cs

Lines 320 to 335 in bd6ef17

	public static void ClearRenderer(AView renderedView)
	{
	var element = (renderedView as IVisualElementRenderer)?.Element;
	var view = element as View;
	if (view != null)
	{
	var renderer = GetRenderer(view);
	if (renderer == renderedView)
	element.ClearValue(RendererProperty);
	renderer?.Dispose();
	renderer = null;
	}
	var layout = view as IVisualElementRenderer;
	layout?.Dispose();
	layout = null;
	}

In our case, using the workaround we have documented above and aligning our custom fast renderer exactly with the pattern used by the stock XF renderers resolved the issue for us.

[hartez]

@velocitysystems Do you know which line of ClearRenderer() was throwing the exception from the original stack trace?