Skip to content

Commit b4270e5

Browse files
committed
Avoid reentrancy issues when dropping AppHost, even harder (#19395)
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 8d41ace (cherry picked from commit 5976de1) Service-Card-Id: PVTI_lADOAF3p4s4AxadtzgfScoo Service-Version: 1.23
1 parent a4c512f commit b4270e5

File tree

1 file changed

+3
-3
lines changed

1 file changed

+3
-3
lines changed

src/cascadia/WindowsTerminal/WindowEmperor.cpp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -878,16 +878,16 @@ LRESULT WindowEmperor::_messageHandler(HWND window, UINT const message, WPARAM c
878878
{
879879
if (host == it->get())
880880
{
881-
// NOTE: The AppHost destructor is highly non-trivial.
881+
// NOTE: AppHost::Close is highly non-trivial.
882882
//
883883
// It _may_ call into XAML, which _may_ pump the message loop, which would then recursively
884884
// re-enter this function, which _may_ then handle another WM_CLOSE_TERMINAL_WINDOW,
885885
// which would change the _windows array, and invalidate our iterator and crash.
886886
//
887-
// We can prevent this by deferring destruction until after the erase() call.
887+
// We can prevent this by deferring Close() until after the erase() call.
888888
const auto strong = *it;
889-
strong->Close();
890889
_windows.erase(it);
890+
strong->Close();
891891
break;
892892
}
893893
}

0 commit comments

Comments
 (0)