Fixed self reference capture in Tab and TerminalPage#3835
Conversation
No feedback from the Azure pipeline
|
Azure pipeline got stuck in unit testing again. I imagine I don't have the god power /azp run. Sorry for the hassle. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
zadjii-msft
left a comment
There was a problem hiding this comment.
Overall this looks okay to me, but I'm mildly worried about that one _rearrangeFrom change, so I'm going to mark myself as changes requested to make sure that's answered.
Thanks!
mkitzan
left a comment
There was a problem hiding this comment.
Thanks for the review @zadjii-msft. I'll commit a fix for these.
zadjii-msft
left a comment
There was a problem hiding this comment.
Hopefully this should resolve most of the crashes we've been seeing with this latest release. Thanks for the help!
|
Happy to help! Also, feel free to assign any goon work to me. I'm sure there's a lot of issues in the backlog with identified solutions that just need a goon to implement them. |
|
Hello @zadjii-msft! Because this pull request has the 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 (
|
Be careful what you wish for 😜 I'll look for some things that might be up your alley |
<!-- 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)? --> 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](#3776 (comment)). <!-- Other than the issue solved, is this relevant to any other issues/existing PRs? --> <!-- Please review the items on the PR checklist before submitting--> * [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 --> `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 --> 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. (cherry picked from commit 7b9728b)
|
🎉 Handy links: |
|
🎉 Handy links: |
Summary of the Pull Request
Every lambda capture in
TabandTerminalPagehas been changed from capturing rawthistostd::weak_ptr<Tab>orwinrt::weak_ref<TerminalPage>. Lambda bodies have been changed to check the weak reference before use.Capturing raw
thisinTab's title change event handler was the root cause of #3776, and is fixed in this PR among other instance of rawthiscapture.The lambda fixes to
TerminalPageare unrelated to the core issue addressed in the PR checklist. Because I was already editingTerminalPage, figured I'd do a weak_ref pass.References
PR Checklist
Detailed Description of the Pull Request / Additional comments
Tabnow inherits fromenable_shared_from_this, which enable accessingTabobjects asstd::weak_ptr<Tab>objects. All instances of lambdas capturingthisnow capturestd::weak_ptr<Tab>instead.TerminalPageis a WinRT type which supportswinrt::weak_ref<TerminalPage>. All previous instance ofTerminalPagelambdas capturingthishas been replaced to capturewinrt::weak_ref<TerminalPage>. These weak pointers/references can only be created after object construction necessitating forTaba 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:
Sorry about all the commits. Will fix my fork after this PR! 😅
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.