microsoft/terminal
1
0
Fork 0
mirror of https://github.com/microsoft/terminal.git synced 2025-12-10 18:43:54 -06:00
Code Issues Packages Projects Releases 138 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
This commit is contained in:
Dustin L. Howett 2025-09-30 16:05:58 -05:00 committed by GitHub
parent 52f9bd6d2c
commit 5976de1600
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
1 changed files with 3 additions and 3 deletions
Show Stats 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;
}
}