Avoid using a spinlock when WeakReference finalization is called by t…

…he GC (dotnet#2474)
realmar · Feb 7, 2020 · a98abf4 · a98abf4
1 parent e1ddf93
commit a98abf4
Show file tree

Hide file tree

Showing 3 changed files with 14 additions and 3 deletions.
diff --git a/src/coreclr/src/vm/threadsuspend.cpp b/src/coreclr/src/vm/threadsuspend.cpp
@@ -23,6 +23,8 @@
 
 bool ThreadSuspend::s_fSuspendRuntimeInProgress = false;
 
+bool ThreadSuspend::s_fSuspended = false;
+
 CLREvent* ThreadSuspend::g_pGCSuspendEvent = NULL;
 
 ThreadSuspend::SUSPEND_REASON ThreadSuspend::m_suspendReason;
@@ -6227,6 +6229,7 @@ void Thread::UnmarkForSuspension(ULONG mask)
 
 void ThreadSuspend::RestartEE(BOOL bFinishedGC, BOOL SuspendSucceded)
 {
+    ThreadSuspend::s_fSuspended = false;
 #ifdef TIME_SUSPEND
     g_SuspendStatistics.StartRestart();
 #endif //TIME_SUSPEND
@@ -6589,6 +6592,7 @@ void ThreadSuspend::SuspendEE(SUSPEND_REASON reason)
 #ifdef TIME_SUSPEND
     g_SuspendStatistics.EndSuspend(reason == SUSPEND_FOR_GC || reason == SUSPEND_FOR_GC_PREP);
 #endif //TIME_SUSPEND
+    ThreadSuspend::s_fSuspended = true;
 }
 
 #if defined(FEATURE_HIJACK) && defined(TARGET_UNIX)

diff --git a/src/coreclr/src/vm/threadsuspend.h b/src/coreclr/src/vm/threadsuspend.h
@@ -192,10 +192,9 @@ class ThreadSuspend
                                           // that the runtime was suspended
     static Thread* m_pThreadAttemptingSuspendForGC;
 
-public:
     static HRESULT SuspendRuntime(ThreadSuspend::SUSPEND_REASON reason);
     static void    ResumeRuntime(BOOL bFinishedGC, BOOL SuspendSucceded);
-
+public:
     // Initialize thread suspension support
     static void    Initialize();
 
@@ -229,13 +228,16 @@ class ThreadSuspend
     // But there won't be any corruption or AV.  More details on the profiler API scenario in VsWhidbey bug 454936.
     static bool     s_fSuspendRuntimeInProgress;
 
+    static bool     s_fSuspended;
+
     static void SetSuspendRuntimeInProgress();
     static void ResetSuspendRuntimeInProgress();
 
     typedef StateHolder<ThreadSuspend::SetSuspendRuntimeInProgress, ThreadSuspend::ResetSuspendRuntimeInProgress> SuspendRuntimeInProgressHolder;
 
 public:
     static bool SysIsSuspendInProgress() { return s_fSuspendRuntimeInProgress; }
+    static bool SysIsSuspended() { return s_fSuspended; }
 
 public:
     //suspend all threads

diff --git a/src/coreclr/src/vm/weakreferencenative.cpp b/src/coreclr/src/vm/weakreferencenative.cpp
@@ -15,6 +15,7 @@
 #include "weakreferencenative.h"
 #include "typestring.h"
 #include "typeparse.h"
+#include "threadsuspend.h"
 
 //************************************************************************
 
@@ -495,7 +496,8 @@ void FinalizeWeakReference(Object * obj)
 
     WEAKREFERENCEREF pThis((WeakReferenceObject *)(obj));
 
-    OBJECTHANDLE handle = AcquireWeakHandleSpinLock(pThis);
+    // The suspension state of the runtime must be prevented from changing while in this function in order for this to be safe.
+    OBJECTHANDLE handle = ThreadSuspend::SysIsSuspended() ? pThis->m_Handle.LoadWithoutBarrier() : AcquireWeakHandleSpinLock(pThis);
     OBJECTHANDLE handleToDestroy = NULL;
     bool isWeakWinRTHandle = false;
 
@@ -518,6 +520,9 @@ void FinalizeWeakReference(Object * obj)
     }
 
     // Release the spin lock
+    // This is necessary even when the spin lock is not acquired
+    // (i.e. When ThreadSuspend::SysIsSuspended() == true)
+    // so that the new handle value is set.
     ReleaseWeakHandleSpinLock(pThis, handle);
 
     if (handleToDestroy != NULL)