[Android] Don't dispose the _labelTextColorDefault on Label Fast Renderer

[samhouts]

Description of Change

#6467 disposes the _labelTextColorDefault on the fast LabelRenderer for Android. This causes a NullPointerException when attempting to access it later.

This does not happen on the legacy renderer, which now also has the same dispose but a slightly different pattern of setting the value. Using the same pattern does not fix the crash on the Fast Renderer, so the best thing is to remove the dispose and possibly revisit this later.

Issues Resolved

• fixes [Bug] Regression in Xamarin.Forms 4.2.0-pre1 (Java.Lang.NullPointerException when using FastRenderers) #6994

API Changes

None

Platforms Affected

• Android

Behavioral/Visual Changes

Before/After Screenshots

Not applicable

Testing Procedure

There is an automated test with instructions.

PR Checklist

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

[hartez]

The old-style renderer shouldn't be disposing of this value, either, but we're getting away with it because of the container.

possibly revisit this later
I don't think we need to; it's unlikely that an extra reference to a member of the same class is causing a leak.

[kvpt]

@samhouts I tested your reproduction, thanks for doing that, I confirm that removing the dispose labelTextColorDefault fix the issue.
But I think this reveal that something is broken somewhere else.
The only way, for me, that the dispose can cause this issue is a race condition between dispose method and OnElementPropertyChanged event handler. Now that the unsubscribing of the handler is at the beginning of the method the probability that this occur is greatly reduced.
To confirm that it's not the issue here, I used a ManualResetEventSlim to prevent that, it not fix the crash so it's confirm that it's not the cause.

So I made some more testing after that, and found that removing the Trigger make the crash disappear.
Other thing really weird, you can test it easily, on your test case, the label dispose (so the method fixed here) is only called once, and on the top label.
So if the crash happen, it's implied that the UpdateColor is called on the top label and this label doesn't have trigger either, so how removing them fix the issue is really weird.
Other question, why the top label is disposed in the first place ? at this time, it make no sense.

From the stacktrace it's cause by :

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

Line 269 in 86650d4

renderer.Dispose();

If I remove it no more crash, and if I navigate back the dispose is called, so I don't know if it a right thing or not.
Is the label rendered need to be destroyed and recreated if you do that ?

stackLayout.Children.Remove(instructions);
stackLayout.Children.Insert(0, instructions);

Actually it's the case.

It will need some more in depth testing.

[samhouts]

@kvpt @hartez I agree with both of your assessments. This was a strange crash to me. Thanks!

[samhouts]

Failing tests don't look related. Also can't repro locally.

[PureWeen]

Yea the Android failure is one I see randomly with Android tests :-/ It seems like the UI test runner just crashes for some reason. I have an issue logged here for it microsoft/appcenter#893

Similarly with the CollectionView on iOS for some reason it fails to navigate to the test and it just gets stuck on the screen listing the tests