Make sure foreground access works for DefTerm #13247

zadjii-msft · 2022-06-08T15:48:35Z

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)
Tested manually - Win+R powershell, notepad spawns on top.

Closes #13211

…'t request focus mode. Weird.

…vents, but that doesn't fix it either

This reverts commit da55ab7.

…fterm-focus-foreground

src/server/ProcessHandle.cpp

DHowett

Chatted offline, there might be a better way. However, I love the signature changes and clarifying comments. We should keep those.

…fterm-focus-foreground

DHowett · 2022-06-09T20:32:44Z

src/inc/HostSignals.hpp

    };

    struct HostSignalSetForegroundData
    {
        uint32_t sizeInBytes;
-        uint32_t processId;
+        uint32_t processId; // THIS IS A HANDLE, NOT A PID


blame @miniksa

Brain fart. Sorry.

DHowett · 2022-06-09T20:40:39Z

Nits: When you prep this as a commit message, remove the commit IDs from the body (they aren't meaningful once squashed), and remove the original notes about the thing we didn't do / the summary/details section

…fterm-focus-foreground

DHowett · 2022-06-09T22:38:43Z

Moved from body:

See also: #12799, the origin of much of this.

This change evolved over multiple phases.

Part the first (`4de1ef7`)

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 (`5ab0799`)

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 (`e970f7c`)

Part the third was changed dramatically, original summary here.

There's certain OS-side checks that make it so only the original conhost can call the ConsoleControl APIs. When launched as the delegation console, OpenConsole forwards these calls to the original conhost that spawned it. The one in question here is ConsoleSetForeground. That 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:

Duplicate client process handles to the original conhost when running in delegation console mode
Pass the value of the handle in the conhost process space to RemoteConsoleControl instead.

This is necessarily tricky, because we assumingly can't make conhost-side changes here. We have to assume that new Terminal's will be running on older versions of Windows 11, and also older Terminals will be running on newer Windows 11's (with this fix). If there's a mismatch in what either is expecting, then the whole thing'll blow up.

By duplicating the handle into the conhost, we can keep passing a HANDLE value that conhost will pass straight through on all versions of Windows 11. That also means OpenConsole needs to manage the lifetime of that handle, because old conhosts will NEVER be able to know that there's a new HANDLE in their process space.

Part the fourth, (`55a7331`)

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)
Closes GUI apps still don't open in the foreground, from a default terminal launch #13211
Tested manually - Win+R powershell, notepad spawns on top.
I work here

lhecker

I love this PR. The comments are great and explain the problems well.
What --resizeQuirk and --win32input mean is not that well explained in this PR and might be hard to understand for newcomers, but I believe that this is clearly out of scope for the PR.

ghost · 2022-06-09T23:12:16Z

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

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 (`@msftbot`) and give me an instruction to get started! Learn more here.

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 · 2022-07-06T18:05:43Z

🎉Windows Terminal v1.14.186 has been released which incorporates this pull request.:tada:

Handy links:

ghost · 2022-07-06T18:07:33Z

🎉Windows Terminal Preview v1.15.186 has been released which incorporates this pull request.:tada:

Handy links:

zadjii-msft added 6 commits June 7, 2022 14:47

This makes sure we re-own the window appropriately, but defterm doesn…

4de1ef7

…'t request focus mode. Weird.

make sure to enable win32-input-mode, to make sure we request focus e…

5ab0799

…vents, but that doesn't fix it either

This was totally wrong

da55ab7

Revert "This was totally wrong"

f117fb8

This reverts commit da55ab7.

this worked

e970f7c

lemme tell you, this is NOT a HANDLE

7d82c81

This comment was marked as resolved.

Sign in to view

zadjii-msft added 2 commits June 8, 2022 10:52

Oh man only one spelling mistake

a4cd034

Merge remote-tracking branch 'origin/main' into dev/migrie/b/13211-de…

9295f95

…fterm-focus-foreground

DHowett reviewed Jun 8, 2022

View reviewed changes

src/server/ProcessHandle.cpp Outdated Show resolved Hide resolved

zadjii-msft mentioned this pull request Jun 8, 2022

Things we can fix when we can safely rev the DefTerm interface #13249

Closed

1 task

DHowett requested changes Jun 8, 2022

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jun 8, 2022

zadjii-msft added 2 commits June 9, 2022 15:10

Merge remote-tracking branch 'origin/main' into dev/migrie/b/13211-de…

b1721f6

…fterm-focus-foreground

This is far simpler

55a7331

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jun 9, 2022

zadjii-msft assigned DHowett Jun 9, 2022

DHowett reviewed Jun 9, 2022

View reviewed changes

DHowett approved these changes Jun 9, 2022

View reviewed changes

DHowett added the Needs-Second It's a PR that needs another sign-off label Jun 9, 2022

ghost requested review from PankajBhojwani, carlos-zamora and lhecker June 9, 2022 20:35

carlos-zamora unassigned DHowett Jun 9, 2022

Merge remote-tracking branch 'origin/main' into dev/migrie/b/13211-de…

9deaa13

…fterm-focus-foreground

lhecker approved these changes Jun 9, 2022

View reviewed changes

DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jun 9, 2022

ghost merged commit 2a7dd8b into main Jun 9, 2022

ghost deleted the dev/migrie/b/13211-defterm-focus-foreground branch June 9, 2022 23:12

ghost removed the Needs-Second It's a PR that needs another sign-off label Jun 10, 2022

ghost mentioned this pull request Jul 6, 2022

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

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make sure foreground access works for DefTerm #13247

Make sure foreground access works for DefTerm #13247

zadjii-msft commented Jun 8, 2022 •

edited by DHowett

Loading

This comment was marked as resolved.

DHowett left a comment

DHowett Jun 9, 2022

miniksa Jun 10, 2022

DHowett commented Jun 9, 2022

DHowett commented Jun 9, 2022

lhecker left a comment •

edited

Loading

ghost commented Jun 9, 2022

ghost commented Jul 6, 2022

ghost commented Jul 6, 2022

Make sure foreground access works for DefTerm #13247

Make sure foreground access works for DefTerm #13247

Conversation

zadjii-msft commented Jun 8, 2022 • edited by DHowett Loading

Part the first

Part the second

Part the third

This comment was marked as resolved.

DHowett left a comment

Choose a reason for hiding this comment

DHowett Jun 9, 2022

Choose a reason for hiding this comment

miniksa Jun 10, 2022

Choose a reason for hiding this comment

DHowett commented Jun 9, 2022

DHowett commented Jun 9, 2022

Part the first (4de1ef7)

Part the second (5ab0799)

Part the third (e970f7c)

Part the fourth, (55a7331)

lhecker left a comment • edited Loading

Choose a reason for hiding this comment

ghost commented Jun 9, 2022

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 (@msftbot) and give me an instruction to get started! Learn more here.

ghost commented Jul 6, 2022

ghost commented Jul 6, 2022

zadjii-msft commented Jun 8, 2022 •

edited by DHowett

Loading

Part the first (`4de1ef7`)

Part the second (`5ab0799`)

Part the third (`e970f7c`)

Part the fourth, (`55a7331`)

lhecker left a comment •

edited

Loading

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 (`@msftbot`) and give me an instruction to get started! Learn more here.