Fix race in BrowserThread::GetCurrentThreadIdentifier.

content/browser/browser_thread_impl.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/browser_thread_impl.h"
 
 #include <string.h>
 
 #include <string>
 
 #include "base/atomicops.h"
 #include "base/bind.h"
 #include "base/compiler_specific.h"
 #include "base/lazy_instance.h"
 #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"
 #include "net/disk_cache/simple/simple_backend_impl.h"
 
 #if defined(OS_ANDROID)
 #include "base/android/jni_android.h"
 #endif
 
 namespace content {
@@ skipping 81 matching lines @@
   // Only atomic operations are used on this array. The delegates are not owned
   // by this array, rather by whoever calls BrowserThread::SetDelegate.
   BrowserThreadDelegate* thread_delegates[BrowserThread::ID_COUNT];
 
   const scoped_refptr<base::SequencedWorkerPool> blocking_pool;
 };
 
 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)
     : Thread(g_browser_thread_names[identifier]),
       identifier_(identifier) {
   Initialize();
 }
 
 BrowserThreadImpl::BrowserThreadImpl(ID identifier,
                                      base::MessageLoop* message_loop)
     : 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
 void BrowserThreadImpl::ShutdownThreadPool() {
   // The goal is to make it impossible for chrome to 'infinite loop' during
   // shutdown, but to reasonably expect that all BLOCKING_SHUTDOWN tasks queued
   // during shutdown get run. There's nothing particularly scientific about the
   // number chosen.
   const int kMaxNewShutdownBlockingTasks = 1000;
   BrowserThreadGlobals& globals = g_globals.Get();
@@ skipping 79 matching lines @@
 void BrowserThreadImpl::Run(base::MessageLoop* message_loop) {
 #if defined(OS_ANDROID)
   // Not to reset thread name to "Thread-???" by VM, attach VM with thread name.
   // Though it may create unnecessary VM thread objects, keeping thread name
   // gives more benefit in debugging in the platform.
   if (!thread_name().empty()) {
     base::android::AttachCurrentThreadWithName(thread_name());
   }
 #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:
       return DBThreadRun(message_loop);
     case BrowserThread::FILE:
       return FileThreadRun(message_loop);
     case BrowserThread::FILE_USER_BLOCKING:
       return FileUserBlockingThreadRun(message_loop);
     case BrowserThread::PROCESS_LAUNCHER:
       return ProcessLauncherThreadRun(message_loop);
     case BrowserThread::CACHE:
       return CacheThreadRun(message_loop);
     case BrowserThread::IO:
       return IOThreadRun(message_loop);
     case BrowserThread::ID_COUNT:
       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() {
   BrowserThreadGlobals& globals = g_globals.Get();
 
   using base::subtle::AtomicWord;
   AtomicWord* storage =
       reinterpret_cast<AtomicWord*>(&globals.thread_delegates[identifier_]);
   AtomicWord stored_pointer = base::subtle::NoBarrier_Load(storage);
   BrowserThreadDelegate* delegate =
       reinterpret_cast<BrowserThreadDelegate*>(stored_pointer);
 
   if (delegate)
     delegate->CleanUp();
+
+  // PostTaskHelper() accesses the message loop while holding this lock.
+  // However, the message loop will soon be destructed without any locking. So
+  // to prevent a race with accessing the message loop in PostTaskHelper(),
+  // remove this thread from the global array now.
+  base::AutoLock lock(globals.lock);
+  globals.threads[identifier_] = NULL;
 }
 
 void BrowserThreadImpl::Initialize() {
   BrowserThreadGlobals& globals = g_globals.Get();
 
   base::AutoLock lock(globals.lock);
   DCHECK(identifier_ >= 0 && identifier_ < ID_COUNT);
   DCHECK(globals.threads[identifier_] == NULL);
   globals.threads[identifier_] = this;
 }
 
 BrowserThreadImpl::~BrowserThreadImpl() {
   // All Thread subclasses must call Stop() in the destructor. This is
   // doubly important here as various bits of code check they are on
   // the right BrowserThread.
   Stop();
 
   BrowserThreadGlobals& globals = g_globals.Get();
   base::AutoLock lock(globals.lock);
   globals.threads[identifier_] = NULL;
 #ifndef NDEBUG
   // Double check that the threads are ordered correctly in the enumeration.
   for (int i = identifier_ + 1; i < ID_COUNT; ++i) {
     DCHECK(!globals.threads[i]) <<
         "Threads must be listed in the reverse order that they die";
   }
 #endif
+
+  // For the UI thread, a BrowserThreadImpl is created and attached to the
+  // current thread instead of spawning a new thread. Because of this, the
+  // BrowserThreadImpl pointer needs to be reset here if |this| is for the
+  // current thread.
+  if (g_browser_thread.Get().Get() == this)
+    g_browser_thread.Get().Set(nullptr);
 }
 
 bool BrowserThreadImpl::StartWithOptions(const Options& options) {
   // The global thread table needs to be locked while a new thread is
   // starting, as the new thread can asynchronously start touching the
   // table (and other thread's message_loop).
   BrowserThreadGlobals& globals = g_globals.Get();
   base::AutoLock lock(globals.lock);
   return Thread::StartWithOptions(options);
 }
@@ skipping 84 matching lines @@
     return false;
 
   BrowserThreadGlobals& globals = g_globals.Get();
   base::AutoLock lock(globals.lock);
   DCHECK(identifier >= 0 && identifier < ID_COUNT);
   return globals.threads[identifier] != NULL;
 }
 
 // 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);
-  return globals.threads[identifier] &&
-         globals.threads[identifier]->message_loop() ==
-             base::MessageLoop::current();
+
+  {
+    BrowserThreadGlobals& globals = g_globals.Get();
+    base::AutoLock lock(globals.lock);
+    // On shutdown, the global thread array could have been set to null, even

[kinuko, 2016/01/19 14:27:20]

nit: The expression 'on shutdown' feels a bit ambiguous, this could be read
either 'on the thread's shutdown' or 'during system shutdown'.

[Anand Mistry (off Chromium), 2016/01/20 02:49:48]

This whole section is gone.

+    // though we're running on the target thread. This would happen for message
+    // loop destruction observers and any object bound to a callback that is
+    // destroyed when the message loop is destroyed. In this case, fall back to
+    // using the thread local. Ideally, only the thread local would be used, but
+    // unit tests have multiple BrowserThreadImpl objects bound to the same
+    // thread.
+    if (globals.threads[identifier]) {
+      return globals.threads[identifier]->message_loop() ==
+          base::MessageLoop::current();
+    }

[kinuko, 2016/01/19 14:27:20]

Hmm I see.  But if that's the case it starts to sound a bit weird we need this
code only for testing.  If that's the case it'd be nicer this code is somehow
hidden in test-only code (like TestBrowserThreadImpl) but this is static code so
we can't do that...

If we remove the thread local impl below will we actually have a problem?

[Anand Mistry (off Chromium), 2016/01/20 02:49:48]

So, with even more staring at code, I've determined this problem doesn't exist
and so I've gotten rid of the thread-local and stuck with locking.

The reason a MessageLoop shutdown problem doesn't exist is that at the time a
Thread destroys it's MessageLoop, it has already set it's message_loop_ member
to null. So any attempt to read Thread::message_loop() during a message loop
destruciton observer will see null. Here, this means CurrentlyOn will return
false, even though you may technically still be on the correct thread.

+  }
+
+  BrowserThreadImpl* browser_thread = g_browser_thread.Get().Get();
+  if (!browser_thread)
+    return false;
+
+  return identifier == browser_thread->identifier_;
 }
 
 static const char* GetThreadName(BrowserThread::ID thread) {
   if (BrowserThread::UI < thread && thread < BrowserThread::ID_COUNT)
     return g_browser_thread_names[thread];
   if (thread == BrowserThread::UI)
     return "Chrome_UIThread";
   return "Unknown Thread";
 }
 
@@ skipping 69 matching lines @@
     const tracked_objects::Location& from_here,
     const base::Closure& task,
     const base::Closure& reply) {
   return GetMessageLoopProxyForThread(identifier)->PostTaskAndReply(from_here,
                                                                     task,
                                                                     reply);
 }
 
 // static
 bool BrowserThread::GetCurrentThreadIdentifier(ID* identifier) {
-  if (g_globals == NULL)
+  BrowserThreadImpl* browser_thread = g_browser_thread.Get().Get();
+  if (!browser_thread)
     return false;
 
-  // 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;
-    }
-  }
-
-  return false;
+  *identifier = browser_thread->identifier_;
+  return true;
 }
 
 // static
 scoped_refptr<base::SingleThreadTaskRunner>
 BrowserThread::GetMessageLoopProxyForThread(ID identifier) {
   return g_task_runners.Get().proxies[identifier];
 }
 
 // static
 base::MessageLoop* BrowserThread::UnsafeGetMessageLoopForThread(ID identifier) {
@@ skipping 16 matching lines @@
   AtomicWord* storage = reinterpret_cast<AtomicWord*>(
       &globals.thread_delegates[identifier]);
   AtomicWord old_pointer = base::subtle::NoBarrier_AtomicExchange(
       storage, reinterpret_cast<AtomicWord>(delegate));
 
   // This catches registration when previously registered.
   DCHECK(!delegate || !old_pointer);
 }
 
 }  // namespace content