Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Crash when opening and closing a tab #3776

Closed
timkoers opened this issue Nov 29, 2019 · 11 comments · Fixed by #3835
Closed

Crash when opening and closing a tab #3776

timkoers opened this issue Nov 29, 2019 · 11 comments · Fixed by #3835
Labels
Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. Severity-Crash Crashes are real bad news.
Milestone
Terminal v0.8

Comments

@timkoers
Copy link

timkoers commented Nov 29, 2019 •
edited
Loading

Environment

Windows 10 Home 18363.476
Windows Terminal (Preview) Store installation - v0.7.3291.0

When opening and closing a 3rd tab, when two tabs are already openend and a single tab is executing a command, the Terminal crashes completely.

I had a Windows PowerShell tab open as my first tab and a Ubuntu shell open as my second tab, opening and closing a new Windows PowerShell tab as the third one.
@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Nov 29, 2019
@mkitzan
Copy link
Contributor

mkitzan commented Nov 30, 2019 •
edited
Loading

I found a very consistent way to crash WT when trying to replicate your issue. Have at least one tab open then in quick sequence use open your WSL terminal (using your keyboard shortcut) then enter the keyboard shortcut to close a tab. I have WSL mapped to Ctrl+Shift+1 and close tab on Ctrl+W. So by hitting Ctrl+Shift+1 then immediately Ctrl+W, WT crashes every time.
Edit: looks like during the creation of the Tab there's a null pointer de-reference in one of the auto generated WinRT functions (L839 of Windows.UI.Xaml.h for those with the dev build).

@mkitzan
Copy link
Contributor

mkitzan commented Nov 30, 2019

Did a check of other issues. This issues sounds like a duplicate/instance of #2248.

@DHowett-MSFT
Copy link
Contributor

In addition, there's at least one instance of us keeping a reference to a predeceased terminal control or tab object during a title changed event. We need to do a pass to make our async event handlers take weak references!

@DHowett-MSFT DHowett-MSFT added Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. Severity-Crash Crashes are real bad news. Help Wanted We encourage anyone to jump in on these. and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Dec 2, 2019
@DHowett-MSFT DHowett-MSFT added this to the Terminal-1912 milestone Dec 2, 2019
@mkitzan
Copy link
Contributor

mkitzan commented Dec 3, 2019 •
edited
Loading

@DHowett this crash instance is due to the Tab capturing itself as a raw pointer in the title change event handler and being de-referenced past its lifetime (as you suspected). In order for Tab to support WinRT weak_ref, we'll have to turn it into a WinRT object so it can be a valid template argument when calling get_weak or make_weak. I'll work on figuring out how to do that.
Bonus raw pointer capture in the same file. Once I figure out the pattern for fixing these, I can do a pass fixing raw pointer captures.

@zadjii-msft
Copy link
Member

@mkitzan it might be easier to change Tab to enable_shared_from_this, and then get a weak pointer to itself. Making the Tab a proper WinRT type is something we'd definitely want to do in the future, but we might want to get this particular crash solved faster than that :P

@mkitzan
Copy link
Contributor

mkitzan commented Dec 3, 2019 •
edited
Loading

Sounds like a good plan for the time being. I'll add a TODO in the source to eventually make a Tab WinRT type.

@mkitzan
Copy link
Contributor

mkitzan commented Dec 4, 2019 •
edited
Loading

@zadjii-msft currently the event handlers are bound to the TermControl and root Pane on construction of Tab. The lambdas can't correctly capture the std::weak_ptr<Tab> because they require the object spawning the std::weak_ptr<Tab> to be of type std::shared_ptr<Tab> (instead of plain Tab). So, to get this to work I'm thinking of adding a weird Tab function member to provide a Tab object with a std::weak_ptr<Tab> to itself before binding the events. Any thoughts on this, because it seems a little 🙃
If there isn't a clean work around, it may make sense to just bite the bullet and make Tab a WinRT type (especially considering we'll probably roll-back this work when it becomes an WinRT type).

@DHowett-MSFT
Copy link
Contributor

If we make Tab inherit from enable_shared_from_this<Tab>, it'll gain a method shared_from_this() that will return a std::shared_ptr<Tab> that you can then weaken 😄

@mkitzan
Copy link
Contributor

mkitzan commented Dec 4, 2019 •
edited
Loading

Thanks @DHowett-MSFT, got too focused on the weak_ptr side of things! Using shared_from_this() still requires a function member to initialize the event binding, because calling shared_from_this() inside the constructor throws bad_weak_ptr exception. Luckily, calling the binding function could fit well in TerminalPage::_RegisterTerminalEvents or just after Tab construction. This fixes the issue, no more crashes when rapidly opening-closing tabs!

@mkitzan mkitzan mentioned this issue Dec 4, 2019
Merged
5 tasks
@ghost ghost added the In-PR This issue has a related PR label Dec 4, 2019
@mkitzan mkitzan mentioned this issue Dec 5, 2019
Closed
@DHowett-MSFT DHowett-MSFT mentioned this issue Dec 5, 2019
Closed
@ghost ghost closed this as completed in #3835 Dec 5, 2019
ghost pushed a commit that referenced this issue Dec 5, 2019
@mkitzan
Fixed self reference capture in Tab and TerminalPage (#3835)
7b9728b 
<!-- 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](#3776 (comment)).

<!-- 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.
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Dec 5, 2019
DHowett-MSFT pushed a commit that referenced this issue Dec 11, 2019
@mkitzan @DHowett
Fixed self reference capture in Tab and TerminalPage (#3835)
949ad80 
<!-- 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)
@ghost
Copy link

ghost commented Dec 12, 2019

🎉This issue was addressed in #3835, which has now been successfully released as Windows Terminal Preview v0.7.3451.0.:tada:

Handy links:

@ghost
Copy link

ghost commented Jan 14, 2020

🎉This issue was addressed in #3835, which has now been successfully released as Windows Terminal Preview v0.8.10091.0.:tada:

Handy links:

donno2048 added a commit to donno2048/terminal that referenced this issue Sep 28, 2020
@donno2048
Fixed self reference capture in Tab and TerminalPage (#3835)
4697ab2 
<!-- 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](microsoft/terminal#3776 (comment)).

<!-- 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 issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. Severity-Crash Crashes are real bad news.
Projects
None yet
Milestone
Terminal v0.8
Development

Successfully merging a pull request may close this issue.

Fixed self reference capture in Tab and TerminalPage mkitzan/terminal
4 participants
@timkoers @DHowett-MSFT @zadjii-msft @mkitzan