Fix race in BrowserThread::GetCurrentThreadIdentifier.

content/browser/browser_thread_impl.cc

Index: content/browser/browser_thread_impl.cc
diff --git a/content/browser/browser_thread_impl.cc b/content/browser/browser_thread_impl.cc
index b86643970e599c88c4bb044765f5e5d15ed0a160..8b714d723b6611c3155e04a4c7bb0546f7739678 100644
--- a/content/browser/browser_thread_impl.cc
+++ b/content/browser/browser_thread_impl.cc
@@ -15,7 +15,7 @@
 #include "base/macros.h"
 #include "base/single_thread_task_runner.h"
 #include "base/threading/sequenced_worker_pool.h"
-#include "base/threading/thread_restrictions.h"
+#include "base/threading/thread_local.h"
 #include "build/build_config.h"
 #include "content/public/browser/browser_thread_delegate.h"
 #include "content/public/browser/content_browser_client.h"
@@ -117,6 +117,9 @@ struct BrowserThreadGlobals {
 base::LazyInstance<BrowserThreadGlobals>::Leaky
     g_globals = LAZY_INSTANCE_INITIALIZER;
 
+base::LazyInstance<base::ThreadLocalPointer<BrowserThreadImpl>>::Leaky
+    g_browser_thread = LAZY_INSTANCE_INITIALIZER;
+
 }  // namespace
 
 BrowserThreadImpl::BrowserThreadImpl(ID identifier)
@@ -130,6 +133,13 @@ BrowserThreadImpl::BrowserThreadImpl(ID identifier,
     : Thread(message_loop->thread_name()), identifier_(identifier) {
   set_message_loop(message_loop);
   Initialize();
+
+  if (message_loop == base::MessageLoop::current()) {
+    // In tests, it's possible for the same current MessageLoop to be assigned
+    // to multiple BrowserThreadImpls.
+    if (!g_browser_thread.Get().Get())
+      g_browser_thread.Get().Set(this);
+  }
 }
 
 // static
@@ -229,11 +239,11 @@ void BrowserThreadImpl::Run(base::MessageLoop* message_loop) {
   }
 #endif
 
-  BrowserThread::ID thread_id = ID_COUNT;
-  if (!GetCurrentThreadIdentifier(&thread_id))
-    return Thread::Run(message_loop);
+  DCHECK(!g_browser_thread.Get().Get());
+  g_browser_thread.Get().Set(this);
+  CHECK_EQ(Thread::message_loop(), message_loop);
 
-  switch (thread_id) {
+  switch (identifier_) {
     case BrowserThread::UI:
       return UIThreadRun(message_loop);
     case BrowserThread::DB:
@@ -252,7 +262,10 @@ void BrowserThreadImpl::Run(base::MessageLoop* message_loop) {
       CHECK(false);  // This shouldn't actually be reached!
       break;
   }
-  Thread::Run(message_loop);
+
+  // |identifier_| must be set to a valid enum value in the constructor, so it
+  // should be impossible to reach here.
+  CHECK(false);
 }
 
 void BrowserThreadImpl::CleanUp() {
@@ -294,6 +307,9 @@ BrowserThreadImpl::~BrowserThreadImpl() {
         "Threads must be listed in the reverse order that they die";
   }
 #endif
+
+  if (g_browser_thread.Get().Get() == this)

[kinuko, 2016/01/08 11:35:50]

Is this condition for tests?

[Anand Mistry (off Chromium), 2016/01/10 23:18:28]

This is for the main/UI thread, where a BrowserThreadImpl object is created for
the current thread.

[kinuko, 2016/01/11 03:52:22]

I see thanks, could we add a short comment about that here?

+    g_browser_thread.Get().Set(nullptr);
 }
 
 bool BrowserThreadImpl::StartWithOptions(const Options& options) {
@@ -398,11 +414,6 @@ bool BrowserThread::IsThreadInitialized(ID identifier) {
 
 // static
 bool BrowserThread::CurrentlyOn(ID identifier) {
-  // We shouldn't use MessageLoop::current() since it uses LazyInstance which
-  // may be deleted by ~AtExitManager when a WorkerPool thread calls this
-  // function.
-  // http://crbug.com/63678
-  base::ThreadRestrictions::ScopedAllowSingleton allow_singleton;
   BrowserThreadGlobals& globals = g_globals.Get();
   base::AutoLock lock(globals.lock);
   DCHECK(identifier >= 0 && identifier < ID_COUNT);
@@ -501,22 +512,12 @@ bool BrowserThread::GetCurrentThreadIdentifier(ID* identifier) {
   if (g_globals == NULL)
     return false;

[kinuko, 2016/01/08 11:35:50]

Do we still need to check this?

[Anand Mistry (off Chromium), 2016/01/10 23:18:28]

Nope. Removed.

 
-  // We shouldn't use MessageLoop::current() since it uses LazyInstance which
-  // may be deleted by ~AtExitManager when a WorkerPool thread calls this
-  // function.
-  // http://crbug.com/63678
-  base::ThreadRestrictions::ScopedAllowSingleton allow_singleton;
-  base::MessageLoop* cur_message_loop = base::MessageLoop::current();
-  BrowserThreadGlobals& globals = g_globals.Get();
-  for (int i = 0; i < ID_COUNT; ++i) {
-    if (globals.threads[i] &&
-        globals.threads[i]->message_loop() == cur_message_loop) {
-      *identifier = globals.threads[i]->identifier_;
-      return true;
-    }
-  }
+  BrowserThreadImpl* browser_thread = g_browser_thread.Get().Get();
+  if (!browser_thread)
+    return false;
 
-  return false;
+  *identifier = browser_thread->identifier_;
+  return true;
 }
 
 // static