Skip to content

Commit

Permalink
Prevent stack overflow in two-way bindings. (#17073)
Browse files Browse the repository at this point in the history
* Add failing test for #16746

* Always read the value from the target object.

The value must be read from the target object instead of using the value from the event because the value may have changed again between the time the event was raised and now: if that occurs in a two-way binding then we end up with a stack overflow.
  • Loading branch information
grokys authored Sep 25, 2024
1 parent d7040e2 commit 6b78783
Show file tree
Hide file tree
Showing 2 changed files with 61 additions and 2 deletions.
6 changes: 4 additions & 2 deletions src/Avalonia.Base/Data/Core/BindingExpression.cs
Original file line number Diff line number Diff line change
Expand Up @@ -512,8 +512,10 @@ private void OnTargetPropertyChanged(object? sender, AvaloniaPropertyChangedEven
Debug.Assert(_mode is BindingMode.TwoWay or BindingMode.OneWayToSource);
Debug.Assert(UpdateSourceTrigger is UpdateSourceTrigger.PropertyChanged);

if (e.Property == TargetProperty)
WriteValueToSource(e.NewValue);
// The value must be read from the target object instead of using the value from the event
// because the value may have changed again between the time the event was raised and now.
if (e.Property == TargetProperty && TryGetTarget(out var target))
WriteValueToSource(target.GetValue(TargetProperty));
}

private object? ConvertFallback(object? fallback, string fallbackName)
Expand Down
57 changes: 57 additions & 0 deletions tests/Avalonia.Markup.UnitTests/Data/BindingTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -667,6 +667,31 @@ public void OneWayToSource_Binding_Does_Not_Override_TwoWay_Binding()
Assert.Equal("TwoWay", source.Foo);
}

[Fact]
public void Target_Undoing_Property_Change_During_TwoWay_Binding_Does_Not_Cause_StackOverflow()
{
var source = new TestStackOverflowViewModel { BoolValue = true };
var target = new TwoWayBindingTest();

source.ResetSetterInvokedCount();

// The AlwaysFalse property is set to false in the PropertyChanged callback. Ensure
// that binding it to an initial `true` value with a two-way binding does not cause a
// stack overflow.
target.Bind(
TwoWayBindingTest.AlwaysFalseProperty,
new Binding(nameof(TestStackOverflowViewModel.BoolValue))
{
Mode = BindingMode.TwoWay,
});

target.DataContext = source;

Assert.Equal(1, source.SetterInvokedCount);
Assert.False(source.BoolValue);
Assert.False(target.AlwaysFalse);
}

private class StyledPropertyClass : AvaloniaObject
{
public static readonly StyledProperty<double> DoubleValueProperty =
Expand Down Expand Up @@ -725,10 +750,24 @@ private class TestStackOverflowViewModel : INotifyPropertyChanged

public const int MaxInvokedCount = 1000;

private bool _boolValue;
private double _value;

public event PropertyChangedEventHandler PropertyChanged;

public bool BoolValue
{
get => _boolValue;
set
{
if (_boolValue != value)
{
_boolValue = value;
SetterInvokedCount++;
PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(BoolValue)));
}
} }

public double Value
{
get => _value;
Expand All @@ -754,20 +793,38 @@ public double Value
}
}
}

public void ResetSetterInvokedCount() => SetterInvokedCount = 0;
}

private class TwoWayBindingTest : Control
{
public static readonly StyledProperty<bool> AlwaysFalseProperty =
AvaloniaProperty.Register<StyledPropertyClass, bool>(nameof(AlwaysFalse));
public static readonly StyledProperty<string> TwoWayProperty =
AvaloniaProperty.Register<TwoWayBindingTest, string>(
"TwoWay",
defaultBindingMode: BindingMode.TwoWay);

public bool AlwaysFalse
{
get => GetValue(AlwaysFalseProperty);
set => SetValue(AlwaysFalseProperty, value);
}

public string TwoWay
{
get => GetValue(TwoWayProperty);
set => SetValue(TwoWayProperty, value);
}

protected override void OnPropertyChanged(AvaloniaPropertyChangedEventArgs change)
{
base.OnPropertyChanged(change);

if (change.Property == AlwaysFalseProperty)
SetCurrentValue(AlwaysFalseProperty, false);
}
}

public class Source : INotifyPropertyChanged
Expand Down

0 comments on commit 6b78783

Please sign in to comment.