[corlib] Ensure Thread.ManagedThreadId is unique per thread.

This fixes bug #667329.

The problem was that ManagedThreadId was created from a per-appdomain
counter, but stored in a per-thread storage, so if two threads could
end up with the same ManagedThreadId if a second appdomain executed
on the second thread first.

mcs/class/corlib/System.Threading/Thread.cs

@@ -102,12 +102,10 @@ namespace System.Threading {
 		private IntPtr unused3;
 		private IntPtr unused4;
 		private IntPtr unused5;
-		private IntPtr unused6;
+		internal int managed_id;
 		#endregion
 #pragma warning restore 169, 414, 649
 
-		internal int managed_id;
-
 		internal byte[] _serialized_principal;
 		internal int _serialized_principal_version;
 
@@ -156,8 +154,6 @@ namespace System.Threading {
 		private MulticastDelegate threadstart;
 		//private string thread_name=null;
 
-		private static int _managed_id_counter;
-
 		[MethodImplAttribute(MethodImplOptions.InternalCall)]
 		private extern void ConstructInternalThread ();
 
@@ -891,10 +887,6 @@ namespace System.Threading {
 		
 #endif
 
-		private static int GetNewManagedId() {
-			return Interlocked.Increment(ref _managed_id_counter);
-		}
-
 		public Thread (ThreadStart start, int maxStackSize)
 		{
 			if (start == null)
@@ -942,16 +934,6 @@ namespace System.Threading {
 		public int ManagedThreadId {
 			[ReliabilityContractAttribute (Consistency.WillNotCorruptState, Cer.Success)]
 			get {
-				// Fastpath
-				if (internal_thread != null && internal_thread.managed_id != 0)
-					return internal_thread.managed_id;
-
-				if (Internal.managed_id == 0) {
-					int new_managed_id = GetNewManagedId ();
-					
-					Interlocked.CompareExchange (ref Internal.managed_id, new_managed_id, 0);
-				}
-				
 				return Internal.managed_id;
 			}
 		}

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

@@ -921,6 +921,107 @@ namespace MonoTests.System.Threading
 		}
 	}
 
+	[TestFixture]
+	[Serializable]
+	public class ThreadTest_ManagedThreadId
+	{
+		AppDomain ad1;
+		AppDomain ad2;
+		MBRO mbro = new MBRO ();
+
+		class MBRO : MarshalByRefObject {
+			public int id_a1;
+			public int id_b1;
+			public int id_b2;
+			public string ad_a1;
+			public string ad_b1;
+			public string ad_b2;
+			public string message;
+		}
+
+		[Test]
+		public void ManagedThreadId_AppDomains ()
+		{
+			AppDomain currentDomain = AppDomain.CurrentDomain;
+			ad1 = AppDomain.CreateDomain ("AppDomain 1", currentDomain.Evidence, currentDomain.SetupInformation);
+			ad2 = AppDomain.CreateDomain ("AppDomain 2", currentDomain.Evidence, currentDomain.SetupInformation);
+
+			Thread a = new Thread (ThreadA);
+			Thread b = new Thread (ThreadB);
+			// execute on AppDomain 1 thread A
+			// execute on AppDomain 2 thread B
+			// execute on AppDomain 1 thread B - must have same ManagedThreadId as Ad 2 on thread B
+			a.Start ();
+			a.Join ();
+			b.Start ();
+			b.Join ();
+
+			AppDomain.Unload (ad1);
+			AppDomain.Unload (ad2);
+
+			if (mbro.message != null)
+				Assert.Fail (mbro.message);
+
+			// Console.WriteLine ("Done id_a1: {0} id_b1: {1} id_b2: {2} ad_a1: {3} ad_b1: {4} ad_b2: {5}", mbro.id_a1, mbro.id_b1, mbro.id_b2, mbro.ad_a1, mbro.ad_b1, mbro.ad_b2);
+
+			Assert.AreEqual ("AppDomain 1", mbro.ad_a1, "Name #1");
+			Assert.AreEqual ("AppDomain 1", mbro.ad_b1, "Name #2");
+			Assert.AreEqual ("AppDomain 2", mbro.ad_b2, "Name #3");
+
+			Assert.AreNotEqual (mbro.id_a1, mbro.id_b1, "Id #1");
+			Assert.AreNotEqual (mbro.id_a1, mbro.id_b2, "Id #2");
+			Assert.AreEqual (mbro.id_b1, mbro.id_b2, "Id #3");
+
+			Assert.AreNotEqual (mbro.id_a1, Thread.CurrentThread.ManagedThreadId, "Id #4");
+			Assert.AreNotEqual (mbro.id_b1, Thread.CurrentThread.ManagedThreadId, "Id #5");
+			Assert.AreNotEqual (mbro.id_b2, Thread.CurrentThread.ManagedThreadId, "Id #6");
+			Assert.AreNotEqual (mbro.ad_a1, AppDomain.CurrentDomain.FriendlyName, "Name #4");
+			Assert.AreNotEqual (mbro.ad_b1, AppDomain.CurrentDomain.FriendlyName, "Name #5");
+			Assert.AreNotEqual (mbro.ad_b2, AppDomain.CurrentDomain.FriendlyName, "Name #6");
+		}
+
+		void A1 ()
+		{
+			mbro.id_a1 = Thread.CurrentThread.ManagedThreadId;
+			mbro.ad_a1 = AppDomain.CurrentDomain.FriendlyName;
+		}
+		
+		void B2 ()
+		{
+			mbro.id_b2 = Thread.CurrentThread.ManagedThreadId;
+			mbro.ad_b2 = AppDomain.CurrentDomain.FriendlyName;
+		}
+
+		void B1 ()
+		{
+			mbro.id_b1 = Thread.CurrentThread.ManagedThreadId;
+			mbro.ad_b1 = AppDomain.CurrentDomain.FriendlyName;
+		}
+
+		void ThreadA (object obj)
+		{
+			// Console.WriteLine ("ThreadA");
+			try {
+				ad1.DoCallBack (A1);
+			} catch (Exception ex) {
+				mbro.message = string.Format ("ThreadA exception: {0}", ex);
+			}
+			// Console.WriteLine ("ThreadA Done");
+		}
+
+		void ThreadB (object obj)
+		{
+			// Console.WriteLine ("ThreadB");
+			try {
+				ad2.DoCallBack (B2);
+				ad1.DoCallBack (B1);
+			} catch (Exception ex) {
+				mbro.message = string.Format ("ThreadB exception: {0}", ex);
+			}
+			// Console.WriteLine ("ThreadB Done");
+		}
+	}
+
 	[TestFixture]
 	public class ThreadApartmentTest
 	{

mono/metadata/object-internals.h

@@ -406,11 +406,11 @@ struct _MonoInternalThread {
 	gsize    flags;
 	gpointer android_tid;
 	gpointer thread_pinning_ref;
+	gint32 managed_id;
 	/* 
 	 * These fields are used to avoid having to increment corlib versions
 	 * when a new field is added to the unmanaged MonoThread structure.
 	 */
-	gpointer unused6;
 };
 
 struct _MonoThread {

mono/metadata/threads.c

@@ -204,6 +204,14 @@ static HANDLE background_change_event;
 
 static gboolean shutting_down = FALSE;
 
+static gint32 managed_thread_id_counter = 0;
+
+static guint32
+get_next_managed_thread_id (void)
+{
+	return InterlockedIncrement (&managed_thread_id_counter);
+}
+
 guint32
 mono_thread_get_tls_key (void)
 {
@@ -718,6 +726,7 @@ MonoInternalThread* mono_thread_create_internal (MonoDomain *domain, gpointer fu
 	internal->tid=tid;
 	internal->apartment_state=ThreadApartmentState_Unknown;
 	internal->thread_pinning_ref = internal;
+	internal->managed_id = get_next_managed_thread_id ();
 	MONO_GC_REGISTER_ROOT (internal->thread_pinning_ref);
 
 	internal->synch_cs = g_new0 (CRITICAL_SECTION, 1);
@@ -841,6 +850,7 @@ mono_thread_attach (MonoDomain *domain)
 #endif
 	thread->apartment_state=ThreadApartmentState_Unknown;
 	thread->thread_pinning_ref = thread;
+	thread->managed_id = get_next_managed_thread_id ();
 	MONO_GC_REGISTER_ROOT (thread->thread_pinning_ref);
 
 	thread->stack_ptr = &tid;
@@ -932,6 +942,7 @@ ves_icall_System_Threading_Thread_ConstructInternalThread (MonoThread *this)
 
 	internal->state = ThreadState_Unstarted;
 	internal->apartment_state = ThreadApartmentState_Unknown;
+	internal->managed_id = get_next_managed_thread_id ();
 
 	InterlockedCompareExchangePointer ((gpointer)&this->internal_thread, internal, NULL);
 }