-
Notifications
You must be signed in to change notification settings - Fork 2k
Fix NullReferenceException in ApplyBindings when DefaultValueCreator mutates other BindableProperties #37148
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
39c19e1
724fbdb
7f09c70
49606b8
9e89f76
62b927c
8935253
fc1536c
53bc365
b84104a
a1521ed
669ac35
ad8249e
204752e
9658428
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -779,20 +779,15 @@ BindablePropertyContext CreateContext(BindableProperty property) | |
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| BindablePropertyContext GetOrCreateContext(BindableProperty property) | ||
| { | ||
| #if NETSTANDARD | ||
| var context = GetContext(property); | ||
| if (context is null) | ||
| { | ||
| context = CreateContext(property); | ||
| _properties.Add(property.InternalId, context); | ||
| } | ||
| #else | ||
| ref var context = ref CollectionsMarshal.GetValueRefOrAddDefault(_properties, property.InternalId, out var exists); | ||
| if (!exists) | ||
| { | ||
| context = CreateContext(property); | ||
| } | ||
| #endif | ||
| if (_properties.TryGetValue(property.InternalId, out var context)) | ||
| return context; | ||
|
|
||
| // Do not use CollectionsMarshal.GetValueRefOrAddDefault: CreateContext invokes | ||
|
praveenkumarkarunanithi marked this conversation as resolved.
praveenkumarkarunanithi marked this conversation as resolved.
|
||
| // DefaultValueCreator, which is arbitrary user code and may mutate other | ||
| // BindableProperties, resizing _properties and invalidating the returned ref. | ||
| // See dotnet/maui#36744. | ||
| context = CreateContext(property); | ||
|
praveenkumarkarunanithi marked this conversation as resolved.
praveenkumarkarunanithi marked this conversation as resolved.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
[moderate] Logic and Correctness Verification — Moving the dictionary insert to after Concrete scenario: a
Suggested guard that preserves the #36744 fix: publish a non-null placeholder context ( |
||
| _properties[property.InternalId] = context; | ||
|
praveenkumarkarunanithi marked this conversation as resolved.
praveenkumarkarunanithi marked this conversation as resolved.
|
||
| return context; | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1717,5 +1717,60 @@ public void SpecificityOfHandlers() | |
| Assert.Equal("manual", bindable.GetValue(prop)); | ||
| } | ||
|
|
||
| // Regression test for https://github.com/dotnet/maui/issues/36744 | ||
| [Fact] | ||
| public void DefaultValueCreatorThatMutatesOtherPropertiesDoesNotCorruptPropertyStore() | ||
|
praveenkumarkarunanithi marked this conversation as resolved.
|
||
| { | ||
| var mock = new MockBindable36744(); | ||
|
|
||
| var triggerValue = mock.GetValue(MockBindable36744.TriggerProperty); | ||
| Assert.NotNull(triggerValue); | ||
| Assert.Same(triggerValue, mock.GetValue(MockBindable36744.TriggerProperty)); | ||
|
praveenkumarkarunanithi marked this conversation as resolved.
|
||
|
|
||
| var exception = Record.Exception(() => mock.BindingContext = new object()); | ||
|
praveenkumarkarunanithi marked this conversation as resolved.
praveenkumarkarunanithi marked this conversation as resolved.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
[minor] Regression Prevention and Test Coverage — The test proves the fix today, but its coverage silently depends on implementation details it never asserts: Suggest adding a direct assertion that the property store itself is intact rather than only asserting " |
||
| Assert.Null(exception); | ||
| } | ||
| } | ||
|
|
||
| internal class MockBindable36744 : BindableObject | ||
|
praveenkumarkarunanithi marked this conversation as resolved.
praveenkumarkarunanithi marked this conversation as resolved.
|
||
| { | ||
| public static readonly BindableProperty P0 = BindableProperty.Create("P0", typeof(int), typeof(MockBindable36744), 0); | ||
| public static readonly BindableProperty P1 = BindableProperty.Create("P1", typeof(int), typeof(MockBindable36744), 0); | ||
| public static readonly BindableProperty P2 = BindableProperty.Create("P2", typeof(int), typeof(MockBindable36744), 0); | ||
| public static readonly BindableProperty P3 = BindableProperty.Create("P3", typeof(int), typeof(MockBindable36744), 0); | ||
| public static readonly BindableProperty P4 = BindableProperty.Create("P4", typeof(int), typeof(MockBindable36744), 0); | ||
| public static readonly BindableProperty P5 = BindableProperty.Create("P5", typeof(int), typeof(MockBindable36744), 0); | ||
| public static readonly BindableProperty P6 = BindableProperty.Create("P6", typeof(int), typeof(MockBindable36744), 0); | ||
| public static readonly BindableProperty P7 = BindableProperty.Create("P7", typeof(int), typeof(MockBindable36744), 0); | ||
| public static readonly BindableProperty P8 = BindableProperty.Create("P8", typeof(int), typeof(MockBindable36744), 0); | ||
| public static readonly BindableProperty P9 = BindableProperty.Create("P9", typeof(int), typeof(MockBindable36744), 0); | ||
| public static readonly BindableProperty P10 = BindableProperty.Create("P10", typeof(int), typeof(MockBindable36744), 0); | ||
| public static readonly BindableProperty P11 = BindableProperty.Create("P11", typeof(int), typeof(MockBindable36744), 0); | ||
| public static readonly BindableProperty P12 = BindableProperty.Create("P12", typeof(int), typeof(MockBindable36744), 0); | ||
| public static readonly BindableProperty P13 = BindableProperty.Create("P13", typeof(int), typeof(MockBindable36744), 0); | ||
| public static readonly BindableProperty P14 = BindableProperty.Create("P14", typeof(int), typeof(MockBindable36744), 0); | ||
|
|
||
| public static readonly BindableProperty TriggerProperty = BindableProperty.Create( | ||
| "Trigger", typeof(object), typeof(MockBindable36744), null, | ||
| defaultValueCreator: b => | ||
| { | ||
| var mb = (MockBindable36744)b; | ||
| mb.SetValue(P0, 1); | ||
| mb.SetValue(P1, 2); | ||
| mb.SetValue(P2, 3); | ||
| mb.SetValue(P3, 4); | ||
| mb.SetValue(P4, 5); | ||
| mb.SetValue(P5, 6); | ||
| mb.SetValue(P6, 7); | ||
| mb.SetValue(P7, 8); | ||
| mb.SetValue(P8, 9); | ||
| mb.SetValue(P9, 10); | ||
| mb.SetValue(P10, 11); | ||
| mb.SetValue(P11, 12); | ||
| mb.SetValue(P12, 13); | ||
| mb.SetValue(P13, 14); | ||
| mb.SetValue(P14, 15); | ||
| return new object(); | ||
| }); | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.