Fix racy access to _tsfTryRedrawCanvas in TermControl #10549

lhecker · 2021-07-02T16:26:45Z

Previously TermControl::Close destroyed all ThrottledFuncs to ensure they're not scheduling any callbacks on the UI thread, as the call to Close signals the point at which the TermControl isn't part of the UI thread anymore. _CursorPositionChanged tried to prevent access to the potentially deallocated _tsfTryRedrawCanvas by checking the std::shared_ptr for nullability, but since the deallocation happens on the UI thread and the nullability check on a background thread, this check introduced a race condition.

This commit solves the issue by not deallocating any ThrottledFuncs anymore and instead checking the _closing flag inside the ThrottledFunc callback on the UI thread.

Additionally this commit cleans up some antipatterns around the use of std::optional.

PR Checklist

Validation Steps Performed

Opening and closing tabs doesn't crash ✔️
Printing long text doesn't crash ✔️
Manual scrolling doesn't crash ✔️
^G / the audible bell doesn't crash ✔️

lhecker · 2021-07-02T16:29:38Z

src/cascadia/TerminalControl/TermControl.h

@@ -183,6 +183,12 @@ namespace winrt::Microsoft::Terminal::Control::implementation

        winrt::Windows::UI::Xaml::Controls::SwapChainPanel::LayoutUpdated_revoker _layoutUpdatedRevoker;

+        inline bool _IsClosing() const noexcept {
+            // _closing isn't atomic and may only be accessed from the main thread.
+            assert(Dispatcher().HasThreadAccess());


I previously wanted to introduce a cool macro for this in WinRTUtils, but I realized, that global state would only work properly if it's shared using a .dll. A .lib like that wouldn't do that for us. 😟

This assert is compiled out in debug builds, correct?

Sorry, compiled out in release builds./

lhecker · 2021-07-02T23:44:21Z

A branch with a minimalistic set of changes for 1.8 is ready for backporting.

This PR in particular needs to be evaluated very carefully and theoretically as the interaction with _closing is basically a "logical race condition". Despite that I forgot to set the _closing variable to true prior to my latest commit, the code worked fine on my PC through many rounds of testing, which just further proves how much of a race condition this really is.

DHowett · 2021-07-06T21:25:20Z

note: i edited your PR body to make sure the Closes verb was repeated for each closed issue. GitHub will not close two issues if you say Closes X, Y.

DHowett · 2021-07-06T21:26:18Z

src/cascadia/TerminalControl/TermControl.h

@@ -183,6 +183,12 @@ namespace winrt::Microsoft::Terminal::Control::implementation

        winrt::Windows::UI::Xaml::Controls::SwapChainPanel::LayoutUpdated_revoker _layoutUpdatedRevoker;

+        inline bool _IsClosing() const noexcept {
+            // _closing isn't atomic and may only be accessed from the main thread.
+            assert(Dispatcher().HasThreadAccess());


Sorry, compiled out in release builds./

ghost · 2021-07-06T21:59:41Z

Hello @lhecker!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

Previously `TermControl::Close` destroyed all `ThrottledFunc`s to ensure they're not scheduling any callbacks on the UI thread, as the call to `Close` signals the point at which the `TermControl` isn't part of the UI thread anymore. `_CursorPositionChanged` tried to prevent access to the potentially deallocated `_tsfTryRedrawCanvas` by checking the `std::shared_ptr` for nullability, but since the deallocation happens on the UI thread and the nullability check on a background thread, this check introduced a race condition. This commit solves the issue by not deallocating any `ThrottledFunc`s anymore and instead checking the `_closing` flag inside the `ThrottledFunc` callback on the UI thread. Additionally this commit cleans up some antipatterns around the use of `std::optional`. ## PR Checklist * [x] Closes #10479 * [x] Closes #10302 ## Validation Steps Performed * Opening and closing tabs doesn't crash ✔️ * Printing long text doesn't crash ✔️ * Manual scrolling doesn't crash ✔️ * ^G / the audible bell doesn't crash ✔️ (cherry picked from commit 83c6bce)

ghost · 2021-07-14T17:01:47Z

🎉Windows Terminal v1.9.1942.0 has been released which incorporates this pull request.:tada:

Handy links:

ghost · 2021-07-14T17:06:05Z

🎉Windows Terminal Preview v1.10.1933.0 has been released which incorporates this pull request.:tada:

Handy links:

Fix racy access to _tsfTryRedrawCanvas in TermControl

1900fad

lhecker commented Jul 2, 2021

View reviewed changes

Fix formatting check

ca63317

DHowett added zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes. zStable-Service-Queued-1.12 A floating label that tracks the current Stable version for servicing purposes. labels Jul 2, 2021

Fix _closing not being set

52ae1f1

DHowett approved these changes Jul 6, 2021

View reviewed changes

lhecker added the Needs-Second It's a PR that needs another sign-off label Jul 6, 2021

ghost requested review from zadjii-msft, miniksa, carlos-zamora, leonMSFT and PankajBhojwani July 6, 2021 21:49

carlos-zamora approved these changes Jul 6, 2021

View reviewed changes

lhecker added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jul 6, 2021

ghost merged commit 83c6bce into main Jul 6, 2021

ghost deleted the dev/lhecker/exit-crash-fix branch July 6, 2021 21:59

DHowett removed zStable-Service-Queued-1.12 A floating label that tracks the current Stable version for servicing purposes. zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes. labels Jul 7, 2021

carlos-zamora mentioned this pull request Jul 12, 2021

Terminal app crashes when the last tab is closed #457

Closed

This was referenced Jul 14, 2021

Exiting a tab sometimes closes the entire window #10302

Closed

Entire Terminal app is randomly crashing while closing tabs #10479

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix racy access to _tsfTryRedrawCanvas in TermControl #10549

Fix racy access to _tsfTryRedrawCanvas in TermControl #10549

lhecker commented Jul 2, 2021 •

edited by DHowett

Loading

lhecker Jul 2, 2021

DHowett Jul 6, 2021

DHowett Jul 6, 2021

lhecker commented Jul 2, 2021

DHowett commented Jul 6, 2021

DHowett Jul 6, 2021

ghost commented Jul 6, 2021

ghost commented Jul 14, 2021

ghost commented Jul 14, 2021

Fix racy access to _tsfTryRedrawCanvas in TermControl #10549

Fix racy access to _tsfTryRedrawCanvas in TermControl #10549

Conversation

lhecker commented Jul 2, 2021 • edited by DHowett Loading

PR Checklist

Validation Steps Performed

lhecker Jul 2, 2021

Choose a reason for hiding this comment

DHowett Jul 6, 2021

Choose a reason for hiding this comment

DHowett Jul 6, 2021

Choose a reason for hiding this comment

lhecker commented Jul 2, 2021

DHowett commented Jul 6, 2021

DHowett Jul 6, 2021

Choose a reason for hiding this comment

ghost commented Jul 6, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

ghost commented Jul 14, 2021

ghost commented Jul 14, 2021

lhecker commented Jul 2, 2021 •

edited by DHowett

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.