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

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.

1 files changed, 52 insertions, 11 deletions

diff --git a/src/dlls/dbgshim/dbgshim.cpp b/src/dlls/dbgshim/dbgshim.cpp
index aeee778543..5f15cabe97 100644
--- a/src/dlls/dbgshim/dbgshim.cpp
+++ b/src/dlls/dbgshim/dbgshim.cpp
@@ -387,6 +387,19 @@ public:
         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;
@@ -394,18 +407,46 @@ public:
 
         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;
     }
 
@@ -517,9 +558,8 @@ public:
         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 @@ public:
                         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))