Fix GC background thread start in OOM (#5840)

This change fixes a problem that happened when the `gc_heap::create_bgc_thread`
calls SetupUnstartedThread and it fails to allocate a Thread object and throws.
The GC doesn't expect that and so when the stack is unwound, the `gc_heap::gc_started`
stays set. Now we try to allocate memory for the Throwable for the exception, so we go
to GC heap and since there is not enough space, we end up calling
`gc_heap::try_allocate_more_space`, which calls `gc_heap::wait_for_gc_done`.
And that’s the end of it, since the `gc_started` is still set and we wait forever.

The fix is to catch exception from the SetupUnstartedThread. I have also fixed couple of
places where this method was being called and the exception was not expected.

As an additional cleanup, I have moved the thread creation in GC to the GCToEEInterface.

Author: janvorli

src/gc/env/gcenv.ee.h

@@ -21,6 +21,8 @@ typedef struct
     CrawlFrame *   cf;
 } GCCONTEXT;
 
+// GC background thread function prototype
+typedef uint32_t (__stdcall *GCBackgroundThreadFunction)(void* param);
 
 class GCToEEInterface
 {
@@ -78,7 +80,7 @@ class GCToEEInterface
 
     static void GcEnumAllocContexts(enum_alloc_context_func* fn, void* param);
 
-    static void AttachCurrentThread(); // does not acquire thread store lock
+    static bool CreateBackgroundThread(Thread** thread, GCBackgroundThreadFunction threadStart, void* arg);
 };
 
 #endif // __GCENV_EE_H__

src/gc/gc.cpp

@@ -26724,55 +26724,11 @@ BOOL gc_heap::prepare_bgc_thread(gc_heap* gh)
 
 BOOL gc_heap::create_bgc_thread(gc_heap* gh)
 {
-    BOOL ret = FALSE;
-
     assert (background_gc_done_event.IsValid());
 
     //dprintf (2, ("Creating BGC thread"));
 
-#ifdef FEATURE_REDHAWK
-
-    // Thread creation is handled a little differently in Redhawk. We create the thread by a call to the OS
-    // (via the PAL) and it starts running immediately. We place a wrapper around the start routine to
-    // initialize the Redhawk Thread context (since that must be done from the thread itself) and also destroy
-    // it as the thread exits. We also set gh->bgc_thread from this wrapper since otherwise we'd have to
-    // search the thread store for one with the matching ID. gc->bgc_thread will be valid by the time we've
-    // finished the event wait below.
-
-    rh_bgc_thread_ctx sContext;
-    sContext.m_pRealStartRoutine = (PTHREAD_START_ROUTINE)gh->bgc_thread_stub;
-    sContext.m_pRealContext = gh;
-
-    if (!PalStartBackgroundGCThread(gh->rh_bgc_thread_stub, &sContext))
-    {
-        goto cleanup;
-    }
-
-#else // FEATURE_REDHAWK
-
-    Thread* current_bgc_thread;
-
-    gh->bgc_thread = SetupUnstartedThread(FALSE);
-    if (!(gh->bgc_thread))
-    {
-        goto cleanup;
-    }
-    
-    current_bgc_thread = gh->bgc_thread;
-    if (!current_bgc_thread->CreateNewThread (0, (DWORD (*)(void*))&(gh->bgc_thread_stub), gh))
-    {
-        goto cleanup;
-    }
-
-    current_bgc_thread->SetBackground (TRUE, FALSE);
-
-    // wait for the thread to be in its main loop, this is to detect the situation
-    // where someone triggers a GC during dll loading where the loader lock is
-    // held.
-    current_bgc_thread->StartThread();
-
-#endif // FEATURE_REDHAWK
-
+    if (GCToEEInterface::CreateBackgroundThread(&gh->bgc_thread, gh->bgc_thread_stub, gh))
     {
         dprintf (2, ("waiting for the thread to reach its main loop"));
         // In chk builds this can easily time out since
@@ -26794,19 +26750,17 @@ BOOL gc_heap::create_bgc_thread(gc_heap* gh)
         }
         //dprintf (2, ("waiting for the thread to reach its main loop Done."));
 
-        ret = TRUE;
+        return TRUE;
     }
 
 cleanup:
 
-    if (!ret)
+    if (gh->bgc_thread)
     {
-        if (gh->bgc_thread)
-        {
-            gh->bgc_thread = 0;
-        }
+        gh->bgc_thread = 0;
     }
-    return ret;
+
+    return FALSE;
 }
 
 BOOL gc_heap::create_bgc_threads_support (int number_of_heaps)

src/gc/gcee.cpp

@@ -827,29 +827,6 @@ void GCHeap::DescrGenerationsToProfiler (gen_walk_fn fn, void *context)
 }
 #endif // defined(GC_PROFILING) || defined(FEATURE_EVENT_TRACE)
 
-#if defined(BACKGROUND_GC) && defined(FEATURE_REDHAWK)
-
-// Helper used to wrap the start routine of background GC threads so we can do things like initialize the
-// Redhawk thread state which requires running in the new thread's context.
-uint32_t WINAPI gc_heap::rh_bgc_thread_stub(void * pContext)
-{
-    rh_bgc_thread_ctx * pStartContext = (rh_bgc_thread_ctx*)pContext;
-
-    // Initialize the Thread for this thread. The false being passed indicates that the thread store lock
-    // should not be acquired as part of this operation. This is necessary because this thread is created in
-    // the context of a garbage collection and the lock is already held by the GC.
-    ASSERT(GCHeap::GetGCHeap()->IsGCInProgress());
-    GCToEEInterface::AttachCurrentThread();
-
-    // Inform the GC which Thread* we are.
-    pStartContext->m_pRealContext->bgc_thread = GetThread();
-
-    // Run the real start procedure and capture its return code on exit.
-    return pStartContext->m_pRealStartRoutine(pStartContext->m_pRealContext);
-}
-
-#endif // BACKGROUND_GC && FEATURE_REDHAWK
-
 #ifdef FEATURE_BASICFREEZE
 segment_handle GCHeap::RegisterFrozenSegment(segment_info *pseginfo)
 {

src/gc/gcpriv.h

@@ -2736,19 +2736,6 @@ class gc_heap
     static
     uint32_t __stdcall bgc_thread_stub (void* arg);
 
-#ifdef FEATURE_REDHAWK
-    // Helper used to wrap the start routine of background GC threads so we can do things like initialize the
-    // Redhawk thread state which requires running in the new thread's context.
-    static uint32_t WINAPI rh_bgc_thread_stub(void * pContext);
-
-    // Context passed to the above.
-    struct rh_bgc_thread_ctx
-    {
-        PTHREAD_START_ROUTINE   m_pRealStartRoutine;
-        gc_heap *               m_pRealContext;
-    };
-#endif //FEATURE_REDHAWK
-
 #endif //BACKGROUND_GC
 
 public:

src/gc/sample/gcenv.ee.cpp

@@ -191,12 +191,6 @@ bool GCToEEInterface::CatchAtSafePoint(Thread * pThread)
     return pThread->CatchAtSafePoint();
 }
 
-// does not acquire thread store lock
-void GCToEEInterface::AttachCurrentThread()
-{
-    ThreadStore::AttachCurrentThread();
-}
-
 void GCToEEInterface::GcEnumAllocContexts (enum_alloc_context_func* fn, void* param)
 {
     Thread * pThread = NULL;
@@ -218,6 +212,12 @@ void GCToEEInterface::SyncBlockCachePromotionsGranted(int /*max_gen*/)
 {
 }
 
+bool GCToEEInterface::CreateBackgroundThread(Thread** thread, GCBackgroundThreadFunction threadStart, void* arg)
+{
+    // TODO: Implement for background GC
+    return false;
+}
+
 void FinalizerThread::EnableFinalization()
 {
     // Signal to finalizer thread that there are objects to finalize
@@ -229,12 +229,6 @@ bool FinalizerThread::HaveExtraWorkForFinalizer()
     return false;
 }
 
-bool REDHAWK_PALAPI PalStartBackgroundGCThread(BackgroundCallback callback, void* pCallbackContext)
-{
-    // TODO: Implement for background GC
-    return false;
-}
-
 bool IsGCSpecialThread()
 {
     // TODO: Implement for background GC

src/vm/assembly.cpp

@@ -2343,8 +2343,6 @@ static void Stress_Thread_Start (LPVOID lpParameter)
     for (n = 0; n < dwThreads-1; n ++)
     {
         threads[n] = SetupUnstartedThread();
-        if (threads[n] == NULL)
-            COMPlusThrowOM();
 
         threads[n]->m_stressThreadCount = dwThreads/2;
         Stress_Thread_Param *param = ((Stress_Thread_Param*)lpParameter)->Clone();

src/vm/ceemain.cpp

@@ -3727,8 +3727,7 @@ void InitializeGarbageCollector()
     IfFailThrow(hr);
 
     // Thread for running finalizers...
-    if (FinalizerThread::FinalizerThreadCreate() != 1)
-        ThrowOutOfMemory();
+    FinalizerThread::FinalizerThreadCreate();
 
     // Now we really have fully initialized the garbage collector
     SetGarbageCollectorFullyInitialized();

src/vm/finalizerthread.cpp

@@ -994,13 +994,13 @@ DWORD __stdcall FinalizerThread::FinalizerThreadStart(void *args)
     return 0;
 }
 
-DWORD FinalizerThread::FinalizerThreadCreate()
+void FinalizerThread::FinalizerThreadCreate()
 {
-    DWORD   dwRet = 0;
-
-    // TODO: The following line should be removed after contract violation is fixed.
-    // See bug 27409
-    SCAN_IGNORE_THROW;
+    CONTRACTL{
+        THROWS;
+        GC_TRIGGERS;
+        MODE_ANY;
+    } CONTRACTL_END;
 
 #ifndef FEATURE_PAL
     if (!CLRMemoryHosted())
@@ -1021,17 +1021,14 @@ DWORD FinalizerThread::FinalizerThreadCreate()
 
     _ASSERTE(g_pFinalizerThread == 0);
     g_pFinalizerThread = SetupUnstartedThread();
-    if (g_pFinalizerThread == 0) {
-        return 0;
-    }
 
     // We don't want the thread block disappearing under us -- even if the
     // actual thread terminates.
     GetFinalizerThread()->IncExternalCount();
 
     if (GetFinalizerThread()->CreateNewThread(0, &FinalizerThreadStart, NULL))
     {
-        dwRet = GetFinalizerThread()->StartThread();
+        DWORD dwRet = GetFinalizerThread()->StartThread();
 
         // When running under a user mode native debugger there is a race
         // between the moment we've created the thread (in CreateNewThread) and 
@@ -1046,23 +1043,17 @@ DWORD FinalizerThread::FinalizerThreadCreate()
         // debugger may have been detached between the time it got the notification
         // and the moment we execute the test below.
         _ASSERTE(dwRet == 1 || dwRet == 2);
-        if (dwRet == 2)
-        {
-            dwRet = 1;
-        }
 
         // workaround wwl: make sure finalizer is ready.  This avoids OOM problem on finalizer
         // thread startup.
         if (CLRTaskHosted()) {
             FinalizerThreadWait(INFINITE);
             if (!s_FinalizerThreadOK)
             {
-                dwRet = 0;
+                ThrowOutOfMemory();
             }
         }
     }
-
-    return dwRet;
 }
 
 void FinalizerThread::SignalFinalizationDone(BOOL fFinalizer)

src/vm/finalizerthread.h

@@ -91,7 +91,7 @@ class FinalizerThread
     static void FinalizeObjectsOnShutdown(LPVOID args);
     static DWORD __stdcall FinalizerThreadStart(void *args);
 
-    static DWORD FinalizerThreadCreate();
+    static void FinalizerThreadCreate();
     static BOOL FinalizerThreadWatchDog();
 };
 

src/vm/gcenv.ee.cpp

@@ -746,3 +746,39 @@ void GCToEEInterface::DisablePreemptiveGC(Thread * pThread)
     WRAPPER_NO_CONTRACT;
     pThread->DisablePreemptiveGC();
 }
+
+bool GCToEEInterface::CreateBackgroundThread(Thread** thread, GCBackgroundThreadFunction threadStart, void* arg)
+{
+    CONTRACTL
+    {
+        NOTHROW;
+        GC_TRIGGERS;
+    }
+    CONTRACTL_END;
+
+    Thread* newThread = NULL;
+    EX_TRY
+    {
+        newThread = SetupUnstartedThread(FALSE);
+        *thread = newThread;
+    }
+    EX_CATCH
+    {
+    }
+    EX_END_CATCH(SwallowAllExceptions);
+
+    if ((newThread != NULL) && newThread->CreateNewThread(0, (LPTHREAD_START_ROUTINE)threadStart, arg))
+    {
+        newThread->SetBackground (TRUE, FALSE);
+
+        // wait for the thread to be in its main loop, this is to detect the situation
+        // where someone triggers a GC during dll loading where the loader lock is
+        // held.
+        newThread->StartThread();
+
+        return true;
+    }
+
+    *thread = NULL;
+    return false;
+}