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

Fixed self reference capture in Tab and TerminalPage (#3835)

Browse source 
<!-- 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
Every lambda capture in `Tab` and `TerminalPage` has been changed from capturing raw `this` to `std::weak_ptr<Tab>` or `winrt::weak_ref<TerminalPage>`. Lambda bodies have been changed to check the weak reference before use. 

Capturing raw `this` in `Tab`'s [title change event handler](https://github.com/microsoft/terminal/blob/master/src/cascadia/TerminalApp/Tab.cpp#L299) was the root cause of #3776, and is fixed in this PR among other instance of raw `this` capture.

The lambda fixes to `TerminalPage` are unrelated to the core issue addressed in the PR checklist. Because I was already editing `TerminalPage`, figured I'd do a [weak_ref pass](https://github.com/microsoft/terminal/issues/3776#issuecomment-560575575).

<!-- 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 #3776, potentially #2248, likely closes others
* [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: #3776

<!-- 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
`Tab` now inherits from `enable_shared_from_this`, which enable accessing `Tab` objects as `std::weak_ptr<Tab>` objects. All instances of lambdas capturing `this` now capture `std::weak_ptr<Tab>` instead. `TerminalPage` is a WinRT type which supports `winrt::weak_ref<TerminalPage>`. All previous instance of `TerminalPage` lambdas capturing `this` has been replaced to capture `winrt::weak_ref<TerminalPage>`. These weak pointers/references can only be created after object construction necessitating for `Tab` a new function called after construction to bind lambdas.

Any anomalous crash related to the following functionality during closing a tab or WT may be fixed by this PR:
- Tab icon updating
- Tab text updating
- Tab dragging
- Clicking new tab button
- Changing active pane
- Closing an active tab
- Clicking on a tab
- Creating the new tab flyout menu

Sorry about all the commits. Will fix my fork after this PR! 😅 

<!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well -->
## Validation Steps Performed
Attempted to repro the steps indicated in issue #3776 with the new changes and failed. When before the changes, the issue could consistently be reproed.
This commit is contained in:
Michael Kitzan 2019-12-05 15:18:22 -08:00 committed by msftbot[bot]
parent f1ff0cf7ba
commit 7b9728b4a9
3 changed files with 140 additions and 68 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

81
src/cascadia/TerminalApp/Tab.cpp
View file

@ -25,10 +25,6 @@ Tab::Tab(const GUID& profile, const TermControl& control)
_activePane = _rootPane;
_AttachEventHandlersToPane(_rootPane);
_AttachEventHandlersToControl(control);
_MakeTabViewItem();
}
@ -108,6 +104,19 @@ std::optional<GUID> Tab::GetFocusedProfile() const noexcept
return _activePane->GetFocusedProfile();
}
// Method Description:
// - Called after construction of a Tab object to bind event handlers to its
// associated Pane and TermControl object
// Arguments:
// - control: reference to the TermControl object to bind event to
// Return Value:
// - <none>
void Tab::BindEventHandlers(const TermControl& control) noexcept
{
_AttachEventHandlersToPane(_rootPane);
_AttachEventHandlersToControl(control);
}
// Method Description:
// - Attempts to update the settings of this tab's tree of panes.
// Arguments:
@ -147,8 +156,13 @@ void Tab::UpdateIcon(const winrt::hstring iconPath)
_lastIconPath = iconPath;
_tabViewItem.Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [this]() {
_tabViewItem.IconSource(GetColoredIcon<winrt::MUX::Controls::IconSource>(_lastIconPath));
std::weak_ptr<Tab> weakThis{ shared_from_this() };
_tabViewItem.Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [weakThis]() {
if (auto tab{ weakThis.lock() })
{
tab->_tabViewItem.IconSource(GetColoredIcon<winrt::MUX::Controls::IconSource>(tab->_lastIconPath));
}
});
}
@ -175,8 +189,13 @@ void Tab::SetTabText(const winrt::hstring& text)
{
// Copy the hstring, so we don't capture a dead reference
winrt::hstring textCopy{ text };
_tabViewItem.Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [text = std::move(textCopy), this]() {
_tabViewItem.Header(winrt::box_value(text));
std::weak_ptr<Tab> weakThis{ shared_from_this() };
_tabViewItem.Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [text = std::move(textCopy), weakThis]() {
if (auto tab{ weakThis.lock() })
{
tab->_tabViewItem.Header(winrt::box_value(text));
}
});
}
@ -296,12 +315,16 @@ void Tab::ClosePane()
// - <none>
void Tab::_AttachEventHandlersToControl(const TermControl& control)
{
control.TitleChanged([this](auto newTitle) {
// The title of the control changed, but not necessarily the title
// of the tab. Get the title of the active pane of the tab, and set
// the tab's text to the active panes' text.
auto newTabTitle = GetActiveTitle();
SetTabText(newTabTitle);
std::weak_ptr<Tab> weakThis{ shared_from_this() };
control.TitleChanged([weakThis](auto newTitle) {
// Check if Tab's lifetime has expired
if (auto tab{ weakThis.lock() })
{
// The title of the control changed, but not necessarily the title of the tab.
// Set the tab's text to the active panes' text.
tab->SetTabText(tab->GetActiveTitle());
}
});
}
@ -316,22 +339,24 @@ void Tab::_AttachEventHandlersToControl(const TermControl& control)
// - <none>
void Tab::_AttachEventHandlersToPane(std::shared_ptr<Pane> pane)
{
pane->GotFocus([this](std::shared_ptr<Pane> sender) {
// Do nothing if it's the same pane as before.
if (sender == _activePane)
std::weak_ptr<Tab> weakThis{ shared_from_this() };
pane->GotFocus([weakThis](std::shared_ptr<Pane> sender) {
// Do nothing if the Tab's lifetime is expired or pane isn't new.
auto tab{ weakThis.lock() };
if (tab && sender != tab->_activePane)
{
return;
// Clear the active state of the entire tree, and mark only the sender as active.
tab->_rootPane->ClearActive();
tab->_activePane = sender;
tab->_activePane->SetActive();
// Update our own title text to match the newly-active pane.
tab->SetTabText(tab->GetActiveTitle());
// Raise our own ActivePaneChanged event.
tab->_ActivePaneChangedHandlers();
}
// Clear the active state of the entire tree, and mark only the sender as active.
_rootPane->ClearActive();
_activePane = sender;
_activePane->SetActive();
// Update our own title text to match the newly-active pane.
SetTabText(GetActiveTitle());
// Raise our own ActivePaneChanged event.
_ActivePaneChangedHandlers();
});
}

5
src/cascadia/TerminalApp/Tab.h
View file

@ -5,11 +5,14 @@
#include <winrt/Microsoft.UI.Xaml.Controls.h>
#include "Pane.h"
class Tab
class Tab : public std::enable_shared_from_this<Tab>
{
public:
Tab(const GUID& profile, const winrt::Microsoft::Terminal::TerminalControl::TermControl& control);
// Called after construction to setup events with weak_ptr
void BindEventHandlers(const winrt::Microsoft::Terminal::TerminalControl::TermControl& control) noexcept;
winrt::Microsoft::UI::Xaml::Controls::TabViewItem GetTabViewItem();
winrt::Windows::UI::Xaml::UIElement GetRootElement();
winrt::Microsoft::Terminal::TerminalControl::TermControl GetActiveTerminalControl() const;

122
src/cascadia/TerminalApp/TerminalPage.cpp
View file

@ -60,23 +60,36 @@ namespace winrt::TerminalApp::implementation
_tabView = _tabRow.TabView();
_rearranging = false;
_tabView.TabDragStarting([this](auto&& /*o*/, auto&& /*a*/) {
_rearranging = true;
_rearrangeFrom = std::nullopt;
_rearrangeTo = std::nullopt;
// weak_ptr to this TerminalPage object lambda capturing
auto weakThis{ get_weak() };
_tabView.TabDragStarting([weakThis](auto&& /*o*/, auto&& /*a*/) {
if (auto page{ weakThis.get() })
{
page->_rearranging = true;
page->_rearrangeFrom = std::nullopt;
page->_rearrangeTo = std::nullopt;
}
});
_tabView.TabDragCompleted([this](auto&& /*o*/, auto&& /*a*/) {
if (_rearrangeFrom.has_value() && _rearrangeTo.has_value() && _rearrangeTo != _rearrangeFrom)
_tabView.TabDragCompleted([weakThis](auto&& /*o*/, auto&& /*a*/) {
if (auto page{ weakThis.get() })
{
auto tab = _tabs.at(_rearrangeFrom.value());
_tabs.erase(_tabs.begin() + _rearrangeFrom.value());
_tabs.insert(_tabs.begin() + _rearrangeTo.value(), tab);
}
auto& from{ page->_rearrangeFrom };
auto& to{ page->_rearrangeTo };
_rearranging = false;
_rearrangeFrom = std::nullopt;
_rearrangeTo = std::nullopt;
if (from.has_value() && to.has_value() && to != from)
{
auto& tabs{ page->_tabs };
auto tab = tabs.at(from.value());
tabs.erase(tabs.begin() + from.value());
tabs.insert(tabs.begin() + to.value(), tab);
}
page->_rearranging = false;
from = std::nullopt;
to = std::nullopt;
}
});
auto tabRowImpl = winrt::get_self<implementation::TabRowControl>(_tabRow);
@ -100,8 +113,11 @@ namespace winrt::TerminalApp::implementation
_RegisterActionCallbacks();
//Event Bindings (Early)
_newTabButton.Click([this](auto&&, auto&&) {
this->_OpenNewTab(std::nullopt);
_newTabButton.Click([weakThis](auto&&, auto&&) {
if (auto page{ weakThis.get() })
{
page->_OpenNewTab(std::nullopt);
}
});
_tabView.SelectionChanged({ this, &TerminalPage::_OnTabSelectionChanged });
_tabView.TabCloseRequested({ this, &TerminalPage::_OnTabCloseRequested });
@ -301,8 +317,13 @@ namespace winrt::TerminalApp::implementation
profileMenuItem.FontWeight(FontWeights::Bold());
}
profileMenuItem.Click([this, profileIndex](auto&&, auto&&) {
this->_OpenNewTab({ profileIndex });
auto weakThis{ get_weak() };
profileMenuItem.Click([profileIndex, weakThis](auto&&, auto&&) {
if (auto page{ weakThis.get() })
{
page->_OpenNewTab({ profileIndex });
}
});
newTabFlyout.Items().Append(profileMenuItem);
}
@ -440,17 +461,21 @@ namespace winrt::TerminalApp::implementation
// Don't capture a strong ref to the tab. If the tab is removed as this
// is called, we don't really care anymore about handling the event.
std::weak_ptr<Tab> weakTabPtr = newTab;
auto weakThis{ get_weak() };
// When the tab's active pane changes, we'll want to lookup a new icon
// for it, and possibly propogate the title up to the window.
newTab->ActivePaneChanged([this, weakTabPtr]() {
if (auto tab = weakTabPtr.lock())
newTab->ActivePaneChanged([weakTabPtr, weakThis]() {
auto page{ weakThis.get() };
auto tab{ weakTabPtr.lock() };
if (page && tab)
{
// Possibly update the icon of the tab.
_UpdateTabIcon(tab);
page->_UpdateTabIcon(tab);
// Possibly update the title of the tab, window to match the newly
// focused pane.
_UpdateTitle(tab);
page->_UpdateTitle(tab);
}
});
@ -467,10 +492,16 @@ namespace winrt::TerminalApp::implementation
tabViewItem.PointerPressed({ this, &TerminalPage::_OnTabClick });
// When the tab is closed, remove it from our list of tabs.
newTab->Closed([tabViewItem, this](auto&& /*s*/, auto&& /*e*/) {
_tabView.Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [tabViewItem, this]() {
_RemoveTabViewItem(tabViewItem);
});
newTab->Closed([tabViewItem, weakThis](auto&& /*s*/, auto&& /*e*/) {
if (auto page{ weakThis.get() })
{
page->_tabView.Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [tabViewItem, weakThis]() {
if (auto page{ weakThis.get() })
{
page->_RemoveTabViewItem(tabViewItem);
}
});
}
});
// This is one way to set the tab's selected background color.
@ -762,19 +793,25 @@ namespace winrt::TerminalApp::implementation
// Add an event handler when the terminal wants to paste data from the Clipboard.
term.PasteFromClipboard({ this, &TerminalPage::_PasteFromClipboardHandler });
// Bind Tab events to the TermControl and the Tab's Pane
hostingTab->BindEventHandlers(term);
// Don't capture a strong ref to the tab. If the tab is removed as this
// is called, we don't really care anymore about handling the event.
std::weak_ptr<Tab> weakTabPtr = hostingTab;
term.TitleChanged([this, weakTabPtr](auto newTitle) {
auto tab = weakTabPtr.lock();
if (!tab)
auto weakThis{ get_weak() };
term.TitleChanged([weakTabPtr, weakThis](auto newTitle) {
auto page{ weakThis.get() };
auto tab{ weakTabPtr.lock() };
if (page && tab)
{
return;
// The title of the control changed, but not necessarily the title
// of the tab. Get the title of the focused pane of the tab, and set
// the tab's text to the focused panes' text.
page->_UpdateTitle(tab);
}
// The title of the control changed, but not necessarily the title
// of the tab. Get the title of the focused pane of the tab, and set
// the tab's text to the focused panes' text.
_UpdateTitle(tab);
});
}
@ -849,9 +886,12 @@ namespace winrt::TerminalApp::implementation
// GH#1117: This is a workaround because _tabView.SelectedIndex(tabIndex)
// sometimes set focus to an incorrect tab after removing some tabs
auto tab = _tabs.at(tabIndex);
_tabView.Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [tab, this]() {
auto tabViewItem = tab->GetTabViewItem();
_tabView.SelectedItem(tabViewItem);
auto weakThis{ get_weak() };
_tabView.Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [tab, weakThis]() {
if (auto page{ weakThis.get() })
{
page->_tabView.SelectedItem(tab->GetTabViewItem());
}
});
}
@ -1350,10 +1390,14 @@ namespace winrt::TerminalApp::implementation
_UpdateTitle(tab);
}
this->Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [this]() {
auto weakThis{ get_weak() };
this->Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [weakThis]() {
// repopulate the new tab button's flyout with entries for each
// profile, which might have changed
_CreateNewTabFlyout();
if (auto page{ weakThis.get() })
{
page->_CreateNewTabFlyout();
}
});
}