Fix race condition in WaitAnyWithSecondMutexAbandoned test.

There is a small race condition during the completion of a native thread join
call. During that period of time the tid is no longer included on the
internal list tracking joinable threads so a thread that will join on the
tid while another thread (like the finalizer thread) is waiting on native
join to complete for the same tid, will cause the managed Thread.Join call
to complete before the native join call has completed. This race could cause
issues on some OS:es that clear's up some thread resources, like abandoned
mutexes when the thread has exited. This race is hit by WaitAnyWithSecondMutexAbandoned
since the call to Thread.Join will return before the thread owning the mutex
has terminated meaning that it doesn't get ownership of the abandoned mutex
as assumed by the test.

Fix makes sure Thread.Join won't complete until native thread join is complete.
Increasing the join timeouts in the test also eliminates a timeout error making it
harder to hit the problematic code path, primarily during reproduction
in the debugger.

Fix also switch to coop mutex for joinable threads.

Author: lateralusX

mcs/class/corlib/Test/System.Threading/WaitHandleTest.cs

@@ -409,7 +409,7 @@ public void WaitOneWithAbandonedMutex ()
 					m.WaitOne ();
 				});
 				thread1.Start ();
-				thread1.Join (1000);
+				Assert.IsTrue (thread1.Join (Timeout.Infinite), "thread1.Join");
 				try {
 					m.WaitOne ();
 					Assert.Fail ("Expected AbandonedMutexException");
@@ -421,7 +421,7 @@ public void WaitOneWithAbandonedMutex ()
 					signalled = m.WaitOne (100);
 				});
 				thread2.Start ();
-				thread2.Join (1000);
+				Assert.IsTrue (thread2.Join (Timeout.Infinite), "thread2.Join");
 				Assert.IsFalse (signalled);
 
 				// Since this thread owns the Mutex releasing it shouldn't fail
@@ -489,7 +489,7 @@ public void WaitAnyWithSecondMutexAbandoned ()
 						m1.ReleaseMutex ();
 					});
 					thread1.Start ();
-					thread1.Join (1000);
+					Assert.IsTrue (thread1.Join (Timeout.Infinite), "thread1.Join");
 					thread2.Start ();
 					while (!mainProceed) {
 						Thread.Sleep (10);
@@ -502,7 +502,7 @@ public void WaitAnyWithSecondMutexAbandoned ()
 						Assert.AreEqual (m2, e.Mutex);
 					} finally {
 						thread2Proceed = true;
-						thread2.Join (1000);
+						Assert.IsTrue (thread2.Join (Timeout.Infinite), "thread2.Join");
 					}
 
 					// Current thread should own the second Mutex now
@@ -511,7 +511,7 @@ public void WaitAnyWithSecondMutexAbandoned ()
 						signalled = WaitHandle.WaitAny (new WaitHandle [] { m1, m2 }, 0);
 					});
 					thread3.Start ();
-					thread3.Join (1000);
+					Assert.IsTrue (thread3.Join (Timeout.Infinite), "thread3.Join");
 					Assert.AreEqual (0, signalled);
 
 					// Since this thread owns the second Mutex releasing it shouldn't fail
@@ -540,7 +540,7 @@ public void WaitAllWithOneAbandonedMutex ()
 						m1.WaitOne ();
 					});
 					thread.Start ();
-					thread.Join (1000);
+					Assert.IsTrue (thread.Join (Timeout.Infinite), "thread.Join");
 					WaitHandle.WaitAll (new WaitHandle [] { m1, m2 });
 				}
 			}

mono/metadata/threads.c

@@ -128,9 +128,9 @@ static void mono_threads_unlock (void);
 static MonoCoopMutex threads_mutex;
 
 /* Controls access to the 'joinable_threads' hash table */
-#define joinable_threads_lock() mono_os_mutex_lock (&joinable_threads_mutex)
-#define joinable_threads_unlock() mono_os_mutex_unlock (&joinable_threads_mutex)
-static mono_mutex_t joinable_threads_mutex;
+#define joinable_threads_lock() mono_coop_mutex_lock (&joinable_threads_mutex)
+#define joinable_threads_unlock() mono_coop_mutex_unlock (&joinable_threads_mutex)
+static MonoCoopMutex joinable_threads_mutex;
 
 /* Holds current status of static data heap */
 static StaticDataInfo thread_static_info;
@@ -161,10 +161,19 @@ static MonoGHashTable *threads_starting_up = NULL;
 static GHashTable *joinable_threads;
 static gint32 joinable_thread_count;
 
+/* mono_threads_join_threads will take threads from joinable_threads list and wait for them. */
+/* When this happens, the tid is not on the list anymore so mono_thread_join assumes the thread has complete */
+/* and will return back to the caller. This could cause a race since caller of join assumes thread has completed */
+/* and on some OS it could cause errors. Keeping the tid's currently pending a native thread join call */
+/* in a separate table (only affecting callers interested in this internal join detail) and look at that table in mono_thread_join */
+/* will close this race. */
+static GHashTable *pending_native_thread_join_calls;
+static MonoCoopCond pending_native_thread_join_calls_event;
+
 static GHashTable *pending_joinable_threads;
 static gint32 pending_joinable_thread_count;
 
-static mono_cond_t zero_pending_joinable_thread_event;
+static MonoCoopCond zero_pending_joinable_thread_event;
 
 static void threads_add_pending_joinable_runtime_thread (MonoThreadInfo *mono_thread_info);
 static gboolean threads_wait_pending_joinable_threads (uint32_t timeout);
@@ -3036,11 +3045,12 @@ void mono_thread_init (MonoThreadStartCB start_cb,
 	mono_coop_mutex_init_recursive (&threads_mutex);
 
 	mono_os_mutex_init_recursive(&interlocked_mutex);
-	mono_os_mutex_init_recursive(&joinable_threads_mutex);
+	mono_coop_mutex_init_recursive(&joinable_threads_mutex);
 
 	mono_os_event_init (&background_change_event, FALSE);
 
-	mono_os_cond_init (&zero_pending_joinable_thread_event);
+	mono_coop_cond_init (&pending_native_thread_join_calls_event);
+	mono_coop_cond_init (&zero_pending_joinable_thread_event);
 
 	mono_init_static_data_info (&thread_static_info);
 	mono_init_static_data_info (&context_static_info);
@@ -3161,7 +3171,8 @@ mono_thread_cleanup (void)
 	mono_os_mutex_destroy (&interlocked_mutex);
 	mono_os_mutex_destroy (&delayed_free_table_mutex);
 	mono_os_mutex_destroy (&small_id_mutex);
-	mono_os_cond_destroy (&zero_pending_joinable_runtime_thread_event);
+	mono_coop_cond_destroy (&zero_pending_joinable_thread_event);
+	mono_coop_cond_destroy (&pending_native_thread_join_calls_event);
 	mono_os_event_destroy (&background_change_event);
 #endif
 }
@@ -5280,7 +5291,7 @@ threads_remove_pending_joinable_thread_nolock (gpointer tid)
 	if (pending_joinable_threads && g_hash_table_lookup_extended (pending_joinable_threads, tid, &orig_key, &value)) {
 		g_hash_table_remove (pending_joinable_threads, tid);
 		if (UnlockedDecrement (&pending_joinable_thread_count) == 0)
-			mono_os_cond_broadcast (&zero_pending_joinable_thread_event);
+			mono_coop_cond_broadcast (&zero_pending_joinable_thread_event);
 	}
 }
 
@@ -5291,12 +5302,12 @@ threads_wait_pending_joinable_threads (uint32_t timeout)
 		joinable_threads_lock ();
 		if (timeout == MONO_INFINITE_WAIT) {
 			while (UnlockedRead (&pending_joinable_thread_count) > 0)
-				mono_os_cond_wait (&zero_pending_joinable_thread_event, &joinable_threads_mutex);
+				mono_coop_cond_wait (&zero_pending_joinable_thread_event, &joinable_threads_mutex);
 		} else {
 			gint64 start = mono_msec_ticks ();
 			gint64 elapsed = 0;
 			while (UnlockedRead (&pending_joinable_thread_count) > 0 && elapsed < timeout) {
-				mono_os_cond_timedwait (&zero_pending_joinable_thread_event, &joinable_threads_mutex, timeout - (uint32_t)elapsed);
+				mono_coop_cond_timedwait (&zero_pending_joinable_thread_event, &joinable_threads_mutex, timeout - (uint32_t)elapsed);
 				elapsed = mono_msec_ticks () - start;
 			}
 		}
@@ -5344,6 +5355,39 @@ mono_threads_add_joinable_runtime_thread (gpointer thread_info)
 	}
 }
 
+static void
+threads_add_pending_native_thread_join_call_nolock (gpointer tid)
+{
+	if (!pending_native_thread_join_calls)
+		pending_native_thread_join_calls = g_hash_table_new (NULL, NULL);
+
+	gpointer orig_key;
+	gpointer value;
+
+	if (!g_hash_table_lookup_extended (pending_native_thread_join_calls, tid, &orig_key, &value))
+		g_hash_table_insert (pending_native_thread_join_calls, tid, tid);
+}
+
+static void
+threads_remove_pending_native_thread_join_call_nolock (gpointer tid)
+{
+	if (pending_native_thread_join_calls)
+		g_hash_table_remove (pending_native_thread_join_calls, tid);
+
+	mono_coop_cond_broadcast (&pending_native_thread_join_calls_event);
+}
+
+static void
+threads_wait_pending_native_thread_join_call_nolock (gpointer tid)
+{
+	gpointer orig_key;
+	gpointer value;
+
+	while (g_hash_table_lookup_extended (pending_native_thread_join_calls, tid, &orig_key, &value)) {
+		mono_coop_cond_wait (&pending_native_thread_join_calls_event, &joinable_threads_mutex);
+	}
+}
+
 /*
  * mono_add_joinable_thread:
  *
@@ -5375,23 +5419,30 @@ void
 mono_threads_join_threads (void)
 {
 	GHashTableIter iter;
-	gpointer key;
-	gpointer value;
-	gboolean found;
+	gpointer key = NULL;
+	gpointer value = NULL;
+	gboolean found = FALSE;
 
 	/* Fastpath */
 	if (!UnlockedRead (&joinable_thread_count))
 		return;
 
 	while (TRUE) {
 		joinable_threads_lock ();
+		if (found) {
+			// Previous native thread join call completed.
+			threads_remove_pending_native_thread_join_call_nolock (key);
+		}
 		found = FALSE;
 		if (g_hash_table_size (joinable_threads)) {
 			g_hash_table_iter_init (&iter, joinable_threads);
 			g_hash_table_iter_next (&iter, &key, (void**)&value);
 			g_hash_table_remove (joinable_threads, key);
 			UnlockedDecrement (&joinable_thread_count);
 			found = TRUE;
+
+			// Add to table of tid's with pending native thread join call.
+			threads_add_pending_native_thread_join_call_nolock (key);
 		}
 		joinable_threads_unlock ();
 		if (found)
@@ -5422,13 +5473,27 @@ mono_thread_join (gpointer tid)
 		g_hash_table_remove (joinable_threads, tid);
 		UnlockedDecrement (&joinable_thread_count);
 		found = TRUE;
+
+		// Add to table of tid's with pending native join call.
+		threads_add_pending_native_thread_join_call_nolock (tid);
+	}
+
+	if (!found) {
+		// Wait for any pending native thread join call not yet completed for this tid.
+		threads_wait_pending_native_thread_join_call_nolock (tid);
 	}
+
 	joinable_threads_unlock ();
 
 	if (!found)
 		return;
 
 	threads_native_thread_join_nolock (tid, value);
+
+	joinable_threads_lock ();
+	// Native thread join call completed for this tid.
+	threads_remove_pending_native_thread_join_call_nolock (tid);
+	joinable_threads_unlock ();
 }
 
 void