Fixed self capture in TermControl by mkitzan · Pull Request #3908 · microsoft/terminal

mkitzan · 2019-12-11T04:34:45Z

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.

References

PR Checklist

Closes Access Violation in TerminalControl!winrt::impl::consume_Windows_UI_Xaml_Controls_Primitives_IRangeBase<winrt::Windows::UI::Xaml::Controls::Primitives::ScrollBar>::Maximum #2947
CLA signed. If not, go over here and sign the CLA
Tests added/passed
Requires documentation to be updated
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: Access Violation in TerminalControl!winrt::impl::consume_Windows_UI_Xaml_Controls_Primitives_IRangeBase<winrt::Windows::UI::Xaml::Controls::Primitives::ScrollBar>::Maximum #2947

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 which were throwing under some circumstances. 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

Validation Steps Performed

Before these changes I was able to consistently repro the issue in #2947. Now, I can no longer repro the issue.

DHowett-MSFT · 2019-12-11T04:44:15Z

Thanks! For this one, actually, I’m wondering if we should use C++/WinRT’s auto_revoke machinery. Since we are largely dealing with other WinRT objects and not manually bound std::functions, we can probably use more of the cool platform accordances.

These auto_revokers get torn down during destruction, which ensures that a destructed TermControl has all of its handlers unbound before it becomes invalid.

DHowett-MSFT · 2019-12-11T04:44:57Z

(There are a couple where we explicitly specify the tear down order, but those shouldn’t need to be weak.)

Novice stuff

mkitzan · 2019-12-11T05:57:20Z

I'll start by rolling back the auto_revoke handlers that I changed to use weak_ref (didn't realize auto_revoke made the event safe), and if I see some easy wins for the others I'll rework them to.

DHowett-MSFT · 2019-12-11T06:25:03Z

Thanks 😄

WinRT has some neat features

mkitzan · 2019-12-11T06:58:23Z

Was able to make one extra event auto_revoke, so all events TermControl owns are auto_revoke. The only real blocker to making the rest auto_revoke is that the XAML Controls in TermControl are using CoreDispatcher.RunAsync for their handler binding.

mkitzan · 2019-12-11T07:57:23Z

Azp unit testing got caught up again.

zadjii-msft · 2019-12-11T14:07:58Z

/azp run

azure-pipelines · 2019-12-11T14:08:09Z

Azure Pipelines successfully started running 1 pipeline(s).

miniksa

I'm OK with this. Thanks.

src/cascadia/TerminalControl/TermControl.cpp

DHowett-MSFT · 2019-12-11T21:59:09Z

src/cascadia/TerminalControl/TermControl.cpp

-        _root.Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [this]() {
-            // Update our control settings
-            _ApplyUISettings();
+        _root.Dispatcher().RunAsync(CoreDispatcherPriority::Normal, [weakThis]() {


nit: you can [weakThis = get_weak()] if the weak ref is only going to be used in the lambda

That form reads better. I'll include it in the next commit.

DHowett-MSFT · 2019-12-11T22:00:11Z

src/cascadia/TerminalControl/TermControl.cpp

+            if (auto control{ weakThis.get() })
+            {
+                // Update our control settings
+                control->_ApplyUISettings();


not 100% sure on how i feel about indirecting through control-> every time; in debug builds each one will be a call to winrt::com_ptr::operator->()...
but i do not know how much, if at all, I care. Thoughts?

(we could capture weakThis,this and then only use this implicitly if the weak got locked. @zadjii-msft?)

I prefer the way the weakThis,this lambdas read. All the control-> just clutters things, not mention your comment about winrt::com_ptr::operator->(). WinRT could almost use a class to support this form of lambda. It would be extensible to all the places where we're having trouble with the { this, pointer-to-member } event format. Anyways, I'll include the fix to these in the next revision commit.

I think I prefer it like this - not capturing this at all makes it harder for us to accidentally do the wrong thing.

I guess there's a "convenient" middle ground...

if (auto strongThis{ weakThis.get() }) { auto& that{ *strongThis }; that->x; that->y; }

I am worried about the constant indirection through operator->... but at the same time, WRL::ComPtr literally is that for all com calls, sooooooooo maybe i should not be worried?

It's just that this is a member function, but we have to write it in a weird way that makes it not look like a member function.

Let me know which form to go with, and I'll include it in the next commit.

DHowett-MSFT · 2019-12-11T23:23:56Z

src/cascadia/TerminalControl/TermControl.cpp

-                localAutoScrollTimer.Stop();
-                // _autoScrollTimer timer, now stopped, is destroyed.
-            }
-


i'm surprised tearing down the timers isn't necessary anymore, but I'm willing to believe it 😄

Those Stop calls were only throwing in the event that TermControl was being destructed at the end of an event handler, which was strange. Could just wrap them in try-catch blocks if the Stop calls really should be there 🤔

The most correct solution might be to make both the timers Tick events take a get_weak() as opposed to this, right? If we had done that from the start, we probably wouldn't have ever needed these lines...

mkitzan · 2019-12-11T23:32:57Z

Thanks for the reviews. I've pushed some changes.

DHowett-MSFT · 2019-12-12T02:07:31Z

I'm closing and reopening to convince the CLA bot.

zadjii-msft

I'm okay with this, but maybe we should make those Tick events take get_weak(). @mkitzan are you planning on making a pass for all the { this, &TermControl::EventHandler } instances?

zadjii-msft · 2019-12-12T21:23:57Z

src/cascadia/TerminalControl/TermControl.cpp

-                localAutoScrollTimer.Stop();
-                // _autoScrollTimer timer, now stopped, is destroyed.
-            }
-


The most correct solution might be to make both the timers Tick events take a get_weak() as opposed to this, right? If we had done that from the start, we probably wouldn't have ever needed these lines...

mkitzan · 2019-12-12T21:44:29Z

Yeah, get_weak for Tick makes sense. I wasn't originally planning on making changes to the existing { this, &TermControl::EventHandler }. However, it would be an easy fix to convert these to { get_weak(), &TermControl::EventHandler }.

ghost · 2019-12-13T13:28:34Z

Hello @zadjii-msft!

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.

ghost · 2020-01-14T17:14:02Z

🎉Windows Terminal Preview v0.8.10091.0 has been released which incorporates this pull request.:tada:

Handy links:

Fixed TermControl lambda captures

bb67a00

Should've ran Format Document the first time

8afce7f

Novice stuff

Changes for auto_revoke

a81fdc1

WinRT has some neat features

mkitzan marked this pull request as ready for review December 11, 2019 18:32

miniksa approved these changes Dec 11, 2019

View reviewed changes

DHowett-MSFT reviewed Dec 11, 2019

View reviewed changes

PR review revision commit

d271c33

DHowett-MSFT reviewed Dec 11, 2019

View reviewed changes

DHowett-MSFT closed this Dec 12, 2019

DHowett-MSFT reopened this Dec 12, 2019

zadjii-msft mentioned this pull request Dec 12, 2019

Add support for setting a tab's color at runtime w/ context menu #3789

Merged

zadjii-msft approved these changes Dec 12, 2019

View reviewed changes

Tick now takes get_weak()

7729685

zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Dec 13, 2019

ghost merged commit a3f3cc8 into microsoft:master Dec 13, 2019

mkitzan deleted the fix-termcontrol-lambdas branch December 14, 2019 01:35

mkitzan mentioned this pull request Dec 31, 2019

Converts Dispatcher().RunAsync to WinRT Coroutines #4051

Merged

5 tasks

ghost mentioned this pull request Jan 14, 2020

Access Violation in TerminalControl!winrt::impl::consume_Windows_UI_Xaml_Controls_Primitives_IRangeBase<winrt::Windows::UI::Xaml::Controls::Primitives::ScrollBar>::Maximum #2947

Closed

TBBle mentioned this pull request Jan 17, 2020

Terminal Crash/Hangs when closing second-last tab quickly #4132

Closed

This pull request was closed.

Conversation

mkitzan commented Dec 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Uh oh!

DHowett-MSFT commented Dec 11, 2019

Uh oh!

DHowett-MSFT commented Dec 11, 2019

Uh oh!

mkitzan commented Dec 11, 2019

Uh oh!

DHowett-MSFT commented Dec 11, 2019

Uh oh!

mkitzan commented Dec 11, 2019

Uh oh!

mkitzan commented Dec 11, 2019

Uh oh!

zadjii-msft commented Dec 11, 2019

Uh oh!

azure-pipelines bot commented Dec 11, 2019

Uh oh!

miniksa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkitzan commented Dec 11, 2019

Uh oh!

DHowett-MSFT commented Dec 12, 2019

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkitzan commented Dec 12, 2019

Uh oh!

ghost commented Dec 13, 2019

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.

Uh oh!

ghost commented Jan 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mkitzan commented Dec 11, 2019 •

edited

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.