starred/terminal
1
0
Fork 0
mirror of https://github.com/microsoft/terminal.git synced 2026-02-11 05:34:57 +00:00
Code Issues 19.0k Packages Projects Releases 20 Wiki Activity

Avoid reentrancy issues when dropping AppHost, even harder (#19395)

Browse Source 
The previous fix in #19296 moved the _destruction_ of AppHost into the
tail end after we manipulate the `_windows` vector; however, it kept the
part which calls into XAML (`Close`) before the `erase`. I suspect that
we still had some reentrancy issues, where we cached an iterator before
the list was modified by another window close event.

That is:

```mermaid
sequenceDiagram
		Emperor->>Emperor: Close Window
		Emperor->>+AppHost: Close (a)
		AppHost->>XAML: Close
		XAML-->>Emperor: pump loop
		Emperor->>Emperor: Close Window
		Emperor->>+AppHost: Close (b)
		AppHost->>XAML: Close
		XAML-->>Emperor: pump loop
		AppHost->>-Emperor: Closed
		Emperor->>Emperor: erase(b)
		AppHost->>-Emperor: Closed
		Emperor->>Emperor: erase(a)
```

Moving the `Close()` to after the `erase` ensures that there are no
cached iterators that survive beyond XAML pumping the message loop.

Fixes 8d41ace3

(cherry picked from commit 5976de1600)
Service-Card-Id: PVTI_lADOAF3p4s4BBcTlzgfScpM
Service-Version: 1.24
This commit is contained in:
Dustin L. Howett
2025-09-30 16:05:58 -05:00
committed by Dustin L. Howett
parent e33bc3d137
commit 6f5b293a56
1 changed files with 3 additions and 3 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
src/cascadia/WindowsTerminal/WindowEmperor.cpp
View File

@@ -880,16 +880,16 @@ LRESULT WindowEmperor::_messageHandler(HWND window, UINT const message, WPARAM c
{
if (host == it->get())
{
// NOTE: The AppHost destructor is highly non-trivial.
// NOTE: AppHost::Close is highly non-trivial.
//
// It _may_ call into XAML, which _may_ pump the message loop, which would then recursively
// re-enter this function, which _may_ then handle another WM_CLOSE_TERMINAL_WINDOW,
// which would change the _windows array, and invalidate our iterator and crash.
//
// We can prevent this by deferring destruction until after the erase() call.
// We can prevent this by deferring Close() until after the erase() call.
const auto strong = *it;
strong->Close();
_windows.erase(it);
strong->Close();
break;
}
}