Code Review: PR #123986

Holistic Assessment

Motivation: This PR removes MethodDescCallSite usage in COM interop exception handling code in comutilnative.cpp, replacing it with direct calls via UnmanagedCallersOnly methods. This is part of a broader effort (#123864) to simplify and modernize the native/managed interop calling pattern.

Approach: The PR introduces three new [UnmanagedCallersOnly] static methods in Exception.CoreCLR.cs that handle BSTR allocation directly in managed code, and adds a new template method InvokeThrowing_Ret<T> to UnmanagedCallersOnlyCaller for methods that return values.

Net positive: ✅ Yes - This is a good simplification that reduces complexity in the native/managed boundary, removes the now-unused BStrFromString helper, and follows the established pattern from the UnmanagedCallersOnlyCaller class.

Detailed Findings

✅ Correctness

1. Proper exception propagation: The new managed methods correctly catch exceptions and store them in the out parameter, matching the expected pattern for UnmanagedCallersOnlyCaller.InvokeThrowing_Ret.

2. BSTR allocation behavior preserved: The new code uses Interop.OleAut32.SysAllocStringLen directly from managed code. The semantics are preserved - returns IntPtr.Zero for null/empty strings where appropriate.

3. Method signatures match: The corelib.h bindings correctly map to the new method signatures, and metasig.h has proper signature definitions.

⚠️ Potential Issues

1. OOM handling difference (Line 143 in comutilnative.cpp):

   • Old code: BStrFromString explicitly threw COMPlusThrowOM() when SysAllocString returned NULL.
   • New code: SysAllocStringLen returning IntPtr.Zero (on OOM) will propagate up as a null BSTR without throwing.
   • Impact: This changes error behavior - instead of throwing OutOfMemoryException, callers may receive NULL BSTRs. The COM error handling path in GetExceptionData may not expect this.
   • Recommendation: Consider checking the return value and throwing OutOfMemoryException when SysAllocStringLen returns IntPtr.Zero for non-null input strings, OR verify that downstream consumers handle null BSTRs correctly.

2. GetHelpContextBstr does not initialize *bstr (Lines 328-342 in Exception.CoreCLR.cs):

   • If an exception is thrown before *bstr is assigned, the output pointer remains uninitialized.
   • The other two methods return IntPtr.Zero in the catch block, but this method doesn't assign anything on exception.
   • Recommendation: Initialize *bstr = IntPtr.Zero; at the start of the method, or assign it in the catch block.

💡 Suggestions

1. Documentation for InvokeThrowing_Ret: Consider adding a brief comment in callhelpers.h explaining that this variant is for methods returning values (similar to the existing class-level comment for InvokeThrowing).

2. Consistency: The InvokeThrowing_Ret template duplicates the sanity check and OVERRIDE_TYPE_LOAD_LEVEL_LIMIT logic from InvokeThrowing. Consider whether these could share a common helper to reduce duplication.

Cross-Cutting Analysis

• FEATURE_COMINTEROP guards: Correctly applied to new managed code and method bindings.
• Old method bindings removed: The old METHOD__EXCEPTION__GET_MESSAGE, METHOD__EXCEPTION__GET_CLASS_NAME, METHOD__EXCEPTION__GET_SOURCE, and METHOD__EXCEPTION__GET_HELP_CONTEXT bindings are correctly removed from corelib.h.
• GetHelpContext visibility change: The method was changed from internal to private, which is correct since it's now only called from within the class.

Test Coverage

No new tests are added. The COM interop exception handling path is typically exercised through existing COM interop tests. Recommend verifying:

1. COM exception description/source/help retrieval still works correctly
2. OOM scenarios don't cause regressions (if the OOM handling change above is intentional)

Summary

Verdict: ⚠️ Needs Minor Changes

The overall approach is sound and follows established patterns. Two items need attention:

1. OOM handling: The loss of explicit COMPlusThrowOM() for BSTR allocation failures should be intentional or addressed.
2. Uninitialized pointer: GetHelpContextBstr should initialize *bstr to IntPtr.Zero before the try block.

After addressing these, the PR is ready to merge.