Use feature markers during terminal-by-default handoff #13160

lhecker · 2022-05-24T19:31:49Z

If we want to make Windows Terminal the default terminal under Windows,
we'll have to make conhost "handoff" incoming connections by default.
But this poses a problem: How can the seldomly updated conhost know
whether the routinely updated Windows Terminal version is actually willing
to accept such handoffs by default (it might be unwilling due to bugs, etc.)?

This commit solves the issue by introducing:

A marker interface (IDefaultTerminalMarker): If it exists,
Windows Terminal indicates its willingness to accept the handoff.
Turning the all-0 GUID from being synonymous for conhost,
to being synonymous for "Let Windows decide". Without this we wouldn't
be able to differentiate between users who consciously chose conhost
as their default terminal, vs. users who want the standard behavior.

Validation Steps Performed

Testing fallback behavior:

Install "Terminal" 1.13
Delete the 2 keys below HKCU\Console\%%Startup
Enable Feature_AttemptHandoff in features.xml
Return true from DefaultApp::CheckShouldTerminalBeDefault
Replace conhost.exe and console.dll with sfpcopy after building
Launching cmd.exe launches as a conhost window ✅
(because "Terminal" 1.13 lacks the marker interface)
Open properties page in conhost.exe
"Let Windows decide" is select by default ✅
Changing the selection writes the new value ✅

Testing the new behavior:

Delete the 2 keys below HKCU\Console\%%Startup
Enable Feature_AttemptHandoff in features.xml
Return true from DefaultApp::CheckShouldTerminalBeDefault
Use CLSID_WindowsTerminalConsoleDev and CLSID_WindowsTerminalTerminalDev
for the initialization of TerminalDelegationPair
Replace conhost.exe and console.dll with sfpcopy after building
Deploy the "Terminal Dev" package
Launching cmd.exe launches "Terminal Dev" ✅
(because "Terminal Dev" has the marker interface)
Open the settings tab
"Let Windows decide" is select by default ✅
Changing the selection and saving writes the new value ✅

src/host/srvinit.cpp

src/server/IoDispatchers.cpp

zadjii-msft

Okay, this all looks like it'll work Terminal-side, and conhost side. My only recommendation would be add more comments. A lot of this defterm stuff is hard to parse, so more comments never hurt.

The code is good enough though, so

src/propslib/DelegationConfig.cpp

src/host/srvinit.cpp

src/server/IoDispatchers.cpp

carlos-zamora

Looks good to me. Thanks!

src/propslib/DelegationConfig.cpp

carlos-zamora · 2022-05-31T22:58:02Z

Dustin wanted to take a look at this, so I'm leaving it unmerged for now :)

carlos-zamora · 2022-05-31T23:07:26Z

@msftbot make sure @DHowett signs off on this

ghost · 2022-05-31T23:07:30Z

Hello @carlos-zamora!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

I'll only merge this pull request if it's approved by @DHowett

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

src/propslib/DelegationConfig.cpp

DHowett

Chatted with Leonard & we need a slightly different solution :)

I redid the entire thing.

lhecker · 2022-06-03T00:03:39Z

I've rewritten this PR, but haven't tested it yet and so converted this PR to a Draft.
I think it should work however, so please feel free to review if you have time to spare. 🙂
I'm updating the PR message to reflect the new approach.

lhecker · 2022-06-03T19:00:21Z

This is what the new option looks like:

zadjii-msft

Okay, I read over this all, it makes sense to me. I again left notes as I went - my nit again is that all this defterm stuff is already hard to parse so more comments is always better.

zadjii-msft · 2022-06-06T17:15:31Z

src/cascadia/TerminalSettingsEditor/Launch.xaml

@@ -93,7 +93,7 @@

                                    <Grid.RowDefinitions>
                                        <!--  profile name  -->
-                                        <RowDefinition Height="*" />
+                                        <RowDefinition Height="20" />


nit: is this redundant with the Height="20" on the Name TextBlock?

Unfortunately we only get the layout in the screenshot above, by specifying the height explicitly twice. The reason is that the flyout (and only the flyout) seemingly doesn't respect the visibility state of the two TextBlocks. Due to that the dropdown box will have the correct height (1 line high) but the flyout list will be 2 lines high, even with both TextBlocks collapsed. I'm not entirely sure how such a bug can occur, but it does. 😄

zadjii-msft · 2022-06-06T17:24:11Z

src/cascadia/TerminalSettingsModel/CascadiaSettings.cpp

+    std::pair<std::vector<Model::DefaultTerminal>, Model::DefaultTerminal> result{ {}, nullptr };
+    til::latch latch{ 1 };
+
+    std::ignore = [&]() -> winrt::fire_and_forget {


I low key really love this "go do something on a backgrouund thread" lambda, without needing to make a whole function declaration for it.

It's like a sneaky way of doing a IAsyncOperation without also making this method async, and letting us return a non winrt type. neato.

But we're not using extractValueFromTaskWithoutMainThreadAwait... because... it's just a static helper in the other file?

extractValueFromTaskWithoutMainThreadAwait expects an (asynchronous) Task which can be co_awaited, but this code executes a synchronous function on a background task instead.

zadjii-msft · 2022-06-06T17:33:54Z

src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp

@@ -61,9 +61,11 @@ static auto extractValueFromTaskWithoutMainThreadAwait(TTask&& task) -> decltype
    til::latch latch{ 1 };

    const auto _ = [&]() -> winrt::fire_and_forget {
+        const auto cleanup = wil::scope_exit([&]() {


not sure I understand why this moved. Just in case task throws?

Yeah I realized this mistake when I wrote the code for CascadiaSettings.cpp and wanted to fix extractValueFromTaskWithoutMainThreadAwait as well.

zadjii-msft · 2022-06-06T17:42:21Z

src/inc/conint.h

@@ -47,7 +47,7 @@ namespace Microsoft::Console::Internal

    namespace DefaultApp
    {
-        [[nodiscard]] HRESULT CheckDefaultAppPolicy(bool& isEnabled) noexcept;
-        [[nodiscard]] HRESULT CheckShouldTerminalBeDefault(bool& isEnabled) noexcept;
+        [[nodiscard]] bool CheckDefaultAppPolicy() noexcept;


IIRC if we change the signature of these methods, we'll have to update the internal-only versions as well, FYI.

Yep! I felt like this made writing the code more comfortable and noticed that the internal code never returns anything but S_OK anyways.

zadjii-msft · 2022-06-06T19:50:44Z

src/server/IoDispatchers.cpp

+        if (Globals.defaultTerminalMarkerCheckRequired)
+        {
+            ::Microsoft::WRL::ComPtr<IDefaultTerminalMarker> marker;
+            if (FAILED(handoff.As(&marker)))
+            {
+                Globals.delegationPair = DelegationConfig::ConhostDelegationPair;
+                Globals.defaultTerminalMarkerCheckRequired = false;
+                return;
+            }
+        }


This is the new section

src/propslib/DelegationConfig.cpp

zadjii-msft · 2022-06-07T15:01:52Z

src/propslib/DelegationConfig.cpp

-                                                      &bytesNeeded);
-
-    if (NTSTATUS_FROM_WIN32(ERROR_SUCCESS) != result)
+    if (values[0] == CLSID_Default || values[1] == CLSID_Default)


notes: if either was missing or failed to parse, use {default, 0, 0}

zadjii-msft · 2022-06-07T15:02:58Z

src/propslib/DelegationConfig.cpp

-    RETURN_IF_FAILED(IIDFromString(buffer.get(), &iid));
-
-    return S_OK;
+    if (values[0] == CLSID_Conhost || values[1] == CLSID_Conhost)


Was the saved value SPECIFICALLY conhost? then return that

oh ho ho so I see, this is where we're being tricky now. Now conhost isn't {0}, it's got it's own GUID now. That makes sense.

src/propslib/DelegationConfig.cpp

zadjii-msft · 2022-06-07T15:06:12Z

src/propslib/DelegationConfig.cpp

-
-    vec.push_back(useInbox);
-
-    auto coinit = wil::CoInitializeEx(COINIT_APARTMENTTHREADED);


We pretty sure the propsheet initializes COM itself? Note that the propsheet can get loaded outside of the context of conhost, just by right-clicking on a .lnk to a commandline application.

I just moved the initialization one call up so that we don't have to do this twice.

src/propslib/DelegationConfig.hpp

src/server/IoDispatchers.cpp

src/cascadia/TerminalSettingsModel/Resources/en-US/Resources.resw

When #13160 introduced a new interface to the IConsoleHandoff idl, it changed midl's RPC proxy stub lookup algorithm from a direct GUID comparison to an unrolled binary search. Now, that would ordinarily not be a problem... However, in #11610, we took a shortcut and replaced `memcmp` -- used only by RPC for GUID comparison -- with a direct GUID-only equality comparator. This worked totally fine, and ordinarily would not be a problem... The unrolled binary search unfortunately _relies on memcmp's contract_: it uses memcmp to match against a fully sorted set. Our memcmp only returned 0 or 1 (equal or not), and it knew nothing about ordering. When a package that contains a PackagedCOM proxy stub is installed, it is selected as the primary proxy stub for any interfaces it can proxy. After all, interfaces are immutable, so it doesn't matter whose proxy you're using. Now, given that we installed a *broken* proxy... *all* IIDs that got whacked by our memcmp issue broke for every consumer. To fix it: instead of implementing memcmp ourselves, we're just going to take a page out of WinAppSDK's book and link this binary using the "Hybrid CRT" model. It will statically link any parts of the STL it uses (none) and dynamically link the ucrt (which is guaranteed to be present on Windows.) Sure, the binary size goes up from 8k to 24k, but... the cost is never having to worry about this again. Closes #13251

If we want to make Windows Terminal the default terminal under Windows, we'll have to make conhost "handoff" incoming connections by default. But this poses a problem: How can the seldomly updated conhost know whether the routinely updated Windows Terminal version is actually willing to accept such handoffs by default (it might be unwilling due to bugs, etc.)? This commit solves the issue by introducing: * A marker interface (`IDefaultTerminalMarker`): If it exists, Windows Terminal indicates its willingness to accept the handoff. * Turning the all-0 GUID from being synonymous for conhost, to being synonymous for "Let Windows decide". Without this we wouldn't be able to differentiate between users who consciously chose conhost as their default terminal, vs. users who want the standard behavior. Testing fallback behavior: * Install "Terminal" 1.13 * Delete the 2 keys below `HKCU\Console\%%Startup` * Enable `Feature_AttemptHandoff` in `features.xml` Return `true` from `DefaultApp::CheckShouldTerminalBeDefault` * Replace `conhost.exe` and `console.dll` with `sfpcopy` after building * Launching `cmd.exe` launches as a conhost window ✅ (because "Terminal" 1.13 lacks the marker interface) * Open properties page in `conhost.exe` "Let Windows decide" is select by default ✅ * Changing the selection writes the new value ✅ Testing the new behavior: * Delete the 2 keys below `HKCU\Console\%%Startup` * Enable `Feature_AttemptHandoff` in `features.xml` Return `true` from `DefaultApp::CheckShouldTerminalBeDefault` * Use `CLSID_WindowsTerminalConsoleDev` and `CLSID_WindowsTerminalTerminalDev` for the initialization of `TerminalDelegationPair` * Replace `conhost.exe` and `console.dll` with `sfpcopy` after building * Deploy the "Terminal Dev" package * Launching `cmd.exe` launches "Terminal Dev" ✅ (because "Terminal Dev" has the marker interface) * Open the settings tab "Let Windows decide" is select by default ✅ * Changing the selection and saving writes the new value ✅ (cherry picked from commit 1b81c65) Service-Card-Id: 82925080 Service-Version: Inbox

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

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

Handy links:

ghost · 2022-07-06T18:09:00Z

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

Handy links:

lhecker force-pushed the dev/lhecker/wt-as-default branch from 59d45c9 to afec10f Compare May 24, 2022 19:34

lhecker added Product-Terminal The new Windows Terminal. Priority-1 A description (P1) Area-DefApp labels May 24, 2022

lhecker commented May 24, 2022

View reviewed changes

src/host/srvinit.cpp Outdated Show resolved Hide resolved

lhecker commented May 24, 2022

View reviewed changes

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

zadjii-msft previously approved these changes May 26, 2022

View reviewed changes

carlos-zamora previously approved these changes May 31, 2022

View reviewed changes

src/propslib/DelegationConfig.cpp Outdated Show resolved Hide resolved

carlos-zamora assigned DHowett May 31, 2022

carlos-zamora requested a review from DHowett May 31, 2022 22:57

ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label May 31, 2022

DHowett reviewed Jun 1, 2022

View reviewed changes

src/propslib/DelegationConfig.cpp Outdated Show resolved Hide resolved

DHowett requested changes Jun 1, 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 1, 2022

lhecker requested review from zadjii-msft, carlos-zamora and DHowett June 2, 2022 23:57

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

lhecker marked this pull request as draft June 2, 2022 23:59

This comment was marked as outdated.

Sign in to view

lhecker force-pushed the dev/lhecker/wt-as-default branch from 79ddc66 to 86a1863 Compare June 3, 2022 19:10

lhecker marked this pull request as ready for review June 3, 2022 19:10

lhecker force-pushed the dev/lhecker/wt-as-default branch 2 times, most recently from fe26551 to 7c92e62 Compare June 3, 2022 20:07

Use feature markers during terminal-by-default handoff

89b4739

lhecker force-pushed the dev/lhecker/wt-as-default branch from 7c92e62 to 89b4739 Compare June 3, 2022 20:11

microsoft deleted a comment from github-actions bot Jun 3, 2022

Improve layout of xaml dropdown

33a69fe

lhecker force-pushed the dev/lhecker/wt-as-default branch from d754068 to 33a69fe Compare June 6, 2022 15:10

zadjii-msft approved these changes Jun 7, 2022

View reviewed changes

DHowett reviewed Jun 7, 2022

View reviewed changes

src/propslib/DelegationConfig.hpp Show resolved Hide resolved

DHowett reviewed Jun 7, 2022

View reviewed changes

src/server/IoDispatchers.cpp Show resolved Hide resolved

DHowett reviewed Jun 7, 2022

View reviewed changes

src/cascadia/TerminalSettingsModel/Resources/en-US/Resources.resw Outdated Show resolved Hide resolved

lhecker added 2 commits June 7, 2022 20:53

Add comments

23c1b96

Address feedback

8a80ac1

DHowett approved these changes Jun 7, 2022

View reviewed changes

ghost merged commit 1b81c65 into main Jun 7, 2022

ghost deleted the dev/lhecker/wt-as-default branch June 7, 2022 19:31

zadjii-msft mentioned this pull request Jun 8, 2022

The DxD markers broke defterm entirely #13251

Closed

DHowett mentioned this pull request Jun 9, 2022

Restore OpenConsoleProxyStub's dependency on the CRT to fix QI #13254

Merged

DHowett added a commit that referenced this pull request Jun 30, 2022

Backport the DefTerm SUI stability fix from #13160

78cda41

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use feature markers during terminal-by-default handoff #13160

Use feature markers during terminal-by-default handoff #13160

lhecker commented May 24, 2022 •

edited

Loading

zadjii-msft left a comment

carlos-zamora left a comment

carlos-zamora commented May 31, 2022

carlos-zamora commented May 31, 2022

ghost commented May 31, 2022

DHowett left a comment

lhecker commented Jun 3, 2022 •

edited

Loading

This comment was marked as outdated.

lhecker commented Jun 3, 2022 •

edited

Loading

zadjii-msft left a comment

zadjii-msft Jun 6, 2022

lhecker Jun 7, 2022

zadjii-msft Jun 6, 2022

zadjii-msft Jun 6, 2022

lhecker Jun 7, 2022

zadjii-msft Jun 6, 2022

lhecker Jun 7, 2022

zadjii-msft Jun 6, 2022

lhecker Jun 7, 2022

zadjii-msft Jun 6, 2022

zadjii-msft Jun 7, 2022

zadjii-msft Jun 7, 2022

zadjii-msft Jun 7, 2022

lhecker Jun 7, 2022

ghost commented Jul 6, 2022

ghost commented Jul 6, 2022


		vec.push_back(useInbox);

		auto coinit = wil::CoInitializeEx(COINIT_APARTMENTTHREADED);

Use feature markers during terminal-by-default handoff #13160

Use feature markers during terminal-by-default handoff #13160

Conversation

lhecker commented May 24, 2022 • edited Loading

Validation Steps Performed

zadjii-msft left a comment

Choose a reason for hiding this comment

carlos-zamora left a comment

Choose a reason for hiding this comment

carlos-zamora commented May 31, 2022

carlos-zamora commented May 31, 2022

ghost commented May 31, 2022

DHowett left a comment

Choose a reason for hiding this comment

lhecker commented Jun 3, 2022 • edited Loading

This comment was marked as outdated.

lhecker commented Jun 3, 2022 • edited Loading

zadjii-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jul 6, 2022

ghost commented Jul 6, 2022

lhecker commented May 24, 2022 •

edited

Loading

lhecker commented Jun 3, 2022 •

edited

Loading

lhecker commented Jun 3, 2022 •

edited

Loading