Fixed self reference capture in Tab and TerminalPage #3835
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
requested changes
Dec 5, 2019
ghost
added
the
Needs-Author-Feedback
mkitzan
commented
Dec 5, 2019
There was a problem hiding this comment.
Thanks for the review @zadjii-msft. I'll commit a fix for these.
ghost
removed
the
Needs-Author-Feedback
zadjii-msft
approved these changes
Dec 5, 2019
zadjii-msft
added
Product-Terminal
ghost
requested review from
|
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. |
carlos-zamora
approved these changes
Dec 5, 2019
zadjii-msft
added
the
AutoMerge
|
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 |
DHowett-MSFT
pushed a commit
that referenced
this pull request
Dec 11, 2019
<!-- 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: |
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.