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

[core] fix memory leaks in bindings #13327

Merged
merged 2 commits into from
Feb 17, 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
10 changes: 5 additions & 5 deletions src/Controls/src/Core/BindableObject.cs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ public BindableObject()

readonly Dictionary<BindableProperty, BindablePropertyContext> _properties = new Dictionary<BindableProperty, BindablePropertyContext>(4);
bool _applying;
object _inheritedContext;
WeakReference _inheritedContext;

Comment on lines -34 to 35
Copy link
Member Author

Choose a reason for hiding this comment

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

Lastly, I found that VisualElement.BackgroundProperty.BindingContext held an instance of a ViewModel. This was the value of Brush.Default._inheritedContext.

/// <include file="../../docs/Microsoft.Maui.Controls/BindableObject.xml" path="//Member[@MemberName='BindingContextProperty']/Docs/*" />
public static readonly BindableProperty BindingContextProperty =
Expand All @@ -41,7 +41,7 @@ public BindableObject()
/// <include file="../../docs/Microsoft.Maui.Controls/BindableObject.xml" path="//Member[@MemberName='BindingContext']/Docs/*" />
public object BindingContext
{
get => _inheritedContext ?? GetValue(BindingContextProperty);
get => _inheritedContext?.Target ?? GetValue(BindingContextProperty);
set => SetValue(BindingContextProperty, value);
}

Expand Down Expand Up @@ -238,7 +238,7 @@ public static void SetInheritedBindingContext(BindableObject bindable, object va
if (bpContext != null && ((bpContext.Attributes & BindableContextAttributes.IsManuallySet) != 0))
return;

object oldContext = bindable._inheritedContext;
object oldContext = bindable._inheritedContext?.Target;

if (ReferenceEquals(oldContext, value))
return;
Expand All @@ -253,7 +253,7 @@ public static void SetInheritedBindingContext(BindableObject bindable, object va
}
else
{
bindable._inheritedContext = value;
bindable._inheritedContext = new WeakReference(value);
}

bindable.ApplyBindings(skipBindingContext: false, fromBindingContextChanged: true);
Expand Down Expand Up @@ -557,7 +557,7 @@ internal void ApplyBindings(bool skipBindingContext, bool fromBindingContextChan

static void BindingContextPropertyBindingChanging(BindableObject bindable, BindingBase oldBindingBase, BindingBase newBindingBase)
{
object context = bindable._inheritedContext;
object context = bindable._inheritedContext?.Target;
var oldBinding = oldBindingBase as Binding;
var newBinding = newBindingBase as Binding;

Expand Down
8 changes: 7 additions & 1 deletion src/Controls/src/Core/BindingExpression.cs
Original file line number Diff line number Diff line change
Expand Up @@ -635,7 +635,7 @@ public void SubscribeTo(INotifyPropertyChanged source, PropertyChangedEventHandl
_listener.SetTarget(listener);
}

public void Unsubscribe()
public void Unsubscribe(bool finalizer = false)
{
INotifyPropertyChanged source;
if (_source.TryGetTarget(out source) && source != null)
Expand All @@ -644,6 +644,10 @@ public void Unsubscribe()
if (bo != null)
bo.BindingContextChanged -= _bchandler;

// If we are called from a finalizer, WeakReference<T>.SetTarget() can throw InvalidOperationException
if (finalizer)
return;
Comment on lines +647 to +649
Copy link
Member Author

Choose a reason for hiding this comment

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


_source.SetTarget(null);
_listener.SetTarget(null);
}
Expand All @@ -668,6 +672,8 @@ class BindingExpressionPart
readonly PropertyChangedEventHandler _changeHandler;
WeakPropertyChangedProxy _listener;

~BindingExpressionPart() => _listener?.Unsubscribe(finalizer: true);
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the core fix, that allows WeakPropertyChangedProxy to be freed appropriately.


public BindingExpressionPart(BindingExpression expression, string content, bool isIndexer = false)
{
_expression = expression;
Expand Down
6 changes: 2 additions & 4 deletions src/Controls/src/Core/Brush.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,9 @@ namespace Microsoft.Maui.Controls
[System.ComponentModel.TypeConverter(typeof(BrushTypeConverter))]
public abstract partial class Brush : Element
{
static ImmutableBrush defaultBrush;
/// <include file="../../docs/Microsoft.Maui.Controls/Brush.xml" path="//Member[@MemberName='Default']/Docs/*" />
public static Brush Default
{
get { return new SolidColorBrush(null); }
}
public static Brush Default => defaultBrush ??= new(null);

public static implicit operator Brush(Color color) => new SolidColorBrush(color);

Expand Down
3 changes: 3 additions & 0 deletions src/Controls/src/Core/TypedBinding.cs
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,9 @@ class PropertyChangedProxy
public BindingExpression.WeakPropertyChangedProxy Listener { get; }
readonly BindingBase _binding;
PropertyChangedEventHandler handler;

~PropertyChangedProxy() => Listener?.Unsubscribe(finalizer: true);

public INotifyPropertyChanged Part
{
get
Expand Down
77 changes: 77 additions & 0 deletions src/Controls/tests/Core.UnitTests/BindingUnitTests.cs
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
using System;
using System.Collections;
using System.Collections.Generic;
using System.ComponentModel;
using System.Globalization;
using System.Linq;
using System.Reflection;
using System.Runtime.CompilerServices;
using System.Threading.Tasks;
using Xunit;
Expand Down Expand Up @@ -1185,6 +1187,81 @@ public void ValueUpdatedWithSelfPathOnTwoWayBinding(bool isDefault)
AssertNoErrorsLogged();
}


[Theory, Category("[Binding] Complex paths")]
[InlineData(BindingMode.OneWay)]
[InlineData(BindingMode.OneWayToSource)]
[InlineData(BindingMode.TwoWay)]
public async Task WeakPropertyChangedProxyDoesNotLeak(BindingMode mode)
{
var proxies = new List<WeakReference>();
WeakReference weakViewModel = null, weakBindable = null;

int i = 0;
void create ()
{
if (i++ < 1024)
{
create();
return;
}

var binding = new Binding("Model.Model[1]");
var bindable = new MockBindable();
weakBindable = new WeakReference(bindable);

var viewmodel = new ComplexMockViewModel
{
Model = new ComplexMockViewModel
{
Model = new ComplexMockViewModel()
}
};

weakViewModel = new WeakReference(viewmodel);

bindable.BindingContext = viewmodel;
bindable.SetBinding(MockBindable.TextProperty, binding);

// Access private members:
// WeakPropertyChangedProxy proxy = binding._expression._parts[i]._listener;
var flags = BindingFlags.NonPublic | BindingFlags.Instance;
var expression = binding.GetType().GetField("_expression", flags).GetValue(binding);
Assert.NotNull(expression);
var parts = expression.GetType().GetField("_parts", flags).GetValue(expression) as IEnumerable;
Assert.NotNull(parts);
foreach (var part in parts)
{
var listener = part.GetType().GetField("_listener", flags).GetValue(part);
if (listener == null)
continue;
proxies.Add(new WeakReference(listener));
}
Assert.NotEmpty(proxies); // Should be at least 1
};
create();

await Task.Yield();
GC.Collect();
GC.WaitForPendingFinalizers();

if (mode == BindingMode.TwoWay || mode == BindingMode.OneWay)
Assert.False(weakViewModel.IsAlive, "ViewModel wasn't collected");
Comment on lines +1248 to +1249
Copy link
Member Author

Choose a reason for hiding this comment

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

Before I changed _inheritedContext to a WeakReference, this assertion would always fail. Brush.Default was holding onto the ViewModel!


if (mode == BindingMode.TwoWay || mode == BindingMode.OneWayToSource)
Assert.False(weakBindable.IsAlive, "Bindable wasn't collected");

// WeakPropertyChangedProxy won't go away until the second GC, BindingExpressionPart unsubscribes in its finalizer
await Task.Yield();
GC.Collect();
GC.WaitForPendingFinalizers();

foreach (var proxy in proxies)
{
Assert.False(proxy.IsAlive, "WeakPropertyChangedProxy wasn't collected");
}
}

[Theory, Category("[Binding] Complex paths")]
[InlineData(BindingMode.OneWay)]
[InlineData(BindingMode.OneWayToSource)]
Expand Down
19 changes: 18 additions & 1 deletion src/Controls/tests/Core.UnitTests/TypedBindingUnitTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System.Diagnostics;
using System.Globalization;
using System.Linq;
using System.Reflection;
using System.Runtime.CompilerServices;
using System.Threading.Tasks;
using Microsoft.Maui.Controls.Internals;
Expand Down Expand Up @@ -1452,7 +1453,7 @@ public async Task BindingUnsubscribesForDeadTarget()
public async Task BindingDoesNotStayAliveForDeadTarget()
{
var viewModel = new TestViewModel();
WeakReference bindingRef = null, buttonRef = null;
WeakReference bindingRef = null, buttonRef = null, proxyRef = null;

int i = 0;
Action create = null;
Expand All @@ -1473,6 +1474,16 @@ public async Task BindingDoesNotStayAliveForDeadTarget()

bindingRef = new WeakReference(binding);
buttonRef = new WeakReference(button);

// Access private members:
// WeakPropertyChangedProxy proxy = binding._handlers[0].Listener;
var flags = BindingFlags.NonPublic | BindingFlags.Instance;
var handlers = binding.GetType().GetField("_handlers", flags).GetValue(binding) as object[];
Assert.NotNull(handlers);
var handler = handlers[0];
var proxy = handler.GetType().GetProperty("Listener").GetValue(handler);
Assert.NotNull(proxy);
proxyRef = new WeakReference(proxy);
};

create();
Expand All @@ -1486,6 +1497,12 @@ public async Task BindingDoesNotStayAliveForDeadTarget()

Assert.False(bindingRef.IsAlive, "Binding should not be alive!");
Assert.False(buttonRef.IsAlive, "Button should not be alive!");

// WeakPropertyChangedProxy won't go away until the second GC, PropertyChangedProxy unsubscribes in its finalizer
await Task.Yield();
GC.Collect();
GC.WaitForPendingFinalizers();
Assert.False(proxyRef.IsAlive, "WeakPropertyChangedProxy should not be alive!");
}

[Fact]
Expand Down