Fix race condition in ReleaseOnDetach test

src/tests/profiler/native/profiler.def

@@ -3,6 +3,6 @@ LIBRARY     Profiler.dll
 EXPORTS
             DllGetClassObject       PRIVATE
             DllCanUnloadNow         PRIVATE
-            PassBoolToProfiler      PRIVATE
+            PassCallbackToProfiler  PRIVATE
             DoPInvoke               PRIVATE
 

src/tests/profiler/native/releaseondetach/releaseondetach.cpp

@@ -3,30 +3,18 @@
 
 #include "releaseondetach.h"
 
-#ifdef WIN32
-#include <Windows.h>
-#else // WIN32
-#include <dlfcn.h>
-#include <iostream>
-#include <fstream>
-#include <string>
-
-using std::string;
-using std::ifstream;
-using std::getline;
-
-#ifdef __APPLE__
-#include <mach-o/dyld.h>
-#endif // __APPLE__
-#endif // WIN32
+#include <thread>
+
+using std::thread;
 
 ReleaseOnDetach *ReleaseOnDetach::Instance;
 
 ReleaseOnDetach::ReleaseOnDetach() :
     _dispenser(NULL),
     _failures(0),
     _detachSucceeded(false),
-    _doneFlag(NULL)
+    _callback(NULL),
+    _callbackSet()
 {
     ReleaseOnDetach::Instance = this;
 }
@@ -51,10 +39,20 @@ ReleaseOnDetach::~ReleaseOnDetach()
 
     fflush(stdout);
 
-    if (_doneFlag != NULL)
+
+    _callbackSet.Wait();
+
+
+    thread callbackThread([&]()
     {
-        *_doneFlag = true;
-    }
+        // The destructor will be called from the profiler detach thread, which causes
+        // some crst order asserts if we call back in to managed code. Spin up
+        // a new thread to avoid that.
+        pCorProfilerInfo->InitializeCurrentThread();
+        _callback();
+    });
+
+    callbackThread.join();
 }
 
 GUID ReleaseOnDetach::GetClsid()
@@ -113,7 +111,7 @@ HRESULT ReleaseOnDetach::ProfilerAttachComplete()
     return S_OK;
 }
 
-HRESULT STDMETHODCALLTYPE ReleaseOnDetach::ProfilerDetachSucceeded()
+HRESULT ReleaseOnDetach::ProfilerDetachSucceeded()
 {
     SHUTDOWNGUARD();
 
@@ -122,7 +120,14 @@ HRESULT STDMETHODCALLTYPE ReleaseOnDetach::ProfilerDetachSucceeded()
     return S_OK;
 }
 
-extern "C" EXPORT void STDMETHODCALLTYPE PassBoolToProfiler(void *boolPtr)
+void ReleaseOnDetach::SetCallback(ProfilerCallback callback)
 {
-    ReleaseOnDetach::Instance->SetBoolPtr(boolPtr);
-}
+    assert(callback != NULL);
+    _callback = callback;
+    _callbackSet.Signal();
+}
+
+extern "C" EXPORT void STDMETHODCALLTYPE PassCallbackToProfiler(ProfilerCallback callback)
+{
+    ReleaseOnDetach::Instance->SetCallback(callback);
+}

src/tests/profiler/native/releaseondetach/releaseondetach.h

@@ -5,9 +5,9 @@
 
 #include "../profiler.h"
 
-#include <string>
+#include <atomic>
 
-typedef HRESULT (*GetDispenserFunc) (const CLSID &pClsid, const IID &pIid, void **ppv);
+typedef void (*ProfilerCallback) (void);
 
 // This test class is very small and doesn't do much. A repeated problem we had was that 
 // if an ICorProfilerCallback* interface was added the developer would forget to add
@@ -32,11 +32,7 @@ class ReleaseOnDetach : public Profiler
     virtual HRESULT STDMETHODCALLTYPE ProfilerAttachComplete();
     virtual HRESULT STDMETHODCALLTYPE ProfilerDetachSucceeded();
 
-    void SetBoolPtr(void *ptr)
-    {
-        assert(ptr != NULL);
-        _doneFlag = reinterpret_cast<bool *>(ptr);
-    }
+    void SetCallback(ProfilerCallback callback);
 
 private:
 
@@ -45,5 +41,6 @@ class ReleaseOnDetach : public Profiler
     IMetaDataDispenserEx* _dispenser;
     std::atomic<int> _failures;
     bool _detachSucceeded;
-    volatile bool *_doneFlag;
+    ProfilerCallback _callback;
+    ManualEvent _callbackSet;
 };

src/tests/profiler/unittest/releaseondetach.cs

@@ -13,35 +13,13 @@ namespace Profiler.Tests
 
     class ReleaseOnShutdown
     {
-        static readonly Guid ReleaseOnShutdownGuid = new Guid("B8C47A29-9C1D-4EEA-ABA0-8E8B3E3B792E");
+        private static readonly Guid ReleaseOnShutdownGuid = new Guid("B8C47A29-9C1D-4EEA-ABA0-8E8B3E3B792E");
 
-        static volatile bool _profilerDone = false;
+        private static ManualResetEvent _profilerDone; 
 
         [DllImport("Profiler")]
-        private static extern void PassBoolToProfiler(IntPtr boolPtr);
-
-        [MethodImpl(MethodImplOptions.NoInlining)]
-        public static void WasteTime()
-        {
-            // Give time for the profiler to detach
-            Console.WriteLine("Waiting for profiler to detach...");
-            bool profilerSetFlag = false;
-            for (int i = 0; i < 100_000; ++i)
-            {
-                Thread.Sleep(TimeSpan.FromMilliseconds(1));
-                if (_profilerDone)
-                {
-                    profilerSetFlag = true;
-                    break;
-                }
-            }
-
-            if (!profilerSetFlag)
-            {
-                Console.WriteLine("Warning: test will fail because the profiler never had its destructor called.");
-            }
-        }
-
+        private static extern void PassCallbackToProfiler(ProfilerCallback callback);
+
         public unsafe static int RunTest(string[] args)
         {
             string profilerName;
@@ -61,18 +39,14 @@ public unsafe static int RunTest(string[] args)
             string rootPath = Path.GetDirectoryName(System.Reflection.Assembly.GetExecutingAssembly().Location);
             string profilerPath = Path.Combine(rootPath, profilerName);
 
+            _profilerDone = new ManualResetEvent(false);
             Console.WriteLine($"Attaching profiler {profilerPath} to self.");
             ProfilerControlHelpers.AttachProfilerToSelf(ReleaseOnShutdownGuid, profilerPath);
 
-            // This warning is that the pointer to the volatile bool won't be treated as volatile,
-            // but that's ok. The loop aboive in WastTime is what needs to read it as volatile.
-            // The native part just sets it.
-            #pragma warning disable CS0420
-            fixed (bool *boolPtr = &_profilerDone)
+            PassCallbackToProfiler(() => _profilerDone.Set());
+            if (!_profilerDone.WaitOne(TimeSpan.FromMinutes(5)))
             {
-                PassBoolToProfiler(new IntPtr(boolPtr));
-
-                WasteTime();
+                Console.WriteLine("Profiler did not set the callback, test will fail.");
             }
 
             return 100;