[Android] Fix various issues in object dispose methods.

[kvpt]

Context

fixes #5611

I notice in my apps a lot of these exceptions on Android, very hard (or even impossible) to reproduce.
I fixed some of them in #4707, #4955, #4974 but I continue to have a certain amount of them.
And there are plenty of them on the Xamarin Forum or in the issues list of this repository (and the previous on bugzilla).

When fixing the dispose method of the TabbedPageRenderer in #4974, I notice that most dispose methods in the Android backend have also issues.

• Missing native Android ressources dispose/remove/detach

This cause memory leak

• EventHander unsubscribed after that the ressource used in it was disposed

This cause race condition and can trigger random "No view found for Id XXX", if an event is trigerred during dispose

• Renderer disposed before the view rendered was removed

This also can cause race condition and can trigger random "No view found for Id XXX" or NullReference exception during layout, if android call layout method and try to restore the renderer during dispose

• Base method not called last

The child class ressources always need to be disposed first, on exception a comment need to explain why

Description of Change

The complete modifications list is :

AppCompat/CarouselPageRenderer

• Unsubscribe CollectionChanged event before the viewPager is disposed
• Missing ClearOnPageChangeListeners on the ViewPager
• Remove the ViewPager before dispose the renderer
• Add missing null check on Element before access to children

AppCompat/FormsAppCompatActivity

• Remove the root layout in destroy before dispose
• UnregisterReceiver before pause

AppCompat/ImageButtonRenderer

• Unsubscribe PropertyChanged event first
• SetOnClickListener to null
• SetOnTouchListener to null
• OnFocusChangeListener to null

AppCompat/MasterDetailPageRenderer

• Unsubscribe events before dispose
• RemoveDrawerListener before dispose renderer

AppCompat/NavigationPageRenderer

• Unsubscribe events before dispose
• Missing unsubscribe CurrentPage PropertyChanged
• Remove obsolete Android Api Check (Support library used)
• Remove fragment before dispose renderer
• Remove TitleView before dispose her renderer
• Unsubscribe Toolbar items PropertyChanged
• Clear Toolbar menu items
• Remove Toolbar view before dispose her renderer
• Remove DrawerLayout view
• Reorder ResetToolbar method and remove view before dispose

AppCompat/Platform

• Unsubscribe FormsAppCompatActivity.BackPressed event before SetPage
• Set Page to null when newRoot == null
• Refactor SetPage so only cleanup method run inside the Handler to prevent possible race-condition in page affection
• Sync cleanup on platform dispose, otherwise a race-condition is possible when application is restored (Fix edge case of issue [Android] Fix old root page dispose when main page change #4707, probably [Bug] System.NotSupportedException: Unable to activate instance of type Xamarin.Forms.Platform.Android.XXXRenderer from native handle #5894)

AppCompat/ShellFragmentContainer

• Call destroy last, no reason to call this method first

AppCompat/TabbedPageRenderer

• Unsubscribe CollectionChanged event before dispose renderer and remove view
• Missing ClearOnPageChangeListeners on the ViewPager
• Wrong ClearOnTabSelectedListeners on the TabLayout
• Dispose renderer last

FastRenderers/AutomationPropertiesProvider

• Missing Unsubscribe Element PropertyChanged

FastRenderers/ButtonRenderer

• OnFocusChangeListener to null
• Unsubscribe Element PropertyChanged event before dispose

FastRenderers/FrameRenderer

• Unsubscribe Element PropertyChanged event before dispose

FastRenderers/ImageRenderer

• Unsubscribe Element PropertyChanged event before dispose

FastRenderers/LabelRenderer

• Unsubscribe Element PropertyChanged event before dispose
• SpannableString Dispose (Not sure if necessary)
• LabelTextColorDefault Dispose (Not sure if necessary)

FastRenderers/VisualElementRenderer

• Unsubscribe Element PropertyChanged event before dispose
• Dispose GestureManager and AutomationPropertiesProvider last

ShellSectionRenderer

• Call base.Destroy last
• Remove and destroy Scrollview

VisualElementPackager

• Unsubscribe events before clear and dispose

VisualElementRenderer

• Simplify and remove redondants null check
• Unsubscribe Element PropertyChanged event before dispose

Issues Resolved

A lot of random and hard to reproduce issues.

API Changes

None

Platforms Affected

• Android

Behavioral/Visual Changes

None

Before/After Screenshots

Not applicable

Testing Procedure

Run all of the solution tests cases, there must be no regression.

PR Checklist

• Has automated tests
• Rebased on top of the target branch at time of PR
• Changes adhere to coding standard

Additionnal words

These kind of issues are, in addition, masked by the contructor (IntPtr/JniHandleOwnership), which makes the leaks very difficult to see. I dont remove them in this PR but I think they are an anti pattern which hide the leak instead of fix it.

To conclude, the stability being the most critical factor, please prioritize this PR.
I'm sure I have missed some issues, so more pair of eyes are welcome.

[hartez]

I very much like this pattern of unsubscribing all the event handlers before unhooking other stuff.

[hartez]

I think the fix in ItemsViewRenderer might not quite be right. Other than that, this looks great!

[kvpt]

I have removed the collection view part, moved to PR 6525.
You can remove the CollectionView label and update the review.

[kvpt]

Rebased to fix conflict (bis).

[jfversluis]

@kvpt build seems to fail on:

"/Users/vsts/agent/2.153.2/work/1/s/Xamarin.Forms.sln" (default target) (1) ->
"/Users/vsts/agent/2.153.2/work/1/s/Xamarin.Forms.Platform.Android/Xamarin.Forms.Platform.Android.csproj" (default target) (5) ->
(CoreCompile target) ->
FastRenderers/FrameRenderer.cs(123,5): error CS0103: The name '_gestureManager' does not exist in the current context [/Users/vsts/agent/2.153.2/work/1/s/Xamarin.Forms.Platform.Android/Xamarin.Forms.Platform.Android.csproj]
AppCompat/Platform.cs(24,8): error CS0414: The field 'Platform._pendingRootChange' is assigned but its value is never used [/Users/vsts/agent/2.153.2/work/1/s/Xamarin.Forms.Platform.Android/Xamarin.Forms.Platform.Android.csproj]

[kvpt]

Thanks for the report, I surely fail the last rebase.
I will fix it.

[lassana]

Has it been included in the latest 4.1 release?

[kvpt]

No, this was included on the master branch, so it will be included in the 4.2 branch.

[JosueDM94]

Has it been included in the latest 4.2.0.618605-pre2?

Android continue giving this error on the FastRenderers for ImageRenderer and LabelRenderer.

AppCenter Logs:

Label log:
JniPeerMembers.AssertSelf (Java.Interop.IJavaPeerable self)
System.ObjectDisposedException: Cannot access a disposed object. Object name: 'Xamarin.Forms.Platform.Android.FastRenderers.LabelRenderer'.

TextView.set_TextFormatted (Java.Lang.ICharSequence value)
TextView.set_Text (System.String value)
LabelRenderer.UpdateText ()
LabelRenderer.OnElementPropertyChanged (System.Object sender, System.ComponentModel.PropertyChangedEventArgs e)

Image log:

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

Java.Interop
JniPeerMembers.AssertSelf (Java.Interop.IJavaPeerable self)
Java.Interop
JniPeerMembers+JniInstanceMethods.InvokeNonvirtualVoidMethod (System.String encodedMember, Java.Interop.IJavaPeerable self, Java.Interop.JniArgumentValue* parameters)
Android.Widget
ImageView.ClearColorFilter ()
Element.OnPropertyChanged (System.String propertyName)

[kvpt]

@JosueDM94 It is normally included since the 4.2.0-pre1.2 version.

Have you noticed a reduction in the number of this type of crashs with since this version or not ?

This PR will normally reduce the probability that this happen but it is in theory possible that dispose and event execute in parallel. On some heavy load or particular scenario it seems to be happening.

From the StackTrace it's very clear that the issue is on this line.

Xamarin.Forms/Xamarin.Forms.Platform.Android/FastRenderers/LabelRenderer.cs

Line 383 in 15f1ccb

Text = Element.Text;

Element.Text return the text but when Android side try to affect it the Control.Text fail because the Control has been disposed.

We have two ways to fix this :

• Add synchronization mechanism between dispose and OnElementChanged/OnElementPropertyChanged (ManualResetEvent)
• Add a check before all the access to Control properties (can be done with extension method)

[JosueDM94]

I tested 4.2.0.618605-pre2, and I was getting the same number of this suddenly crash on the app.

This is the whole scenario, I have a MasterDetailPage, and on the master page, I have a listview with a view cell that contains a label and image, those elements had attached triggers to change the color from the image and text of the option selected. I applied a bindable effect to the image to change the tint color from the image source and the label I'm using the text color property, at least for the effect, I could control the exception with a try-catch with ObjectDisposedException.
Must of the time I'm only changing the detail page and using modal navigation for some cases., I'm not sure why the master page that doesn't change the page assigned is doing the dispose of those controls.

[kvpt]

For the ImageRenderer I have no idea because I don't find method ClearColorFilter in the code for the ImageRenderer, the only place where this method seem to be used is in the Switch renderer and SwitchCell renderer, strange.
For the LabelRenderer, what is cause the crash is the text change and anything else (at least for this stacktrace).
When do you change or affect the label text ?

[JosueDM94]

Label and Image trigger where change the text and image color from the menu selection.

<Image.Triggers>
                                        <DataTrigger TargetType="Image" Binding="{Binding IsSelected}" Value="true">
                                            <Setter Property="Effects:TintColorEffect.TintColor" Value="{StaticResource MediumBlueColor}"/>
                                        </DataTrigger>
                                    </Image.Triggers>

<Label.Triggers>
                                        <DataTrigger TargetType="Label" Binding="{Binding IsSelected}" Value="true">
                                            <Setter Property="TextColor" Value="{StaticResource MediumBlueColor}"/>
                                        </DataTrigger>
                                    </Label.Triggers>

This is the effect code for the tint color to apply to the Image control

protected override void OnElementPropertyChanged(PropertyChangedEventArgs args)
        {
            base.OnElementPropertyChanged(args);
            if (args.PropertyName.Equals(JobTime.Effects.TintColorEffect.TintColorProperty.PropertyName))
            {
                ApplyTintColor();
            }
        }

private void ApplyTintColor()
        {
            try
            {
                tintColor = JobTime.Effects.TintColorEffect.GetTintColor(Element).ToAndroid();
                var colorFilter = new PorterDuffColorFilter(tintColor, PorterDuff.Mode.SrcIn);
                if (Control != null && Control is ImageView imageView)
                {
                    imageView.ClearColorFilter();
                    imageView.SetColorFilter(colorFilter);
                }
            }
            catch(ObjectDisposedException e)
            {
                Crashes.TrackError(e);
            }
        }

[kvpt]

We have two separates issues here :

• The LabelRenderer crash => I think it's caused by some race condition between the text changed event and the disposing of the control (and so of the page).
  It's not related to the code you provided here.

• The Effect ObjectDisposedException => Here, it's because the effect is never detached and so the event is triggered on a disposed control, See issue 3159 and issue 2520, at this time the catch is the only way to deal with that.