Skip to content

Commit

Permalink
Fixed self capture in TermControl (#3908)
Browse files Browse the repository at this point in the history 
<!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? -->
## Summary of the Pull Request
An asynchronous event handler capturing raw `this` in `TermControl` was causing an exception to be thrown when a scroll update event occurred after closing the active tab. This PR replaces all non-auto_revoke lambda captures in `TermControl` to capture (and validate) a `winrt::weak_ref` instead of using raw `this`.

<!-- Other than the issue solved, is this relevant to any other issues/existing PRs? --> 
## References

<!-- Please review the items on the PR checklist before submitting-->
## PR Checklist
* [x] Closes #2947
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [ ] Tests added/passed
* [ ] Requires documentation to be updated
* [x] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #2947

<!-- Provide a more detailed description of the PR, other things fixed or any additional comments/features here -->
## Detailed Description of the Pull Request / Additional comments
`TermControl` is already a WinRT type so no changes were required to enable the `winrt::weak_ref` functionality. There was only one strange change I had to make. In the destructor's helper function `Close`, I had to remove two calls to [`Stop`](https://docs.microsoft.com/en-us/uwp/api/windows.ui.xaml.dispatchertimer.stop#Windows_UI_Xaml_DispatcherTimer_Stop) which were throwing under [some circumstances](microsoft/terminal#2947 (comment)). Fortunately, these calls don't appear to be critical, but definitely a spot to look into when reviewing this PR.

Beyond scrolling, any anomalous crash related to the following functionality while closing a tab or WT may be fixed by this PR:
- Settings updating
- Changing background color

<!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well -->
## Validation Steps Performed
Before these changes I was able to consistently repro the issue in #2947. Now, I can no longer repro the issue.
  • Loading branch information
@donno2048
donno2048 committed Dec 13, 2019
1 parent 80465f0 commit f2b4e2e
Show file tree
Hide file tree
Showing 2 changed files with 89 additions and 81 deletions.
169 changes: 88 additions & 81 deletions src/cascadia/TerminalControl/TermControl.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -118,10 +118,11 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
// in any layout change chain. That gives us great flexibility in finding the right point
// at which to initialize our renderer (and our terminal).
// Any earlier than the last layout update and we may not know the terminal's starting size.

if (_InitializeTerminal())
{
// Only let this succeed once.
this->_layoutUpdatedRevoker.revoke();
_layoutUpdatedRevoker.revoke();
}
});

Expand All @@ -145,8 +146,8 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation

// These are important:
// 1. When we get tapped, focus us
this->Tapped([this](auto&, auto& e) {
this->Focus(FocusState::Pointer);
_tappedRevoker = this->Tapped(winrt::auto_revoke, [this](auto&, auto& e) {
Focus(FocusState::Pointer);
e.Handled(true);
});
// 2. Make sure we can be focused (why this isn't `Focusable` I'll never know)
Expand All @@ -157,11 +158,8 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
// DON'T CALL _InitializeTerminal here - wait until the swap chain is loaded to do that.

// Subscribe to the connection's disconnected event and call our connection closed handlers.
_connectionStateChangedRevoker = _connection.StateChanged(winrt::auto_revoke, [weakThis = get_weak()](auto&& /*s*/, auto&& /*v*/) {
if (auto strongThis{ weakThis.get() })
{
strongThis->_ConnectionStateChangedHandlers(*strongThis, nullptr);
}
_connectionStateChangedRevoker = _connection.StateChanged(winrt::auto_revoke, [this](auto&& /*s*/, auto&& /*v*/) {
_ConnectionStateChangedHandlers(*this, nullptr);
});
}

Expand All @@ -177,34 +175,38 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation

// Dispatch a call to the UI thread to apply the new settings to the
// terminal.
_root.Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [this]() {
// Update our control settings
_ApplyUISettings();
_root.Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [weakThis{ get_weak() }, this]() {
// If 'weakThis' is locked, then we can safely work with 'this'
if (auto control{ weakThis.get() })
{
// Update our control settings
_ApplyUISettings();

// Update DxEngine's SelectionBackground
_renderEngine->SetSelectionBackground(_settings.SelectionBackground());
// Update DxEngine's SelectionBackground
_renderEngine->SetSelectionBackground(_settings.SelectionBackground());

// Update the terminal core with its new Core settings
_terminal->UpdateSettings(_settings);
// Update the terminal core with its new Core settings
_terminal->UpdateSettings(_settings);

// Refresh our font with the renderer
_UpdateFont();
// Refresh our font with the renderer
_UpdateFont();

const auto width = _swapChainPanel.ActualWidth();
const auto height = _swapChainPanel.ActualHeight();
if (width != 0 && height != 0)
{
// If the font size changed, or the _swapchainPanel's size changed
// for any reason, we'll need to make sure to also resize the
// buffer. _DoResize will invalidate everything for us.
auto lock = _terminal->LockForWriting();
_DoResize(width, height);
}
const auto width = _swapChainPanel.ActualWidth();
const auto height = _swapChainPanel.ActualHeight();
if (width != 0 && height != 0)
{
// If the font size changed, or the _swapchainPanel's size changed
// for any reason, we'll need to make sure to also resize the
// buffer. _DoResize will invalidate everything for us.
auto lock = _terminal->LockForWriting();
_DoResize(width, height);
}

// set TSF Foreground
Media::SolidColorBrush foregroundBrush{};
foregroundBrush.Color(ColorRefToColor(_settings.DefaultForeground()));
_tsfInputControl.Foreground(foregroundBrush);
// set TSF Foreground
Media::SolidColorBrush foregroundBrush{};
foregroundBrush.Color(ColorRefToColor(_settings.DefaultForeground()));
_tsfInputControl.Foreground(foregroundBrush);
}
});
}

Expand Down Expand Up @@ -362,30 +364,34 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
// - <none>
void TermControl::_BackgroundColorChanged(const uint32_t color)
{
_root.Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [this, color]() {
const auto R = GetRValue(color);
const auto G = GetGValue(color);
const auto B = GetBValue(color);
_root.Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [weakThis{ get_weak() }, this, color]() {
// If 'weakThis' is locked, then we can safely work with 'this'
if (auto control{ weakThis.get() })
{
const auto R = GetRValue(color);
const auto G = GetGValue(color);
const auto B = GetBValue(color);

winrt::Windows::UI::Color bgColor{};
bgColor.R = R;
bgColor.G = G;
bgColor.B = B;
bgColor.A = 255;
winrt::Windows::UI::Color bgColor{};
bgColor.R = R;
bgColor.G = G;
bgColor.B = B;
bgColor.A = 255;

if (auto acrylic = _root.Background().try_as<Media::AcrylicBrush>())
{
acrylic.FallbackColor(bgColor);
acrylic.TintColor(bgColor);
}
else if (auto solidColor = _root.Background().try_as<Media::SolidColorBrush>())
{
solidColor.Color(bgColor);
}
if (auto acrylic = _root.Background().try_as<Media::AcrylicBrush>())
{
acrylic.FallbackColor(bgColor);
acrylic.TintColor(bgColor);
}
else if (auto solidColor = _root.Background().try_as<Media::SolidColorBrush>())
{
solidColor.Color(bgColor);
}

// Set the default background as transparent to prevent the
// DX layer from overwriting the background image or acrylic effect
_settings.DefaultBackground(ARGB(0, R, G, B));
// Set the default background as transparent to prevent the
// DX layer from overwriting the background image or acrylic effect
_settings.DefaultBackground(ARGB(0, R, G, B));
}
});
}

Expand Down Expand Up @@ -449,10 +455,15 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
}

auto chain = _renderEngine->GetSwapChain();
_swapChainPanel.Dispatcher().RunAsync(CoreDispatcherPriority::High, [=]() {
auto lock = _terminal->LockForWriting();
auto nativePanel = _swapChainPanel.as<ISwapChainPanelNative>();
nativePanel->SetSwapChain(chain.Get());

_swapChainPanel.Dispatcher().RunAsync(CoreDispatcherPriority::High, [weakThis{ get_weak() }, this, chain]() {
// If 'weakThis' is locked, then we can safely work with 'this'
if (auto control{ weakThis.get() })
{
auto lock = _terminal->LockForWriting();
auto nativePanel = _swapChainPanel.as<ISwapChainPanelNative>();
nativePanel->SetSwapChain(chain.Get());
}
});
}

Expand Down Expand Up @@ -528,6 +539,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
THROW_IF_FAILED(dxEngine->Enable());
_renderEngine = std::move(dxEngine);

// This event is explicitly revoked in the destructor: does not need weak_ref
auto onRecieveOutputFn = [this](const hstring str) {
_terminal->Write(str.c_str());
};
Expand All @@ -537,11 +549,15 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
_terminal->SetWriteInputCallback(inputFn);

auto chain = _renderEngine->GetSwapChain();
_swapChainPanel.Dispatcher().RunAsync(CoreDispatcherPriority::High, [this, chain]() {
_terminal->LockConsole();
auto nativePanel = _swapChainPanel.as<ISwapChainPanelNative>();
nativePanel->SetSwapChain(chain.Get());
_terminal->UnlockConsole();

_swapChainPanel.Dispatcher().RunAsync(CoreDispatcherPriority::High, [weakThis{ get_weak() }, this, chain]() {
if (auto control{ weakThis.get() })
{
_terminal->LockConsole();
auto nativePanel = _swapChainPanel.as<ISwapChainPanelNative>();
nativePanel->SetSwapChain(chain.Get());
_terminal->UnlockConsole();
}
});

// Set up the height of the ScrollViewer and the grid we're using to fake our scrolling height
Expand Down Expand Up @@ -615,7 +631,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation

static constexpr auto AutoScrollUpdateInterval = std::chrono::microseconds(static_cast<int>(1.0 / 30.0 * 1000000));
_autoScrollTimer.Interval(AutoScrollUpdateInterval);
_autoScrollTimer.Tick({ this, &TermControl::_UpdateAutoScroll });
_autoScrollTimer.Tick({ get_weak(), &TermControl::_UpdateAutoScroll });

// Set up blinking cursor
int blinkTime = GetCaretBlinkTime();
Expand All @@ -624,7 +640,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
// Create a timer
_cursorTimer = std::make_optional(DispatcherTimer());
_cursorTimer.value().Interval(std::chrono::milliseconds(blinkTime));
_cursorTimer.value().Tick({ this, &TermControl::_BlinkCursor });
_cursorTimer.value().Tick({ get_weak(), &TermControl::_BlinkCursor });
_cursorTimer.value().Start();
}
else
Expand Down Expand Up @@ -1506,15 +1522,19 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
}

// Update our scrollbar
_scrollBar.Dispatcher().RunAsync(CoreDispatcherPriority::Low, [=]() {
_scrollBar.Dispatcher().RunAsync(CoreDispatcherPriority::Low, [weakThis{ get_weak() }, this, viewTop, viewHeight, bufferSize]() {
// Even if we weren't closed/closing few lines above, we might be
// while waiting for this block of code to be dispatched.
if (_closing.load())
// If 'weakThis' is locked, then we can safely work with 'this'
if (auto control{ weakThis.get() })
{
return;
}
if (_closing.load())
{
return;
}

_ScrollbarUpdater(_scrollBar, viewTop, viewHeight, bufferSize);
_ScrollbarUpdater(_scrollBar, viewTop, viewHeight, bufferSize);
}
});

// Set this value as our next expected scroll position.
Expand Down Expand Up @@ -1602,19 +1622,6 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
_connection.TerminalOutput(_connectionOutputEventToken);
_connectionStateChangedRevoker.revoke();

// Clear out the cursor timer, so it doesn't trigger again on us once we're destructed.
if (auto localCursorTimer{ std::exchange(_cursorTimer, std::nullopt) })
{
localCursorTimer->Stop();
// cursorTimer timer, now stopped, is destroyed.
}

if (auto localAutoScrollTimer{ std::exchange(_autoScrollTimer, nullptr) })
{
localAutoScrollTimer.Stop();
// _autoScrollTimer timer, now stopped, is destroyed.
}

if (auto localConnection{ std::exchange(_connection, nullptr) })
{
localConnection.Close();
Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalControl/TermControl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation
TSFInputControl _tsfInputControl;

event_token _connectionOutputEventToken;
TermControl::Tapped_revoker _tappedRevoker;
TerminalConnection::ITerminalConnection::StateChanged_revoker _connectionStateChangedRevoker;

std::unique_ptr<::Microsoft::Terminal::Core::Terminal> _terminal;
Expand Down

0 comments on commit f2b4e2e

Please sign in to comment.