Fix DesktopWindowXamlSource related crashes when closing a window by lhecker · Pull Request #15338 · microsoft/terminal

lhecker · 2023-05-12T14:31:56Z

XAML/WinUI may pump the event loop internally. One of the functions
that does this right now is DesktopWindowXamlSource::Close().

This is problematic in the previous code, because we'd set _window
to nullptr before calling Close() and so any of the IslandWindow
callbacks may be invoked during shutdown, which then try to potentially
access _window and end up crashing. This commit fixes the issue by
simply not nulling out the _window and calling Close() directly.

Furthermore, NonClientIslandWindow may directly access WinUI
objects in its message handlers which also crashes.

I've had this happen roughly ~1% of my test exits in a debug build
and every single time on a (artificial) very slow CPU.

Validation Steps Performed

Closing a window destroys the rendering instance ✅

lhecker · 2023-05-12T14:32:22Z

src/cascadia/WindowsTerminal/AppHost.cpp

-    // _window callbacks don't need to be treated this way, because:
-    // * IslandWindow isn't a WinRT type (so it doesn't have neat revokers like this)
-    // * This particular bug scenario applies when we've already freed the window.


In short, these 3 lines are wrong. 😄

lhecker · 2023-05-12T14:33:39Z

src/cascadia/WindowsTerminal/AppHost.cpp

-    {
-        _showHideWindowThrottler->Run(args.ShowOrHide());
-    }
+    _showHideWindowThrottler->Run(args.ShowOrHide());


This if check is a common pattern we got with our throttled funcs. It's generally not very safe, because it fails to work as soon as Run() is called from a background thread. This code still works correctly, because the _revokers are nulled out before the _showHideWindowThrottler is reset.

lhecker · 2023-05-12T14:35:12Z

src/cascadia/TerminalApp/TerminalPage.cpp

    // - args: the arguments specifying how to set the display status to ShowWindow for our window handle
    winrt::fire_and_forget TerminalPage::_ShowWindowChangedHandler(const IInspectable /*sender*/, const Microsoft::Terminal::Control::ShowWindowArgs args)
    {
-        co_await resume_foreground(Dispatcher());


With the if check for _showHideWindowThrottler removed, we can safely drop this resume_foreground. It's also a nice safety fix, because it doesn't hold onto this strongly.

lhecker · 2023-05-12T16:20:55Z

src/cascadia/WindowsTerminal/NonClientIslandWindow.cpp

+    // Avoid further callbacks into XAML/WinUI-land after we've Close()d the DesktopWindowXamlSource
+    // inside `IslandWindow::Close()`. XAML thanks us for doing that by not crashing. Thank you XAML.
+    SetWindowLongPtr(_dragBarWindow.get(), GWLP_USERDATA, 0);
+    IslandWindow::Close();


Without this, NonClientIslandWindow would crash here every single time I close a window:

terminal/src/cascadia/WindowsTerminal/NonClientIslandWindow.cpp

Line 92 in c6215c8

const auto buttonWidthInDips{ _titlebar.CaptionButtonWidth() };

DHowett · 2023-05-12T18:07:41Z

src/cascadia/TerminalApp/TerminalPage.cpp

    // - sender (not used)
    // - args: the arguments specifying how to set the display status to ShowWindow for our window handle
-    winrt::fire_and_forget TerminalPage::_ShowWindowChangedHandler(const IInspectable /*sender*/, const Microsoft::Terminal::Control::ShowWindowArgs args)
+    void TerminalPage::_ShowWindowChangedHandler(const IInspectable /*sender*/, const Microsoft::Terminal::Control::ShowWindowArgs args)


note that changing this from a coroutine means that the excess copies on the arguments are not required. you could change them to const&

DHowett · 2023-05-15T17:27:08Z

src/cascadia/TerminalApp/TerminalPage.cpp

    // - sender (not used)
    // - args: the arguments specifying how to set the display status to ShowWindow for our window handle
-    void TerminalPage::_ShowWindowChangedHandler(const IInspectable /*sender*/, const Microsoft::Terminal::Control::ShowWindowArgs args)
+    void TerminalPage::_ShowWindowChangedHandler(const IInspectable& /*sender*/, const Microsoft::Terminal::Control::ShowWindowArgs args)


you only fixed the first arg..!

…5338) XAML/WinUI may pump the event loop internally. One of the functions that does this right now is `DesktopWindowXamlSource::Close()`. This is problematic in the previous code, because we'd set `_window` to `nullptr` before calling `Close()` and so any of the `IslandWindow` callbacks may be invoked during shutdown, which then try to potentially access `_window` and end up crashing. This commit fixes the issue by simply not nulling out the `_window` and calling `Close()` directly. Furthermore, `NonClientIslandWindow` may directly access WinUI objects in its message handlers which also crashes. I've had this happen roughly ~1% of my test exits in a debug build and every single time on a (artificial) very slow CPU. ## Validation Steps Performed * Closing a window destroys the rendering instance ✅ (cherry picked from commit ba39db5) Service-Card-Id: 89191262 Service-Version: 1.18

lhecker added Issue-Bug It either shouldn't be doing this or needs an investigation. Area-UserInterface Issues pertaining to the user interface of the Console or Terminal Product-Terminal The new Windows Terminal. Priority-3 A description (P3) labels May 12, 2023

lhecker commented May 12, 2023

View reviewed changes

lhecker force-pushed the dev/lhecker/xaml-source-close-crash branch from fa31979 to 0801694 Compare May 12, 2023 14:34

lhecker commented May 12, 2023

View reviewed changes

Fix a rare crash when closing a window

6b73290

lhecker force-pushed the dev/lhecker/xaml-source-close-crash branch from 0801694 to 6b73290 Compare May 12, 2023 14:51

Fix a not so rare crash

d825607

lhecker changed the title ~~Fix a rare crash when closing a window~~ Fix DesktopWindowXamlSource related crashes when closing a window May 12, 2023

lhecker commented May 12, 2023

View reviewed changes

zadjii-msft approved these changes May 12, 2023

View reviewed changes

DHowett approved these changes May 12, 2023

View reviewed changes

Address feedback

ecc7a03

lhecker added the AutoMerge Marked for automatic merge by the bot when requirements are met label May 15, 2023

microsoft-github-policy-service bot enabled auto-merge (squash) May 15, 2023 13:36

microsoft-github-policy-service bot merged commit ba39db5 into main May 15, 2023

microsoft-github-policy-service bot deleted the dev/lhecker/xaml-source-close-crash branch May 15, 2023 14:11

DHowett reviewed May 15, 2023

View reviewed changes

j4james mentioned this pull request May 16, 2023

Windows Terminal throws an exception on exit #15364

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix DesktopWindowXamlSource related crashes when closing a window#15338

Fix DesktopWindowXamlSource related crashes when closing a window#15338
microsoft-github-policy-service[bot] merged 3 commits intomainfrom
dev/lhecker/xaml-source-close-crash

lhecker commented May 12, 2023 •

edited

Loading

Uh oh!

lhecker May 12, 2023

Uh oh!

lhecker May 12, 2023 •

edited

Loading

Uh oh!

lhecker May 12, 2023

Uh oh!

lhecker May 12, 2023

Uh oh!

DHowett May 12, 2023

Uh oh!

DHowett May 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

lhecker commented May 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Validation Steps Performed

Uh oh!

lhecker May 12, 2023

Choose a reason for hiding this comment

Uh oh!

lhecker May 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lhecker May 12, 2023

Choose a reason for hiding this comment

Uh oh!

lhecker May 12, 2023

Choose a reason for hiding this comment

Uh oh!

DHowett May 12, 2023

Choose a reason for hiding this comment

Uh oh!

DHowett May 15, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lhecker commented May 12, 2023 •

edited

Loading

lhecker May 12, 2023 •

edited

Loading