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

GUI apps still don't open in the foreground, from a default terminal launch #13211

Closed
vefatica opened this issue May 31, 2022 · 16 comments · Fixed by #13247
Closed

GUI apps still don't open in the foreground, from a default terminal launch #13211

vefatica opened this issue May 31, 2022 · 16 comments · Fixed by #13247
Assignees
Labels
Area-DefApp Area-Windowing Window frame, quake mode, tearout Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-0 Bugs that we consider release-blocking/recall-class (P0) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. Severity-Blocking We won't ship a release like this! No-siree.

Comments

@vefatica
Copy link

A TCC user said that GUI apps started by TCC in WT don't get focus (unlike other shells in WT and unlike TCC in a console). I said I didn't think I could get TCC and/or WT to act that way if I tried. Have I overlooked anything ... any ideas? Thanks!

@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 May 31, 2022
@DHowett
Copy link
Member

DHowett commented May 31, 2022

This is not actionable without knowing the version of Windows Terminal the TCC user has installed -- we made some significant windowing changes in Preview 1.14.

@DHowett DHowett added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label May 31, 2022
@vefatica
Copy link
Author

His setup is the same as mine. I have no such problem.

v:> ver
TCC 28.02.18 x64
Microsoft Windows 10 Pro for Workstations
10.0.19044.1706 (2009, 21H2)
WindowsTerminalPreview 1.14.1432.0

@ghost ghost added Needs-Attention The core contributors need to come back around and look at this ASAP. and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels May 31, 2022
@vefatica
Copy link
Author

vefatica commented Jun 1, 2022

The user in question is using WindowsTerminalPreview 1.14.2205.25002. I am using WindowsTerminalPreview 1.14.2205.23002.

Is that newer build available on GitHub?

@zadjii-msft
Copy link
Member

I don't think the revision numbers make a difference here.

Do you have a link to the original bug report?

There was a LOT of work in this area in 1.14, in #2988, #12515, and every thread linked to those. How are the GUI apps getting spawned by TCC? Via ShellExecute?

@zadjii-msft zadjii-msft added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Attention The core contributors need to come back around and look at this ASAP. labels Jun 2, 2022
@vefatica
Copy link
Author

vefatica commented Jun 2, 2022

Here's a link to the original report.

I suspect with CreateProcessW. WinDbg isn't being very cooperative:

0:001> bp shell32!ShellExecuteW
0:001> bp shell32!ShellExecuteExW
0:001> bp kernel32!CreateProcessW
Couldn't resolve error at 'kernel32!CreateProcessW'

It does not break on ShellEXecute[Ex] when I issue "notepad".

@ghost ghost added Needs-Attention The core contributors need to come back around and look at this ASAP. and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Jun 2, 2022
@vefatica
Copy link
Author

vefatica commented Jun 2, 2022

I tried again. WinDbg does break at CreateProcessW when I issue "notepad".

@vefatica
Copy link
Author

vefatica commented Jun 4, 2022

Mike, if you're following, you'll be interested in #18. The user reports that whatever shell is in WT's FIRST tab will exhibit this problem.

In a nutshell ... it's not only TCC.

@vefatica
Copy link
Author

vefatica commented Jun 4, 2022

And, if I read his comments accurately, he has a setup in which WT is the default handler for console apps (as opposed to conhost). Musy be (?) he has some sort of insider status.

@ohenryx2
Copy link

ohenryx2 commented Jun 5, 2022

I was the originator of this bug report, over on the JPSoft forums. I mistakenly thought the problem was limited to TCC, but that is not the case.

I am not an “Insider”, this is a public build of Windows Terminal Preview, and a public build of Windows.

Microsoft Windows
Version 21H2 (OS Build 22000.708)

Windows Terminal Preview
Version 1.14.1452.0

I obtained Windows Terminal Preview from the Microsoft Store. It offered to make itself the default handler for shells, and I accepted the offer. I have shortcuts on the desktop for cmd.exe and tcc.exe (Jpsoft). If I click on either of those shortcuts, they start in a new Windows Terminal Preview tabbed window. And from there, if I start an application (notepad, for instance), the focus stays in the Windows Terminal Preview window, it does not shift to the newly launched application. The same behavior for tcc and for cmd. Doesn’t matter if I type in “notepad” or “start notepad”.

Also, if I launch Powershell Preview from the Start Menu, it starts up in a Windows Terminal tabbed window, and exhibits the same behavior.

Now once I have a window up for Windows Terminal Preview, if I start another tab, the problem does not exist in the secondary tab. And it doesn’t matter what’s running in the secondary tab, cmd, tcc, pwsh, all behave normally. Launch an application, that new application gets the focus.

And if I kill the primary tab, so the secondary tab becomes the primary tab, there is now no problem in the new primary tab.

I also have Windows Terminal Version: 1.12.10983.0
Installed on this system. I have placed a shortcut on the desktop for
C:\Users\Henry\AppData\Local\Microsoft\WindowsApps\Microsoft.WindowsTerminal_8wekyb3d8bbwe\wt.exe c:\tcmd\tcc.exe

That launches tcc under the older version of WT, and the problem does not then exist.

@zadjii-msft zadjii-msft added Product-Terminal The new Windows Terminal. Area-DefApp Area-Windowing Window frame, quake mode, tearout Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-0 Bugs that we consider release-blocking/recall-class (P0) and removed Needs-Attention The core contributors need to come back around and look at this ASAP. labels Jun 6, 2022
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Jun 6, 2022
@zadjii-msft zadjii-msft added this to the Terminal v1.15 milestone Jun 6, 2022
@zadjii-msft
Copy link
Member

Thanks for all the background guys. I had a feeling this was related to defterm, and everything here confirms that suspicion. I bet the focus/foreground handler isn't triggered for defterm connections. I'll take a look.

@zadjii-msft zadjii-msft added the Severity-Blocking We won't ship a release like this! No-siree. label Jun 6, 2022
@vefatica
Copy link
Author

vefatica commented Jun 6, 2022

Mike, can defterm be configured manually on Windows 10? I have onle unzipped installations of WT? Once installed, by any means, is it easy to switch it on/off? Thanks!

@zadjii-msft
Copy link
Member

can defterm be configured manually on Windows 10?

Nope.

is it easy to switch it on/off? Thanks!

Yep, it's a dropdown in the Settings UI, in the Settings app, and in the Console properties sheet.

@zadjii-msft zadjii-msft self-assigned this Jun 7, 2022
@zadjii-msft
Copy link
Member

zadjii-msft commented Jun 7, 2022

TerminalPage::_InitControl - tells the control the window is visible (but not focused). It also tells the connection about the owning hwnd.

I'm betting that the owner is never set for that pane. If it were set, then deactivating and reactivating that pane would re-mark the pane as focused, fixing the issue. I bet the conpty here never knows the owner, so when it checks if the owner is focused - it never is.

I can't just stick a DebugBreak() in TerminalPage::_OnNewConnection, cause that is called in the COM RPC call, so that results in the server throwing an exception and conhost just saying fuck it.

Where is _hostingHwnd set, and in defterm, is that after _OnNewConnection?

_hostingHwnd is set by the _logic.try_as<IInitializeWithWindow>().Initialize(...) in AppHost, which is called in winmain. But long before that, we _HandleCommandlineArgs, which if (IsHandoffListener), we SetInboundListener, which sets up the ConptyConnection::StartInboundListener. That means it's totally possible for us to IMMEDIATELY get a inbound connection. If fact, I'm betting we always do, so that we already have a tab when we get to the setting up the window buisiness.

We may be able to remedy this by checking if we don't have a _hostingHwnd yet, we should iterate on all the controls and set the OwningHwnd() (which also would need to be changed to make sure to update the connection now, too).


Even after fixing that (4de1ef7), bp openconsole!Microsoft::Console::VirtualTerminal::InteractDispatch::FocusChanged is never hit in the defterm conpty. Why?

Microsoft_Terminal_Control!Microsoft::Console::VirtualTerminal::TerminalInput::HandleFocus will tell you why. Focus Event Mode isn't enabled in the Terminal for a defterm connection (apparently).

It's cause focus event requesting is tied to w32 input mode, which apparently isn't requested for defterm


EVEN AFTER FIXING THAT

it still doesn't work. I'm 100% betting that this is due to the ConsoleControl plumbing for defterm

image

I was right

Error Code Symbolic Name Error Description Header
0x80070006 E_HANDLE Invalid handle winerror.h

case HostSignals::SetForeground:
{
auto msg = _ReceiveTypedPacket<HostSignalSetForegroundData>();
LOG_IF_NTSTATUS_FAILED(ServiceLocator::LocateConsoleControl()->SetForeground(ULongToHandle(msg.processId), msg.isForeground));
break;
}

I'm gonna have to peek behind the OS curtain, and see where that API goes. I bet they reject the call in that fashion, because the process we're giving FG to is attached to OpenConsole, not conhost, but conhost does the ConsoleControl (or something like that)

There's only one place in xxxConsoleControl that looks like it returns a STATUS_INVALID_HANDLE. I don't love that. Presumably ObReferenceObjectByHandle might also be able to return STATUS_INVALID_HANDLE. In the origin conhost, set the bp at user32!ConsoleControl.


I totally know why this don't work. OC tells conhost the handle to the process to request FG for, but that handle doesn't exist in conhost. We're literally just passing the HANDLE value.

RemoteConsoleControl passes the handle value, literally, to conhost. We can change that to be a PID, but that would have to be paired with a OS conhost change to accept a PID. So it'd have to stay a HANDLE, but there's no way for OC to dupe to the conhost we're connected to, right? Like, there's no connection BACK to the origin conhost... is there? There's just a pipe... Oh, thanks @miniksa for preparing for this inevitability:
image

So we should be able to DuplicateHandle the handle to the original conhost.... but without OS-side changes to conhost, each of those process handles is just gonna get leaked by conhost, each time. Can we close a handle in another process? DuplicateHandle(..., DUPLICATE_CLOSE_SOURCE) seems like we should be able to, but that sounds like a surefire race condition. Not hard to imagine that we fire off the message to the pipe, and close the handle on their side before they even process the message.

Other stupid idea: conhost is never gonna close these handles, so just maintain a map of openconsole handle,pid -> conhost handle on the openconsole side. Whenever we call this method, lookup to see if we've called it for this HANDLE to this specific PID. If we haven't, then duplicate the handle over to conhost, and stash that HANDLE value.

Or maybe more obviously - openconsole can store the conhost side of the handle in the process list we already maintain, and just wait to close the conhost side of the handle until we prune the process entry (when openconsole is done with it.

@zadjii-msft zadjii-msft changed the title GUI apps started by TCC in WT don't get focus (question) GUI apps still don't open in the foreground, from a default terminal launch Jun 8, 2022
@ghost ghost added the In-PR This issue has a related PR label Jun 8, 2022
@zadjii-msft zadjii-msft removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Jun 9, 2022
@ghost ghost closed this as completed in #13247 Jun 9, 2022
ghost pushed a commit that referenced this issue Jun 9, 2022
See also: #12799, the origin of much of this.

This change evolved over multiple phases. 

### Part the first

When we create a defterm connection in `TerminalPage::_OnNewConnection`,
we don't have the hosting HWND yet, so the tab gets created without one.
We'll later get called with the owner, in `Initialize`. 

To remedy this, we need to:
* In `Initialize`, make sure to update any existing controls with the
  new owner.
* In `ControlCore`, actually propogate the new owner down to the
  connection

### Part the second

DefTerm launches don't actually request focus mode, so the Terminal
never sends them focus events. We need those focus events so that the
console can request foreground rights.

To remedy this, we need to:
* pass `--win32input` to the commandline used to initialize OpenConsole
  in ConPTY mode. We request focus events at the same time we request
  win32-input-mode.
* I also added `--resizeQuirk`, because _by all accounts that should be
  there_. Resizing in defterm windows should be _wacky_ without it, and
  I'm a little surprised we haven't seen any bugs due to this yet.

### Part the third

`ConsoleSetForeground` expects a `HANDLE` to the process we want to give
foreground rights to. The problem is, the wire format we used _also_
decided that a HANDLE value was a good idea. It's not. If we pass the
literal value of the HANDLE to the process from OpenConsole to conhost,
so conhost can call that API, the value that conhost uses there will
most likely be an invalid handle. The HANDLE's value is its value in
_OpenConsole_, not in conhost.

To remedy this, we need to:
* Just not forward `ConsoleSetForeground`. Turns out, we _can_ just call
  that in OpenConsole safely. There's no validation. So just instantiate
  a static version of the Win32 version of ConsoleControl, just to use
  for SetForeground. (thanks Dustin)

* [x] Tested manually - Win+R `powershell`, `notepad` spawns on top.

Closes #13211
@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 Jun 9, 2022
@miniksa
Copy link
Member

miniksa commented Jun 10, 2022

@zadjii-msft sorry to leave you with this headache but great analysis and I'm glad that process information I left behind was worth it!

DHowett pushed a commit that referenced this issue Jun 30, 2022
See also: #12799, the origin of much of this.

This change evolved over multiple phases.

When we create a defterm connection in `TerminalPage::_OnNewConnection`,
we don't have the hosting HWND yet, so the tab gets created without one.
We'll later get called with the owner, in `Initialize`.

To remedy this, we need to:
* In `Initialize`, make sure to update any existing controls with the
  new owner.
* In `ControlCore`, actually propogate the new owner down to the
  connection

DefTerm launches don't actually request focus mode, so the Terminal
never sends them focus events. We need those focus events so that the
console can request foreground rights.

To remedy this, we need to:
* pass `--win32input` to the commandline used to initialize OpenConsole
  in ConPTY mode. We request focus events at the same time we request
  win32-input-mode.
* I also added `--resizeQuirk`, because _by all accounts that should be
  there_. Resizing in defterm windows should be _wacky_ without it, and
  I'm a little surprised we haven't seen any bugs due to this yet.

`ConsoleSetForeground` expects a `HANDLE` to the process we want to give
foreground rights to. The problem is, the wire format we used _also_
decided that a HANDLE value was a good idea. It's not. If we pass the
literal value of the HANDLE to the process from OpenConsole to conhost,
so conhost can call that API, the value that conhost uses there will
most likely be an invalid handle. The HANDLE's value is its value in
_OpenConsole_, not in conhost.

To remedy this, we need to:
* Just not forward `ConsoleSetForeground`. Turns out, we _can_ just call
  that in OpenConsole safely. There's no validation. So just instantiate
  a static version of the Win32 version of ConsoleControl, just to use
  for SetForeground. (thanks Dustin)

* [x] Tested manually - Win+R `powershell`, `notepad` spawns on top.

Closes #13211

(cherry picked from commit 2a7dd8b)
Service-Card-Id: 83026847
Service-Version: 1.14
@ghost
Copy link

ghost commented Jul 6, 2022

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

Handy links:

@ghost
Copy link

ghost commented Jul 6, 2022

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

Handy links:

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-DefApp Area-Windowing Window frame, quake mode, tearout Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-0 Bugs that we consider release-blocking/recall-class (P0) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. Severity-Blocking We won't ship a release like this! No-siree.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants