From a560c3b6d32bdebcf5e941fb768ed6ad5e9fbd11 Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Thu, 18 Aug 2016 00:19:15 +0200 Subject: [PATCH] Use WeakReference in BindingNotification. So that it doesn't keep objects alive when cached by `.Publish().RefCount()` in `ExpressionObserver`. Added a leak test to test that. --- src/Avalonia.Base/AvaloniaObject.cs | 2 +- src/Avalonia.Base/Data/BindingNotification.cs | 67 ++++++++++++++----- src/Avalonia.Base/PriorityValue.cs | 2 +- .../Avalonia.Markup/Data/ExpressionSubject.cs | 19 ++++-- .../ExpressionObserverTests.cs | 18 +++++ .../Data/BindingTests.cs | 5 +- 6 files changed, 86 insertions(+), 27 deletions(-) diff --git a/src/Avalonia.Base/AvaloniaObject.cs b/src/Avalonia.Base/AvaloniaObject.cs index 78d87daea8..0f2afc6474 100644 --- a/src/Avalonia.Base/AvaloniaObject.cs +++ b/src/Avalonia.Base/AvaloniaObject.cs @@ -577,7 +577,7 @@ namespace Avalonia { if (notification.HasValue) { - notification.Value = TypeUtilities.CastOrDefault(notification.Value, type); + notification.SetValue(TypeUtilities.CastOrDefault(notification.Value, type)); } return notification; diff --git a/src/Avalonia.Base/Data/BindingNotification.cs b/src/Avalonia.Base/Data/BindingNotification.cs index 1c8a8067f8..d61e9ad3b2 100644 --- a/src/Avalonia.Base/Data/BindingNotification.cs +++ b/src/Avalonia.Base/Data/BindingNotification.cs @@ -44,14 +44,19 @@ namespace Avalonia.Data public static readonly BindingNotification UnsetValue = new BindingNotification(AvaloniaProperty.UnsetValue); + // Null cannot be held in WeakReference as it's indistinguishable from an expired value so + // use this value in its place. + private static readonly object NullValue = new object(); + + private WeakReference _value; + /// /// Initializes a new instance of the class. /// /// The binding value. public BindingNotification(object value) { - Value = value; - HasValue = true; + _value = new WeakReference(value ?? NullValue); } /// @@ -66,7 +71,6 @@ namespace Avalonia.Data throw new ArgumentException($"'errorType' may not be None"); } - Value = AvaloniaProperty.UnsetValue; Error = error; ErrorType = errorType; } @@ -80,20 +84,42 @@ namespace Avalonia.Data public BindingNotification(Exception error, BindingErrorType errorType, object fallbackValue) : this(error, errorType) { - Value = fallbackValue; - HasValue = true; + _value = new WeakReference(fallbackValue ?? NullValue); } /// /// Gets the value that should be passed to the target when /// is true. /// - public object Value { get; set; } + /// + /// If this property is read when is false then it will return + /// . + /// + public object Value + { + get + { + if (_value != null) + { + object result; + + if (_value.TryGetTarget(out result)) + { + return result == NullValue ? null : result; + } + } + + // There's the possibility of a race condition in that HasValue can return true, + // and then the value is GC'd before Value is read. We should be ok though as + // we return UnsetValue which should be a safe alternative. + return AvaloniaProperty.UnsetValue; + } + } /// /// Gets a value indicating whether should be pushed to the target. /// - public bool HasValue { get; set; } + public bool HasValue => _value != null; /// /// Gets the error that occurred on the source, if any. @@ -179,14 +205,7 @@ namespace Avalonia.Data Contract.Requires(e != null); Contract.Requires(type != BindingErrorType.None); - if (Error != null) - { - Error = new AggregateException(Error, e); - } - else - { - Error = e; - } + Error = Error != null ? new AggregateException(Error, e) : e; if (type == BindingErrorType.Error || ErrorType == BindingErrorType.Error) { @@ -194,10 +213,26 @@ namespace Avalonia.Data } } + /// + /// Removes the and makes return null. + /// + public void ClearValue() + { + _value = null; + } + + /// + /// Sets the . + /// + public void SetValue(object value) + { + _value = new WeakReference(value ?? NullValue); + } + private static bool ExceptionEquals(Exception a, Exception b) { return a?.GetType() == b?.GetType() && - a.Message == b.Message; + a?.Message == b?.Message; } } } diff --git a/src/Avalonia.Base/PriorityValue.cs b/src/Avalonia.Base/PriorityValue.cs index cfe6e8818d..11bd6c61cd 100644 --- a/src/Avalonia.Base/PriorityValue.cs +++ b/src/Avalonia.Base/PriorityValue.cs @@ -259,7 +259,7 @@ namespace Avalonia if (notification?.HasValue == true) { - notification.Value = castValue; + notification.SetValue(castValue); } if (notification == null || notification.HasValue) diff --git a/src/Markup/Avalonia.Markup/Data/ExpressionSubject.cs b/src/Markup/Avalonia.Markup/Data/ExpressionSubject.cs index 5a7bb80e87..85609c247c 100644 --- a/src/Markup/Avalonia.Markup/Data/ExpressionSubject.cs +++ b/src/Markup/Avalonia.Markup/Data/ExpressionSubject.cs @@ -255,7 +255,7 @@ namespace Avalonia.Markup.Data } } - private BindingNotification Merge(object a, BindingNotification b) + private static BindingNotification Merge(object a, BindingNotification b) { var an = a as BindingNotification; @@ -270,7 +270,7 @@ namespace Avalonia.Markup.Data } } - private BindingNotification Merge(BindingNotification a, object b) + private static BindingNotification Merge(BindingNotification a, object b) { var bn = b as BindingNotification; @@ -280,17 +280,22 @@ namespace Avalonia.Markup.Data } else { - a.Value = b; - a.HasValue = true; + a.SetValue(b); } return a; } - private BindingNotification Merge(BindingNotification a, BindingNotification b) + private static BindingNotification Merge(BindingNotification a, BindingNotification b) { - a.Value = b.Value; - a.HasValue = b.HasValue; + if (b.HasValue) + { + a.SetValue(b.Value); + } + else + { + a.ClearValue(); + } if (b.Error != null) { diff --git a/tests/Avalonia.LeakTests/ExpressionObserverTests.cs b/tests/Avalonia.LeakTests/ExpressionObserverTests.cs index 11eb232b82..3dbc62424f 100644 --- a/tests/Avalonia.LeakTests/ExpressionObserverTests.cs +++ b/tests/Avalonia.LeakTests/ExpressionObserverTests.cs @@ -35,6 +35,24 @@ namespace Avalonia.LeakTests Assert.Equal(0, memory.GetObjects(where => where.Type.Is>()).ObjectsCount)); } + [Fact] + public void Should_Not_Keep_Source_Alive_ObservableCollection_With_DataValidation() + { + Func run = () => + { + var source = new { Foo = new AvaloniaList { "foo", "bar" } }; + var target = new ExpressionObserver(source, "Foo", true); + + target.Subscribe(_ => { }); + return target; + }; + + var result = run(); + + dotMemory.Check(memory => + Assert.Equal(0, memory.GetObjects(where => where.Type.Is>()).ObjectsCount)); + } + [Fact] public void Should_Not_Keep_Source_Alive_NonIntegerIndexer() { diff --git a/tests/Avalonia.Markup.Xaml.UnitTests/Data/BindingTests.cs b/tests/Avalonia.Markup.Xaml.UnitTests/Data/BindingTests.cs index 4ea281a79c..b07a031e15 100644 --- a/tests/Avalonia.Markup.Xaml.UnitTests/Data/BindingTests.cs +++ b/tests/Avalonia.Markup.Xaml.UnitTests/Data/BindingTests.cs @@ -166,15 +166,16 @@ namespace Avalonia.Markup.Xaml.UnitTests.Data [Fact] public void DataContext_Binding_Should_Produce_Correct_Results() { + var viewModel = new { Foo = "bar" }; var root = new Decorator { - DataContext = new { Foo = "bar" }, + DataContext = viewModel, }; var child = new Control(); var values = new List(); - child.GetObservable(Border.DataContextProperty).Subscribe(x => values.Add(x)); + child.GetObservable(Control.DataContextProperty).Subscribe(x => values.Add(x)); child.Bind(Control.DataContextProperty, new Binding("Foo")); // When binding to DataContext and the target isn't found, the binding should produce