[controls] fix memory leak in VisualElement.Background (#13656)

* [controls] fix memory leak in `VisualElement.Background`

Fixes: #12344
Fixes: #13557
Context: https://github.com/dotnet-presentations/dotnet-maui-workshop

While testing the `Monkey Finder` sample, I found the following
scenario causes an issue:

1. Declare a `{StaticResource}` `Brush` at the `Application` level,
   with a lifetime of the entire application.

2. Set `Background` on an item in a `CollectionView`, `ListView`, etc.

3. Scroll a lot, navigate away, etc.

4. The `Brush` will hold onto any `View`'s indefinitely.

The core problem here being `VisualElement` does:

    void NotifyBackgroundChanges()
    {
        if (Background is ImmutableBrush)
            return;

        if (Background != null)
        {
            Background.Parent = this;
            Background.PropertyChanged += OnBackgroundChanged;

            if (Background is GradientBrush gradientBrush)
                gradientBrush.InvalidateGradientBrushRequested += InvalidateGradientBrushRequested;
        }
    }

If no user code sets `Background` to `null`, these events remain
subscribed.

To fix this:

1. Create a `WeakNotifyCollectionChangedProxy` type for event subscription.

2. Don't set `Background.Parent = this`

~~ General Cleanup ~~

Through doing other fixes related to memory leaks & C# events, we've
started to gain a collection of `WeakEventProxy`-related types.

I created some core `internal` types to be reused:

* `abstract WeakEventProxy<TSource, TEventHandler>`
* `WeakNotifyCollectionChangedProxy`

The following classes now make use of these new shared types:

* `BindingExpression`
* `BindableLayout`
* `ListProxy`
* `VisualElement`

This should hopefully reduce mistakes and reuse code in this area.

~~ Concerns ~~

Since, we are no longer doing:

    Background.Parent = this;

This is certainly a behavior change. It is now replaced with:

    SetInheritedBindingContext(Background, BindingContext);

I had to update one unit test that was asserting `Background.Parent`,
which can assert `Background.BindingContext` instead.

As such, this change is definitely sketchy and I wouldn't backport to
.NET 7 immediately. We might test it out in a .NET 8 preview first.

* Auto-format source code

* Add test & subscribe to InvalidateGradientBrushRequested

---------

Co-authored-by: GitHub Actions Autoformatter <autoformat@example.com>

src/Controls/src/Core/BindableLayout.cs

@@ -201,7 +201,7 @@ internal static void Clear(this IBindableLayout layout)
  class BindableLayoutController
  {
  readonly WeakReference<IBindableLayout> _layoutWeakReference;
- readonly WeakCollectionChangedProxy _collectionChangedProxy = new();
+ readonly WeakNotifyCollectionChangedProxy _collectionChangedProxy = new();
  IEnumerable _itemsSource;
  DataTemplate _itemTemplate;
  DataTemplateSelector _itemTemplateSelector;
@@ -395,45 +395,5 @@ void ItemsSourceCollectionChanged(object sender, NotifyCollectionChangedEventArg
  if (e.Action != NotifyCollectionChangedAction.Reset)
  UpdateEmptyView(layout);
  }
-
- class WeakCollectionChangedProxy
- {
- WeakReference<NotifyCollectionChangedEventHandler> _handler;
- WeakReference<INotifyCollectionChanged> _source;
-
- void OnCollectionChanged(object sender, NotifyCollectionChangedEventArgs e)
- {
- if (_handler.TryGetTarget(out var handler))
- {
- handler(sender, e);
- }
- else
- {
- Unsubscribe();
- }
- }
-
- public void Subscribe(INotifyCollectionChanged source, NotifyCollectionChangedEventHandler handler)
- {
- if (_source is not null && _source.TryGetTarget(out var s))
- {
- s.CollectionChanged -= OnCollectionChanged;
- }
-
- _source = new WeakReference<INotifyCollectionChanged>(source);
- _handler = new WeakReference<NotifyCollectionChangedEventHandler>(handler);
- source.CollectionChanged += OnCollectionChanged;
- }
-
- public void Unsubscribe()
- {
- if (_source is not null && _source.TryGetTarget(out var s))
- {
- s.CollectionChanged -= OnCollectionChanged;
- }
- _source = null;
- _handler = null;
- }
- }
  }
 }

src/Controls/src/Core/BindingExpression.cs

@@ -606,63 +606,47 @@ public BindingPair(BindingExpressionPart part, object source, bool isLast)
  public object Source { get; private set; }
  }
 
- internal class WeakPropertyChangedProxy
+ internal class WeakPropertyChangedProxy : WeakEventProxy<INotifyPropertyChanged, PropertyChangedEventHandler>
  {
- readonly WeakReference<INotifyPropertyChanged> _source = new WeakReference<INotifyPropertyChanged>(null);
- readonly WeakReference<PropertyChangedEventHandler> _listener = new WeakReference<PropertyChangedEventHandler>(null);
- readonly PropertyChangedEventHandler _handler;
- readonly EventHandler _bchandler;
- internal WeakReference<INotifyPropertyChanged> Source => _source;
+ public WeakPropertyChangedProxy() { }
 
- public WeakPropertyChangedProxy()
+ public WeakPropertyChangedProxy(INotifyPropertyChanged source, PropertyChangedEventHandler listener)
  {
- _handler = new PropertyChangedEventHandler(OnPropertyChanged);
- _bchandler = new EventHandler(OnBCChanged);
+ Subscribe(source, listener);
  }
 
- public WeakPropertyChangedProxy(INotifyPropertyChanged source, PropertyChangedEventHandler listener) : this()
- {
- SubscribeTo(source, listener);
- }
 
- public void SubscribeTo(INotifyPropertyChanged source, PropertyChangedEventHandler listener)
+
+ public override void Subscribe(INotifyPropertyChanged source, PropertyChangedEventHandler listener)
  {
- source.PropertyChanged += _handler;
- var bo = source as BindableObject;
- if (bo != null)
- bo.BindingContextChanged += _bchandler;
- _source.SetTarget(source);
- _listener.SetTarget(listener);
+ source.PropertyChanged += OnPropertyChanged;
+ if (source is BindableObject bo)
+ bo.BindingContextChanged += OnBCChanged;
+
+ base.Subscribe(source, listener);
  }
 
- public void Unsubscribe(bool finalizer = false)
+ public override void Unsubscribe()
  {
- INotifyPropertyChanged source;
- if (_source.TryGetTarget(out source) && source != null)
- source.PropertyChanged -= _handler;
- var bo = source as BindableObject;
- if (bo != null)
- bo.BindingContextChanged -= _bchandler;
-
- // If we are called from a finalizer, WeakReference<T>.SetTarget() can throw InvalidOperationException
- if (finalizer)
- return;
+ if (TryGetSource(out var source))
+ source.PropertyChanged -= OnPropertyChanged;
+ if (source is BindableObject bo)
+ bo.BindingContextChanged -= OnBCChanged;
 
- _source.SetTarget(null);
- _listener.SetTarget(null);
+ base.Unsubscribe();
  }
 
  void OnPropertyChanged(object sender, PropertyChangedEventArgs e)
  {
- if (_listener.TryGetTarget(out var handler) && handler != null)
+ if (TryGetHandler(out var handler))
  handler(sender, e);
  else
  Unsubscribe();
  }
 
  void OnBCChanged(object sender, EventArgs e)
  {
- OnPropertyChanged(sender, new PropertyChangedEventArgs("BindingContext"));
+ OnPropertyChanged(sender, new PropertyChangedEventArgs(nameof(BindableObject.BindingContext)));
  }
  }
 
@@ -672,7 +656,7 @@ class BindingExpressionPart
  readonly PropertyChangedEventHandler _changeHandler;
  WeakPropertyChangedProxy _listener;
 
- ~BindingExpressionPart() => _listener?.Unsubscribe(finalizer: true);
+ ~BindingExpressionPart() => _listener?.Unsubscribe();
 
  public BindingExpressionPart(BindingExpression expression, string content, bool isIndexer = false)
  {
@@ -687,7 +671,7 @@ public BindingExpressionPart(BindingExpression expression, string content, bool
  public void Subscribe(INotifyPropertyChanged handler)
  {
  INotifyPropertyChanged source;
- if (_listener != null && _listener.Source.TryGetTarget(out source) && ReferenceEquals(handler, source))
+ if (_listener != null && _listener.TryGetSource(out source) && ReferenceEquals(handler, source))
  // Already subscribed
  return;
 

src/Controls/src/Core/Internals/MaybeNullWhenAttribute.cs

@@ -0,0 +1,152 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+// This was copied from https://github.com/dotnet/runtime/blob/39b9607807f29e48cae4652cd74735182b31182e/src/libraries/System.Private.CoreLib/src/System/Diagnostics/CodeAnalysis/NullableAttributes.cs
+// and updated to have the scope of the attributes be internal.
+
+#nullable disable
+
+namespace System.Diagnostics.CodeAnalysis
+{
+#if !NETCOREAPP
+
+ /// <summary>Specifies that null is allowed as an input even if the corresponding type disallows it.</summary>
+ [AttributeUsage(AttributeTargets.Field | AttributeTargets.Parameter | AttributeTargets.Property, Inherited = false)]
+ internal sealed class AllowNullAttribute : Attribute { }
+
+ /// <summary>Specifies that null is disallowed as an input even if the corresponding type allows it.</summary>
+ [AttributeUsage(AttributeTargets.Field | AttributeTargets.Parameter | AttributeTargets.Property, Inherited = false)]
+ internal sealed class DisallowNullAttribute : Attribute { }
+
+ /// <summary>Specifies that an output may be null even if the corresponding type disallows it.</summary>
+ [AttributeUsage(AttributeTargets.Field | AttributeTargets.Parameter | AttributeTargets.Property | AttributeTargets.ReturnValue, Inherited = false)]
+ internal sealed class MaybeNullAttribute : Attribute { }
+
+ /// <summary>Specifies that an output will not be null even if the corresponding type allows it.</summary>
+ [AttributeUsage(AttributeTargets.Field | AttributeTargets.Parameter | AttributeTargets.Property | AttributeTargets.ReturnValue, Inherited = false)]
+ internal sealed class NotNullAttribute : Attribute { }
+
+ /// <summary>Specifies that when a method returns <see cref="ReturnValue"/>, the parameter may be null even if the corresponding type disallows it.</summary>
+ [AttributeUsage(AttributeTargets.Parameter, Inherited = false)]
+ internal sealed class MaybeNullWhenAttribute : Attribute
+ {
+ /// <summary>Initializes the attribute with the specified return value condition.</summary>
+ /// <param name="returnValue">
+ /// The return value condition. If the method returns this value, the associated parameter may be null.
+ /// </param>
+ public MaybeNullWhenAttribute(bool returnValue) => ReturnValue = returnValue;
+
+ /// <summary>Gets the return value condition.</summary>
+ public bool ReturnValue { get; }
+ }
+
+ /// <summary>Specifies that when a method returns <see cref="ReturnValue"/>, the parameter will not be null even if the corresponding type allows it.</summary>
+ [AttributeUsage(AttributeTargets.Parameter, Inherited = false)]
+ internal sealed class NotNullWhenAttribute : Attribute
+ {
+ /// <summary>Initializes the attribute with the specified return value condition.</summary>
+ /// <param name="returnValue">
+ /// The return value condition. If the method returns this value, the associated parameter will not be null.
+ /// </param>
+ public NotNullWhenAttribute(bool returnValue) => ReturnValue = returnValue;
+
+ /// <summary>Gets the return value condition.</summary>
+ public bool ReturnValue { get; }
+ }
+
+ /// <summary>Specifies that the output will be non-null if the named parameter is non-null.</summary>
+ [AttributeUsage(AttributeTargets.Parameter | AttributeTargets.Property | AttributeTargets.ReturnValue, AllowMultiple = true, Inherited = false)]
+ internal sealed class NotNullIfNotNullAttribute : Attribute
+ {
+ /// <summary>Initializes the attribute with the associated parameter name.</summary>
+ /// <param name="parameterName">
+ /// The associated parameter name. The output will be non-null if the argument to the parameter specified is non-null.
+ /// </param>
+ public NotNullIfNotNullAttribute(string parameterName) => ParameterName = parameterName;
+
+ /// <summary>Gets the associated parameter name.</summary>
+ public string ParameterName { get; }
+ }
+
+ /// <summary>Applied to a method that will never return under any circumstance.</summary>
+ [AttributeUsage(AttributeTargets.Method, Inherited = false)]
+ internal sealed class DoesNotReturnAttribute : Attribute { }
+
+ /// <summary>Specifies that the method will not return if the associated Boolean parameter is passed the specified value.</summary>
+ [AttributeUsage(AttributeTargets.Parameter, Inherited = false)]
+ internal sealed class DoesNotReturnIfAttribute : Attribute
+ {
+ /// <summary>Initializes the attribute with the specified parameter value.</summary>
+ /// <param name="parameterValue">
+ /// The condition parameter value. Code after the method will be considered unreachable by diagnostics if the argument to
+ /// the associated parameter matches this value.
+ /// </param>
+ public DoesNotReturnIfAttribute(bool parameterValue) => ParameterValue = parameterValue;
+
+ /// <summary>Gets the condition parameter value.</summary>
+ public bool ParameterValue { get; }
+ }
+
+#endif
+
+#if !NETCOREAPP || NETCOREAPP3_1
+
+ /// <summary>Specifies that the method or property will ensure that the listed field and property members have not-null values.</summary>
+ [AttributeUsage(AttributeTargets.Method | AttributeTargets.Property, Inherited = false, AllowMultiple = true)]
+ internal sealed class MemberNotNullAttribute : Attribute
+ {
+ /// <summary>Initializes the attribute with a field or property member.</summary>
+ /// <param name="member">
+ /// The field or property member that is promised to be not-null.
+ /// </param>
+ public MemberNotNullAttribute(string member) => Members = new[] { member };
+
+ /// <summary>Initializes the attribute with the list of field and property members.</summary>
+ /// <param name="members">
+ /// The list of field and property members that are promised to be not-null.
+ /// </param>
+ public MemberNotNullAttribute(params string[] members) => Members = members;
+
+ /// <summary>Gets field or property member names.</summary>
+ public string[] Members { get; }
+ }
+
+ /// <summary>Specifies that the method or property will ensure that the listed field and property members have not-null values when returning with the specified return value condition.</summary>
+ [AttributeUsage(AttributeTargets.Method | AttributeTargets.Property, Inherited = false, AllowMultiple = true)]
+ internal sealed class MemberNotNullWhenAttribute : Attribute
+ {
+ /// <summary>Initializes the attribute with the specified return value condition and a field or property member.</summary>
+ /// <param name="returnValue">
+ /// The return value condition. If the method returns this value, the associated parameter will not be null.
+ /// </param>
+ /// <param name="member">
+ /// The field or property member that is promised to be not-null.
+ /// </param>
+ public MemberNotNullWhenAttribute(bool returnValue, string member)
+ {
+ ReturnValue = returnValue;
+ Members = new[] { member };
+ }
+
+ /// <summary>Initializes the attribute with the specified return value condition and list of field and property members.</summary>
+ /// <param name="returnValue">
+ /// The return value condition. If the method returns this value, the associated parameter will not be null.
+ /// </param>
+ /// <param name="members">
+ /// The list of field and property members that are promised to be not-null.
+ /// </param>
+ public MemberNotNullWhenAttribute(bool returnValue, params string[] members)
+ {
+ ReturnValue = returnValue;
+ Members = members;
+ }
+
+ /// <summary>Gets the return value condition.</summary>
+ public bool ReturnValue { get; }
+
+ /// <summary>Gets field or property member names.</summary>
+ public string[] Members { get; }
+ }
+
+#endif
+}