Skip to content

Commit 6f5b293

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_lADOAF3p4s4BBcTlzgfScpM Service-Version: 1.24
1 parent e33bc3d commit 6f5b293

1 file changed

Lines changed: 3 additions & 3 deletions

File tree

src/cascadia/WindowsTerminal/WindowEmperor.cpp

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

0 commit comments

Comments
 (0)