Skip to content

Commit

Permalink
[controls] fix memory leak in VisualElement.Background (dotnet#13656)
Browse files Browse the repository at this point in the history
* [controls] fix memory leak in `VisualElement.Background`

Fixes: dotnet#12344
Fixes: dotnet#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 <[email protected]>
  • Loading branch information
jonathanpeppers and GitHub Actions Autoformatter authored Mar 8, 2023
1 parent 5397eef commit 58a42e5
Show file tree
Hide file tree
Showing 16 changed files with 401 additions and 148 deletions.
42 changes: 1 addition & 41 deletions src/Controls/src/Core/BindableLayout.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
}
}
}
58 changes: 21 additions & 37 deletions src/Controls/src/Core/BindingExpression.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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)));
}
}

Expand All @@ -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)
{
Expand All @@ -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;

Expand Down
152 changes: 152 additions & 0 deletions src/Controls/src/Core/Internals/MaybeNullWhenAttribute.cs
Original file line number Diff line number Diff line change
@@ -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
}
Loading

0 comments on commit 58a42e5

Please sign in to comment.