[Bug] ImgeButtonRenderer used with Effect is disposed before calling OnDetach which may cause exception

[Cfun1]

Description

A follow-up of https://github.com/xamarin/XamarinCommunityToolkit/issues/994

When using IconTintColorEffect with an ImageButton under Shell, the OnDetached event should be called when we navigate away from the page, but it is not, it is called once we navigate back to that page at that time the control with effect is already disposed but not null which causes an exception. For Android, exactly at line

https://github.com/xamarin/XamarinCommunityToolkit/blob/01bfcdf98b3c5a34bbad224e4da321f8bc842f4d/src/CommunityToolkit/Xamarin.CommunityToolkit/Effects/IconTintColor/IconTintColorEffectRouter.android.cs#L56

When we have an Image, or a Button with the same effect, the OnDetached method is called as expected when we navigate away from the page.

Steps to Reproduce

1. Run the repo.
2. Navigate to the second bottom tab and return to the first one.

Expected Behavior

No Crash and OnDetach is called when navigating away from the page.

Actual Behavior

App crash with

System.ObjectDisposedException: 'Cannot access a disposed object. Object name: 'Xamarin.Forms.Platform.Android.ImageButtonRenderer'.'

and OnDetach is not called when navigating away from the page (is called when navigating back o it).

Basic Information

• Version with issue: XF 5.0.0.2012
• Last known good version: -
• Platform Target Frameworks:
  • iOS: Not affected.
  • Android: 10.0 API 29
  • UWP: Not tested.
• Nuget Packages Xamarin Community Toolkit: V1.1.0-pre1

Reproduction Link

https://github.com/pictos/XCT.SandBox/tree/icontint-disposed-exception

App1.zip

Workaround

https://github.com/xamarin/XamarinCommunityToolkit/pull/996

[PureWeen]

@Cfun1 not sure if we'll be able to fix this behavior on 5.0 but for now you could check to see if the view is still valid before applying the tint color

Here's an extension we use

using System;

namespace Xamarin.Forms.Platform.Android
{
    internal static class JavaObjectExtensions
    {
        public static bool IsDisposed(this Java.Lang.Object obj)
        {
            return obj.Handle == IntPtr.Zero;
        }

        public static bool IsAlive(this Java.Lang.Object obj)
        {
            if (obj == null)
                return false;

            return !obj.IsDisposed();
        }

        public static bool IsDisposed(this global::Android.Runtime.IJavaObject obj)
        {
            return obj.Handle == IntPtr.Zero;
        }

        public static bool IsAlive(this global::Android.Runtime.IJavaObject obj)
        {
            if (obj == null)
                return false;

            return !obj.IsDisposed();
        }
    }
}

would that work to avoid the exception now from the XCT side?

[Cfun1]

@PureWeen thanks, we have already merged a temporary easy fix that avoids this bug (link in this issue workaround), consisting of simply catching the exception and just ignoring it, waiting for this bug to be fixed, then we could remove. Do you think it better to replace that approach with the extension you have presented or it's fine ? cc @pictos

[pictos]

@Cfun1 exceptions are expensive so I believe that will be good to replicate this extension class into XCT and use it before.

@PureWeen can expand the reason to not fix it on 5.0? Is it because will be a breaking change or because will demand a lot of time to fix + review? Asking because I can take a look at it next month if this can be merged on the 5.0 branch