Fix logical condition in ComWrappers.TryGetComInterfaceDispatch

[elinor-fung]

No description provided.

[Copilot]

Pull request overview

Fixes an always-true conditional in ComWrappers.TryGetComInterfaceDispatch so the “tagged interface” fallback path is only taken when the COM object’s vtable does not match the known runtime-provided IUnknown / IReferenceTrackerTarget implementations.

Changes:

• Replace knownQI != A || knownQI != B with knownQI != A && knownQI != B to correctly detect “neither matches”.
• Prevent unnecessary QueryInterface(IID_TaggedImpl) + version checks when the vtable is already recognized.

[dotnet-policy-service]

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

[github-actions]

🤖 Copilot Code Review — PR #127241

Note

This review was AI-generated by GitHub Copilot.

Holistic Assessment

Motivation: The || operator in the condition knownQI != A || knownQI != B creates a tautology — it is always true when A != B, rendering the fast-path vtable check in TryGetComInterfaceDispatch completely ineffective. This is a real bug that deserves a fix.

Approach: Changing || to && is the minimal, correct fix. No alternative approach is needed.

Summary: ✅ LGTM. Classic boolean logic bug fix (!=/|| tautology → !=/&&). The fix is verified correct via truth table analysis, has no risk of behavioral regression (the fallback path was always returning the correct result), and no similar patterns exist elsewhere in the codebase. Three independent model reviews concur.

---

Detailed Findings

✅ Logic Fix — Correct (flagged by all 3 models)

Truth table confirms the fix:

knownQI matches	Old (||)	New (&&)	Expected
DefaultIUnknownVftblPtr[0]	true ❌	false ✅	Skip fallback
DefaultIReferenceTrackerTargetVftblPtr[0]	true ❌	false ✅	Skip fallback
Neither	true ✅	true ✅	Enter fallback

The old || form was a tautology (always true for distinct vtable pointers), causing every TryGetComInterfaceDispatch call to unconditionally enter the expensive QueryInterface-based fallback path — even for standard managed object wrappers whose vtable pointers are known.

✅ Impact Assessment — Performance-Only, No Correctness Risk (flagged by all 3 models)

The bug caused unnecessary work on every call across all 3 call sites (TryGetObject at line 108, NativeObjectWrapper constructor at line 615, and Unwrap in TryGetOrCreateObjectForComInstanceInternal at line 1143):

• Unnecessary Marshal.QueryInterface for IID_TaggedImpl
• Unmanaged function pointer call to IsCurrentVersion
• Extra Marshal.Release

Since the fallback path still returned the correct ComInterfaceDispatch*, callers always received correct results. The fix restores the intended fast path — no behavioral change in return values. No caller could have been relying on the buggy behavior.

✅ Cross-Codebase Scan — No Sibling Issues (flagged by all 3 models)

• No other != ... || != ... pattern against these vtable pointers exists in the file or the broader interop codebase.
• TryGetComInterfaceDispatch exists only in the shared ComWrappers.cs (consumed by both CoreCLR and NativeAOT via platform-specific partial classes ComWrappers.CoreCLR.cs and ComWrappers.NativeAot.cs).
• DefaultIUnknownVftblPtr / DefaultIReferenceTrackerTargetVftblPtr are defined in those platform partials but not compared elsewhere.

💡 Regression Test — Optional Follow-Up (flagged by all 3 models)

A test verifying that standard managed wrappers take the fast path (no QueryInterface needed) would guard against reintroduction. However, since this was a performance bug and the fallback is functionally correct, this is low priority and appropriate as a follow-up rather than a blocker for this PR.

---

Models contributing: Claude Opus 4.6 (primary), GPT-5.3-Codex, Claude Sonnet 4.5

Generated by Code Review for issue #127241 · ◷