Compile OpenConsoleProxy without CRT #11610

lhecker · 2021-10-25T21:59:55Z

After this commit OpenConsoleProxy will be built without a CRT.
This cuts down its binary size and DLL dependency bloat.
We hope that this fixes a COM server activation bug if the
user doesn't have a CRT installed globally on their system.

Fixes #11529

miniksa

If it works, I'm good.

src/host/proxy/Host.Proxy.vcxproj

DHowett

Queued https://microsoft.visualstudio.com/Dart/_build/results?buildId=40697594&view=results

DHowett · 2021-10-25T23:54:46Z

src/host/proxy/nodefaultlib_shim.h

+// but we need one to compile IID_GENERIC_CHECK_IID.
+// Luckily we only ever use memcmp for IIDs.
+#pragma function(memcmp)
+inline int memcmp(const IID* a, const IID* b, size_t count)


WOW. Clever, but scary. No way to ensure that we do not explode if a dependency creeps in!

If we ever introduce another dependency on memcmp we could just write a generic one. It's fairly easy to write one, but I figured this one is a bit leaner (since it just delegates).

DHowett · 2021-10-25T23:58:29Z

src/host/proxy/Host.Proxy.vcxproj

-      <AdditionalIncludeDirectories>..;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
-      <CallingConvention>StdCall</CallingConvention>
+      <AdditionalIncludeDirectories>.;..;%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories>
+      <CallingConvention Condition="'$(Platform)'!='ARM64'">StdCall</CallingConvention>


this should not break arm64 .... does it?

The compiler was complaining on ARM64 that /Zg isn't a valid option actually so I silenced it.

Co-authored-by: Dustin L. Howett <duhowett@microsoft.com>

miniksa · 2021-10-26T18:31:31Z

Validated on ARM64 with https://dev.azure.com/microsoft/Dart/_build/results?buildId=40697594&view=results from this branch. It works now

ghost · 2021-10-26T19:08:22Z

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.

happybear88 · 2021-10-26T19:13:04Z

Hi! Sorry to crash your dev party, but what's CRT? I came from the now closed #11529, and I don't quite understand the assumption if the user doesn't have a CRT installed globally on their system.

11529 was repro on Windows 11 out of the box. What exactly did you expect to be installed globally that wasn't shipped with the OS? Also makes me wonder how it was tested.

Thanks,

DHowett · 2021-10-26T19:19:59Z

Sure! So, the "CRT" is the C Runtime. It is distributed in the "Visual C++ Redistributable" package that you've probably seen on most of your computers.

That package includes a few DLLs: vcruntime140.dll, msvcp140.dll, and so on. Those DLLs provide support for the C and C++ programming languages. It's usually automatically installed when it's needed, but the way Terminal installs relies on a different mechanism for finding its DLLs.

They're also not installed by default on Windows. You can find it here, or wait for it to be installed by another piece of software.

Michael and Leonard tested on a fresh Windows 11 machine that could not launch Terminal by default.

The fix here is to not depend on those DLLs at all. "OpenConsoleProxy" is another DLL file that tells Windows how to activate Terminal, and it doesn't depend on anything from those DLLs. They were marked as "required" by mistake.

We did not find this in our own testing because all of our development computers have the Visual C++ Redistributable installed already, since it comes with so much other software (like Visual Studio!)

zadjii-msft · 2021-10-26T19:20:28Z

Sorry, when we say CRT we mean the C Runtime Library. We didn't expect this dll to have a dependency on the CRT at all. That's what we get for re-using our build scripts for every project in our solution blindly 😅.

happybear88 · 2021-10-26T19:21:52Z

@DHowett thanks, that's a perfect explanation

happybear88 · 2021-10-27T10:20:48Z

Hello! I do confirm that installing VC++ redist facilitates the expected behavior.

However, this only applies to non-elevated cmd / powershell. Their elevated instances still open as their own separate windows, regardless of whether an elevated WT is open or not (i.e. "windowingBehavior": "useExisting" has no effect on the behavior)

Is this the expected behavior? If not, please point to the existing issue, as I can't find it (tried this query and variants).

zadjii-msft · 2021-10-27T10:50:48Z

Their elevated instances still open as their own separate window

Alas, that is by design. From #10276 (comment)

Unfortunately, this is by design right now. Due to a limitation in the app platform it is impossible for Terminal to be "discovered" as the handler when the application that needs a terminal is running elevated.

After this commit OpenConsoleProxy will be built without a CRT. This cuts down its binary size and DLL dependency bloat. We hope that this fixes a COM server activation bug if the user doesn't have a CRT installed globally on their system. Fixes #11529 (cherry picked from commit def1bdd)

ghost · 2021-12-14T20:45:00Z

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

Handy links:

ghost · 2021-12-14T20:46:27Z

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

Handy links:

This reverts commit def1bdd.

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

Compile OpenConsoleProxy without CRT

2ab9296

lhecker requested a review from miniksa October 25, 2021 21:59

ghost added Area-DefApp Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Terminal The new Windows Terminal. labels Oct 25, 2021

miniksa approved these changes Oct 25, 2021

View reviewed changes

DHowett reviewed Oct 25, 2021

View reviewed changes

src/host/proxy/Host.Proxy.vcxproj Outdated Show resolved Hide resolved

DHowett reviewed Oct 25, 2021

View reviewed changes

src/host/proxy/Host.Proxy.vcxproj Show resolved Hide resolved

This comment has been minimized.

Sign in to view

lhecker force-pushed the dev/lhecker/proxy-no-default-lib branch from 36772ef to 17526d6 Compare October 25, 2021 23:18

This comment has been minimized.

Sign in to view

Fix ARM64 build

9ee9026

lhecker force-pushed the dev/lhecker/proxy-no-default-lib branch from 17526d6 to 9ee9026 Compare October 25, 2021 23:27

DHowett approved these changes Oct 25, 2021

View reviewed changes

DHowett reviewed Oct 25, 2021

View reviewed changes

Address Dustin's feedback

32908fa

Co-authored-by: Dustin L. Howett <duhowett@microsoft.com>

DHowett added zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes. zStable-Service-Queued-1.12 A floating label that tracks the current Stable version for servicing purposes. labels Oct 26, 2021

DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Oct 26, 2021

ghost merged commit def1bdd into main Oct 26, 2021

ghost deleted the dev/lhecker/proxy-no-default-lib branch October 26, 2021 19:08

zadjii-msft mentioned this pull request Oct 26, 2021

[Windows 11 22000.65] Default Terminal does not work with 1.9! #10594

Closed

DHowett removed zStable-Service-Queued-1.12 A floating label that tracks the current Stable version for servicing purposes. zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes. labels Dec 13, 2021

ghost mentioned this pull request Dec 14, 2021

"Default Terminal" mysteriously doesn't work at all - boots up the vintage console instead #11529

Closed

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

Revert "Compile OpenConsoleProxy without CRT (#11610)"

00bf356

This reverts commit def1bdd.

DHowett mentioned this pull request Jun 9, 2022

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

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compile OpenConsoleProxy without CRT #11610

Compile OpenConsoleProxy without CRT #11610

lhecker commented Oct 25, 2021 •

edited by DHowett

Loading

miniksa left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

DHowett left a comment

DHowett Oct 25, 2021

lhecker Oct 26, 2021 •

edited

Loading

DHowett Oct 25, 2021

lhecker Oct 26, 2021

miniksa commented Oct 26, 2021 •

edited

Loading

ghost commented Oct 26, 2021

happybear88 commented Oct 26, 2021

DHowett commented Oct 26, 2021

zadjii-msft commented Oct 26, 2021

happybear88 commented Oct 26, 2021

happybear88 commented Oct 27, 2021 •

edited

Loading

zadjii-msft commented Oct 27, 2021

ghost commented Dec 14, 2021

ghost commented Dec 14, 2021

Compile OpenConsoleProxy without CRT #11610

Compile OpenConsoleProxy without CRT #11610

Conversation

lhecker commented Oct 25, 2021 • edited by DHowett Loading

miniksa left a comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

DHowett left a comment

Choose a reason for hiding this comment

DHowett Oct 25, 2021

Choose a reason for hiding this comment

lhecker Oct 26, 2021 • edited Loading

Choose a reason for hiding this comment

DHowett Oct 25, 2021

Choose a reason for hiding this comment

lhecker Oct 26, 2021

Choose a reason for hiding this comment

miniksa commented Oct 26, 2021 • edited Loading

ghost commented Oct 26, 2021

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.

happybear88 commented Oct 26, 2021

DHowett commented Oct 26, 2021

zadjii-msft commented Oct 26, 2021

happybear88 commented Oct 26, 2021

happybear88 commented Oct 27, 2021 • edited Loading

zadjii-msft commented Oct 27, 2021

ghost commented Dec 14, 2021

ghost commented Dec 14, 2021

lhecker commented Oct 25, 2021 •

edited by DHowett

Loading

lhecker Oct 26, 2021 •

edited

Loading

miniksa commented Oct 26, 2021 •

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.

happybear88 commented Oct 27, 2021 •

edited

Loading