Fix memory leak in VisualElement.Shadow

[jsuarezruiz]

Description of Change

This is very much to 58a42e5 and 61fbb00

I used the same technique in 58a42e5, and added a unit test to validate the changes.

NOTE: It's a Draft until we review how to properly propagate the Parent to the Shadow (and others).

[jsuarezruiz]

This is a generic proxy to manage the PropertyChanged event.

[jonathanpeppers]

This PR looks correct, if the tests pass.

So the only outstanding issue is what to do about Parent. I don't know if we need to solve that before merging?

[jonathanpeppers]

@mandel-macaque the reason to make a local here is in case two threads are in here at once.

[mandel-macaque]

@mandel-macaque the reason to make a local here is in case two threads are in here at once.

@jonathanpeppers That is valid point in a multithreaded case, BUT in that case the assignment would be an issue since the ??= operator will be expanded to a if (f != null) f = new(); Therefore we have two possible cases:

1. We are only in the UI thread and there is not need for the local variable.
2. We are in a multithreaded environment and then the ??= is dangerous because there can be two threads trying to assign the _shadowProxy variable.

Regarding point 2: https://sharplab.io/#v2:CYLg1APgAgTAjAWAFDKgZgASwwdQJYB2wA9gO4YgYCSAIngM4AOx9AhgEYA2AphgN7IMQjIOHosAFgx0mLXgAoAlP1HDhAenUZ6AW1YAnAC4ZDAC0IBzehlOtGjbgVUYAvs+fiocAGy5CJUgB+DABhfW5WQwVlASQ1NSgAdgxgbgAzVgBXTkMAbmc3FCRC1BhMWLUPHwxidgArbgBjQ2CAfSj6YwBeDAJszny4sUwvX1qG5owAFW5OjCUVIfiMdtnjQMCegm5ycabDJUHloSSVjryC9yWRYqA===

You can see how a ??= is the following:

public static class Program
{
    [System.Runtime.CompilerServices.Nullable(2)]
    private static object _test;

    [System.Runtime.CompilerServices.NullableContext(1)]
    public static object Test()
    {
        if (_test == null)
        {
            _test = new object();
        }
        return _test;
    }
}

Two threads can be creating two diff _shadowObjets. So either for 1 or 2 the code is not correct.

[mandel-macaque]

@jonathanpeppers @jsuarezruiz for reference: dotnet/docs#27613 BUT the using null conditional operators IS thread safe (the joys of the documentation): Null-conditional operators ?. and ?[]

So be careful, some are thread-safe, but not all are thread-safe. Example:

static object? _test = null;
    public static object Test () {
        _test ??= new object();
        _test?.ToString ();
        return _test;
    }

Generated:

[System.Runtime.CompilerServices.NullableContext(1)]
    public static object Test()
    {
        if (_test == null)
        {
            _test = new object();
        }
        object test = _test;
        if (test != null)
        {
            test.ToString();
        }
        return _test;
    }

[jonathanpeppers]

As seen in: #13997

I think you can change this to:

_shadowHandler ??= (sender, e) => OnPropertyChanged(nameof(Shadow));
_shadowProxy ??= new();
proxy.Subscribe(shadow, _shadowHandler);

Where _shadowHandler is a member field to hold onto the EventHandler and make this test pass: https://github.com/dotnet/maui/pull/13960/files#r1139276236

[mandel-macaque]

Ok, just so that people get why we need the extra local var, we are not trying to fix a thread race condition, but a memory race condition in which the GC kicks ins that is why the code is confusing. There are not thread race conditions because everything should be happening in the main/ui thread. Is just that the fix is to use a local var to keep the ref until we don't longer need it.

[jsuarezruiz]

Right, the test was not passing in that case.
Applied changes to save the EventHandlers in a member field.

[jonathanpeppers]

This looks good now, hopefully CI is green. 👍