cDAC: Remove ad-hoc StrategyBasedComWrappers instantiation

[max-charlamb]

Note

This PR was AI/Copilot-generated.

Summary

Removes all direct StrategyBasedComWrappers instantiation from the cDAC, fixing a bug in ClrDataAppDomain.IsSameObject and improving the entrypoint code to use the standard ComInterfaceMarshaller<T> API.

Changes

Bug fix: ClrDataAppDomain.IsSameObject (always returned S_FALSE)

The previous code created a new StrategyBasedComWrappers() and called GetOrCreateObjectForComInstance with CreateObjectFlags.None. This always creates a new RCW (a ComObject wrapper) — it never unwraps CCWs back to their original managed object. So the obj is ClrDataAppDomain check always failed, and IsSameObject always returned S_FALSE.

Fix: Use ComWrappers.TryGetObject(nint, out object?) instead. This is a static method that correctly unwraps any CCW (from any ComWrappers instance) back to its original managed object, making the identity comparison work as intended.

Cleanup: Entrypoints.cs — use ComInterfaceMarshaller<T>

The three entrypoint functions (CreateSosInterface, CreateDacDbiInterface, CLRDataCreateInstanceImpl) each created a new StrategyBasedComWrappers() per call. These are replaced with ComInterfaceMarshaller<T> calls, which internally use StrategyBasedComWrappers.DefaultMarshallingInstance — the same process-wide singleton used by all source-generated COM interop code. This ensures cache coherence and eliminates the need to instantiate StrategyBasedComWrappers directly.

Testing

• All 1691 cDAC unit tests pass
• Dump tests require a full runtime build (clr+libs) which was not available in this worktree; the changes are limited to COM interop plumbing and do not affect contract logic

[Copilot]

Pull request overview

Removes per-call StrategyBasedComWrappers allocations in cDAC COM entrypoints and fixes COM identity comparison for ClrDataAppDomain.IsSameObject by unwrapping CCWs to their underlying managed objects.

Changes:

• Reuse a single static StrategyBasedComWrappers instance in cDAC entrypoints instead of allocating one per call.
• Fix ClrDataAppDomain.IsSameObject to use ComWrappers.TryGetObject (and add a null check) so identity comparisons work for CCWs.

Reviewed changes

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

File	Description
src/native/managed/cdac/mscordaccore_universal/Entrypoints.cs	Replaces per-entrypoint StrategyBasedComWrappers instantiation with a shared static instance for COM wrapping/unwrapping.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataAppDomain.cs	Fixes IsSameObject to correctly unwrap CCWs (and handle null input) so it no longer always returns S_FALSE.

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

src/native/managed/cdac/mscordaccore_universal/Entrypoints.cs:120

• ComInterfaceMarshaller<IDacDbiInterface>.ConvertToManaged will throw (e.g., InvalidCastException) if legacyImplPtr is not actually an IDacDbiInterface pointer. Since this method is [UnmanagedCallersOnly] and has no try/catch, that exception would tear down the process, and the subsequent is not Legacy.IDacDbiInterface check will never be reached. Prefer validating via Marshal.QueryInterface for the IDacDbiInterface IID (return E_NOINTERFACE/COR_E_INVALIDCAST on failure) and only then calling ConvertToManaged, or wrap the conversion in a try/catch and translate failures to an HRESULT.

        if (legacyImplPtr != IntPtr.Zero)
        {
            legacyObj = ComInterfaceMarshaller<IDacDbiInterface>.ConvertToManaged((void*)legacyImplPtr);
            if (legacyObj is not Legacy.IDacDbiInterface)
            {
                *obj = IntPtr.Zero;
                return HResults.COR_E_INVALIDCAST; // E_NOINTERFACE
            }

[dotnet-policy-service]

Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag
See info in area-owners.md if you want to be subscribed.

[github-actions]

🤖 Copilot Code Review — PR #127113

Note

This review was AI/Copilot-generated (claude-opus-4.6 primary, with gpt-5.3-codex and goldeneye sub-agent reviews synthesized).

Holistic Assessment

Motivation: The PR fixes a real bug in ClrDataAppDomain.IsSameObject (which always returned S_FALSE due to incorrect CCW handling) and removes ad-hoc StrategyBasedComWrappers instantiation in favor of the standard ComInterfaceMarshaller<T> API. Both goals are well-justified.

Approach: Using ComWrappers.TryGetObject for identity comparison and ComInterfaceMarshaller<T> for the entrypoints are the correct APIs for this codebase. The changes align with how source-generated COM interop is designed to work.

Summary: ⚠️ Needs Human Review. The IsSameObject fix is a clear correctness improvement. The Entrypoints.cs cleanup is sound in the common case but introduces a subtle behavioral regression in one error path (CreateDacDbiInterface) that two independent model reviews flagged. A human reviewer should evaluate whether this edge case matters in practice.

---

Detailed Findings

✅ ClrDataAppDomain.IsSameObject — Correct bug fix

The old code created new StrategyBasedComWrappers() and called GetOrCreateObjectForComInstance with CreateObjectFlags.None. A fresh ComWrappers instance with None (no unwrap) always creates a new RCW (ComObject), never returning the original ClrDataAppDomain. The obj is ClrDataAppDomain check always failed, so IsSameObject always returned S_FALSE.

ComWrappers.TryGetObject is the right fix — it's a static method that checks if a COM pointer is a CCW from any ComWrappers instance and returns the underlying managed object. Verified that it returns false for null/zero pointers (safe behavior).

The #if DEBUG validation block (_legacyImpl.IsSameObject(appDomain)) would have been firing debug asserts whenever the native DAC returned S_OK but the cDAC returned S_FALSE — this fix resolves that.

✅ Entrypoints.cs — Singleton DefaultMarshallingInstance is correct

ComInterfaceMarshaller<T> internally uses StrategyBasedComWrappers.DefaultMarshallingInstance — the process-wide singleton. This provides cache coherence with the generated COM interop code and avoids creating redundant wrappers. The ConvertToUnmanaged calls return interface-specific pointers (via CastIUnknownToInterfaceType → QI), but since all COM interfaces inherit from IUnknown (vtable slots 0/1/2 = QI/AddRef/Release), the native callers — which expect IUnknown** per cdac_reader.h:34 — can safely QI from the returned pointer. Verified against the native header and C++ call sites in cdac.cpp.

⚠️ CreateDacDbiInterface — Defensive error path now throws instead of returning HRESULT (flagged by 2/2 sub-agents)

Entrypoints.cs:115: ComInterfaceMarshaller<IDacDbiInterface>.ConvertToManaged internally hard-casts (T) the result (ComInterfaceMarshaller.cs:57). Since ComObject implements IDynamicInterfaceCastable, this triggers a QI for IDacDbiInterface. If the COM pointer doesn't support it, InvalidCastException is thrown.

The old code returned a ComObject as object and then checked legacyObj is not Legacy.IDacDbiInterface gracefully. The new code makes that check dead code — ConvertToManaged would throw first, and since CreateDacDbiInterface is [UnmanagedCallersOnly] with no try-catch, that exception is process-fatal.

Practical impact: Low — the caller is expected to always pass a valid IDacDbiInterface pointer (per cdac_reader.h:40). But the old code was intentionally defensive. A human reviewer should decide whether this edge case warrants preserving the graceful HRESULT return path.

💡 CLRDataCreateInstanceImpl — Pre-existing: Marshal.QueryInterface HRESULT unchecked (not a regression)

Entrypoints.cs:197: Marshal.QueryInterface((nint)ccw, *pIID, out nint ptrToIface) — the return value is not checked. If QI fails, ptrToIface is 0, *iface is null, and the method returns 0 (success). This was identical in the old code. Not introduced by this PR, but worth noting as a follow-up.

💡 Minor: ConvertToUnmanaged returns specific interface pointer, not IUnknown

For CreateSosInterface and CreateDacDbiInterface, ConvertToUnmanaged<T> now returns T* (e.g., ISOSDacInterface*) rather than the old IUnknown* from GetOrCreateComInterfaceForObject. This is semantically a stronger type (more specific interface) but is a valid IUnknown* in the COM model. No behavioral issue, just a difference worth noting for future readers.

---

Models contributing: claude-opus-4.6 (primary), gpt-5.3-codex (sub-agent), goldeneye (sub-agent). Both sub-agents independently flagged the CreateDacDbiInterface error-path regression.

Generated by Code Review for issue #127113 · ◷