From 49f54b5275a87e6a06b21d214ee02562d5db0c17 Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Thu, 12 Sep 2019 13:34:43 +0200 Subject: [PATCH 1/7] Added failing test for #2912. --- .../Data/BindingTests.cs | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/tests/Avalonia.Markup.UnitTests/Data/BindingTests.cs b/tests/Avalonia.Markup.UnitTests/Data/BindingTests.cs index 0ba06980af..1cc007033b 100644 --- a/tests/Avalonia.Markup.UnitTests/Data/BindingTests.cs +++ b/tests/Avalonia.Markup.UnitTests/Data/BindingTests.cs @@ -196,6 +196,29 @@ namespace Avalonia.Markup.UnitTests.Data Assert.Equal("baz", target.Text); } + [Fact] + public void OneWayToSource_Binding_Should_Not_StackOverflow_With_Null_Value() + { + var target = new TextBlock { Text = null }; + var binding = new Binding + { + Path = "Foo", + Mode = BindingMode.OneWayToSource, + }; + + target.Bind(TextBox.TextProperty, binding); + + var source = new Source { Foo = "foo" }; + target.DataContext = source; + + Assert.Null(source.Foo); + + // When running tests under NCrunch, NCrunch replaces the standard StackOverflowException + // with its own, which will be caught by our code. Detect the stackoverflow anyway, by + // making sure the target property was only set once. + Assert.Equal(1, source.FooSetCount); + } + [Fact] public void Default_BindingMode_Should_Be_Used() { @@ -630,10 +653,13 @@ namespace Avalonia.Markup.UnitTests.Data set { _foo = value; + ++FooSetCount; RaisePropertyChanged(); } } + public int FooSetCount { get; private set; } + public event PropertyChangedEventHandler PropertyChanged; private void RaisePropertyChanged([CallerMemberName] string prop = "") From 9acf82d47b60799136bf6835e6332a7bf8da0c0a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dariusz=20Komosi=C5=84ski?= Date: Thu, 12 Sep 2019 16:38:35 +0200 Subject: [PATCH 2/7] !B Fix expression nodes not knowing if last value is actually null or dead. --- src/Avalonia.Base/Data/Core/ExpressionNode.cs | 8 ++++++-- src/Avalonia.Base/Data/Core/SettableNode.cs | 5 +++++ tests/Avalonia.Markup.UnitTests/Data/BindingTests.cs | 2 +- 3 files changed, 12 insertions(+), 3 deletions(-) diff --git a/src/Avalonia.Base/Data/Core/ExpressionNode.cs b/src/Avalonia.Base/Data/Core/ExpressionNode.cs index ce40b3e517..ca7d980bcf 100644 --- a/src/Avalonia.Base/Data/Core/ExpressionNode.cs +++ b/src/Avalonia.Base/Data/Core/ExpressionNode.cs @@ -8,9 +8,13 @@ namespace Avalonia.Data.Core public abstract class ExpressionNode { private static readonly object CacheInvalid = new object(); + protected static readonly WeakReference UnsetReference = new WeakReference(AvaloniaProperty.UnsetValue); + protected static readonly WeakReference NullReference = + new WeakReference(null); + private WeakReference _target = UnsetReference; private Action _subscriber; private bool _listening; @@ -98,7 +102,7 @@ namespace Avalonia.Data.Core if (notification == null) { - LastValue = new WeakReference(value); + LastValue = value != null ? new WeakReference(value) : NullReference; if (Next != null) { @@ -111,7 +115,7 @@ namespace Avalonia.Data.Core } else { - LastValue = new WeakReference(notification.Value); + LastValue = notification.Value != null ? new WeakReference(notification.Value) : NullReference; if (Next != null) { diff --git a/src/Avalonia.Base/Data/Core/SettableNode.cs b/src/Avalonia.Base/Data/Core/SettableNode.cs index eb98b9e8d6..d0a918dc88 100644 --- a/src/Avalonia.Base/Data/Core/SettableNode.cs +++ b/src/Avalonia.Base/Data/Core/SettableNode.cs @@ -29,6 +29,11 @@ namespace Avalonia.Data.Core if (!isLastValueAlive) { + if (value == null && LastValue == NullReference) + { + return true; + } + return false; } diff --git a/tests/Avalonia.Markup.UnitTests/Data/BindingTests.cs b/tests/Avalonia.Markup.UnitTests/Data/BindingTests.cs index 1cc007033b..cbb6abbf7f 100644 --- a/tests/Avalonia.Markup.UnitTests/Data/BindingTests.cs +++ b/tests/Avalonia.Markup.UnitTests/Data/BindingTests.cs @@ -216,7 +216,7 @@ namespace Avalonia.Markup.UnitTests.Data // When running tests under NCrunch, NCrunch replaces the standard StackOverflowException // with its own, which will be caught by our code. Detect the stackoverflow anyway, by // making sure the target property was only set once. - Assert.Equal(1, source.FooSetCount); + Assert.Equal(2, source.FooSetCount); } [Fact] From 661254c9cf142a2ade9664618e516cf3807ff80c Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Thu, 12 Sep 2019 16:45:37 +0200 Subject: [PATCH 3/7] Add issue number to test. --- tests/Avalonia.Markup.UnitTests/Data/BindingTests.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/Avalonia.Markup.UnitTests/Data/BindingTests.cs b/tests/Avalonia.Markup.UnitTests/Data/BindingTests.cs index cbb6abbf7f..a0a285ee4e 100644 --- a/tests/Avalonia.Markup.UnitTests/Data/BindingTests.cs +++ b/tests/Avalonia.Markup.UnitTests/Data/BindingTests.cs @@ -199,6 +199,7 @@ namespace Avalonia.Markup.UnitTests.Data [Fact] public void OneWayToSource_Binding_Should_Not_StackOverflow_With_Null_Value() { + // Issue #2912 var target = new TextBlock { Text = null }; var binding = new Binding { From 65e457a38b9b8d6b1f32ec52721980b248222576 Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Thu, 12 Sep 2019 16:51:41 +0200 Subject: [PATCH 4/7] Added OneWayToSource binding to BindingDemo. Now crashing with an NRE. --- samples/BindingDemo/MainWindow.xaml | 1 + 1 file changed, 1 insertion(+) diff --git a/samples/BindingDemo/MainWindow.xaml b/samples/BindingDemo/MainWindow.xaml index a232a06383..4cb7cc012e 100644 --- a/samples/BindingDemo/MainWindow.xaml +++ b/samples/BindingDemo/MainWindow.xaml @@ -24,6 +24,7 @@ + From 15e388da7521cc9a59bf625cade6886ab55570bb Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Thu, 12 Sep 2019 18:32:07 +0200 Subject: [PATCH 5/7] Adding failing binding tests. - `OneTime` bindings are failing to release their subscription if the `DataContext` is unset when the binding is made. - Combining `OneTime` and `OneWayToSource` bindings are causing an NRE. The NRE is being caught but it too results in a subscription being leaked. --- .../Data/BindingTests.cs | 41 ++++++++++++++++++- 1 file changed, 39 insertions(+), 2 deletions(-) diff --git a/tests/Avalonia.Markup.UnitTests/Data/BindingTests.cs b/tests/Avalonia.Markup.UnitTests/Data/BindingTests.cs index a0a285ee4e..7e053392c7 100644 --- a/tests/Avalonia.Markup.UnitTests/Data/BindingTests.cs +++ b/tests/Avalonia.Markup.UnitTests/Data/BindingTests.cs @@ -154,6 +154,18 @@ namespace Avalonia.Markup.UnitTests.Data Assert.Equal("bar", source.Foo); } + [Fact] + public void OneTime_Binding_Releases_Subscription_If_DataContext_Set_Later() + { + var target = new TextBlock(); + var source = new Source { Foo = "foo" }; + + target.Bind(TextBlock.TextProperty, new Binding("Foo", BindingMode.OneTime)); + target.DataContext = source; + + Assert.Equal(0, source.SubscriberCount); + } + [Fact] public void OneWayToSource_Binding_Should_Be_Set_Up() { @@ -567,6 +579,23 @@ namespace Avalonia.Markup.UnitTests.Data Assert.Equal(expected, child.DoubleValue); } + [Fact] + public void Combined_OneTime_And_OneWayToSource_Bindings_Should_Release_Subscriptions() + { + var target1 = new TextBlock(); + var target2 = new TextBlock(); + var root = new Panel { Children = { target1, target2 } }; + var source = new Source { Foo = "foo" }; + + using (target1.Bind(TextBlock.TextProperty, new Binding("Foo", BindingMode.OneTime))) + using (target2.Bind(TextBlock.TextProperty, new Binding("Foo", BindingMode.OneWayToSource))) + { + root.DataContext = source; + } + + Assert.Equal(0, source.SubscriberCount); + } + private class StyledPropertyClass : AvaloniaObject { public static readonly StyledProperty DoubleValueProperty = @@ -646,6 +675,7 @@ namespace Avalonia.Markup.UnitTests.Data public class Source : INotifyPropertyChanged { + private PropertyChangedEventHandler _propertyChanged; private string _foo; public string Foo @@ -661,11 +691,18 @@ namespace Avalonia.Markup.UnitTests.Data public int FooSetCount { get; private set; } - public event PropertyChangedEventHandler PropertyChanged; + + public int SubscriberCount { get; private set; } + + public event PropertyChangedEventHandler PropertyChanged + { + add { _propertyChanged += value; ++SubscriberCount; } + remove { _propertyChanged += value; --SubscriberCount; } + } private void RaisePropertyChanged([CallerMemberName] string prop = "") { - PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(prop)); + _propertyChanged?.Invoke(this, new PropertyChangedEventArgs(prop)); } } From d3f8d08a47ce1a65c3f975d9d614f749792e6b88 Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Thu, 12 Sep 2019 18:39:52 +0200 Subject: [PATCH 6/7] Make sure OneTime bindings unsubscribe. `OneTime` bindings unsubscribe when the first value is pushed, meaning that they unsubscribe _during_ `ExpressionNode.StartListeningCore`. Make sure we handle this by: - Setting `_listening = true` before calling `StartListeningCore` - Subscribe to current value in `InpcPropertyAccessorPlugin` before sending current value, because we're unsubscribed when `SendCurrentValue` exits --- src/Avalonia.Base/Data/Core/ExpressionNode.cs | 2 +- .../Data/Core/Plugins/InpcPropertyAccessorPlugin.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Avalonia.Base/Data/Core/ExpressionNode.cs b/src/Avalonia.Base/Data/Core/ExpressionNode.cs index ca7d980bcf..c2e5c8e4f3 100644 --- a/src/Avalonia.Base/Data/Core/ExpressionNode.cs +++ b/src/Avalonia.Base/Data/Core/ExpressionNode.cs @@ -140,8 +140,8 @@ namespace Avalonia.Data.Core } else if (target != AvaloniaProperty.UnsetValue) { - StartListeningCore(_target); _listening = true; + StartListeningCore(_target); } else { diff --git a/src/Avalonia.Base/Data/Core/Plugins/InpcPropertyAccessorPlugin.cs b/src/Avalonia.Base/Data/Core/Plugins/InpcPropertyAccessorPlugin.cs index 4716b45340..cbceb58204 100644 --- a/src/Avalonia.Base/Data/Core/Plugins/InpcPropertyAccessorPlugin.cs +++ b/src/Avalonia.Base/Data/Core/Plugins/InpcPropertyAccessorPlugin.cs @@ -103,8 +103,8 @@ namespace Avalonia.Data.Core.Plugins protected override void SubscribeCore() { - SendCurrentValue(); SubscribeToChanges(); + SendCurrentValue(); } protected override void UnsubscribeCore() From de4e69b0ba5f5ddbc67c2479076e3c1d1bfe85b4 Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Thu, 12 Sep 2019 19:07:24 +0200 Subject: [PATCH 7/7] Remove OneWayToSource binding from BindingDemo. Until #2983 is fixed. --- samples/BindingDemo/MainWindow.xaml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/samples/BindingDemo/MainWindow.xaml b/samples/BindingDemo/MainWindow.xaml index 4cb7cc012e..b57a9a0a9e 100644 --- a/samples/BindingDemo/MainWindow.xaml +++ b/samples/BindingDemo/MainWindow.xaml @@ -24,7 +24,9 @@ - +