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

@MichalStrehovsky This PR should fix the contract violations you found.

@MichalStrehovsky This PR should fix the contract violations you found.

Sounds good. We'll only be able to test this once #126517 merges since right now the test doesn't run at all.

🤖 Copilot Code Review — PR #126571

Holistic Assessment

Motivation: This PR addresses contract annotation correctness in COM interop code. It fixes three distinct issues: (1) ClearLPWSTRArray and ClearLPSTRArray call CoTaskMemFree which can trigger managed IMallocSpy callbacks — violating their NOTHROW contract, (2) ComPreStubWorker was annotated THROWS despite the INSTALL_UNWIND_AND_CONTINUE_HANDLER macros catching all exceptions before they escape, and (3) the fForIDispatch parameter on CheckParentComVisibility/CheckParentComVisibilityNoThrow was never read, making it dead code. All three are clearly correct from inspecting the code.

Approach: The approach is appropriate for each change: PERMANENT_CONTRACT_VIOLATION(ThrowsViolation, ReasonRuntimeReentrancy) for the IMallocSpy case follows existing precedent (e.g., eetoprofinterfaceimpl.cpp uses this for profiler callouts, olecontexthelpers.cpp documents the same IMallocSpy concern). BEGIN_CONTRACT_VIOLATION / END_CONTRACT_VIOLATION for ComPreStubWorker matches the pattern used in interoputil.cpp. The fForIDispatch parameter removal and CheckParentComVisibilityNoThrow deletion are straightforward dead-code cleanup.

Summary: ✅ LGTM. All changes are correct, well-motivated, and consistent with established codebase patterns. The dead fForIDispatch parameter was confirmed unused by inspecting the old implementation — CheckParentComVisibilityNoThrow only called HasInvisibleParent() without referencing the parameter. All callers have been updated consistently across comcallablewrapper.cpp, stdinterfaces.cpp, and stubhelpers.cpp.

Detailed Findings

✅ Contract Correctness — ClearLPWSTRArray / ClearLPSTRArray IMallocSpy annotation

The PERMANENT_CONTRACT_VIOLATION(ThrowsViolation, ReasonRuntimeReentrancy) annotation is correctly placed after CONTRACTL_END and before the CoTaskMemFree loop. CoTaskMemFree can trigger managed IMallocSpy callbacks (the same concern documented in olecontexthelpers.cpp:17). The ReasonRuntimeReentrancy reason code is the correct choice, matching how the codebase documents this class of reentrancy (e.g., SafeQueryInterface cited in the enum definition). Both ClearLPWSTRArray and ClearLPSTRArray are correctly annotated.

I verified that ClearBSTRArray (which uses SysFreeString, not CoTaskMemFree) and ClearInterfaceArray (which uses SafeReleasePreemp) are not affected by IMallocSpy and correctly do not need this annotation.

✅ Contract Correctness — ComPreStubWorker NOTHROW contract

The outer function contract change from STATIC_CONTRACT_THROWS to STATIC_CONTRACT_NOTHROW is semantically correct. The INSTALL_UNWIND_AND_CONTINUE_HANDLER macro sets up a PAL_CPP_TRY/PAL_CPP_CATCH block that catches all Exception-derived and std::bad_alloc exceptions. The BEGIN_CONTRACT_VIOLATION(ThrowsViolation) properly annotates the inner region where exceptions may be thrown before being caught by the handler. The comment explaining why the violation is needed is helpful.

✅ Dead Code Removal — CheckParentComVisibilityNoThrow and fForIDispatch

The removed CheckParentComVisibilityNoThrow method was a trivial wrapper that just returned !HasInvisibleParent(). Its THROWS contract annotation was also misleading (named "NoThrow" but contracted as THROWS). The fForIDispatch parameter was accepted by both CheckParentComVisibility and CheckParentComVisibilityNoThrow but never read in either implementation. All six call sites (3 in comcallablewrapper.cpp, 2 in stdinterfaces.cpp, 1 in stubhelpers.cpp) have been updated. The forwarding methods in ComMethodTable struct have also been updated and the NoThrow variant removed.

💡 Observation — #ifdef FEATURE_INTERPRETER early return inside contract violation scope

In ComPreStubWorker, the return pInterpreterTarget; at line 326 exits from inside the BEGIN_CONTRACT_VIOLATION block without reaching END_CONTRACT_VIOLATION. Since ContractViolationHolder (unlike AutoCleanupContractViolationHolder) does not call Leave() in its destructor, the ThrowsViolation mask would leak on the thread in checked/debug builds when this path is taken. However, this early return was pre-existing (before this PR) and already bypassed UNINSTALL_UNWIND_AND_CONTINUE_HANDLER — the contract violation is just one more thing added to the existing set of leaked cleanup on that path. This is not a blocking issue, but worth noting for a potential future cleanup of the interpreter path.

Generated by Code Review for issue #126571 · ◷

@copilot Now that #126517 has been checked in (see #126571 (comment)), please update src/tests/Interop/COM/ExtensionPoints/ExtensionPoints.cs so we can fix #126567. Please also update this PR's description.

@copilot Now that #126517 has been checked in (see #126571 (comment)), please update `src/tests/Interop/COM/Ex...

Updated in e782db0: ExtensionPoints.cs no longer carries the ActiveIssue(126567) skip, so the Validate_Managed_IMallocSpy test is re-enabled now that #126517 is merged. I also refreshed the PR description to mention the test re-enable.