From 64518efc5184d67208a7d0c3b7df9c66a364a562 Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Wed, 22 Jun 2022 12:59:40 +0200 Subject: [PATCH 1/4] Add failing test for #8372. --- .../AvaloniaObjectTests_SetValue.cs | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_SetValue.cs b/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_SetValue.cs index 954a609315..72162a4d8e 100644 --- a/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_SetValue.cs +++ b/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_SetValue.cs @@ -17,6 +17,21 @@ namespace Avalonia.Base.UnitTests Assert.Equal("foodefault", target.GetValue(Class1.FooProperty)); } + [Fact] + public void ClearValue_Resets_Value_To_Style_value() + { + Class1 target = new Class1(); + + target.SetValue(Class1.FooProperty, "style", BindingPriority.Style); + target.SetValue(Class1.FooProperty, "local"); + + Assert.Equal("local", target.GetValue(Class1.FooProperty)); + + target.ClearValue(Class1.FooProperty); + + Assert.Equal("style", target.GetValue(Class1.FooProperty)); + } + [Fact] public void ClearValue_Raises_PropertyChanged() { From f33d4e881f3b4fb4db9a13279ad8bf1c648b3151 Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Wed, 22 Jun 2022 13:01:37 +0200 Subject: [PATCH 2/4] Correctly clear local value in PriorityValue. --- src/Avalonia.Base/PropertyStore/PriorityValue.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Avalonia.Base/PropertyStore/PriorityValue.cs b/src/Avalonia.Base/PropertyStore/PriorityValue.cs index 112cf6619f..182b2638c4 100644 --- a/src/Avalonia.Base/PropertyStore/PriorityValue.cs +++ b/src/Avalonia.Base/PropertyStore/PriorityValue.cs @@ -121,6 +121,7 @@ namespace Avalonia.PropertyStore public void ClearLocalValue() { + _localValue = default; UpdateEffectiveValue(new AvaloniaPropertyChangedEventArgs( _owner, Property, From c9e10f0d2f88346caeb461b900199b6fb571653d Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Wed, 22 Jun 2022 15:53:16 +0200 Subject: [PATCH 3/4] Added additional failing test. Exposed by the previous fix for #8372: re-entrancy in `PropertySetterInstance.Dispose()` is causing detaching a style to call `ClearValue` on the property. Previously this wasn't a problem as `ClearValue` didn't work, but now it is. (Also added one passing test which tests the same scenario in `PropertySetterBindingInstance` for future coverage) --- .../Styling/SetterTests.cs | 42 ++++++++++++++++--- 1 file changed, 36 insertions(+), 6 deletions(-) diff --git a/tests/Avalonia.Base.UnitTests/Styling/SetterTests.cs b/tests/Avalonia.Base.UnitTests/Styling/SetterTests.cs index ed4c78aa3e..c684466200 100644 --- a/tests/Avalonia.Base.UnitTests/Styling/SetterTests.cs +++ b/tests/Avalonia.Base.UnitTests/Styling/SetterTests.cs @@ -150,13 +150,43 @@ namespace Avalonia.Base.UnitTests.Styling Assert.Equal(BindingPriority.StyleTrigger, control.GetDiagnostic(TextBlock.TagProperty).Priority); } - private IBinding CreateMockBinding(AvaloniaProperty property) + [Fact] + public void Disposing_Setter_Should_Preserve_LocalValue() { - var subject = new Subject(); - var descriptor = InstancedBinding.OneWay(subject); - var binding = Mock.Of(x => - x.Initiate(It.IsAny(), property, null, false) == descriptor); - return binding; + var control = new Canvas(); + var setter = new Setter(TextBlock.TagProperty, "foo"); + + var instance = setter.Instance(control); + instance.Start(true); + instance.Activate(); + + control.Tag = "bar"; + + instance.Dispose(); + + Assert.Equal("bar", control.Tag); + } + + [Fact] + public void Disposing_Binding_Setter_Should_Preserve_LocalValue() + { + var control = new Canvas(); + var source = new { Foo = "foo" }; + var setter = new Setter(TextBlock.TagProperty, new Binding + { + Source = source, + Path = nameof(source.Foo), + }); + + var instance = setter.Instance(control); + instance.Start(true); + instance.Activate(); + + control.Tag = "bar"; + + instance.Dispose(); + + Assert.Equal("bar", control.Tag); } private class TestConverter : IValueConverter From 857bfb5bd2b863825c09fbfb780dc379fba4d345 Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Wed, 22 Jun 2022 16:00:29 +0200 Subject: [PATCH 4/4] Prevent re-entrancy in PropertySetterInstance.Dispose. The call to `_subscription.Dispose()` causes `BindingEntry.Dispose()` to call `_subscription.Dispose()`, but in this case the `BindingEntry._subscription` instance is the `PropertySetterInstance`! Except now `PropertySetterInstance._subscription` is null, and so `PropertySetterInstance.Dispose` called `ClearValue`, which is obviously wrong. --- .../Styling/PropertySetterInstance.cs | 27 ++++++++++++++----- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/src/Avalonia.Base/Styling/PropertySetterInstance.cs b/src/Avalonia.Base/Styling/PropertySetterInstance.cs index c4e8f47e67..9028224cc1 100644 --- a/src/Avalonia.Base/Styling/PropertySetterInstance.cs +++ b/src/Avalonia.Base/Styling/PropertySetterInstance.cs @@ -18,7 +18,7 @@ namespace Avalonia.Styling private readonly DirectPropertyBase? _directProperty; private readonly T _value; private IDisposable? _subscription; - private bool _isActive; + private State _state; public PropertySetterInstance( IStyleable target, @@ -40,6 +40,8 @@ namespace Avalonia.Styling _value = value; } + private bool IsActive => _state == State.Active; + public void Start(bool hasActivator) { if (hasActivator) @@ -70,31 +72,35 @@ namespace Avalonia.Styling public void Activate() { - if (!_isActive) + if (!IsActive) { - _isActive = true; + _state = State.Active; PublishNext(); } } public void Deactivate() { - if (_isActive) + if (IsActive) { - _isActive = false; + _state = State.Inactive; PublishNext(); } } public override void Dispose() { + if (_state == State.Disposed) + return; + _state = State.Disposed; + if (_subscription is object) { var sub = _subscription; _subscription = null; sub.Dispose(); } - else if (_isActive) + else if (IsActive) { if (_styledProperty is object) { @@ -114,7 +120,14 @@ namespace Avalonia.Styling private void PublishNext() { - PublishNext(_isActive ? new BindingValue(_value) : default); + PublishNext(IsActive ? new BindingValue(_value) : default); + } + + private enum State + { + Inactive, + Active, + Disposed, } } }