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.

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

Clicking on a tab sometimes causes a crash #1188

Closed
AndrewMSHowe opened this issue Jun 9, 2019 · 1 comment
Closed

Clicking on a tab sometimes causes a crash #1188

AndrewMSHowe opened this issue Jun 9, 2019 · 1 comment
Assignees
Labels
Area-User Interface Issues pertaining to the user interface of the Console or Terminal 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. Severity-Crash Crashes are real bad news.

Comments

@AndrewMSHowe
Copy link

Environment

I don't think it matters; commit 2da5b0b

Steps to reproduce

Add some tabs, close some tabs. Might have to fiddle about with focus or something. It doesn't always happen straight away.

Expected behavior

It doesn't crash.

Actual behavior

It crashes, losing all your tabs.

The crash is here, on line 93 of Microsoft.Terminal.TerminalControl.h in Title():

check_hresult(WINRT_SHIM(Microsoft::Terminal::TerminalControl::ITermControl)->get_Title(&value));

The calling code is at line 1037..1041 of App.cpp in GetTitle():

                try
                {
                    return _GetFocusedControl().Title();
                }
                CATCH_LOG();

And the caller of that was _OnTabSelectionChanged().

Unfortunately it is not a thrown exception but an access violation, so the C++ exception handling doesn't help.

Inside _GetFocusedControl(), it would be possible for _GetFocusedTabIndex() to return -1, but it doesn't seem to be in this case. Also it could conceivably return >= _tabs.size() but it isn't doing that either. The problem is that _GetFocusedTerminalControl() returns nullptr, which according to its documentation is a reasonable thing to do.

I changed the code inside the try{} to:

                    auto focusedControl = _GetFocusedControl();
                    if (focusedControl)
                        return focusedControl.Title();

This seems to stop it crashing. But there are other places where _GetFocusedControl() is called without checking for a null result. So perhaps another tab should be considered focused, or something.

Also if you create shells quickly, some of them end up with no title. I think that's related, because if you click on one of those tabs, the window title (can be seen by hovering over the app's icon in the taskbar) is "Windows Terminal" which is GetTitle()s default.

@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 Jun 9, 2019
@AndrewMSHowe
Copy link
Author

Found better repro steps:

  1. Hold down Ctrl-T
  2. Observe some tabs appearing shorter, with no title.
  3. Retire to a safe distance.
  4. Click on one of the short tabs.

@AndrewMSHowe AndrewMSHowe changed the title Closing a tab sometimes causes a crash Clicking on a tab sometimes causes a crash Jun 9, 2019
@DHowett-MSFT DHowett-MSFT added Area-User Interface Issues pertaining to the user interface of the Console or Terminal 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. and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Jun 10, 2019
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Jun 10, 2019
@DHowett-MSFT DHowett-MSFT added this to the Terminal v0.2 milestone Jun 10, 2019
@DHowett-MSFT DHowett-MSFT self-assigned this Jun 11, 2019
@ghost ghost added the Needs-Tag-Fix Doesn't match tag requirements label Jun 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-User Interface Issues pertaining to the user interface of the Console or Terminal 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. Severity-Crash Crashes are real bad news.
Projects
None yet
Development

No branches or pull requests

2 participants