Crash when CommandBarFlyout closes

[jonthysell]

Describe the bug
React Native Windows had a problem with keyboard navigation when using the XAML CommandBarFlyout (microsoft/react-native-windows#8306). They fixed this by switching to the MUX CommandBarFlyout, which fixed keyboard navigation but causes a crash when the flyout closes:

From microsoft/react-native-windows#8306 (comment) :

Exception thrown: read access violation.
this->m_sharedState.m_ptr was nullptr.

And call stack:

 	Windows.UI.Xaml.dll!CDependencyObject::NWPropagateDirtyFlag(DirtyFlags::Value flags) Line 3055	C++
 	[Inline Frame] Windows.UI.Xaml.dll!CUIElement::NWSetDirtyFlagsFromChildAndPropagate(DirtyFlags::Value flags, unsigned int) Line 115	C++
 	Windows.UI.Xaml.dll!CUIElement::NWSetSubgraphDirty(CDependencyObject * pTarget, DirtyFlags::Value flags) Line 315	C++
 	Windows.UI.Xaml.dll!CUIElement::RemoveLayoutTransitionRenderer(CLayoutTransitionElement * pLayoutTransitionElement) Line 3575	C++
 	Windows.UI.Xaml.dll!CLayoutTransitionElement::DetachTransition(CUIElement * pTarget, CTransitionRoot * pParent) Line 11339	C++
 	Windows.UI.Xaml.dll!CFocusRectManager::ReleaseResources(bool isDeviceLost, bool cleanupDComp, bool clearRenderData) Line 576	C++
 	Windows.UI.Xaml.dll!CFocusRectManager::UpdateFocusRect(CCoreServices * core, CDependencyObject * focusedObject, CDependencyObject * focusTargetObject, DirectUI::FocusNavigationDirection focusNavigationDirection, bool cleanOnly) Line 645	C++
 	Windows.UI.Xaml.dll!CFocusManager::UpdateFocusRect(const DirectUI::FocusNavigationDirection focusNavigationDirection, bool cleanOnly) Line 2838	C++
 	Windows.UI.Xaml.dll!CCoreServices::NWDrawTree(HWWalk * pHWWalk, CWindowRenderTarget * pRenderTarget, VisualTree * pVisualTree, unsigned int forceRedraw, XRECT_WH * prcDirtyRect) Line 6376	C++
 	Windows.UI.Xaml.dll!CCoreServices::NWDrawMainTree(CWindowRenderTarget * pIRenderTarget, bool fForceRedraw, XRECT_WH * prcDirtyRect) Line 6084	C++
 	Windows.UI.Xaml.dll!CWindowRenderTarget::Draw(CCoreServices * fForceRedraw, unsigned int prcDirtyRect, XRECT_WH *) Line 136	C++
 	Windows.UI.Xaml.dll!CXcpBrowserHost::OnTick() Line 545	C++
 	Windows.UI.Xaml.dll!CXcpDispatcher::Tick() Line 1449	C++
 	Windows.UI.Xaml.dll!CXcpDispatcher::OnReentrancyProtectedWindowMessage(HWND__ * msg, unsigned int lParam, unsigned __int64) Line 1041	C++
 	[Inline Frame] Windows.UI.Xaml.dll!CXcpDispatcher::ProcessMessage(HWND__ *) Line 890	C++
 	Windows.UI.Xaml.dll!CXcpDispatcher::WindowProc(HWND__ * hwnd, unsigned int msg, unsigned __int64 wParam, __int64 lParam) Line 839	C++
 	Windows.UI.Xaml.dll!CDeferredInvoke::DispatchQueuedMessage(bool * dispatchedWork, bool * hasMoreWork) Line 301	C++
 	[Inline Frame] Windows.UI.Xaml.dll!CXcpDispatcher::MessageTimerCallback() Line 1534	C++
 	Windows.UI.Xaml.dll!CXcpDispatcher::MessageTimerCallbackStatic(void * myUserData) Line 1526	C++
 	CoreMessaging.dll!00007ffa44e153db()	Unknown
 	CoreMessaging.dll!00007ffa44e3c85b()	Unknown
 	CoreMessaging.dll!00007ffa44e1996b()	Unknown
 	CoreMessaging.dll!00007ffa44e18e26()	Unknown
 	CoreMessaging.dll!00007ffa44e17061()	Unknown
 	CoreMessaging.dll!00007ffa44e16e83()	Unknown
 	user32.dll!UserCallWinProcCheckWow()	Unknown
 	user32.dll!DispatchClientMessage()	Unknown
 	user32.dll!__fnDWORD�()	Unknown
 	ntdll.dll!00007ffa4a5b0c54()	Unknown
 	win32u.dll!00007ffa48341104()	Unknown
 	user32.dll!GetMessageW()	Unknown
>	Playground-win32.exe!RunPlayground(int showCmd, bool useWebDebugger) Line 406	C++
 	Playground-win32.exe!WinMain(HINSTANCE__ * instance, HINSTANCE__ * __formal, char * __formal, int showCmd) Line 442	C++
 	Playground-win32.exe!invoke_main() Line 107	C++
 	Playground-win32.exe!__scrt_common_main_seh() Line 288	C++
 	Playground-win32.exe!__scrt_common_main() Line 331	C++
 	Playground-win32.exe!WinMainCRTStartup(void * __formal) Line 17	C++
 	kernel32.dll!00007ffa49357034()	Unknown
 	ntdll.dll!00007ffa4a562651()	Unknown

They were able to workaround the issue by making sure that there are no secondary commands:

 flyout.Closing([](winrt::IInspectable const &sender, auto &&) {
    const auto &flyout = sender.as<winrt::Microsoft::UI::Xaml::Controls::TextCommandBarFlyout>();
    flyout.SecondaryCommands().Clear();
  });

Steps to reproduce the bug

Steps to reproduce the behavior:

1. Launch RNW playground app (here's a branch where the switch to WinUIU2 has been made: https://github.com/lyahdav/react-native-windows/tree/win-ui-2-cbf-with-crash-on-flyout-closing-fix)
2. File -> Open rntester example
3. Right-click on textinput at top of screen to open flyout
4. Click off of flyout

Expected behavior
Flyout closes without crashing.

Screenshots

Version Info

NuGet package version:
[Microsoft.UI.Xaml 2.6.1-prerelease]

Windows app type:

UWP	Win32
Yes	Yes

Windows version	Saw the problem?
Insider Build (xxxxx)
May 2021 Update (19043)	Yes
October 2020 Update (19042)
May 2020 Update (19041)
November 2019 Update (18363)
May 2019 Update (18362)
October 2018 Update (17763)
April 2018 Update (17134)
Fall Creators Update (16299)
Creators Update (15063)

Device form factor	Saw the problem?
Desktop	Yes
Xbox
Surface Hub
IoT

Additional context

[jonthysell]

I can try to find a smaller example repro.

[llongley]

A simple repro app would be a big help, if you could provide it. Thanks!

[lyahdav]

In the steps to reproduce, before step 4, you have to use the keyboard up or down arrows to navigate the CommandBarFlyout first so that there's a focus rect visible. Makes sense given the exception call stack includes UpdateFocusRect.

[lyahdav]

A simple repro app would be a big help, if you could provide it. Thanks!

@llongley Here you go: https://github.com/lyahdav/XamlIslandsFlyoutBug/tree/text-box-for-testing-command-bar-flyout-without-dpi-awareness-with-winui2. This is a plain Islands app with WinUI 2 CommandBarFlyout and the OS XAML CommandBarFlyout. The OS one doesn't have this crash, while WinUI 2 does. I'm pretty sure I saw this crash in a UWP app using WinUI 2 CommandBarFlyout as well, so not specific to Islands. And here's a video of the crash:

Screen.Recording.2021-08-25.at.1.46.12.PM.mov

[lyahdav]

Actually when I upgrade that project from Microsoft.UI.Xaml.2.6.1-prerelease.210709001 to Microsoft.UI.Xaml.2.7.0-prerelease.210816001 this crash goes away, so it's been fixed already. @jonthysell you can close this.

[ghost]

This issue has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment.