-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
[shell] [windows] fix NRE when clearing shell items #15220
Conversation
Fixes: dotnet#13775 You can cause MAUI to throw an NRE by doing: 1. Debug/run, have XAML hot reload enabled 2. Comment out the `ShellContent` in `AppShell.xaml`, hot reload 3. Uncomment the `ShellContent` in `AppShell.xaml`, hot reload Crashes with: System.NullReferenceException: Object reference not set to an instance of an object. at Microsoft.Maui.Controls.Handlers.ShellSectionHandler.SetVirtualView(IElement view) at Microsoft.Maui.Controls.Handlers.ShellItemHandler.UpdateCurrentItem() at Microsoft.Maui.Controls.Handlers.ShellItemHandler.MapCurrentItem(ShellItemHandler handler, ShellItem item) at Microsoft.Maui.PropertyMapper`2.<>c__DisplayClass5_0.<Add>b__0(IElementHandler h, IElement v) at Microsoft.Maui.PropertyMapper.UpdatePropertyCore(String key, IElementHandler viewHandler, IElement virtualView) at Microsoft.Maui.PropertyMapper.UpdateProperties(IElementHandler viewHandler, IElement virtualView) at Microsoft.Maui.Handlers.ElementHandler.SetVirtualView(IElement view) at Microsoft.Maui.Controls.Handlers.ShellItemHandler.SetVirtualView(IElement view) at Microsoft.Maui.Controls.Platform.ShellView.CreateShellItemView() at Microsoft.Maui.Controls.Platform.ShellView.SwitchShellItem(ShellItem newItem, Boolean animate) at Microsoft.Maui.Controls.Handlers.ShellHandler.MapCurrentItem(ShellHandler handler, Shell view) at Microsoft.Maui.PropertyMapper`2.<>c__DisplayClass5_0.<Add>b__0(IElementHandler h, IElement v) at Microsoft.Maui.PropertyMapper.UpdatePropertyCore(String key, IElementHandler viewHandler, IElement virtualView) at Microsoft.Maui.PropertyMapper.UpdateProperty(IElementHandler viewHandler, IElement virtualView, String property) at Microsoft.Maui.Handlers.ElementHandler.UpdateValue(String property) at Microsoft.Maui.Controls.Element.OnPropertyChanged(String propertyName) at Microsoft.Maui.Controls.Shell.OnPropertyChanged(String propertyName) at Microsoft.Maui.Controls.BindableObject.SetValueActual(BindableProperty property, BindablePropertyContext context, Object value, Boolean currentlyApplying, SetValueFlags attributes, Boolean silent) at Microsoft.Maui.Controls.BindableObject.SetValueCore(BindableProperty property, Object value, SetValueFlags attributes, SetValuePrivateFlags privateAttributes) at Microsoft.Maui.Controls.BindableObject.SetValueCore(BindableProperty property, Object value, SetValueFlags attributes) at Microsoft.Maui.Controls.Element.SetValueFromRenderer(BindableProperty property, Object value) at Microsoft.Maui.Controls.ShellNavigationManager.GoToAsync(ShellNavigationParameters shellNavigationParameters, ShellNavigationRequest navigationRequest) at Microsoft.Maui.Controls.Shell.<Initialize>g__SetCurrentItem|172_1() at Microsoft.Maui.Controls.Shell.<Initialize>b__172_0(Object s, NotifyCollectionChangedEventArgs e) at System.Threading.Tasks.Task.<>c.<ThrowAsync>b__128_0(Object state) at Microsoft.UI.Dispatching.DispatcherQueueSynchronizationContext.<>c__DisplayClass2_0.<Post>b__0() I could reproduce this in a test by simply doing: shell.CurrentItem = new ShellContent { Content = page }; shell.Items.Clear(); shell.CurrentItem = new ShellContent { Content = page }; I added appropriate null checking and the problem appears to be solved now.
If this PR is merged before this one: I can update #15098 to fix the merge conflicts. |
((IShellController)_shellSection.FindParentOfType<Shell>()!).RemoveAppearanceObserver(this); | ||
|
||
var shell = _shellSection.FindParentOfType<Shell>() as IShellController; | ||
shell?.RemoveAppearanceObserver(this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to store shell locally when the observer is set on line 75 that way we can successfully remove (this) from the list of AppearanceObservers. Otherwise, this Handler will probably remain inside the list of appearance observers inside shell.
maui/src/Controls/src/Core/Shell/Shell.cs
Line 313 in f6cf7c7
List<(IAppearanceObserver Observer, Element Pivot)> _appearanceObservers = new List<(IAppearanceObserver Observer, Element Pivot)>(); |
Or trigger this tear down code via
class ActionDisposable : IDisposable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like this is a second problem.
I updated the test to check for this, and then fixed it. But if this fails on iOS/Android it's a problem in other handlers I didn't touch?
* [shell] [windows] fix NRE when clearing shell items Fixes: #13775 You can cause MAUI to throw an NRE by doing: 1. Debug/run, have XAML hot reload enabled 2. Comment out the `ShellContent` in `AppShell.xaml`, hot reload 3. Uncomment the `ShellContent` in `AppShell.xaml`, hot reload Crashes with: System.NullReferenceException: Object reference not set to an instance of an object. at Microsoft.Maui.Controls.Handlers.ShellSectionHandler.SetVirtualView(IElement view) at Microsoft.Maui.Controls.Handlers.ShellItemHandler.UpdateCurrentItem() at Microsoft.Maui.Controls.Handlers.ShellItemHandler.MapCurrentItem(ShellItemHandler handler, ShellItem item) at Microsoft.Maui.PropertyMapper`2.<>c__DisplayClass5_0.<Add>b__0(IElementHandler h, IElement v) at Microsoft.Maui.PropertyMapper.UpdatePropertyCore(String key, IElementHandler viewHandler, IElement virtualView) at Microsoft.Maui.PropertyMapper.UpdateProperties(IElementHandler viewHandler, IElement virtualView) at Microsoft.Maui.Handlers.ElementHandler.SetVirtualView(IElement view) at Microsoft.Maui.Controls.Handlers.ShellItemHandler.SetVirtualView(IElement view) at Microsoft.Maui.Controls.Platform.ShellView.CreateShellItemView() at Microsoft.Maui.Controls.Platform.ShellView.SwitchShellItem(ShellItem newItem, Boolean animate) at Microsoft.Maui.Controls.Handlers.ShellHandler.MapCurrentItem(ShellHandler handler, Shell view) at Microsoft.Maui.PropertyMapper`2.<>c__DisplayClass5_0.<Add>b__0(IElementHandler h, IElement v) at Microsoft.Maui.PropertyMapper.UpdatePropertyCore(String key, IElementHandler viewHandler, IElement virtualView) at Microsoft.Maui.PropertyMapper.UpdateProperty(IElementHandler viewHandler, IElement virtualView, String property) at Microsoft.Maui.Handlers.ElementHandler.UpdateValue(String property) at Microsoft.Maui.Controls.Element.OnPropertyChanged(String propertyName) at Microsoft.Maui.Controls.Shell.OnPropertyChanged(String propertyName) at Microsoft.Maui.Controls.BindableObject.SetValueActual(BindableProperty property, BindablePropertyContext context, Object value, Boolean currentlyApplying, SetValueFlags attributes, Boolean silent) at Microsoft.Maui.Controls.BindableObject.SetValueCore(BindableProperty property, Object value, SetValueFlags attributes, SetValuePrivateFlags privateAttributes) at Microsoft.Maui.Controls.BindableObject.SetValueCore(BindableProperty property, Object value, SetValueFlags attributes) at Microsoft.Maui.Controls.Element.SetValueFromRenderer(BindableProperty property, Object value) at Microsoft.Maui.Controls.ShellNavigationManager.GoToAsync(ShellNavigationParameters shellNavigationParameters, ShellNavigationRequest navigationRequest) at Microsoft.Maui.Controls.Shell.<Initialize>g__SetCurrentItem|172_1() at Microsoft.Maui.Controls.Shell.<Initialize>b__172_0(Object s, NotifyCollectionChangedEventArgs e) at System.Threading.Tasks.Task.<>c.<ThrowAsync>b__128_0(Object state) at Microsoft.UI.Dispatching.DispatcherQueueSynchronizationContext.<>c__DisplayClass2_0.<Post>b__0() I could reproduce this in a test by simply doing: shell.CurrentItem = new ShellContent { Content = page }; shell.Items.Clear(); shell.CurrentItem = new ShellContent { Content = page }; I added appropriate null checking and the problem appears to be solved now. * Update test, call RemoveAppearanceObserver more reliably
Fixes: #13775
You can cause MAUI to throw an NRE by doing:
Debug/run, have XAML hot reload enabled
Comment out the
ShellContent
inAppShell.xaml
, hot reloadUncomment the
ShellContent
inAppShell.xaml
, hot reloadCrashes with:
I could reproduce this in a test by simply doing:
I added appropriate null checking and the problem appears to be solved now.