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.

Sign up for GitHub

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

Jump to bottom

The DxD markers broke defterm entirely #13251

Closed
zadjii-msft opened this issue Jun 8, 2022 · 0 comments · Fixed by #13254
Closed

The DxD markers broke defterm entirely #13251

zadjii-msft opened this issue Jun 8, 2022 · 0 comments · Fixed by #13254
Labels
Area-DefApp 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.
Milestone
Terminal v1.15

Comments

@zadjii-msft
Copy link
Member

DON'T MERGE main into your branches

Broke in #13160

We're investigating. This seems to be an OS side issue, but if it's not just an internal bug, then we will likely need to rethink DxD entirely.
@zadjii-msft zadjii-msft added Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. Severity-Blocking We won't ship a release like this! No-siree. Priority-0 Bugs that we consider release-blocking/recall-class (P0) Area-DefApp labels Jun 8, 2022
@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 Jun 8, 2022
@zadjii-msft zadjii-msft added this to the Terminal v1.15 milestone Jun 8, 2022
@zadjii-msft zadjii-msft mentioned this issue Jun 9, 2022
Merged
@ghost ghost added the In-PR This issue has a related PR label Jun 9, 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 #13254 Jun 9, 2022
@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
ghost pushed a commit that referenced this issue Jun 9, 2022
@DHowett
Restore OpenConsoleProxyStub's dependency on the CRT to fix QI (#13254)
cd35bc5 
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
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Area-DefApp 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
Milestone
Terminal v1.15
Development

Successfully merging a pull request may close this issue.

Restore OpenConsoleProxyStub's dependency on the CRT to fix QI microsoft/terminal
1 participant
@zadjii-msft