Fix debugger launch race hitting breakpoints in startup code. (#8951) (#9060)

The attached flag was been set asynchronously relative to the DebugActiveProcess
returning.  This could cause a race where the initial module load notification being
missed/not sent to the debugger.

This fix sets the attached flag before any notifications sent during launch if the runtime was
launched/attached using the startup handshake after dbgshim tells the runtime to "continue"
when the runtime startup API callback returns.

Also fixes another race condition in dbgshim where EnumerateCLRs returns a NULL continue event
handle because the coreclr module was loaded but the g_hContinueStartupEvent wasn't initialized
on the runtime side yet. Changed the static initialization of g_hContinueStartupEvent to
INVALID_HANDLE_VALUE and the InternalEnumerateCLRs sleep/retry loop to retry when any of the
handles are INVALID_HANDLE_VALUE. This fixes the race only when you have the latest dbgshim
and coreclr binaries and the old/new mixes still function but don't fix the race.

Author: Mike McLaughlin

src/debug/ee/debugger.cpp

@@ -1895,7 +1895,7 @@ void Debugger::SendCreateProcess(DebuggerLockHolder * pDbgLockHolder)
 
 #if defined(FEATURE_CORECLR) && !defined(FEATURE_PAL)
 
-HANDLE g_hContinueStartupEvent = NULL;
+HANDLE g_hContinueStartupEvent = INVALID_HANDLE_VALUE;
 
 CLR_ENGINE_METRICS g_CLREngineMetrics = {
     sizeof(CLR_ENGINE_METRICS), 
@@ -1942,7 +1942,7 @@ void NotifyDebuggerOfTelestoStartup()
     // enumeration of this process will get back a valid continue event
     // the instant we signal the startup notification event.
 
-    CONSISTENCY_CHECK(NULL == g_hContinueStartupEvent);
+    CONSISTENCY_CHECK(INVALID_HANDLE_VALUE == g_hContinueStartupEvent);
     g_hContinueStartupEvent = WszCreateEvent(NULL, TRUE, FALSE, NULL);
     CONSISTENCY_CHECK(INVALID_HANDLE_VALUE != g_hContinueStartupEvent); // we reserve this value for error conditions in EnumerateCLRs
 
@@ -2170,7 +2170,14 @@ HRESULT Debugger::Startup(void)
     // Signal the debugger (via dbgshim) and wait until it is ready for us to 
     // continue. This needs to be outside the lock and after the transport is
     // initialized.
-    PAL_NotifyRuntimeStarted();
+    if (PAL_NotifyRuntimeStarted())
+    {
+        // The runtime was successfully launched and attached so mark it now
+        // so no notifications are missed especially the initial module load 
+        // which would cause debuggers problems with reliable setting breakpoints 
+        // in startup code or Main.
+       MarkDebuggerAttachedInternal();
+    }
 #endif // FEATURE_PAL
 
     // We don't bother changing this process's permission.

src/dlls/dbgshim/dbgshim.cpp

@@ -387,25 +387,66 @@ class RuntimeStartupHelper
         return hr;
     }
 
+    bool AreAllHandlesValid(HANDLE *handleArray, DWORD arrayLength)
+    {
+        for (int i = 0; i < (int)arrayLength; i++)
+        {
+            HANDLE h = handleArray[i];
+            if (h == INVALID_HANDLE_VALUE)
+            {
+                return false;
+            }
+        }
+        return true;
+    }
+
     HRESULT InternalEnumerateCLRs(HANDLE** ppHandleArray, _In_reads_(*pdwArrayLength) LPWSTR** ppStringArray, DWORD* pdwArrayLength)
     {
         int numTries = 0;
         HRESULT hr;
 
         while (numTries < 25)
         {
+            hr = EnumerateCLRs(m_processId, ppHandleArray, ppStringArray, pdwArrayLength);
+
             // EnumerateCLRs uses the OS API CreateToolhelp32Snapshot which can return ERROR_BAD_LENGTH or 
             // ERROR_PARTIAL_COPY. If we get either of those, we try wait 1/10th of a second try again (that
-            // is the recommendation of the OS API owners)
-            hr = EnumerateCLRs(m_processId, ppHandleArray, ppStringArray, pdwArrayLength);
+            // is the recommendation of the OS API owners).
             if ((hr != HRESULT_FROM_WIN32(ERROR_PARTIAL_COPY)) && (hr != HRESULT_FROM_WIN32(ERROR_BAD_LENGTH)))
             {
-                break;
+                // Just return any other error or if no handles were found (which means the coreclr module wasn't found yet).
+                if (FAILED(hr) || *ppHandleArray == NULL || *pdwArrayLength <= 0)
+                {
+                    return hr;
+                }
+                // If EnumerateCLRs succeeded but any of the handles are INVALID_HANDLE_VALUE, then sleep and retry
+                // also. This fixes a race condition where dbgshim catches the coreclr module just being loaded but 
+                // before g_hContinueStartupEvent has been initialized.
+                if (AreAllHandlesValid(*ppHandleArray, *pdwArrayLength))
+                {
+                    return hr;
+                }
+                // Clean up memory allocated in EnumerateCLRs since this path it succeeded
+                CloseCLREnumeration(*ppHandleArray, *ppStringArray, *pdwArrayLength);
+
+                *ppHandleArray = NULL;
+                *ppStringArray = NULL;
+                *pdwArrayLength = 0;
             }
+
+            // Sleep and retry enumerating the runtimes
             Sleep(100);
             numTries++;
+
+            if (m_canceled)
+            {
+                break;
+            }
         }
 
+        // Indicate a timeout
+        hr = HRESULT_FROM_WIN32(ERROR_TIMEOUT);
+
         return hr;
     }
 
@@ -433,7 +474,7 @@ class RuntimeStartupHelper
         DWORD arrayLength = 0;
 
         // Wake up runtime(s)
-        HRESULT hr = InternalEnumerateCLRs(&handleArray, &stringArray, &arrayLength);
+        HRESULT hr = EnumerateCLRs(m_processId, &handleArray, &stringArray, &arrayLength);
         if (SUCCEEDED(hr))
         {
             WakeRuntimes(handleArray, arrayLength);
@@ -517,9 +558,8 @@ class RuntimeStartupHelper
         bool coreclrExists = false;
 
         HRESULT hr = InvokeStartupCallback(&coreclrExists);
-        // Because the target process is suspended on create, the toolhelp apis fail with the below errors even
-        // with the retry logic in InternalEnumerateCLRs.
-        if (SUCCEEDED(hr) || (hr == HRESULT_FROM_WIN32(ERROR_PARTIAL_COPY)) || (hr == HRESULT_FROM_WIN32(ERROR_BAD_LENGTH)))
+        // The retry logic in InternalEnumerateCLRs failed if ERROR_TIMEOUT was returned.
+        if (SUCCEEDED(hr) || (hr == HRESULT_FROM_WIN32(ERROR_TIMEOUT)))
         {
             if (!coreclrExists && !m_canceled)
             {
@@ -531,8 +571,11 @@ class RuntimeStartupHelper
                         hr = InvokeStartupCallback(&coreclrExists);
                         if (SUCCEEDED(hr))
                         {
-                            // We should always find a coreclr module
-                            _ASSERTE(coreclrExists);
+                            // We should always find a coreclr module so fail if we don't
+                            if (!coreclrExists)
+                            {
+                                hr = E_FAIL;
+                            }
                         }
                     }
                 }
@@ -1120,8 +1163,6 @@ EnumerateCLRs(
 
             HANDLE hContinueStartupEvent = INVALID_HANDLE_VALUE;
             HRESULT hr = GetContinueStartupEvent(debuggeePID, pStringArray[idx], &hContinueStartupEvent);
-            _ASSERTE(SUCCEEDED(hr) == (hContinueStartupEvent != INVALID_HANDLE_VALUE));
-
             pEventArray[idx] = hContinueStartupEvent;
 #else
             pEventArray[idx] = NULL;
@@ -1729,7 +1770,7 @@ GetContinueStartupEvent(
             ThrowHR(E_FAIL);
         }
 
-        if (NULL != continueEvent)
+        if (NULL != continueEvent && INVALID_HANDLE_VALUE != continueEvent)
         {
             if (!DuplicateHandle(hProcess, continueEvent, GetCurrentProcess(), &continueEvent, 
                 EVENT_MODIFY_STATE, FALSE, 0))

src/pal/src/thread/process.cpp

@@ -1879,7 +1879,7 @@ PAL_UnregisterForRuntimeStartup(
     None
 
 Return value:
-    TRUE - succeeded, FALSE - failed
+    TRUE - successfully launched by debugger, FALSE - not launched or some failure in the handshake
 --*/
 BOOL
 PALAPI
@@ -1889,7 +1889,7 @@ PAL_NotifyRuntimeStarted()
     char continueSemName[CLR_SEM_MAX_NAMELEN];
     sem_t *startupSem = SEM_FAILED;
     sem_t *continueSem = SEM_FAILED;
-    BOOL result = TRUE;
+    BOOL launched = FALSE;
 
     UINT64 processIdDisambiguationKey = 0;
     GetProcessIdDisambiguationKey(gPID, &processIdDisambiguationKey);
@@ -1899,9 +1899,7 @@ PAL_NotifyRuntimeStarted()
 
     TRACE("PAL_NotifyRuntimeStarted opening continue '%s' startup '%s'\n", continueSemName, startupSemName);
 
-
-    // Open the debugger startup semaphore. If it doesn't exists, then we do nothing and
-    // the function is successful.
+    // Open the debugger startup semaphore. If it doesn't exists, then we do nothing and return
     startupSem = sem_open(startupSemName, 0);
     if (startupSem == SEM_FAILED)
     {
@@ -1913,26 +1911,26 @@ PAL_NotifyRuntimeStarted()
     if (continueSem == SEM_FAILED)
     {
         ASSERT("sem_open(%s) failed: %d (%s)\n", continueSemName, errno, strerror(errno));
-        result = FALSE;
         goto exit;
     }
 
     // Wake up the debugger waiting for startup
     if (sem_post(startupSem) != 0)
     {
         ASSERT("sem_post(startupSem) failed: errno is %d (%s)\n", errno, strerror(errno));
-        result = FALSE;
         goto exit;
     }
 
     // Now wait until the debugger's runtime startup notification is finished
     if (sem_wait(continueSem) != 0)
     {
         ASSERT("sem_wait(continueSem) failed: errno is %d (%s)\n", errno, strerror(errno));
-        result = FALSE;
         goto exit;
     }
 
+    // Returns that the runtime was successfully launched for debugging
+    launched = TRUE;
+
 exit:
     if (startupSem != SEM_FAILED)
     {
@@ -1942,7 +1940,7 @@ PAL_NotifyRuntimeStarted()
     {
         sem_close(continueSem);
     }
-    return result;
+    return launched;
 }
 
 /*++