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.

11 files changed, 64 insertions, 129 deletions

diff --git a/src/gc/env/gcenv.ee.h b/src/gc/env/gcenv.ee.h
index 6fd538be93..42f47c4b18 100644
--- a/src/gc/env/gcenv.ee.h
+++ b/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 @@ public:
 
     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__
diff --git a/src/gc/gc.cpp b/src/gc/gc.cpp
index 9c04356617..adc3a14f56 100644
--- a/src/gc/gc.cpp
+++ b/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)
diff --git a/src/gc/gcee.cpp b/src/gc/gcee.cpp
index a73770168d..3578b6d7f5 100755
--- a/src/gc/gcee.cpp
+++ b/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)
 {
diff --git a/src/gc/gcpriv.h b/src/gc/gcpriv.h
index 9f6abe40d4..703059a636 100644
--- a/src/gc/gcpriv.h
+++ b/src/gc/gcpriv.h
@@ -2736,19 +2736,6 @@ protected:
     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:
diff --git a/src/gc/sample/gcenv.ee.cpp b/src/gc/sample/gcenv.ee.cpp
index 1d380d0a91..11befab1b9 100644
--- a/src/gc/sample/gcenv.ee.cpp
+++ b/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
diff --git a/src/vm/assembly.cpp b/src/vm/assembly.cpp
index 3de92206d4..06effd7240 100644
--- a/src/vm/assembly.cpp
+++ b/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();
diff --git a/src/vm/ceemain.cpp b/src/vm/ceemain.cpp
index 9a54adf7b7..f3e8c3b28d 100644
--- a/src/vm/ceemain.cpp
+++ b/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();
diff --git a/src/vm/finalizerthread.cpp b/src/vm/finalizerthread.cpp
index 3960719c65..5d51d33cfb 100644
--- a/src/vm/finalizerthread.cpp
+++ b/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,9 +1021,6 @@ 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.
@@ -1031,7 +1028,7 @@ DWORD FinalizerThread::FinalizerThreadCreate()
 
     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,10 +1043,6 @@ 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.
@@ -1057,12 +1050,10 @@ DWORD FinalizerThread::FinalizerThreadCreate()
             FinalizerThreadWait(INFINITE);
             if (!s_FinalizerThreadOK)
             {
-                dwRet = 0;
+                ThrowOutOfMemory();
             }
         }
     }
-
-    return dwRet;
 }
 
 void FinalizerThread::SignalFinalizationDone(BOOL fFinalizer)
diff --git a/src/vm/finalizerthread.h b/src/vm/finalizerthread.h
index 42a9e9f250..90dd3e9ec9 100644
--- a/src/vm/finalizerthread.h
+++ b/src/vm/finalizerthread.h
@@ -91,7 +91,7 @@ public:
     static void FinalizeObjectsOnShutdown(LPVOID args);
     static DWORD __stdcall FinalizerThreadStart(void *args);
 
-    static DWORD FinalizerThreadCreate();
+    static void FinalizerThreadCreate();
     static BOOL FinalizerThreadWatchDog();
 };
 
diff --git a/src/vm/gcenv.ee.cpp b/src/vm/gcenv.ee.cpp
index 7cce1716ae..1bb4ee815b 100644
--- a/src/vm/gcenv.ee.cpp
+++ b/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;
+}
diff --git a/src/vm/threads.cpp b/src/vm/threads.cpp
index 9e1a528370..5624bb1026 100644
--- a/src/vm/threads.cpp
+++ b/src/vm/threads.cpp
@@ -1118,13 +1118,10 @@ Thread* SetupUnstartedThread(BOOL bRequiresTSL)
     _ASSERTE(ThreadInited());
     Thread* pThread = new Thread();
 
-    if (pThread)
-    {
-        FastInterlockOr((ULONG *) &pThread->m_State,
-                        (Thread::TS_Unstarted | Thread::TS_WeOwn));
+    FastInterlockOr((ULONG *) &pThread->m_State,
+                    (Thread::TS_Unstarted | Thread::TS_WeOwn));
 
-        ThreadStore::AddThread(pThread, bRequiresTSL);
-    }
+    ThreadStore::AddThread(pThread, bRequiresTSL);
 
     return pThread;
 }