Fix Windows x86 exception handling issue (#4830)

This change fixes an exception handling issue that happens on x86 on Windows
when exception is raised in System.RuntimeType.MakeGenericType.
The problem was caused by GCPROTECT_HOLDER macro in RuntimeTypeHandle::GetTypeByName
that causes popping of GCFrame (and zeroing its m_next field) that's in the middle of the thread's frames list during
the stack unwinding.
That breaks the list and when UnwindFrames happen later and tries to walk the stack, the StackFrameIterator::NextRaw
asserts when checking the validity of the list.
The fix is to move the keepAlive to the managed caller of the RuntimeTypeHandle::GetTypeByName QCall, which removes
the need for the GCPROTECT_HOLDER.
Since it was the only usage of that holder and of the underlying FrameWithCookieHolder class, I've removed those.
In addition to that, I've modified COMModule::GetType and AssemblyNative::GetType to use the same pattern, since
they could also suffer from the problem the GCPROTECT_HOLDER was attempting to fix.

1 files changed, 11 insertions, 23 deletions

diff --git a/src/vm/runtimehandles.cpp b/src/vm/runtimehandles.cpp
index d0f36cb95c..601a8bc8f9 100644
--- a/src/vm/runtimehandles.cpp
+++ b/src/vm/runtimehandles.cpp
@@ -1883,7 +1883,8 @@ void QCALLTYPE RuntimeTypeHandle::GetTypeByName(LPCWSTR pwzClassName, BOOL bThro
 #ifdef FEATURE_HOSTED_BINDER
                                                 ICLRPrivBinder * pPrivHostBinder,
 #endif
-                                                BOOL bLoadTypeFromPartialNameHack, QCall::ObjectHandleOnStack retType)
+                                                BOOL bLoadTypeFromPartialNameHack, QCall::ObjectHandleOnStack retType,
+                                                QCall::ObjectHandleOnStack keepAlive)
 {
     QCALL_CONTRACT;
     
@@ -1894,32 +1895,19 @@ void QCALLTYPE RuntimeTypeHandle::GetTypeByName(LPCWSTR pwzClassName, BOOL bThro
     if (!pwzClassName)
             COMPlusThrowArgumentNull(W("className"),W("ArgumentNull_String"));
 
-    GCX_COOP();
     {
-        OBJECTREF keepAlive = NULL;
-
-        // BEGIN_QCALL/END_QCALL define try/catch scopes for potential exceptions thrown when bThrowOnError is enabled.
-        // Originally, in case of an exception the GCFrame was removed from the Thread's Frame chain in the catch block, in UnwindAndContinueRethrowHelperInsideCatch.
-        // However, the catch block declared some local variables that overlapped the location of the now out of scope GCFrame and OBJECTREF, therefore corrupting
-        // those values. Having the GCX_COOP/GCX_PREEMP switching GC modes, allowed a situation where in case of an exception, the thread would wait for a GC to complete
-        // while still having the GCFrame in the Thread's Frame chain, but with a corrupt OBJECTREF due to stack location reuse in the catch block.
-        // The solution is to force the removal of GCFrame (and the Frames above) from the Thread's Frame chain before entering the catch block, at the time of 
-        // FrameWithCookieHolder's destruction.
-        GCPROTECT_HOLDER(keepAlive);
-
-        {
-            GCX_PREEMP();
-            typeHandle = TypeName::GetTypeManaged(pwzClassName, NULL, bThrowOnError, bIgnoreCase, bReflectionOnly, /*bProhibitAsmQualifiedName =*/ FALSE, pStackMark, bLoadTypeFromPartialNameHack, &keepAlive
+        typeHandle = TypeName::GetTypeManaged(pwzClassName, NULL, bThrowOnError, bIgnoreCase, bReflectionOnly, /*bProhibitAsmQualifiedName =*/ FALSE, pStackMark,
+                                              bLoadTypeFromPartialNameHack, (OBJECTREF*)keepAlive.m_ppObject
 #ifdef FEATURE_HOSTED_BINDER
-                                                  , pPrivHostBinder
+                                              , pPrivHostBinder
 #endif
-                );
-        }
+            );
+    }
 
-        if (!typeHandle.IsNull())
-        {
-            retType.Set(typeHandle.GetManagedClassObject());
-        }
+    if (!typeHandle.IsNull())
+    {
+        GCX_COOP();
+        retType.Set(typeHandle.GetManagedClassObject());
     }
 
     END_QCALL;