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

Closing a tab in focus mode causes terminal to become unresponsive #7916

Closed
zadjii-msft opened this issue Oct 13, 2020 · 7 comments
Closed
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. Priority-1 A description (P1) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone

Comments

@zadjii-msft
Copy link
Member

Windows Terminal Preview
Version: 1.5.2831.0
  1. Open the Terminal
  2. Enter focus mode
  3. Run a command that opens a bunch of tabs/panes. I'm using
        { "command": { "action": "wt", "commandline": "new-tab --title OpenConsole cmd.exe /k #work 15 ; split-pane --title OpenConsole cmd.exe /k #work 15 ; split-pane -H cmd.exe /k media-commandline ; new-tab --title \"Symbols Script\" powershell dev\\symbols.ps1 ; new-tab -p \"Ubuntu 18.04\" ; new-tab -p \"microsoft/Terminal\" ; sp -V -p \"microsoft/Terminal\" ; sp -H -p \"microsoft/Terminal\" ; focus-tab -t 0" }, "name": "Good Morning" },

But it probably repros for something simpler
4. Close the first tab

expected

The tab closes, and the terminal works as usual

actual

The tab stays open, and now the terminal is seemingly hung. The focus has left the original control, but hasn't gone anywhere. There's no tab strip to be able to manually focus another tab. Clicking the control also doesn't focus it. Since the focused control is responsible for routing keyboard events, the terminal won't respond to any KB shortcuts, since there's no focused control

@zadjii-msft zadjii-msft added Issue-Bug It either shouldn't be doing this or needs an investigation. Area-User Interface Issues pertaining to the user interface of the Console or Terminal Product-Terminal The new Windows Terminal. Priority-1 A description (P1) labels Oct 13, 2020
@zadjii-msft zadjii-msft added this to the Terminal v2.0 milestone Oct 13, 2020
@ghost ghost added the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Oct 13, 2020
@DHowett DHowett removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Oct 16, 2020
@Don-Vito
Copy link
Contributor

@zadjii-msft - there is an "event handling catastrophe here" 😄.
I never tried to tested, just reading the code - please join my journey:

Closing the tab triggers two actions - shutting the existing tab down and moving the focus to the next tab.
Shutting down the tab, causes shutting down of all the panes that causes shutting down of all terminal controls, that causes closing the panes, that results in closing child panes and asynchronously setting them to nullptr.

And then comes a _MoveFocus that invokes _UnzoomIfNeeded that in the focus mode calls activeTab->ExitZoom(), that invokes activePane->Restore, that access _firstChild and _secondChild that might be null already

@DHowett
Copy link
Member

DHowett commented Nov 11, 2020

Oh yeah, we have a number of event handling catastrophies in our code.

@Don-Vito
Copy link
Contributor

@zadjii-msft - and if this is not enough there are a lot of "funny" races. E.g.,

Exit Zoom might be called twice. From one hand it is called from _MoveFocus->_UnzoomIfNeeded

void TerminalPage::_UnZoomIfNeeded()
    {
        if (auto focusedTab = _GetFocusedTab())
        {
            if (auto activeTab = _GetTerminalTabImpl(focusedTab))
            {
                if (activeTab->IsZoomed())
                {
                    // Remove the content from the tab first, so Pane::UnZoom can
                    // re-attach the content to the tree w/in the pane
                    _tabContent.Children().Clear();
                    // In ExitZoom, we'll change the Tab's Content(), triggering the
                    // content changed event, which will re-attach the tab's new content
                    // root to the tree.
                    activeTab->ExitZoom();
                }
            }
        }
    }

On the other hand it might get called from

pane->Closed([weakThis](auto&& /*s*/, auto && /*e*/) -> winrt::fire_and_forget {
            if (auto tab{ weakThis.get() })
            {
                if (tab->_zoomedPane)
                {
                    co_await winrt::resume_foreground(tab->Content().Dispatcher());

                    tab->Content(tab->_rootPane->GetRootElement());
                    tab->ExitZoom();
                }
            }
        });

This is funny because the co_await in the last one will allow both functions to enter ExitZoom

@Don-Vito
Copy link
Contributor

I am not sure where to start from. The immediate mitigation that will hopefully help is to check all pointers against nulls in the Restore method (zommedPane, firstChild and secondChild).

@Don-Vito
Copy link
Contributor

In any case the issue I described is irrelevant for this one. Here the closed tab is not multipane, and it should be something else.

@CornCobs
Copy link

Hi! I was about to post a new issue but I think this issue is similar so I thought I would add it here instead.

Windows details:

Major  Minor  Build  Revision
-----  -----  -----  --------
10     0      19041  610

Problem description:

I've set focus mode to be my default. My default profile is powershell but often I open wt for WSL. My natural workflow would thus be:

  1. Open wt
  2. Open WSL (Ctrl+Shift+3)
  3. Switch back to powershell (Alt+Tab)
  4. Exit powershell (use the exit command)

Expected behavior: Tab 1 (PS) closes, Tab 2 (WSL) remains open

Actual behavior: The whole app closes silently. I am not sure if it crashes or the program simply exits.

@ghost ghost added the In-PR This issue has a related PR label Dec 10, 2020
@ghost ghost removed the In-PR This issue has a related PR label Dec 11, 2020
@ghost ghost closed this as completed in 8f60cfa Dec 11, 2020
@ghost ghost added the Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. label Dec 11, 2020
@ghost
Copy link

ghost commented Jan 28, 2021

🎉This issue was addressed in #8549, which has now been successfully released as Windows Terminal Preview v1.6.10272.0.:tada:

Handy links:

mpela81 pushed a commit to mpela81/terminal that referenced this issue Jan 28, 2021
## PR Checklist
* [x] Closes microsoft#7916
* [x] CLA signed. 
* [ ] Tests added/passed
* [ ] Documentation updated. 
* [ ] Schema updated.
* [ ] I've discussed this with core contributors already. 

## Detailed Description of the Pull Request / Additional comments
Upon tab close the tabview is responsible to issue tab selection for the next active tab.
However this doesn't happen when tabview is hidden.
There was a special treatment for this scenario for full screen mode.
Added the same treatment to focus mode (as the tabview is not visible in this case as well).

## Validation Steps Performed
Manual tests
This issue was closed.
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. Priority-1 A description (P1) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
Development

No branches or pull requests

4 participants