maxmustermann/terminal
1
0
Fork 0
Code Issues Pull requests Projects Releases Wiki Activity

Fix racy access to _tsfTryRedrawCanvas in TermControl (#10549)

Browse source 
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 ✔️
This commit is contained in:
Leonard Hecker 2021-07-06 23:59:44 +02:00 committed by GitHub
parent 59239e3b07
commit 83c6bce73d
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
2 changed files with 65 additions and 75 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

127
src/cascadia/TerminalControl/TermControl.cpp
View file

@ -48,9 +48,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
{
TermControl::TermControl(IControlSettings settings,
TerminalConnection::ITerminalConnection connection) :
_initializedTerminal{ false },
_settings{ settings },
_closing{ false },
_isInternalScrollBarUpdate{ false },
_autoScrollVelocity{ 0 },
_autoScrollingPointerPoint{ std::nullopt },
@ -110,11 +108,15 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// Core eventually won't have access to. When we get to
// https://github.com/microsoft/terminal/projects/5#card-50760282
// then we'll move the applicable ones.
//
// These four throttled functions are triggered by terminal output and interact with the UI.
// Since Close() is the point after which we are removed from the UI, but before the
// destructor has run, we MUST check control->_IsClosing() before actually doing anything.
_tsfTryRedrawCanvas = std::make_shared<ThrottledFuncTrailing<>>(
Dispatcher(),
TsfRedrawInterval,
[weakThis = get_weak()]() {
if (auto control{ weakThis.get() })
if (auto control{ weakThis.get() }; !control->_IsClosing())
{
control->TSFInputControl().TryRedrawCanvas();
}
@ -124,7 +126,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
Dispatcher(),
UpdatePatternLocationsInterval,
[weakThis = get_weak()]() {
if (auto control{ weakThis.get() })
if (auto control{ weakThis.get() }; !control->_IsClosing())
{
control->_core->UpdatePatternLocations();
}
@ -134,7 +136,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
Dispatcher(),
TerminalWarningBellInterval,
[weakThis = get_weak()]() {
if (auto control{ weakThis.get() })
if (auto control{ weakThis.get() }; !control->_IsClosing())
{
control->_WarningBellHandlers(*control, nullptr);
}
@ -144,14 +146,14 @@ namespace winrt::Microsoft::Terminal::Control::implementation
Dispatcher(),
ScrollBarUpdateInterval,
[weakThis = get_weak()](const auto& update) {
if (auto control{ weakThis.get() })
if (auto control{ weakThis.get() }; !control->_IsClosing())
{
control->_isInternalScrollBarUpdate = true;
auto scrollBar = control->ScrollBar();
if (update.newValue.has_value())
if (update.newValue)
{
scrollBar.Value(update.newValue.value());
scrollBar.Value(*update.newValue);
}
scrollBar.Maximum(update.newMaximum);
scrollBar.Minimum(update.newMinimum);
@ -205,7 +207,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
void TermControl::SearchMatch(const bool goForward)
{
if (_closing)
if (_IsClosing())
{
return;
}
@ -296,7 +298,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// - newSettings: the new settings to set
void TermControl::_UpdateSettingsFromUIThread(IControlSettings newSettings)
{
if (_closing)
if (_IsClosing())
{
return;
}
@ -314,7 +316,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// - newAppearance: the new appearance to set
void TermControl::_UpdateAppearanceFromUIThread(IControlAppearance newAppearance)
{
if (_closing)
if (_IsClosing())
{
return;
}
@ -527,7 +529,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
Windows::UI::Xaml::Automation::Peers::AutomationPeer TermControl::OnCreateAutomationPeer()
try
{
if (_initializedTerminal && !_closing) // only set up the automation peer if we're ready to go live
if (_initializedTerminal && !_IsClosing()) // only set up the automation peer if we're ready to go live
{
// create a custom automation peer with this code pattern:
// (https://docs.microsoft.com/en-us/windows/uwp/design/accessibility/custom-automation-peers)
@ -747,7 +749,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
void TermControl::_CharacterHandler(winrt::Windows::Foundation::IInspectable const& /*sender*/,
Input::CharacterReceivedRoutedEventArgs const& e)
{
if (_closing)
if (_IsClosing())
{
return;
}
@ -845,7 +847,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// win32-input-mode, then we'll send all these keystrokes to the
// terminal - it's smart enough to ignore the keys it doesn't care
// about.
if (_closing ||
if (_IsClosing() ||
e.OriginalKey() == VirtualKey::LeftWindows ||
e.OriginalKey() == VirtualKey::RightWindows)
@ -997,12 +999,12 @@ namespace winrt::Microsoft::Terminal::Control::implementation
keyDown) :
true;
if (_cursorTimer.has_value())
if (_cursorTimer)
{
// Manually show the cursor when a key is pressed. Restarting
// the timer prevents flickering.
_core->CursorOn(true);
_cursorTimer.value().Start();
_cursorTimer->Start();
}
return handled;
@ -1027,7 +1029,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
void TermControl::_PointerPressedHandler(Windows::Foundation::IInspectable const& sender,
Input::PointerRoutedEventArgs const& args)
{
if (_closing)
if (_IsClosing())
{
return;
}
@ -1078,7 +1080,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
void TermControl::_PointerMovedHandler(Windows::Foundation::IInspectable const& /*sender*/,
Input::PointerRoutedEventArgs const& args)
{
if (_closing)
if (_IsClosing())
{
return;
}
@ -1151,7 +1153,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
void TermControl::_PointerReleasedHandler(Windows::Foundation::IInspectable const& sender,
Input::PointerRoutedEventArgs const& args)
{
if (_closing)
if (_IsClosing())
{
return;
}
@ -1193,7 +1195,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
void TermControl::_MouseWheelHandler(Windows::Foundation::IInspectable const& /*sender*/,
Input::PointerRoutedEventArgs const& args)
{
if (_closing)
if (_IsClosing())
{
return;
}
@ -1295,7 +1297,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
void TermControl::_ScrollbarChangeHandler(Windows::Foundation::IInspectable const& /*sender*/,
Controls::Primitives::RangeBaseValueChangedEventArgs const& args)
{
if (_isInternalScrollBarUpdate || _closing)
if (_isInternalScrollBarUpdate || _IsClosing())
{
// The update comes from ourselves, more specifically from the
// terminal. So we don't have to update the terminal because it
@ -1361,15 +1363,15 @@ namespace winrt::Microsoft::Terminal::Control::implementation
void TermControl::_TryStartAutoScroll(Windows::UI::Input::PointerPoint const& pointerPoint, const double scrollVelocity)
{
// Allow only one pointer at the time
if (!_autoScrollingPointerPoint.has_value() ||
_autoScrollingPointerPoint.value().PointerId() == pointerPoint.PointerId())
if (!_autoScrollingPointerPoint ||
_autoScrollingPointerPoint->PointerId() == pointerPoint.PointerId())
{
_autoScrollingPointerPoint = pointerPoint;
_autoScrollVelocity = scrollVelocity;
// If this is first time the auto scroll update is about to be called,
// kick-start it by initializing its time delta as if it started now
if (!_lastAutoScrollUpdateTime.has_value())
if (!_lastAutoScrollUpdateTime)
{
_lastAutoScrollUpdateTime = std::chrono::high_resolution_clock::now();
}
@ -1388,8 +1390,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// - pointerId: id of pointer for which to stop auto scroll
void TermControl::_TryStopAutoScroll(const uint32_t pointerId)
{
if (_autoScrollingPointerPoint.has_value() &&
pointerId == _autoScrollingPointerPoint.value().PointerId())
if (_autoScrollingPointerPoint &&
pointerId == _autoScrollingPointerPoint->PointerId())
{
_autoScrollingPointerPoint = std::nullopt;
_autoScrollVelocity = 0;
@ -1415,15 +1417,15 @@ namespace winrt::Microsoft::Terminal::Control::implementation
{
const auto timeNow = std::chrono::high_resolution_clock::now();
if (_lastAutoScrollUpdateTime.has_value())
if (_lastAutoScrollUpdateTime)
{
static constexpr double microSecPerSec = 1000000.0;
const double deltaTime = std::chrono::duration_cast<std::chrono::microseconds>(timeNow - _lastAutoScrollUpdateTime.value()).count() / microSecPerSec;
const double deltaTime = std::chrono::duration_cast<std::chrono::microseconds>(timeNow - *_lastAutoScrollUpdateTime).count() / microSecPerSec;
ScrollBar().Value(ScrollBar().Value() + _autoScrollVelocity * deltaTime);
if (_autoScrollingPointerPoint.has_value())
if (_autoScrollingPointerPoint)
{
_SetEndSelectionPointAtCursor(_autoScrollingPointerPoint.value().Position());
_SetEndSelectionPointAtCursor(_autoScrollingPointerPoint->Position());
}
}
@ -1439,7 +1441,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
void TermControl::_GotFocusHandler(Windows::Foundation::IInspectable const& /* sender */,
RoutedEventArgs const& /* args */)
{
if (_closing)
if (_IsClosing())
{
return;
}
@ -1468,16 +1470,16 @@ namespace winrt::Microsoft::Terminal::Control::implementation
TSFInputControl().NotifyFocusEnter();
}
if (_cursorTimer.has_value())
if (_cursorTimer)
{
// When the terminal focuses, show the cursor immediately
_core->CursorOn(true);
_cursorTimer.value().Start();
_cursorTimer->Start();
}
if (_blinkTimer.has_value())
if (_blinkTimer)
{
_blinkTimer.value().Start();
_blinkTimer->Start();
}
_interactivity->GainFocus();
@ -1498,7 +1500,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
void TermControl::_LostFocusHandler(Windows::Foundation::IInspectable const& /* sender */,
RoutedEventArgs const& /* args */)
{
if (_closing)
if (_IsClosing())
{
return;
}
@ -1517,15 +1519,15 @@ namespace winrt::Microsoft::Terminal::Control::implementation
TSFInputControl().NotifyFocusLeave();
}
if (_cursorTimer.has_value())
if (_cursorTimer)
{
_cursorTimer.value().Stop();
_cursorTimer->Stop();
_core->CursorOn(false);
}
if (_blinkTimer.has_value())
if (_blinkTimer)
{
_blinkTimer.value().Stop();
_blinkTimer->Stop();
}
// Check if there is an unfocused config we should set the appearance to
@ -1544,7 +1546,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
void TermControl::_SwapChainSizeChanged(winrt::Windows::Foundation::IInspectable const& /*sender*/,
SizeChangedEventArgs const& e)
{
if (!_initializedTerminal || _closing)
if (!_initializedTerminal || _IsClosing())
{
return;
}
@ -1596,7 +1598,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
void TermControl::_CursorTimerTick(Windows::Foundation::IInspectable const& /* sender */,
Windows::Foundation::IInspectable const& /* e */)
{
if (!_closing)
if (!_IsClosing())
{
_core->BlinkCursor();
}
@ -1610,7 +1612,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
void TermControl::_BlinkTimerTick(Windows::Foundation::IInspectable const& /* sender */,
Windows::Foundation::IInspectable const& /* e */)
{
if (!_closing)
if (!_IsClosing())
{
_core->BlinkAttributeTick();
}
@ -1638,13 +1640,6 @@ namespace winrt::Microsoft::Terminal::Control::implementation
void TermControl::_ScrollPositionChanged(const IInspectable& /*sender*/,
const Control::ScrollPositionChangedArgs& args)
{
// Since this callback fires from non-UI thread, we might be already
// closed/closing.
if (_closing.load())
{
return;
}
ScrollBarUpdate update;
const auto hiddenContent = args.BufferSize() - args.ViewHeight();
update.newMaximum = hiddenContent;
@ -1664,10 +1659,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
void TermControl::_CursorPositionChanged(const IInspectable& /*sender*/,
const IInspectable& /*args*/)
{
if (_tsfTryRedrawCanvas)
{
_tsfTryRedrawCanvas->Run();
}
_tsfTryRedrawCanvas->Run();
}
hstring TermControl::Title()
@ -1700,7 +1692,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// if we should defer which formats are copied to the global setting
bool TermControl::CopySelectionToClipboard(bool singleLine, const Windows::Foundation::IReference<CopyFormat>& formats)
{
if (_closing)
if (_IsClosing())
{
return false;
}
@ -1717,21 +1709,12 @@ namespace winrt::Microsoft::Terminal::Control::implementation
void TermControl::Close()
{
if (!_closing.exchange(true))
if (!_IsClosing())
{
_closing = true;
_core->ReceivedOutput(_coreOutputEventToken);
_RestorePointerCursorHandlers(*this, nullptr);
// These four throttled functions are triggered by terminal output and interact with the UI.
// Since Close() is the point after which we are removed from the UI, but before the destructor
// has run, we should disconnect them *right now*. If we don't, they may fire between the
// throttle delay (from the final output) and the dtor.
_tsfTryRedrawCanvas.reset();
_updatePatternLocations.reset();
_updateScrollBar.reset();
_playWarningBell.reset();
// Disconnect the TSF input control so it doesn't receive EditContext events.
TSFInputControl().Close();
_autoScrollTimer.Stop();
@ -2097,7 +2080,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// - <none>
void TermControl::_CompositionCompleted(winrt::hstring text)
{
if (_closing)
if (_IsClosing())
{
return;
}
@ -2174,7 +2157,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
winrt::fire_and_forget TermControl::_DragDropHandler(Windows::Foundation::IInspectable /*sender*/,
DragEventArgs e)
{
if (_closing)
if (_IsClosing())
{
return;
}
@ -2261,7 +2244,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
void TermControl::_DragOverHandler(Windows::Foundation::IInspectable const& /*sender*/,
DragEventArgs const& e)
{
if (_closing)
if (_IsClosing())
{
return;
}
@ -2422,7 +2405,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// Stop the timer and switch off the light
_bellLightTimer.Stop();
if (!_closing)
if (!_IsClosing())
{
VisualBellLight::SetIsTarget(RootGrid(), false);
}
@ -2465,7 +2448,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
if (auto self{ weakThis.get() })
{
auto lastHoveredCell = _core->GetHoveredCell();
if (lastHoveredCell.has_value())
if (lastHoveredCell)
{
const auto uriText = _core->GetHoveredUriText();
if (!uriText.empty())

13
src/cascadia/TerminalControl/TermControl.h
View file

@ -149,9 +149,9 @@ namespace winrt::Microsoft::Terminal::Control::implementation
winrt::com_ptr<SearchBoxControl> _searchBox;
IControlSettings _settings;
std::atomic<bool> _closing;
bool _focused;
bool _initializedTerminal;
bool _closing{ false };
bool _focused{ false };
bool _initializedTerminal{ false };
std::shared_ptr<ThrottledFuncTrailing<>> _tsfTryRedrawCanvas;
std::shared_ptr<ThrottledFuncTrailing<>> _updatePatternLocations;
@ -183,6 +183,13 @@ 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());
return _closing;
}
void _UpdateSettingsFromUIThread(IControlSettings newSettings);
void _UpdateAppearanceFromUIThread(IControlAppearance newAppearance);
void _ApplyUISettings(const IControlSettings&);