starred/terminal
1
0
Fork 0
mirror of https://github.com/microsoft/terminal.git synced 2026-05-19 13:06:47 +00:00
Code Issues 19.0k Packages Projects Releases 20 Wiki Activity

Fix race conditions in ControlCore (#15334)

Browse Source 
`ControlCore` contained two bugs:
* Race condition on access of the 3 throttled funcs which may now
  be `reset()` during tear out
* The `ScrollPositionChanged` event emitter was written incorrectly
  and would emit the event from the background thread without
  throttling during tear out
This commit is contained in:
Leonard Hecker
2023-05-12 22:11:22 +02:00
committed by GitHub
parent d6eb022975
commit 6a26fd68c4
4 changed files with 57 additions and 52 deletions
Download Patch File Download Diff File Expand all files Collapse all files

83
src/cascadia/TerminalControl/ControlCore.cpp
View File

@@ -82,9 +82,10 @@ namespace winrt::Microsoft::Terminal::Control::implementation
_actualFont{ DEFAULT_FONT_FACE, 0, DEFAULT_FONT_WEIGHT, { 0, DEFAULT_FONT_SIZE }, CP_UTF8, false }
{
_settings = winrt::make_self<implementation::ControlSettings>(settings, unfocusedAppearance);
_terminal = std::make_shared<::Microsoft::Terminal::Core::Terminal>();
_setupDispatcherAndCallbacks();
Connection(connection);
_terminal->SetWriteInputCallback([this](std::wstring_view wstr) {
@@ -137,17 +138,10 @@ namespace winrt::Microsoft::Terminal::Control::implementation
_renderer->SetBackgroundColorChangedCallback([this]() { _rendererBackgroundColorChanged(); });
_renderer->SetFrameColorChangedCallback([this]() { _rendererTabColorChanged(); });
_renderer->SetRendererEnteredErrorStateCallback([weakThis = get_weak()]() {
if (auto strongThis{ weakThis.get() })
{
strongThis->_RendererEnteredErrorStateHandlers(*strongThis, nullptr);
}
});
_renderer->SetRendererEnteredErrorStateCallback([this]() { _RendererEnteredErrorStateHandlers(nullptr, nullptr); });
THROW_IF_FAILED(localPointerToThread->Initialize(_renderer.get()));
}
_setupDispatcherAndCallbacks();
UpdateSettings(settings, unfocusedAppearance);
}
@@ -179,7 +173,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// need to hop across the process boundary every time text is output.
// We can throttle this to once every 8ms, which will get us out of
// the way of the main output & rendering threads.
_tsfTryRedrawCanvas = std::make_shared<ThrottledFuncTrailing<>>(
const auto shared = _shared.lock();
shared->tsfTryRedrawCanvas = std::make_shared<ThrottledFuncTrailing<>>(
_dispatcher,
TsfRedrawInterval,
[weakThis = get_weak()]() {
@@ -191,7 +186,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// NOTE: Calling UpdatePatternLocations from a background
// thread is a workaround for us to hit GH#12607 less often.
_updatePatternLocations = std::make_unique<til::throttled_func_trailing<>>(
shared->updatePatternLocations = std::make_unique<til::throttled_func_trailing<>>(
UpdatePatternLocationsInterval,
[weakTerminal = std::weak_ptr{ _terminal }]() {
if (const auto t = weakTerminal.lock())
@@ -201,7 +196,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
}
});
_updateScrollBar = std::make_shared<ThrottledFuncTrailing<Control::ScrollPositionChangedArgs>>(
shared->updateScrollBar = std::make_shared<ThrottledFuncTrailing<Control::ScrollPositionChangedArgs>>(
_dispatcher,
ScrollBarUpdateInterval,
[weakThis = get_weak()](const auto& update) {
@@ -231,9 +226,10 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// Clear out any throttled funcs that we had wired up to run on this UI
// thread. These will be recreated in _setupDispatcherAndCallbacks, when
// we're re-attached to a new control (on a possibly new UI thread).
_tsfTryRedrawCanvas.reset();
_updatePatternLocations.reset();
_updateScrollBar.reset();
const auto shared = _shared.lock();
shared->tsfTryRedrawCanvas.reset();
shared->updatePatternLocations.reset();
shared->updateScrollBar.reset();
}
void ControlCore::AttachToNewControl(const Microsoft::Terminal::Control::IKeyBindings& keyBindings)
@@ -318,7 +314,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
{ // scope for terminalLock
auto terminalLock = _terminal->LockForWriting();
if (_initializedTerminal)
if (_initializedTerminal.load(std::memory_order_relaxed))
{
return false;
}
@@ -403,7 +399,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
THROW_IF_FAILED(_renderEngine->Enable());
_initializedTerminal = true;
_initializedTerminal.store(true, std::memory_order_relaxed);
} // scope for TerminalLock
// Start the connection outside of lock, because it could
@@ -423,7 +419,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// - <none>
void ControlCore::EnablePainting()
{
if (_initializedTerminal)
if (_initializedTerminal.load(std::memory_order_relaxed))
{
_renderer->EnablePainting();
}
@@ -656,9 +652,10 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// itself - it was initiated by the mouse wheel, or the scrollbar.
_terminal->UserScrollViewport(viewTop);
if (_updatePatternLocations)
const auto shared = _shared.lock_shared();
if (shared->updatePatternLocations)
{
(*_updatePatternLocations)();
(*shared->updatePatternLocations)();
}
}
@@ -825,7 +822,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// Update the terminal core with its new Core settings
_terminal->UpdateSettings(*_settings);
if (!_initializedTerminal)
if (!_initializedTerminal.load(std::memory_order_relaxed))
{
// If we haven't initialized, there's no point in continuing.
// Initialization will handle the renderer settings.
@@ -1434,10 +1431,11 @@ namespace winrt::Microsoft::Terminal::Control::implementation
const int viewHeight,
const int bufferSize)
{
if (!_initializedTerminal)
if (!_initializedTerminal.load(std::memory_order_relaxed))
{
return;
}
// Clear the regex pattern tree so the renderer does not try to render them while scrolling
// We're **NOT** taking the lock here unlike _scrollbarChangeHandler because
// we are already under lock (since this usually happens as a result of writing).
@@ -1448,20 +1446,18 @@ namespace winrt::Microsoft::Terminal::Control::implementation
auto update{ winrt::make<ScrollPositionChangedArgs>(viewTop,
viewHeight,
bufferSize) };
if (!_inUnitTests && _updateScrollBar)
{
_updateScrollBar->Run(update);
}
else
if (_inUnitTests) [[unlikely]]
{
_ScrollPositionChangedHandlers(*this, update);
}
// Additionally, start the throttled update of where our links are.
if (_updatePatternLocations)
else
{
(*_updatePatternLocations)();
const auto shared = _shared.lock_shared();
if (shared->updateScrollBar)
{
shared->updateScrollBar->Run(update);
}
}
}
@@ -1469,9 +1465,10 @@ namespace winrt::Microsoft::Terminal::Control::implementation
{
// When the buffer's cursor moves, start the throttled func to
// eventually dispatch a CursorPositionChanged event.
if (_tsfTryRedrawCanvas)
const auto shared = _shared.lock_shared();
if (shared->tsfTryRedrawCanvas)
{
_tsfTryRedrawCanvas->Run();
shared->tsfTryRedrawCanvas->Run();
}
}
@@ -1482,11 +1479,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation
void ControlCore::_terminalShowWindowChanged(bool showOrHide)
{
if (_initializedTerminal)
{
auto showWindow = winrt::make_self<implementation::ShowWindowArgs>(showOrHide);
_ShowWindowChangedHandlers(*this, *showWindow);
}
auto showWindow = winrt::make_self<implementation::ShowWindowArgs>(showOrHide);
_ShowWindowChangedHandlers(*this, *showWindow);
}
// Method Description:
@@ -1617,6 +1611,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation
const auto weakThis{ get_weak() };
// Concurrent read of _dispatcher is safe, because Detach() calls WaitForPaintCompletionAndDisable()
// which blocks until this call returns. _dispatcher will only be changed afterwards.
co_await wil::resume_foreground(_dispatcher);
if (auto core{ weakThis.get() })
@@ -1686,7 +1682,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
Core::Point ControlCore::CursorPosition() const
{
// If we haven't been initialized yet, then fake it.
if (!_initializedTerminal)
if (!_initializedTerminal.load(std::memory_order_relaxed))
{
return { 0, 0 };
}
@@ -1805,9 +1801,10 @@ namespace winrt::Microsoft::Terminal::Control::implementation
_terminal->Write(hstr);
// Start the throttled update of where our hyperlinks are.
if (_updatePatternLocations)
const auto shared = _shared.lock_shared();
if (shared->updatePatternLocations)
{
(*_updatePatternLocations)();
(*shared->updatePatternLocations)();
}
}
catch (...)
@@ -2016,7 +2013,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// - <none>
void ControlCore::WindowVisibilityChanged(const bool showOrHide)
{
if (_initializedTerminal)
if (_initializedTerminal.load(std::memory_order_relaxed))
{
// show is true, hide is false
if (auto conpty{ _connection.try_as<TerminalConnection::ConptyConnection>() })

13
src/cascadia/TerminalControl/ControlCore.h
View File

@@ -266,7 +266,14 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// clang-format on
private:
bool _initializedTerminal{ false };
struct SharedState
{
std::shared_ptr<ThrottledFuncTrailing<>> tsfTryRedrawCanvas;
std::unique_ptr<til::throttled_func_trailing<>> updatePatternLocations;
std::shared_ptr<ThrottledFuncTrailing<Control::ScrollPositionChangedArgs>> updateScrollBar;
};
std::atomic<bool> _initializedTerminal{ false };
bool _closing{ false };
TerminalConnection::ITerminalConnection _connection{ nullptr };
@@ -313,9 +320,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
uint64_t _owningHwnd{ 0 };
winrt::Windows::System::DispatcherQueue _dispatcher{ nullptr };
std::shared_ptr<ThrottledFuncTrailing<>> _tsfTryRedrawCanvas;
std::unique_ptr<til::throttled_func_trailing<>> _updatePatternLocations;
std::shared_ptr<ThrottledFuncTrailing<Control::ScrollPositionChangedArgs>> _updateScrollBar;
til::shared_mutex<SharedState> _shared;
til::point _contextMenuBufferPosition{ 0, 0 };

12
src/cascadia/TerminalControl/ControlCore.idl
View File

@@ -149,26 +149,28 @@ namespace Microsoft.Terminal.Control
Boolean ShouldShowSelectCommand();
Boolean ShouldShowSelectOutput();
event FontSizeChangedEventArgs FontSizeChanged;
// These events are called from some background thread
event Windows.Foundation.TypedEventHandler<Object, CopyToClipboardEventArgs> CopyToClipboard;
event Windows.Foundation.TypedEventHandler<Object, TitleChangedEventArgs> TitleChanged;
event Windows.Foundation.TypedEventHandler<Object, Object> WarningBell;
event Windows.Foundation.TypedEventHandler<Object, Object> TabColorChanged;
event Windows.Foundation.TypedEventHandler<Object, Object> BackgroundColorChanged;
event Windows.Foundation.TypedEventHandler<Object, Object> TaskbarProgressChanged;
event Windows.Foundation.TypedEventHandler<Object, Object> RendererEnteredErrorState;
event Windows.Foundation.TypedEventHandler<Object, ShowWindowArgs> ShowWindowChanged;
// These events are always called from the UI thread (bugs aside)
event FontSizeChangedEventArgs FontSizeChanged;
event Windows.Foundation.TypedEventHandler<Object, ScrollPositionChangedArgs> ScrollPositionChanged;
event Windows.Foundation.TypedEventHandler<Object, Object> CursorPositionChanged;
event Windows.Foundation.TypedEventHandler<Object, Object> TaskbarProgressChanged;
event Windows.Foundation.TypedEventHandler<Object, Object> ConnectionStateChanged;
event Windows.Foundation.TypedEventHandler<Object, Object> HoveredHyperlinkChanged;
event Windows.Foundation.TypedEventHandler<Object, Object> RendererEnteredErrorState;
event Windows.Foundation.TypedEventHandler<Object, Object> SwapChainChanged;
event Windows.Foundation.TypedEventHandler<Object, RendererWarningArgs> RendererWarning;
event Windows.Foundation.TypedEventHandler<Object, NoticeEventArgs> RaiseNotice;
event Windows.Foundation.TypedEventHandler<Object, TransparencyChangedEventArgs> TransparencyChanged;
event Windows.Foundation.TypedEventHandler<Object, Object> ReceivedOutput;
event Windows.Foundation.TypedEventHandler<Object, FoundResultsArgs> FoundMatch;
event Windows.Foundation.TypedEventHandler<Object, ShowWindowArgs> ShowWindowChanged;
event Windows.Foundation.TypedEventHandler<Object, UpdateSelectionMarkersEventArgs> UpdateSelectionMarkers;
event Windows.Foundation.TypedEventHandler<Object, OpenHyperlinkEventArgs> OpenHyperlink;
event Windows.Foundation.TypedEventHandler<Object, Object> CloseTerminalRequested;

1
src/cascadia/UnitTests_Control/pch.h
View File

@@ -46,6 +46,7 @@ Licensed under the MIT license.
// Manually include til after we include Windows.Foundation to give it winrt superpowers
#include "til.h"
#include <til/mutex.h>
#include <til/winrt.h>
#include "ThrottledFunc.h"