Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Mar 8, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 { }
Comment on lines +21 to +23
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got this from:

https://source.dot.net/#Microsoft.Build.Framework/NullableAttributes.cs,68093cc4b5713519

So we can use these NRT annotations in a netstandard2.0 library.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We did bad things in essentials:

#if !NETSTANDARD
		[return: NotNullIfNotNull("defaultValue")]
#endif

One day this can all go away and we use net6/7 everywhere.


/// <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