Skip to content

Unwrap RCWs that are passed to Marshal.GetIUnknownForObject when using the global marshalling ComWrappers instance #115436

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.

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jkoritzinsky
Copy link
Member

Fixes issue reported in #115317

Depends on #113907

@Copilot Copilot AI review requested due to automatic review settings May 9, 2025 22:08
@jkoritzinsky jkoritzinsky added area-System.Runtime.InteropServices blocked Issue/PR is blocked on something - see comments labels May 9, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This pull request implements changes to support unwrapping RCWs when passed to Marshal.GetIUnknownForObject under the global marshalling ComWrappers instance. Key changes include refactoring of TrackerObjectManager methods, updates to COM‐aware weak reference handling in both native and CoreCLR code, and enhancements to GC collection APIs to support new parameters such as lowMemoryPressure.

Reviewed Changes

Copilot reviewed 60 out of 60 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
TrackerObjectManager.NativeAot.cs Updated to use partial methods and new patterns for registering and tracking external COM objects.
ComAwareWeakReference.CoreCLR.cs Adjusted signature and logic to differentiate between built-in COM and ComWrappers RCWs.
GC.NativeAot.cs Extended GC.Collect overloads to include a lowMemoryPressure parameter.
interop/inc/interoplibimports.h Modified the IteratorNext API signature from HRESULT to bool.
System.Private.CoreLib.csproj Updated project file to include new CoreCLR sources for ComAwareWeakReference and TrackerObjectManager.
(Several other interop and debug files) Various changes to align unmanaged COM interop logic with the new unwrapping behavior.
Comments suppressed due to low confidence (3)

src/coreclr/interop/inc/interoplibimports.h:25

  • Ensure that changing the return type of IteratorNext from HRESULT to bool is thoroughly validated for backward compatibility across all interop consumers.
bool IteratorNext(_In_ RuntimeCallContext* runtimeContext, _Outptr_result_maybenull_ void** trackerTarget, _Outptr_result_maybenull_ InteropLib::OBJECTHANDLE* proxyObject) noexcept;

src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/TrackerObjectManager.NativeAot.cs:94

  • Ensure that the new AddReferencePath method is consistently used across the interop code for proper reference tracking and verify its thread-safety in concurrent scenarios.
public static bool AddReferencePath(object target, object foundReference)

src/coreclr/nativeaot/System.Private.CoreLib/src/System/GC.NativeAot.cs:145

  • Ensure that the new lowMemoryPressure parameter is correctly propagated to RuntimeImports.RhCollect and that any downstream logic handles this flag as expected to avoid unexpected GC behavior.
Collect(generation, mode, blocking, compacting, lowMemoryPressure: false);

Copy link
Contributor

Tagging subscribers to this area: @dotnet/interop-contrib
See info in area-owners.md if you want to be subscribed.

@jkoritzinsky jkoritzinsky force-pushed the global-marshalling-unwrap branch from d197c7c to 7e860ac Compare May 10, 2025 23:06
@jkoritzinsky jkoritzinsky requested a review from Copilot May 10, 2025 23:10
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR updates the marshalling behavior for RCWs by unwrapping COM objects passed to Marshal.GetIUnknownForObject when using the global marshalling ComWrappers instance. Key changes include updating test assertions to use Assert.Same for reference equality, modifying the global marshalling logic in ComWrappers.cs to favor an existing COM instance if available, and adjusting the behavior in ComWrappers.CoreCLR.cs for obtaining COM interfaces and objects.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
src/tests/Interop/COM/ComWrappers/GlobalInstance/GlobalInstance.cs Updates test assertions to check for reference equality and adds a new IID_IUNKNOWN constant.
src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.cs Adds a conditional for trying to obtain an existing COM instance before creating one, and propagates CreateObjectFlags via the API.
src/coreclr/System.Private/CoreLib/src/System/Runtime/InteropServices/ComWrappers.CoreCLR.cs Adjusts global instance handling to short-circuit when the global instance is null and delegates to the updated methods in ComWrappers.cs.
Comments suppressed due to low confidence (1)

src/coreclr/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.CoreCLR.cs:80

  • When s_globalInstanceForMarshalling is null, consider returning null instead of IntPtr.Zero to better match the expected object return type and maintain consistency with similar methods.
if (s_globalInstanceForMarshalling == null)

@jkoritzinsky jkoritzinsky removed the blocked Issue/PR is blocked on something - see comments label May 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants