[release/8.0-rc2] IDispatch should accept HRESULT as valuetype (#…

…92494) * IDispatch should accept HRESULT as valuetype This is a regression from .NET Framework. The current behavior has existed since IDispatch was introduced into .NET Core. Added tests for the current behavior. * Ensure vtables line up for early bound. * Add larger comment to workaround. * Comment --------- Co-authored-by: Aaron R Robinson <arobins@microsoft.com>
dotnet · Sep 23, 2023 · 1e50b05 · 1e50b05
1 parent ee89b7c
commit 1e50b05
Show file tree

Hide file tree

Showing 8 changed files with 67 additions and 3 deletions.
diff --git a/src/coreclr/vm/callsiteinspect.cpp b/src/coreclr/vm/callsiteinspect.cpp
@@ -433,7 +433,8 @@ void CallsiteInspect::PropagateOutParametersBackToCallsite(
                 *(ARG_SLOT *)(frame->GetReturnValuePtr()) = retVal;
             }
 #ifdef ENREGISTERED_RETURNTYPE_MAXSIZE
-            else if (argit.HasNonStandardByvalReturn())
+            else if (argit.HasNonStandardByvalReturn()
+                    && !(flags & CallsiteDetails::HResultReturn))
             {
                 // In these cases, put the pointer to the return buffer into
                 // the frame's return value slot.

diff --git a/src/coreclr/vm/callsiteinspect.h b/src/coreclr/vm/callsiteinspect.h
@@ -25,6 +25,7 @@ struct CallsiteDetails
         BeginInvoke     = 0x01,
         EndInvoke       = 0x02,
         Ctor            = 0x04,
+        HResultReturn   = 0x08,
     };
     INT32 Flags;
 };

diff --git a/src/coreclr/vm/clrtocomcall.cpp b/src/coreclr/vm/clrtocomcall.cpp
@@ -364,7 +364,7 @@ UINT32 CLRToCOMEventCallWorker(ComPlusMethodFrame* pFrame, ComPlusCallMethodDesc
     return 0;
 }
 
-CallsiteDetails CreateCallsiteDetails(_In_ FramedMethodFrame *pFrame)
+static CallsiteDetails CreateCallsiteDetails(_In_ FramedMethodFrame *pFrame)
 {
     CONTRACTL
     {
@@ -442,10 +442,20 @@ CallsiteDetails CreateCallsiteDetails(_In_ FramedMethodFrame *pFrame)
         SigTypeContext::InitTypeContext(pMD, actualType, &typeContext);
     }
 
+    // If the signature is marked preserve sig, then the return
+    // is required to be an HRESULT, per COM rules. We set a flag to
+    // indicate this state to avoid issues when a C# developer defines
+    // an HRESULT in C# as a ValueClass with a single int field. This
+    // is convenient but does violate the COM ABI. Setting the flag
+    // lets us permit this convention and allow either a 4 byte primitive
+    // or the commonly used C# type "struct HResult { int Value; }".
+    if (IsMiPreserveSig(pMD->GetImplAttrs()))
+        callsiteFlags |= CallsiteDetails::HResultReturn;
+
     _ASSERTE(!signature.IsEmpty() && pModule != nullptr);
 
     // Create details
-    return CallsiteDetails{ { signature, pModule, &typeContext }, pFrame, pMD, fIsDelegate };
+    return CallsiteDetails{ { signature, pModule, &typeContext }, pFrame, pMD, fIsDelegate, callsiteFlags };
 }
 
 UINT32 CLRToCOMLateBoundWorker(

diff --git a/src/tests/Interop/COM/NETClients/IDispatch/Program.cs b/src/tests/Interop/COM/NETClients/IDispatch/Program.cs
@@ -139,6 +139,21 @@ static void Validate_Exception()
                 Assert.Equal(GetErrorCodeFromHResult(e.HResult), errorCode);
                 // Failing HRESULT exceptions contain CLR generated messages
             }
+
+            // Calling methods through IDispatch::Invoke() (i.e., late-bound) doesn't
+            // propagate the HRESULT when marked with PreserveSig. It is always 0.
+            {
+                Console.WriteLine($"Calling {nameof(DispatchTesting.TriggerException)} (PreserveSig) with {nameof(IDispatchTesting_Exception.Int)} {errorCode}...");
+                var dispatchTesting2 = (IDispatchTestingPreserveSig1)dispatchTesting;
+                Assert.Equal(0, dispatchTesting2.TriggerException(IDispatchTesting_Exception.Int, errorCode));
+            }
+
+            {
+                // Validate the HRESULT as a value type construct works for IDispatch.
+                Console.WriteLine($"Calling {nameof(DispatchTesting.TriggerException)} (PreserveSig, ValueType) with {nameof(IDispatchTesting_Exception.Int)} {errorCode}...");
+                var dispatchTesting3 = (IDispatchTestingPreserveSig2)dispatchTesting;
+                Assert.Equal(0, dispatchTesting3.TriggerException(IDispatchTesting_Exception.Int, errorCode).Value);
+            }
         }
 
         static void Validate_StructNotSupported()

diff --git a/src/tests/Interop/COM/NETServer/DispatchTesting.cs b/src/tests/Interop/COM/NETServer/DispatchTesting.cs
@@ -57,6 +57,7 @@ public void TriggerException(IDispatchTesting_Exception excep, int errorCode)
         case IDispatchTesting_Exception.Disp:
             throw new Exception();
         case IDispatchTesting_Exception.HResult:
+        case IDispatchTesting_Exception.Int:
             throw new System.ComponentModel.Win32Exception(errorCode);
         }
     }

diff --git a/src/tests/Interop/COM/NativeServer/DispatchTesting.h b/src/tests/Interop/COM/NativeServer/DispatchTesting.h
@@ -243,6 +243,8 @@ class DispatchTesting : public UnknownImpl, public IDispatchTesting
             return DISP_E_EXCEPTION;
         case IDispatchTesting_Exception_HResult:
             return HRESULT_FROM_WIN32(errorCode);
+        case IDispatchTesting_Exception_Int:
+            return errorCode;
         default:
             return S_FALSE; // Return a success case to indicate failure to trigger a failure.
         }

diff --git a/src/tests/Interop/COM/ServerContracts/Server.Contracts.cs b/src/tests/Interop/COM/ServerContracts/Server.Contracts.cs
@@ -209,6 +209,7 @@ public enum IDispatchTesting_Exception
     {
         Disp,
         HResult,
+        Int,
     }
 
     [StructLayout(LayoutKind.Sequential)]
@@ -220,6 +221,12 @@ public struct HFA_4
         public float w;
     }
 
+    [StructLayout(LayoutKind.Sequential)]
+    public struct HRESULT
+    {
+        public int Value;
+    }
+
     [ComVisible(true)]
     [Guid("a5e04c1c-474e-46d2-bbc0-769d04e12b54")]
     [InterfaceType(ComInterfaceType.InterfaceIsIDispatch)]
@@ -257,6 +264,32 @@ void DoubleNumeric_ReturnByRef (
         System.Collections.IEnumerator GetEnumerator();
     }
 
+    [ComVisible(true)]
+    [Guid("a5e04c1c-474e-46d2-bbc0-769d04e12b54")]
+    [InterfaceType(ComInterfaceType.InterfaceIsIDispatch)]
+    public interface IDispatchTestingPreserveSig1
+    {
+        void Reserved1();
+        void Reserved2();
+        void Reserved3();
+
+        [PreserveSig]
+        int TriggerException(IDispatchTesting_Exception excep, int errorCode);
+    }
+
+    [ComVisible(true)]
+    [Guid("a5e04c1c-474e-46d2-bbc0-769d04e12b54")]
+    [InterfaceType(ComInterfaceType.InterfaceIsIDispatch)]
+    public interface IDispatchTestingPreserveSig2
+    {
+        void Reserved1();
+        void Reserved2();
+        void Reserved3();
+
+        [PreserveSig]
+        HRESULT TriggerException(IDispatchTesting_Exception excep, int errorCode);
+    }
+
     [ComVisible(true)]
     [Guid("83AFF8E4-C46A-45DB-9D91-2ADB5164545E")]
     [InterfaceType(ComInterfaceType.InterfaceIsIDispatch)]

diff --git a/src/tests/Interop/COM/ServerContracts/Server.Contracts.h b/src/tests/Interop/COM/ServerContracts/Server.Contracts.h
@@ -385,6 +385,7 @@ enum IDispatchTesting_Exception
 {
     IDispatchTesting_Exception_Disp,
     IDispatchTesting_Exception_HResult,
+    IDispatchTesting_Exception_Int,
 };
 
 struct __declspec(uuid("a5e04c1c-474e-46d2-bbc0-769d04e12b54"))