diff --git a/src/Avalonia.Base/AvaloniaObject.cs b/src/Avalonia.Base/AvaloniaObject.cs index 8b93cf2fb1..34278c397f 100644 --- a/src/Avalonia.Base/AvaloniaObject.cs +++ b/src/Avalonia.Base/AvaloniaObject.cs @@ -51,6 +51,21 @@ namespace Avalonia /// private EventHandler _propertyChanged; + private DeferredSetter _directDeferredSetter; + + /// + /// Delayed setter helper for direct properties. Used to fix #855. + /// + private DeferredSetter DirectPropertyDeferredSetter + { + get + { + return _directDeferredSetter ?? + (_directDeferredSetter = new DeferredSetter()); + } + } + + /// /// Initializes a new instance of the class. /// @@ -552,6 +567,45 @@ namespace Avalonia } } + /// + /// A callback type for encapsulating complex logic for setting direct properties. + /// + /// The type of the property. + /// The value to which to set the property. + /// The backing field for the property. + /// A wrapper for the property-changed notification. + protected delegate void SetAndRaiseCallback(T value, ref T field, Action notifyWrapper); + + /// + /// Sets the backing field for a direct avalonia property, raising the + /// event if the value has changed. + /// + /// The type of the property. + /// The property. + /// The backing field. + /// A callback called to actually set the value to the backing field. + /// The value. + /// + /// True if the value changed, otherwise false. + /// + protected bool SetAndRaise( + AvaloniaProperty property, + ref T field, + SetAndRaiseCallback setterCallback, + T value) + { + Contract.Requires(setterCallback != null); + return DirectPropertyDeferredSetter.SetAndNotify( + property, + ref field, + (object val, ref T backing, Action notify) => + { + setterCallback((T)val, ref backing, notify); + return true; + }, + value); + } + /// /// Sets the backing field for a direct avalonia property, raising the /// event if the value has changed. @@ -566,17 +620,32 @@ namespace Avalonia protected bool SetAndRaise(AvaloniaProperty property, ref T field, T value) { VerifyAccess(); - if (!object.Equals(field, value)) - { - var old = field; - field = value; - RaisePropertyChanged(property, old, value, BindingPriority.LocalValue); - return true; - } - else - { - return false; - } + return SetAndRaise( + property, + ref field, + (T val, ref T backing, Action notifyWrapper) + => SetAndRaiseCore(property, ref backing, val, notifyWrapper), + value); + } + + /// + /// Default assignment logic for SetAndRaise. + /// + /// The type of the property. + /// The property. + /// The backing field. + /// The value. + /// A wrapper for the property-changed notification. + /// + /// True if the value changed, otherwise false. + /// + private bool SetAndRaiseCore(AvaloniaProperty property, ref T field, T value, Action notifyWrapper) + { + var old = field; + field = value; + + notifyWrapper(() => RaisePropertyChanged(property, old, value, BindingPriority.LocalValue)); + return true; } /// diff --git a/src/Avalonia.Base/AvaloniaObjectExtensions.cs b/src/Avalonia.Base/AvaloniaObjectExtensions.cs index e96679e643..1da2ecb942 100644 --- a/src/Avalonia.Base/AvaloniaObjectExtensions.cs +++ b/src/Avalonia.Base/AvaloniaObjectExtensions.cs @@ -138,17 +138,9 @@ namespace Avalonia AvaloniaProperty property, BindingPriority priority = BindingPriority.LocalValue) { - // TODO: Subject.Create is not yet in stable Rx : once it is, remove the - // AnonymousSubject classes and use Subject.Create. - var output = new Subject(); - var result = new AnonymousSubject( - Observer.Create( - x => output.OnNext(x), - e => output.OnError(e), - () => output.OnCompleted()), + return Subject.Create( + Observer.Create(x => o.SetValue(property, x, priority)), o.GetObservable(property)); - o.Bind(property, output, priority); - return result; } /// @@ -169,17 +161,9 @@ namespace Avalonia AvaloniaProperty property, BindingPriority priority = BindingPriority.LocalValue) { - // TODO: Subject.Create is not yet in stable Rx : once it is, remove the - // AnonymousSubject classes from this file and use Subject.Create. - var output = new Subject(); - var result = new AnonymousSubject( - Observer.Create( - x => output.OnNext(x), - e => output.OnError(e), - () => output.OnCompleted()), + return Subject.Create( + Observer.Create(x => o.SetValue(property, x, priority)), o.GetObservable(property)); - o.Bind(property, output, priority); - return result; } /// diff --git a/src/Avalonia.Base/PriorityValue.cs b/src/Avalonia.Base/PriorityValue.cs index 12a9e20528..9b5318083a 100644 --- a/src/Avalonia.Base/PriorityValue.cs +++ b/src/Avalonia.Base/PriorityValue.cs @@ -28,8 +28,10 @@ namespace Avalonia { private readonly Type _valueType; private readonly SingleOrDictionary _levels = new SingleOrDictionary(); - private object _value; + private readonly Func _validate; + private static readonly DeferredSetter delayedSetter = new DeferredSetter(); + private (object value, int priority) _value; /// /// Initializes a new instance of the class. @@ -47,8 +49,7 @@ namespace Avalonia Owner = owner; Property = property; _valueType = valueType; - _value = AvaloniaProperty.UnsetValue; - ValuePriority = int.MaxValue; + _value = (AvaloniaProperty.UnsetValue, int.MaxValue); _validate = validate; } @@ -77,16 +78,12 @@ namespace Avalonia /// /// Gets the current value. /// - public object Value => _value; + public object Value => _value.value; /// /// Gets the priority of the binding that is currently active. /// - public int ValuePriority - { - get; - private set; - } + public int ValuePriority => _value.priority; /// /// Adds a new binding. @@ -246,25 +243,36 @@ namespace Avalonia /// The priority level that the value came from. private void UpdateValue(object value, int priority) { - var notification = value as BindingNotification; + delayedSetter.SetAndNotify(this, + ref _value, + UpdateCore, + (value, priority)); + } + + private bool UpdateCore( + (object value, int priority) update, + ref (object value, int priority) backing, + Action notify) + { + var val = update.value; + var notification = val as BindingNotification; object castValue; if (notification != null) { - value = (notification.HasValue) ? notification.Value : null; + val = (notification.HasValue) ? notification.Value : null; } - if (TypeUtilities.TryConvertImplicit(_valueType, value, out castValue)) + if (TypeUtilities.TryConvertImplicit(_valueType, val, out castValue)) { - var old = _value; + var old = backing.value; if (_validate != null && castValue != AvaloniaProperty.UnsetValue) { castValue = _validate(castValue); } - ValuePriority = priority; - _value = castValue; + backing = (castValue, update.priority); if (notification?.HasValue == true) { @@ -273,7 +281,7 @@ namespace Avalonia if (notification == null || notification.HasValue) { - Owner?.Changed(this, old, _value); + notify(() => Owner?.Changed(this, old, Value)); } if (notification != null) @@ -284,14 +292,15 @@ namespace Avalonia else { Logger.Error( - LogArea.Binding, + LogArea.Binding, Owner, "Binding produced invalid value for {$Property} ({$PropertyType}): {$Value} ({$ValueType})", - Property.Name, - _valueType, - value, - value?.GetType()); + Property.Name, + _valueType, + val, + val?.GetType()); } + return true; } } } diff --git a/src/Avalonia.Base/Reactive/AnonymousSubject`1.cs b/src/Avalonia.Base/Reactive/AnonymousSubject`1.cs deleted file mode 100644 index c7bed15ecb..0000000000 --- a/src/Avalonia.Base/Reactive/AnonymousSubject`1.cs +++ /dev/null @@ -1,16 +0,0 @@ -// Copyright (c) The Avalonia Project. All rights reserved. -// Licensed under the MIT license. See licence.md file in the project root for full license information. - -using System; -using System.Reactive.Subjects; - -namespace Avalonia.Reactive -{ - public class AnonymousSubject : AnonymousSubject, ISubject - { - public AnonymousSubject(IObserver observer, IObservable observable) - : base(observer, observable) - { - } - } -} diff --git a/src/Avalonia.Base/Reactive/AnonymousSubject`2.cs b/src/Avalonia.Base/Reactive/AnonymousSubject`2.cs deleted file mode 100644 index 18551d564e..0000000000 --- a/src/Avalonia.Base/Reactive/AnonymousSubject`2.cs +++ /dev/null @@ -1,49 +0,0 @@ -// Copyright (c) The Avalonia Project. All rights reserved. -// Licensed under the MIT license. See licence.md file in the project root for full license information. - -using System; -using System.Reactive.Subjects; - -namespace Avalonia.Reactive -{ - public class AnonymousSubject : ISubject - { - private readonly IObserver _observer; - private readonly IObservable _observable; - - public AnonymousSubject(IObserver observer, IObservable observable) - { - _observer = observer; - _observable = observable; - } - - public void OnCompleted() - { - _observer.OnCompleted(); - } - - public void OnError(Exception error) - { - if (error == null) - throw new ArgumentNullException("error"); - - _observer.OnError(error); - } - - public void OnNext(T value) - { - _observer.OnNext(value); - } - - public IDisposable Subscribe(IObserver observer) - { - if (observer == null) - throw new ArgumentNullException("observer"); - - // - // [OK] Use of unsafe Subscribe: non-pretentious wrapping of an observable sequence. - // - return _observable.Subscribe/*Unsafe*/(observer); - } - } -} diff --git a/src/Avalonia.Base/Utilities/DeferredSetter.cs b/src/Avalonia.Base/Utilities/DeferredSetter.cs new file mode 100644 index 0000000000..fdfa160134 --- /dev/null +++ b/src/Avalonia.Base/Utilities/DeferredSetter.cs @@ -0,0 +1,156 @@ +using System; +using System.Collections.Generic; +using System.Reactive.Disposables; +using System.Runtime.CompilerServices; +using System.Text; + +namespace Avalonia.Utilities +{ + /// + /// A utility class to enable deferring assignment until after property-changed notifications are sent. + /// + /// The type of the object that represents the property. + /// The type of value with which to track the delayed assignment. + class DeferredSetter + where TProperty: class + { + private struct NotifyDisposable : IDisposable + { + private readonly SettingStatus status; + + internal NotifyDisposable(SettingStatus status) + { + this.status = status; + status.Notifying = true; + } + + public void Dispose() + { + status.Notifying = false; + } + } + + /// + /// Information on current setting/notification status of a property. + /// + private class SettingStatus + { + public bool Notifying { get; set; } + + private Queue pendingValues; + + public Queue PendingValues + { + get + { + return pendingValues ?? (pendingValues = new Queue()); + } + } + } + + private readonly ConditionalWeakTable setRecords = new ConditionalWeakTable(); + + /// + /// Mark the property as currently notifying. + /// + /// The property to mark as notifying. + /// Returns a disposable that when disposed, marks the property as done notifying. + private NotifyDisposable MarkNotifying(TProperty property) + { + Contract.Requires(!IsNotifying(property)); + + return new NotifyDisposable(setRecords.GetOrCreateValue(property)); + } + + /// + /// Check if the property is currently notifying listeners. + /// + /// The property. + /// If the property is currently notifying listeners. + private bool IsNotifying(TProperty property) + => setRecords.TryGetValue(property, out var value) && value.Notifying; + + /// + /// Add a pending assignment for the property. + /// + /// The property. + /// The value to assign. + private void AddPendingSet(TProperty property, TSetRecord value) + { + Contract.Requires(IsNotifying(property)); + + setRecords.GetOrCreateValue(property).PendingValues.Enqueue(value); + } + + /// + /// Checks if there are any pending assignments for the property. + /// + /// The property to check. + /// If the property has any pending assignments. + private bool HasPendingSet(TProperty property) + { + return setRecords.TryGetValue(property, out var status) && status.PendingValues.Count != 0; + } + + /// + /// Gets the first pending assignment for the property. + /// + /// The property to check. + /// The first pending assignment for the property. + private TSetRecord GetFirstPendingSet(TProperty property) + { + return setRecords.GetOrCreateValue(property).PendingValues.Dequeue(); + } + + public delegate bool SetterDelegate(TSetRecord record, ref TValue backing, Action notifyCallback); + + /// + /// Set the property and notify listeners while ensuring we don't get into a stack overflow as happens with #855 and #824 + /// + /// The property to set. + /// The backing field for the property + /// + /// A callback that actually sets the property. + /// The first parameter is the value to set, and the second is a wrapper that takes a callback that sends the property-changed notification. + /// + /// The value to try to set. + public bool SetAndNotify( + TProperty property, + ref TValue backing, + SetterDelegate setterCallback, + TSetRecord value) + { + Contract.Requires(setterCallback != null); + if (!IsNotifying(property)) + { + bool updated = false; + if (!object.Equals(value, backing)) + { + updated = setterCallback(value, ref backing, notification => + { + using (MarkNotifying(property)) + { + notification(); + } + }); + } + while (HasPendingSet(property)) + { + updated |= setterCallback(GetFirstPendingSet(property), ref backing, notification => + { + using (MarkNotifying(property)) + { + notification(); + } + }); + } + return updated; + } + else if(!object.Equals(value, backing)) + { + AddPendingSet(property, value); + } + return false; + } + } +} diff --git a/src/Avalonia.Controls/Primitives/SelectingItemsControl.cs b/src/Avalonia.Controls/Primitives/SelectingItemsControl.cs index 563d394919..2e668fda95 100644 --- a/src/Avalonia.Controls/Primitives/SelectingItemsControl.cs +++ b/src/Avalonia.Controls/Primitives/SelectingItemsControl.cs @@ -151,15 +151,23 @@ namespace Avalonia.Controls.Primitives { if (_updateCount == 0) { - var old = SelectedIndex; - var effective = (value >= 0 && value < Items?.Cast().Count()) ? value : -1; - - if (old != effective) + SetAndRaise(SelectedIndexProperty, ref _selectedIndex, (int val, ref int backing, Action notifyWrapper) => { - _selectedIndex = effective; - RaisePropertyChanged(SelectedIndexProperty, old, effective, BindingPriority.LocalValue); - SelectedItem = ElementAt(Items, effective); - } + var old = backing; + var effective = (val >= 0 && val < Items?.Cast().Count()) ? val : -1; + + if (old != effective) + { + backing = effective; + notifyWrapper(() => + RaisePropertyChanged( + SelectedIndexProperty, + old, + effective, + BindingPriority.LocalValue)); + SelectedItem = ElementAt(Items, effective); + } + }, value); } else { @@ -183,31 +191,41 @@ namespace Avalonia.Controls.Primitives { if (_updateCount == 0) { - var old = SelectedItem; - var index = IndexOf(Items, value); - var effective = index != -1 ? value : null; - - if (!object.Equals(effective, old)) + SetAndRaise(SelectedItemProperty, ref _selectedItem, (object val, ref object backing, Action notifyWrapper) => { - _selectedItem = effective; - RaisePropertyChanged(SelectedItemProperty, old, effective, BindingPriority.LocalValue); - SelectedIndex = index; + var old = backing; + var index = IndexOf(Items, val); + var effective = index != -1 ? val : null; - if (effective != null) + if (!object.Equals(effective, old)) { - if (SelectedItems.Count != 1 || SelectedItems[0] != effective) + backing = effective; + + notifyWrapper(() => + RaisePropertyChanged( + SelectedItemProperty, + old, + effective, + BindingPriority.LocalValue)); + + SelectedIndex = index; + + if (effective != null) + { + if (SelectedItems.Count != 1 || SelectedItems[0] != effective) + { + _syncingSelectedItems = true; + SelectedItems.Clear(); + SelectedItems.Add(effective); + _syncingSelectedItems = false; + } + } + else if (SelectedItems.Count > 0) { - _syncingSelectedItems = true; SelectedItems.Clear(); - SelectedItems.Add(effective); - _syncingSelectedItems = false; } } - else if (SelectedItems.Count > 0) - { - SelectedItems.Clear(); - } - } + }, value); } else { diff --git a/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Binding.cs b/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Binding.cs index cd77d9cc88..c75150ca6d 100644 --- a/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Binding.cs +++ b/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Binding.cs @@ -2,22 +2,22 @@ // Licensed under the MIT license. See licence.md file in the project root for full license information. using System; -using System.Collections; -using System.Collections.Generic; +using System.ComponentModel; +using System.Reactive.Concurrency; using System.Reactive.Linq; using System.Reactive.Subjects; -using Microsoft.Reactive.Testing; +using System.Threading; +using System.Threading.Tasks; using Avalonia.Data; using Avalonia.Logging; -using Avalonia.UnitTests; -using Xunit; -using System.Threading.Tasks; +using Avalonia.Markup.Xaml.Data; using Avalonia.Platform; -using System.Threading; -using Moq; -using System.Reactive.Disposables; -using System.Reactive.Concurrency; using Avalonia.Threading; +using Avalonia.UnitTests; +using Avalonia.Diagnostics; +using Microsoft.Reactive.Testing; +using Moq; +using Xunit; namespace Avalonia.Base.UnitTests { @@ -363,7 +363,7 @@ namespace Avalonia.Base.UnitTests Assert.True(called); } } - + [Fact] public async Task Bind_With_Scheduler_Executes_On_Scheduler() { @@ -387,6 +387,37 @@ namespace Avalonia.Base.UnitTests } } + [Fact] + public void SetValue_Should_Not_Cause_StackOverflow_And_Have_Correct_Values() + { + var viewModel = new TestStackOverflowViewModel() + { + Value = 50 + }; + + var target = new Class1(); + + target.Bind(Class1.DoubleValueProperty, + new Binding("Value") { Mode = BindingMode.TwoWay, Source = viewModel }); + + var child = new Class1(); + + child[!!Class1.DoubleValueProperty] = target[!!Class1.DoubleValueProperty]; + + Assert.Equal(1, viewModel.SetterInvokedCount); + + // Issues #855 and #824 were causing a StackOverflowException at this point. + target.DoubleValue = 51.001; + + Assert.Equal(2, viewModel.SetterInvokedCount); + + double expected = 51; + + Assert.Equal(expected, viewModel.Value); + Assert.Equal(expected, target.DoubleValue); + Assert.Equal(expected, child.DoubleValue); + } + [Fact] public void IsAnimating_On_Property_With_No_Value_Returns_False() { @@ -445,6 +476,15 @@ namespace Avalonia.Base.UnitTests public static readonly StyledProperty QuxProperty = AvaloniaProperty.Register("Qux", 5.6); + + public static readonly StyledProperty DoubleValueProperty = + AvaloniaProperty.Register(nameof(DoubleValue)); + + public double DoubleValue + { + get { return GetValue(DoubleValueProperty); } + set { SetValue(DoubleValueProperty, value); } + } } private class Class2 : Class1 @@ -471,5 +511,40 @@ namespace Avalonia.Base.UnitTests return InstancedBinding.OneTime(_source); } } + + private class TestStackOverflowViewModel : INotifyPropertyChanged + { + public int SetterInvokedCount { get; private set; } + + public const int MaxInvokedCount = 1000; + + private double _value; + + public event PropertyChangedEventHandler PropertyChanged; + + public double Value + { + get { return _value; } + set + { + if (_value != value) + { + SetterInvokedCount++; + if (SetterInvokedCount < MaxInvokedCount) + { + _value = (int)value; + if (_value > 75) _value = 75; + if (_value < 25) _value = 25; + } + else + { + _value = value; + } + + PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Value))); + } + } + } + } } -} +} \ No newline at end of file diff --git a/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Direct.cs b/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Direct.cs index e9cb2bf450..6d10f50276 100644 --- a/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Direct.cs +++ b/tests/Avalonia.Base.UnitTests/AvaloniaObjectTests_Direct.cs @@ -3,10 +3,11 @@ using System; using System.Collections.Generic; +using System.ComponentModel; using System.Reactive.Subjects; -using Avalonia; using Avalonia.Data; using Avalonia.Logging; +using Avalonia.Markup.Xaml.Data; using Avalonia.UnitTests; using Xunit; @@ -208,7 +209,7 @@ namespace Avalonia.Base.UnitTests { var target = new Class1(); - Assert.Throws(() => + Assert.Throws(() => target.SetValue(Class1.BarProperty, "newvalue")); } @@ -217,7 +218,7 @@ namespace Avalonia.Base.UnitTests { var target = new Class1(); - Assert.Throws(() => + Assert.Throws(() => target.SetValue((AvaloniaProperty)Class1.BarProperty, "newvalue")); } @@ -227,7 +228,7 @@ namespace Avalonia.Base.UnitTests var target = new Class1(); var source = new Subject(); - Assert.Throws(() => + Assert.Throws(() => target.Bind(Class1.BarProperty, source)); } @@ -439,12 +440,46 @@ namespace Avalonia.Base.UnitTests Assert.Equal(BindingMode.OneWayToSource, bar.GetMetadata().DefaultBindingMode); } + [Fact] + public void SetValue_Should_Not_Cause_StackOverflow_And_Have_Correct_Values() + { + var viewModel = new TestStackOverflowViewModel() + { + Value = 50 + }; + + var target = new Class1(); + + target.Bind(Class1.DoubleValueProperty, new Binding("Value") + { + Mode = BindingMode.TwoWay, + Source = viewModel + }); + + var child = new Class1(); + + child[!!Class1.DoubleValueProperty] = target[!!Class1.DoubleValueProperty]; + + Assert.Equal(1, viewModel.SetterInvokedCount); + + // Issues #855 and #824 were causing a StackOverflowException at this point. + target.DoubleValue = 51.001; + + Assert.Equal(2, viewModel.SetterInvokedCount); + + double expected = 51; + + Assert.Equal(expected, viewModel.Value); + Assert.Equal(expected, target.DoubleValue); + Assert.Equal(expected, child.DoubleValue); + } + private class Class1 : AvaloniaObject { public static readonly DirectProperty FooProperty = AvaloniaProperty.RegisterDirect( - "Foo", - o => o.Foo, + "Foo", + o => o.Foo, (o, v) => o.Foo = v, unsetValue: "unset"); @@ -453,14 +488,21 @@ namespace Avalonia.Base.UnitTests public static readonly DirectProperty BazProperty = AvaloniaProperty.RegisterDirect( - "Bar", - o => o.Baz, - (o,v) => o.Baz = v, + "Bar", + o => o.Baz, + (o, v) => o.Baz = v, unsetValue: -1); + public static readonly DirectProperty DoubleValueProperty = + AvaloniaProperty.RegisterDirect( + nameof(DoubleValue), + o => o.DoubleValue, + (o, v) => o.DoubleValue = v); + private string _foo = "initial"; private readonly string _bar = "bar"; private int _baz = 5; + private double _doubleValue; public string Foo { @@ -478,6 +520,12 @@ namespace Avalonia.Base.UnitTests get { return _baz; } set { SetAndRaise(BazProperty, ref _baz, value); } } + + public double DoubleValue + { + get { return _doubleValue; } + set { SetAndRaise(DoubleValueProperty, ref _doubleValue, value); } + } } private class Class2 : AvaloniaObject @@ -497,5 +545,40 @@ namespace Avalonia.Base.UnitTests set { SetAndRaise(FooProperty, ref _foo, value); } } } + + private class TestStackOverflowViewModel : INotifyPropertyChanged + { + public int SetterInvokedCount { get; private set; } + + public const int MaxInvokedCount = 1000; + + private double _value; + + public event PropertyChangedEventHandler PropertyChanged; + + public double Value + { + get { return _value; } + set + { + if (_value != value) + { + SetterInvokedCount++; + if (SetterInvokedCount < MaxInvokedCount) + { + _value = (int)value; + if (_value > 75) _value = 75; + if (_value < 25) _value = 25; + } + else + { + _value = value; + } + + PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Value))); + } + } + } + } } -} +} \ No newline at end of file diff --git a/tests/Avalonia.Benchmarks/Avalonia.Benchmarks.csproj b/tests/Avalonia.Benchmarks/Avalonia.Benchmarks.csproj index 1d987e2238..c16b89e0b6 100644 --- a/tests/Avalonia.Benchmarks/Avalonia.Benchmarks.csproj +++ b/tests/Avalonia.Benchmarks/Avalonia.Benchmarks.csproj @@ -49,6 +49,7 @@ + diff --git a/tests/Avalonia.Benchmarks/Base/Properties.cs b/tests/Avalonia.Benchmarks/Base/Properties.cs new file mode 100644 index 0000000000..0a020961d5 --- /dev/null +++ b/tests/Avalonia.Benchmarks/Base/Properties.cs @@ -0,0 +1,43 @@ +using System; +using System.Reactive.Subjects; +using BenchmarkDotNet.Attributes; + +namespace Avalonia.Benchmarks.Base +{ + [MemoryDiagnoser] + public class AvaloniaObjectBenchmark + { + private Class1 target = new Class1(); + private Subject intBinding = new Subject(); + + public AvaloniaObjectBenchmark() + { + target.SetValue(Class1.IntProperty, 123); + } + + [Benchmark] + public void ClearAndSetIntProperty() + { + target.ClearValue(Class1.IntProperty); + target.SetValue(Class1.IntProperty, 123); + } + + [Benchmark] + public void BindIntProperty() + { + using (target.Bind(Class1.IntProperty, intBinding)) + { + for (var i = 0; i < 100; ++i) + { + intBinding.OnNext(i); + } + } + } + + class Class1 : AvaloniaObject + { + public static readonly AvaloniaProperty IntProperty = + AvaloniaProperty.Register("Int"); + } + } +} diff --git a/tests/Avalonia.Controls.UnitTests/ListBoxTests_Single.cs b/tests/Avalonia.Controls.UnitTests/ListBoxTests_Single.cs index c7992fe80f..fee4994ee3 100644 --- a/tests/Avalonia.Controls.UnitTests/ListBoxTests_Single.cs +++ b/tests/Avalonia.Controls.UnitTests/ListBoxTests_Single.cs @@ -1,11 +1,15 @@ // Copyright (c) The Avalonia Project. All rights reserved. // Licensed under the MIT license. See licence.md file in the project root for full license information. +using System.Collections.Generic; +using System.ComponentModel; using System.Linq; using Avalonia.Controls.Presenters; using Avalonia.Controls.Templates; +using Avalonia.Data; using Avalonia.Input; using Avalonia.LogicalTree; +using Avalonia.Markup.Xaml.Data; using Avalonia.Styling; using Avalonia.VisualTree; using Xunit; @@ -199,6 +203,71 @@ namespace Avalonia.Controls.UnitTests Assert.Equal(1, target.SelectedIndex); } + [Fact] + public void SelectedItem_Should_Not_Cause_StackOverflow() + { + var viewModel = new TestStackOverflowViewModel() + { + Items = new List { "foo", "bar", "baz" } + }; + + var target = new ListBox + { + Template = new FuncControlTemplate(CreateListBoxTemplate), + DataContext = viewModel, + Items = viewModel.Items + }; + + target.Bind(ListBox.SelectedItemProperty, + new Binding("SelectedItem") { Mode = BindingMode.TwoWay }); + + Assert.Equal(0, viewModel.SetterInvokedCount); + + // In Issue #855, a Stackoverflow occured here. + target.SelectedItem = viewModel.Items[2]; + + Assert.Equal(viewModel.Items[1], target.SelectedItem); + Assert.Equal(1, viewModel.SetterInvokedCount); + } + + private class TestStackOverflowViewModel : INotifyPropertyChanged + { + public List Items { get; set; } + + public int SetterInvokedCount { get; private set; } + + public const int MaxInvokedCount = 1000; + + private string _selectedItem; + + public event PropertyChangedEventHandler PropertyChanged; + + public string SelectedItem + { + get { return _selectedItem; } + set + { + if (_selectedItem != value) + { + SetterInvokedCount++; + + int index = Items.IndexOf(value); + + if (MaxInvokedCount > SetterInvokedCount && index > 0) + { + _selectedItem = Items[index - 1]; + } + else + { + _selectedItem = value; + } + + PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(SelectedItem))); + } + } + } + } + private Control CreateListBoxTemplate(ITemplatedControl parent) { return new ScrollViewer @@ -237,4 +306,4 @@ namespace Avalonia.Controls.UnitTests target.Presenter.ApplyTemplate(); } } -} +} \ No newline at end of file diff --git a/tests/Avalonia.Controls.UnitTests/Primitives/RangeBaseTests.cs b/tests/Avalonia.Controls.UnitTests/Primitives/RangeBaseTests.cs index d3ed077cbf..2dfb30a9f0 100644 --- a/tests/Avalonia.Controls.UnitTests/Primitives/RangeBaseTests.cs +++ b/tests/Avalonia.Controls.UnitTests/Primitives/RangeBaseTests.cs @@ -2,7 +2,12 @@ // Licensed under the MIT license. See licence.md file in the project root for full license information. using System; +using System.ComponentModel; using Avalonia.Controls.Primitives; +using Avalonia.Controls.Templates; +using Avalonia.Data; +using Avalonia.Markup.Xaml.Data; +using Avalonia.Styling; using Xunit; namespace Avalonia.Controls.UnitTests.Primitives @@ -87,8 +92,111 @@ namespace Avalonia.Controls.UnitTests.Primitives Assert.Throws(() => target.Value = double.NegativeInfinity); } + [Theory] + [InlineData(true)] + [InlineData(false)] + public void SetValue_Should_Not_Cause_StackOverflow(bool useXamlBinding) + { + var viewModel = new TestStackOverflowViewModel() + { + Value = 50 + }; + + Track track = null; + + var target = new TestRange() + { + Template = new FuncControlTemplate(c => + { + track = new Track() + { + Width = 100, + Orientation = Orientation.Horizontal, + [~~Track.MinimumProperty] = c[~~RangeBase.MinimumProperty], + [~~Track.MaximumProperty] = c[~~RangeBase.MaximumProperty], + + Name = "PART_Track", + Thumb = new Thumb() + }; + + if (useXamlBinding) + { + track.Bind(Track.ValueProperty, new Binding("Value") + { + Mode = BindingMode.TwoWay, + Source = c, + Priority = BindingPriority.Style + }); + } + else + { + track[~~Track.ValueProperty] = c[~~RangeBase.ValueProperty]; + } + + return track; + }), + Minimum = 0, + Maximum = 100, + DataContext = viewModel + }; + + target.Bind(TestRange.ValueProperty, new Binding("Value") { Mode = BindingMode.TwoWay }); + + target.ApplyTemplate(); + track.Measure(new Size(100, 0)); + track.Arrange(new Rect(0, 0, 100, 0)); + + Assert.Equal(1, viewModel.SetterInvokedCount); + + // Issues #855 and #824 were causing a StackOverflowException at this point. + target.Value = 51.001; + + Assert.Equal(2, viewModel.SetterInvokedCount); + + double expected = 51; + + Assert.Equal(expected, viewModel.Value); + Assert.Equal(expected, target.Value); + Assert.Equal(expected, track.Value); + } + private class TestRange : RangeBase { } + + private class TestStackOverflowViewModel : INotifyPropertyChanged + { + public int SetterInvokedCount { get; private set; } + + public const int MaxInvokedCount = 1000; + + private double _value; + + public event PropertyChangedEventHandler PropertyChanged; + + public double Value + { + get { return _value; } + set + { + if (_value != value) + { + SetterInvokedCount++; + if (SetterInvokedCount < MaxInvokedCount) + { + _value = (int)value; + if (_value > 75) _value = 75; + if (_value < 25) _value = 25; + } + else + { + _value = value; + } + + PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Value))); + } + } + } + } } } \ No newline at end of file diff --git a/tests/Avalonia.Markup.Xaml.UnitTests/Avalonia.Markup.Xaml.UnitTests.csproj b/tests/Avalonia.Markup.Xaml.UnitTests/Avalonia.Markup.Xaml.UnitTests.csproj index eb6dc8b5e5..a3e4ad1418 100644 --- a/tests/Avalonia.Markup.Xaml.UnitTests/Avalonia.Markup.Xaml.UnitTests.csproj +++ b/tests/Avalonia.Markup.Xaml.UnitTests/Avalonia.Markup.Xaml.UnitTests.csproj @@ -21,6 +21,7 @@ + diff --git a/tests/Avalonia.Markup.Xaml.UnitTests/Data/BindingTests.cs b/tests/Avalonia.Markup.Xaml.UnitTests/Data/BindingTests.cs index 9a08073920..71c5385c23 100644 --- a/tests/Avalonia.Markup.Xaml.UnitTests/Data/BindingTests.cs +++ b/tests/Avalonia.Markup.Xaml.UnitTests/Data/BindingTests.cs @@ -338,6 +338,145 @@ namespace Avalonia.Markup.Xaml.UnitTests.Data Assert.Equal("foo", target.Content); } + [Fact] + public void StyledProperty_SetValue_Should_Not_Cause_StackOverflow_And_Have_Correct_Values() + { + var viewModel = new TestStackOverflowViewModel() + { + Value = 50 + }; + + var target = new StyledPropertyClass(); + + target.Bind(StyledPropertyClass.DoubleValueProperty, + new Binding("Value") { Mode = BindingMode.TwoWay, Source = viewModel }); + + var child = new StyledPropertyClass(); + + child.Bind(StyledPropertyClass.DoubleValueProperty, + new Binding("DoubleValue") + { + Mode = BindingMode.TwoWay, + Source = target + }); + + Assert.Equal(1, viewModel.SetterInvokedCount); + + //here in real life stack overflow exception is thrown issue #855 and #824 + target.DoubleValue = 51.001; + + Assert.Equal(2, viewModel.SetterInvokedCount); + + double expected = 51; + + Assert.Equal(expected, viewModel.Value); + Assert.Equal(expected, target.DoubleValue); + Assert.Equal(expected, child.DoubleValue); + } + + [Fact] + public void SetValue_Should_Not_Cause_StackOverflow_And_Have_Correct_Values() + { + var viewModel = new TestStackOverflowViewModel() + { + Value = 50 + }; + + var target = new DirectPropertyClass(); + + target.Bind(DirectPropertyClass.DoubleValueProperty, new Binding("Value") + { + Mode = BindingMode.TwoWay, + Source = viewModel + }); + + var child = new DirectPropertyClass(); + + child.Bind(DirectPropertyClass.DoubleValueProperty, + new Binding("DoubleValue") + { + Mode = BindingMode.TwoWay, + Source = target + }); + + Assert.Equal(1, viewModel.SetterInvokedCount); + + //here in real life stack overflow exception is thrown issue #855 and #824 + target.DoubleValue = 51.001; + + Assert.Equal(2, viewModel.SetterInvokedCount); + + double expected = 51; + + Assert.Equal(expected, viewModel.Value); + Assert.Equal(expected, target.DoubleValue); + Assert.Equal(expected, child.DoubleValue); + } + + private class StyledPropertyClass : AvaloniaObject + { + public static readonly StyledProperty DoubleValueProperty = + AvaloniaProperty.Register(nameof(DoubleValue)); + + public double DoubleValue + { + get { return GetValue(DoubleValueProperty); } + set { SetValue(DoubleValueProperty, value); } + } + } + + private class DirectPropertyClass : AvaloniaObject + { + public static readonly DirectProperty DoubleValueProperty = + AvaloniaProperty.RegisterDirect( + nameof(DoubleValue), + o => o.DoubleValue, + (o, v) => o.DoubleValue = v); + + private double _doubleValue; + public double DoubleValue + { + get { return _doubleValue; } + set { SetAndRaise(DoubleValueProperty, ref _doubleValue, value); } + } + } + + private class TestStackOverflowViewModel : INotifyPropertyChanged + { + public int SetterInvokedCount { get; private set; } + + public const int MaxInvokedCount = 1000; + + private double _value; + + public event PropertyChangedEventHandler PropertyChanged; + + public double Value + { + get { return _value; } + set + { + if (_value != value) + { + SetterInvokedCount++; + if (SetterInvokedCount < MaxInvokedCount) + { + _value = (int)value; + if (_value > 75) _value = 75; + if (_value < 25) _value = 25; + } + else + { + _value = value; + } + + PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Value))); + } + } + } + } + + [Fact] public void Binding_With_Null_Path_Works() {