UndockedRegFreeWinRT doesn't check Dynamic packages for WinRT Metadata #1649

DrusTheAxe · 2021-10-26T06:43:48Z

Add PACKAGE_FILTER_STATIC|DYNAMIC in ResolveThirdPartyType() so metadata resolution also doesn't just search static package graph entries (and fail to find metadata in dynamic packages)

Kudos to @huichen123 for spotting the problem

Manually tested by applying privates to the UndockedRegFreeWinRT IntegratedTest (URFW_ExeUsingWinAppSDK.exe) and tracing code flow in the debugger for the various permutations of usage.

closes #2382

…'t just search static package graph entries (and fail to find metadata in dynamic packages)

DrusTheAxe · 2021-10-26T06:43:56Z

/azp run

azure-pipelines · 2021-10-26T06:44:10Z

Azure Pipelines successfully started running 1 pipeline(s).

huichen123 · 2021-10-26T14:49:43Z

        return RO_E_METADATA_NAME_NOT_FOUND;

Then unpackaged WAS app will return RO_E_METADATA_NAME_NOT_FOUND, and then fail in TrueRoGetMetaDataFile.

Refers to: dev/UndockedRegFreeWinRT/typeresolution.cpp:464 in 1f6f5f7. [](commit_id = 1f6f5f7, deletion_comment = False)

huichen123 · 2021-10-26T16:14:20Z

            // For compatibility purposes, if we fail to find the type in the unpackaged location, we should return

I feel we still need to do static check above, but here call GetCurrentPackageInfo(DYNAMIC), and try to find the type in each dependency path.

Refers to: dev/UndockedRegFreeWinRT/typeresolution.cpp:455 in 1f6f5f7. [](commit_id = 1f6f5f7, deletion_comment = False)

EHO-makai

Feels like there is a missing test or tests here?

DrusTheAxe · 2021-10-26T18:01:06Z

Feels like there is a missing test or tests here?

We've got a manual test how we discovered it and will verify.

URFW has zero direct tests here. Known issue on the backlog.

DrusTheAxe · 2021-10-26T18:10:00Z

            // For compatibility purposes, if we fail to find the type in the unpackaged location, we should return
I feel we still need to do static check above, but here call GetCurrentPackageInfo(DYNAMIC), and try to find the type in each dependency path.

@huichen123 Why split the check for Static+Dynamic instead of the 1 call above?

huichen123 · 2021-10-26T18:24:37Z

            // For compatibility purposes, if we fail to find the type in the unpackaged location, we should return
I feel we still need to do static check above, but here call GetCurrentPackageInfo(DYNAMIC), and try to find the type in each dependency path.
@huichen123 Why split the check for Static+Dynamic instead of the 1 call above?

for packaged app, we don't need to handle here.

DrusTheAxe · 2021-10-26T18:34:26Z

            // For compatibility purposes, if we fail to find the type in the unpackaged location, we should return
I feel we still need to do static check above, but here call GetCurrentPackageInfo(DYNAMIC), and try to find the type in each dependency path.
@huichen123
        Hui Chen
        FTE Why split the check for Static+Dynamic instead of the 1 call above?
for packaged app, we don't need to handle here.

Is it harmful or just inefficient?

huichen123 · 2021-10-26T18:38:28Z

            // For compatibility purposes, if we fail to find the type in the unpackaged location, we should return
I feel we still need to do static check above, but here call GetCurrentPackageInfo(DYNAMIC), and try to find the type in each dependency path.
@huichen123
        Hui Chen
        FTE Why split the check for Static+Dynamic instead of the 1 call above?
for packaged app, we don't need to handle here.
Is it harmful or just inefficient?

inefficient? I don't know this code. You'd want to check with the owner. Regardless, it needs to handle unpackaged app with dynamic package graph.

DrusTheAxe · 2021-10-26T18:55:26Z

inefficient? I don't know this code. You'd want to check with the owner.

I ping'd @kythant above. It's a bit of a mixed ownership

needs to handle unpackaged app with dynamic package graph.

Yes, hence this PR.

huichen123 · 2021-10-26T18:59:32Z

inefficient? I don't know this code. You'd want to check with the owner.

I ping'd @kythant above. It's a bit of a mixed ownership

needs to handle unpackaged app with dynamic package graph.

Yes, hence this PR.
This PR doesn't fix the issue. With this fix, the function will return RO_E_METADATA_NAME_NOT_FOUND for unpackaged WAS app, then TrueRoGetMetaDataFile will be used, which won't find for unpackaged app.

DrusTheAxe · 2021-10-26T20:34:59Z

inefficient? I don't know this code. You'd want to check with the owner.

I ping'd @kythant above. It's a bit of a mixed ownership

needs to handle unpackaged app with dynamic package graph.

Yes, hence this PR.
This PR doesn't fix the issue. With this fix, the function will return RO_E_METADATA_NAME_NOT_FOUND for unpackaged WAS app, then TrueRoGetMetaDataFile will be used, which won't find for unpackaged app.

Aha! Yes, sorry, brain fart on my part. Update posted.

DrusTheAxe · 2021-10-26T20:35:18Z

/azp run

…'t just search static package graph entries (and fail to find metadata in dynamic packages)

…red to get the whole package graph

…esolveThirdPartyType

riverar · 2022-05-10T03:58:50Z

dev/UndockedRegFreeWinRT/typeresolution.cpp

-        // Walk the dynamic package graph looking for the requested metadata
-        const uint32_t filter{ PACKAGE_FILTER_HEAD | PACKAGE_FILTER_DIRECT | PACKAGE_FILTER_IS_IN_RELATED_SET | PACKAGE_FILTER_DYNAMIC };
+        // Walk the package graph looking for the requested metadata
+        const uint32_t filter{ PACKAGE_FILTER_HEAD | PACKAGE_FILTER_DIRECT | PACKAGE_FILTER_IS_IN_RELATED_SET | PACKAGE_FILTER_DYNAMIC | PACKAGE_FILTER_STATIC };


This is an undocumented flag, though is in the public SDK.

#define PACKAGE_PROPERTY_STATIC 0x00080000 #define PACKAGE_FILTER_STATIC PACKAGE_PROPERTY_STATIC

Can you shed some light on what it does and/or edit the docs? https://docs.microsoft.com/windows/win32/appxpkg/package-constants

Doc update is coming. In the meantime...

PACKAGE_PROPERTY_STATIC - search the package graph's static entries

PACKAGE_PROPERTY_DYNAMIC - search the package graph's dynamic entries

Packaged apps are born with entries in their package graph, aka static package graph entries, aka the static portion of the package graph or more simply, the static package graph.

Unpackaged apps are born with an empty package graph.

The Dynamic Dependency API adds entries dynamically to the package graph, aka dynamic package graph entries, aka the dynamic portion of the package graph or more simply, the dynamic package graph

For backwards compatibility reasons GetCurrentPackageInfo[2](...filter...) assumes if filter doesn't contain PACKAGE_FILTER_DYNAMIC then only look at the static package graph. IOW calling GetCurrentPackageInfo[2]() without PACKAGE_FILTER_DYNAMIC is implicitly the same as calling GetCurrentPackageInfo2withPACKAGE_FILTER_STATIC`.

This is explained in the comment block in DynamicGetCurrentPackageInfo2()indev\DynamicDependency\API\MddDetourPacckageGraph.cpp`:

// Legacy callers may not be aware of dynamic packages and a common mistake is to assume PACKAGE_FILTER_HEAD will return the // main package as the first entry which is not going to be true when packages have been added dynamically to the head of the graph. // Maintain pre-dynamic behavior by requiring the caller to opt into receiving dynamic packages.

Thus GetCurrentPackageInfo[2]() are dynamic-oblivous unless explicitly told to consider dynamic package graph entries.

OTOH GetCurrentPackageInfo3() assumes the caller is dynamic-savvy so it doesn't make any such backwards compatibility game. If you specify neither option to GetCurrentPackageInfo3() it's equivalent to specifying both (i.e. search static+dynamic entries).

…esolveThirdPartyType

…PartyType' of https://github.com/microsoft/ProjectReunion into user/drustheaxe/URFW-GetCurrentPackageInfo-ResolveThirdPartyType

DrusTheAxe · 2022-05-18T03:46:44Z

/azp run

azure-pipelines · 2022-05-18T03:47:22Z

Azure Pipelines successfully started running 1 pipeline(s).

DrusTheAxe · 2022-05-18T04:34:16Z

/azp run

azure-pipelines · 2022-05-18T04:34:28Z

Azure Pipelines successfully started running 1 pipeline(s).

dev/UndockedRegFreeWinRT/typeresolution.cpp

BenJKuhn

Comments are suggestions / stylistic. Non-blocking, so signing off.

DrusTheAxe · 2022-05-19T00:58:41Z

/azp run

azure-pipelines · 2022-05-19T00:58:52Z

Azure Pipelines successfully started running 1 pipeline(s).

DrusTheAxe · 2022-05-19T01:01:24Z

Feels like there is a missing test or tests here?

Tests exist in the aggregator pipeline

As noted in the description, I manually tested by using those tests to see the repro fail, then updated microsoft.windowsappruntime.dll and watched the test succeed. Plus a lot of interesting walk-the-debugger-through-the-code.

Functional tests will be added here (under /test/UndockedRegFreeWinRT/) after we address some issues in our test infrastructure.

Non-blocking. See comment

#1649) * Added PACKAGE_FILTER_STATIC|DYNAMIC so metadata resolution also doesn't just search static package graph entries (and fail to find metadata in dynamic packages) * Revise ResolveThirdPartyType() to be savvy to dynamic package graph * Incorporated feedback * Added PACKAGE_FILTER_STATIC|DYNAMIC so metadata resolution also doesn't just search static package graph entries (and fail to find metadata in dynamic packages) * Revise ResolveThirdPartyType() to be savvy to dynamic package graph * Incorporated feedback * Add Dynamic+Static filters when retrieving the package graph as required to get the whole package graph * Fix check for not-found-in-package-graph-let's-try-exe-dir * Updated by VS * Incorporated feedback

#1649) (#2537) * Added PACKAGE_FILTER_STATIC|DYNAMIC so metadata resolution also doesn't just search static package graph entries (and fail to find metadata in dynamic packages) * Revise ResolveThirdPartyType() to be savvy to dynamic package graph * Incorporated feedback * Added PACKAGE_FILTER_STATIC|DYNAMIC so metadata resolution also doesn't just search static package graph entries (and fail to find metadata in dynamic packages) * Revise ResolveThirdPartyType() to be savvy to dynamic package graph * Incorporated feedback * Add Dynamic+Static filters when retrieving the package graph as required to get the whole package graph * Fix check for not-found-in-package-graph-let's-try-exe-dir * Updated by VS * Incorporated feedback

Added PACKAGE_FILTER_STATIC|DYNAMIC so metadata resolution also doesn…

1f6f5f7

…'t just search static package graph entries (and fail to find metadata in dynamic packages)

DrusTheAxe added the area-UndockedRegFreeWinRT label Oct 26, 2021

DrusTheAxe added this to the 1.0 (2021 Q4) milestone Oct 26, 2021

DrusTheAxe requested review from BenJKuhn, Scottj1s, MikeHillberg, jonwis, huichen123, kythant, evelynwu-msft and EHO-makai October 26, 2021 06:43

DrusTheAxe self-assigned this Oct 26, 2021

ghost added the needs-triage label Oct 26, 2021

EHO-makai previously requested changes Oct 26, 2021

View reviewed changes

DrusTheAxe requested a review from EHO-makai October 26, 2021 18:01

DrusTheAxe removed the needs-triage label Oct 26, 2021

Revise ResolveThirdPartyType() to be savvy to dynamic package graph

bbea2ff

DrusTheAxe added 3 commits April 21, 2022 12:07

Added PACKAGE_FILTER_STATIC|DYNAMIC so metadata resolution also doesn…

10289f5

…'t just search static package graph entries (and fail to find metadata in dynamic packages)

Revise ResolveThirdPartyType() to be savvy to dynamic package graph

ad4f7fd

Incorporated feedback

c9bc151

DrusTheAxe force-pushed the user/drustheaxe/URFW-GetCurrentPackageInfo-ResolveThirdPartyType branch from dbf83fb to c9bc151 Compare April 21, 2022 19:07

DrusTheAxe added 2 commits May 9, 2022 20:35

Add Dynamic+Static filters when retrieving the package graph as requi…

164583a

…red to get the whole package graph

Merge branch 'main' into user/drustheaxe/URFW-GetCurrentPackageInfo-R…

05d6335

…esolveThirdPartyType

riverar reviewed May 10, 2022

View reviewed changes

DrusTheAxe added 2 commits May 16, 2022 22:26

Merge branch 'main' into user/drustheaxe/URFW-GetCurrentPackageInfo-R…

d358c00

…esolveThirdPartyType

Merge branch 'user/drustheaxe/URFW-GetCurrentPackageInfo-ResolveThird…

2a72543

…PartyType' of https://github.com/microsoft/ProjectReunion into user/drustheaxe/URFW-GetCurrentPackageInfo-ResolveThirdPartyType

Fix check for not-found-in-package-graph-let's-try-exe-dir

69b3480

DrusTheAxe requested review from huichen123 and jonwis May 18, 2022 04:38

BenJKuhn reviewed May 18, 2022

View reviewed changes

dev/UndockedRegFreeWinRT/typeresolution.cpp Show resolved Hide resolved

BenJKuhn reviewed May 18, 2022

View reviewed changes

dev/UndockedRegFreeWinRT/typeresolution.cpp Outdated Show resolved Hide resolved

BenJKuhn approved these changes May 18, 2022

View reviewed changes

DrusTheAxe added 2 commits May 18, 2022 14:10

Updated by VS

4317cac

Incorporated feedback

5a238c2

DrusTheAxe merged commit 2c23172 into main May 19, 2022

DrusTheAxe deleted the user/drustheaxe/URFW-GetCurrentPackageInfo-ResolveThirdPartyType branch May 19, 2022 03:28

DrusTheAxe mentioned this pull request May 20, 2022

UndockedRegFreeWinRT: RoGetMetaDataFile() fix for 1.1-servicing #2537

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UndockedRegFreeWinRT doesn't check Dynamic packages for WinRT Metadata #1649

UndockedRegFreeWinRT doesn't check Dynamic packages for WinRT Metadata #1649

DrusTheAxe commented Oct 26, 2021 •

edited

Loading

DrusTheAxe commented Oct 26, 2021

azure-pipelines bot commented Oct 26, 2021

huichen123 commented Oct 26, 2021

huichen123 commented Oct 26, 2021

EHO-makai left a comment

DrusTheAxe commented Oct 26, 2021

DrusTheAxe commented Oct 26, 2021 •

edited

Loading

huichen123 commented Oct 26, 2021

DrusTheAxe commented Oct 26, 2021

huichen123 commented Oct 26, 2021

DrusTheAxe commented Oct 26, 2021

huichen123 commented Oct 26, 2021

DrusTheAxe commented Oct 26, 2021

DrusTheAxe commented Oct 26, 2021

riverar May 10, 2022

DrusTheAxe May 18, 2022

DrusTheAxe commented May 18, 2022

azure-pipelines bot commented May 18, 2022

DrusTheAxe commented May 18, 2022

azure-pipelines bot commented May 18, 2022

BenJKuhn left a comment

DrusTheAxe commented May 19, 2022

azure-pipelines bot commented May 19, 2022

DrusTheAxe commented May 19, 2022

UndockedRegFreeWinRT doesn't check Dynamic packages for WinRT Metadata #1649

UndockedRegFreeWinRT doesn't check Dynamic packages for WinRT Metadata #1649

Conversation

DrusTheAxe commented Oct 26, 2021 • edited Loading

DrusTheAxe commented Oct 26, 2021

azure-pipelines bot commented Oct 26, 2021

huichen123 commented Oct 26, 2021

huichen123 commented Oct 26, 2021

EHO-makai left a comment

Choose a reason for hiding this comment

DrusTheAxe commented Oct 26, 2021

DrusTheAxe commented Oct 26, 2021 • edited Loading

huichen123 commented Oct 26, 2021

DrusTheAxe commented Oct 26, 2021

huichen123 commented Oct 26, 2021

DrusTheAxe commented Oct 26, 2021

huichen123 commented Oct 26, 2021

DrusTheAxe commented Oct 26, 2021

DrusTheAxe commented Oct 26, 2021

riverar May 10, 2022

Choose a reason for hiding this comment

DrusTheAxe May 18, 2022

Choose a reason for hiding this comment

DrusTheAxe commented May 18, 2022

azure-pipelines bot commented May 18, 2022

DrusTheAxe commented May 18, 2022

azure-pipelines bot commented May 18, 2022

BenJKuhn left a comment

Choose a reason for hiding this comment

DrusTheAxe commented May 19, 2022

azure-pipelines bot commented May 19, 2022

DrusTheAxe commented May 19, 2022

DrusTheAxe commented Oct 26, 2021 •

edited

Loading

DrusTheAxe commented Oct 26, 2021 •

edited

Loading