microsoft / microsoft-ui-xaml

Windows UI Library: the latest Windows 10 native controls and Fluent styles for your applications
MIT License
6.35k stars 678 forks source link

Crash when CommandBarFlyout closes #5680

Closed jonthysell closed 3 years ago

jonthysell commented 3 years ago

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

From https://github.com/microsoft/react-native-windows/issues/8306#issuecomment-897267971 :

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 commented 3 years ago

I can try to find a smaller example repro.

llongley commented 3 years ago

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

lyahdav commented 3 years ago

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 commented 3 years ago

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:

https://user-images.githubusercontent.com/359157/130862846-6c44de13-d7c3-4f85-96ec-32e14f93c9b4.mov

lyahdav commented 3 years ago

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 commented 3 years ago

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.