Reland "Refactor BrowserThreadImpl, BrowserProcessSubThread, and BrowserMainLoop"

This is a reland of d260e9cf

It was reverted because of crbug.com/824716, these weren't new crashes
but known crashes mislabeled as a fallout of this change.
http://cl/190471699 fixes the crash backend to not rely on
"content::BrowserThreadImpl::IOThreadRun" being in the signature.

Only diff in this CL is to use base::debug::Alias() in methods that we
don't want optimized (i.e. IOThreadRun) out as CHECK_GT was seen as
optimized out in some of the reported crashes (even though the same
pattern as before was used by this CL..?)

Original change's description:
> Refactor BrowserThreadImpl, BrowserProcessSubThread, and BrowserMainLoop
>
> This brings back the invariant that BrowserThread::IO isn't available
> before BrowserMainLoop::CreateThreads(). This was broken to fix issue
> 729596 to bring up the thread earlier for ServiceManager but it is
> important that code that posts to BrowserThread::IO statically have an
> happens-after relationship to BrowserMainLoop::CreateThreads(). Exposing
> it statically earlier put that invariant at risk.
>
> Thankfully fixing issue 815225 resulted in finally reaching the long
> sought goal of only having BrowserThread::UI/IO. Now that the IO thread
> is also kicked off before it's named statically, BrowserThreadImpl no
> longer needs to be a base::Thread, hence this refactoring.
>
> Before this CL:
>  * BrowserThreadImpl was a base::Thread
>    (could be a fake thread if SetMessageLoop was used)
>  * BrowserProcessSubThread was a BrowserThreadImpl
>    (performed a bit more initialization)
>  * BrowserProcessSubThread was only used in production (in
>    BrowserMainLoop)
>  * BrowserThreadImpl was used for fake threads (BrowserMainLoop for
>    BrowserThread::UI) and for testing (TestBrowserThread(Impl)).
>  * BrowserThreadImpl overrode Init/Run/CleanUp() from base::Thread to
>    perform some sanity checks as well as drive IOThread's Delegate (ref.
>    BrowserThread::SetIOThreadDelegate())
>  * BrowserProcessSubThread re-overrode Init/Run/CleanUp() to perform
>    per-thread //content initialization (tests missed out on that per
>    TestBrowserThread bypassing BrowserProcessSubThread by directly
>    subclassing BrowserThreadImpl).
>
> With this CL:
>  * BrowserThreadImpl is merely a scoped object that binds a provided
>    SingleThreadTaskRunner to a BrowserThread::ID.
>  * BrowserProcessSubThread is a base::Thread and performs all of the
>    initialization and cleanup specific to //content (this means it now
>    also manages BrowserThread::SetIOThreadDelegate())
>  * BrowserProcessSubThread can be brought up early before being bound to
>    a BrowserThread::ID (BrowserMainLoop handles that through
>    BrowserProcessSubThread ::RegisterAsBrowserThread())
>
> Unfortunate exceptions required for this CL:
>  * IOThread::Init() (invoked through BrowserThreadDelegate) perfoms
>    blocking operations this was previously performed before installed
>    the ThreadRestrictions on BrowserThread::IO. But now that //content
>    is initialized after bringing up the thread, a
>    base::ScopedAllowBlocking is required in scope of IOThread::Init().
>  * TestBrowserThread previously bypassing BrowserProcessSubThread by
>    directly subclassing BrowserThreadImpl meant it wasn't subject to
>    ThreadRestrictions (unfortunate becomes it denies allowance
>    verification to product code running in unit tests). Adding it back
>    causes DCHECKs, as such
>    BrowserProcessSubThread::AllowBlockingForTesting was added to allow
>    this CL to pass CQ.
>
> Of note:
>  * BrowserProcessSubThread is still written as though it supports many
>    BrowserThread::IDs but in practice it's mostly always
>    BrowserThread::IO (except in ThreadWatcherTest I think). This change
>    was big enough that I didn't bother also breaking that
>    generalization.
>  * BrowserThreadImpl's constructor was made private to ensure only
>    BrowserProcessSubThread and a few select callers get to drive it (to
>    avoid previous missed initialization issues)
>  * Atomics to manage BrowserThread::SetIOThreadDelegate were removed.
>    Restriction was instead added that this only be called before
>    initialization and after shutdown (this was already the case).
>
> Follow-ups to this CL:
>  * //ios duplicates this logic and will need to undergo the same change
>    as a follow-up
>  * Fixing ios will allow removal of base::Thread::SetMessageLoop hack :)
>  * Removing BrowserThreadGlobals::lock_ to address crbug.com/821034 will
>    be much easier
>  * BrowserThread post APIs should DCHECK rather than no-op if using a
>    BrowserThread::ID before it's registered.
>
> Bug: 815225, 821034, 729596
> Change-Id: If1038f23079df72203b1e95c7d26647f8824a726
> Reviewed-on: https://chromium-review.googlesource.com/969104
> Reviewed-by: John Abd-El-Malek <jam@chromium.org>
> Commit-Queue: Gabriel Charette <gab@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#544440}

TBR=jam@chromium.org

Bug: 815225, 821034, 729596, 824716
Change-Id: I9a180975c69a008f8519d1d3d44663aa58a74a92
Reviewed-on: https://chromium-review.googlesource.com/980793Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Commit-Queue: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#546104}

base/threading/thread.h

@@ -271,6 +271,9 @@ class BASE_EXPORT Thread : PlatformThread::Delegate {
   static bool GetThreadWasQuitProperly();
 
   // Bind this Thread to an existing MessageLoop instead of starting a new one.
+  // TODO(gab): Remove this after ios/ has undergone the same surgery as
+  // BrowserThreadImpl (ref.
+  // https://chromium-review.googlesource.com/c/chromium/src/+/969104).
   void SetMessageLoop(MessageLoop* message_loop);
 
   bool using_external_message_loop() const {

base/threading/thread_restrictions.h

@@ -38,6 +38,7 @@ namespace content {
 class BrowserGpuChannelHostFactory;
 class BrowserGpuMemoryBufferManager;
 class BrowserMainLoop;
+class BrowserProcessSubThread;
 class BrowserShutdownProfileDumper;
 class BrowserSurfaceViewManager;
 class BrowserTestBase;
@@ -211,6 +212,7 @@ class BASE_EXPORT ScopedAllowBlocking {
   // in unit tests to avoid the friend requirement.
   FRIEND_TEST_ALL_PREFIXES(ThreadRestrictionsTest, ScopedAllowBlocking);
   friend class android_webview::ScopedAllowInitGLBindings;
+  friend class content::BrowserProcessSubThread;
   friend class cronet::CronetPrefsManager;
   friend class cronet::CronetURLRequestContext;
   friend class resource_coordinator::TabManagerDelegate;  // crbug.com/778703

chrome/browser/browser_process_impl_unittest.cc

@@ -31,10 +31,9 @@ class BrowserProcessImplTest : public ::testing::Test {
       : stashed_browser_process_(g_browser_process),
         loop_(base::MessageLoop::TYPE_UI),
         ui_thread_(content::BrowserThread::UI, &loop_),
-        io_thread_(new content::TestBrowserThread(content::BrowserThread::IO)),
         command_line_(base::CommandLine::NO_PROGRAM),
-        browser_process_impl_(std::make_unique<BrowserProcessImpl>(
-            base::ThreadTaskRunnerHandle::Get().get())) {
+        browser_process_impl_(
+            new BrowserProcessImpl(base::ThreadTaskRunnerHandle::Get().get())) {
     // Create() and StartWithDefaultParams() TaskScheduler in seperate steps to
     // properly simulate the browser process' lifecycle.
     base::TaskScheduler::Create("BrowserProcessImplTest");
@@ -49,17 +48,20 @@ class BrowserProcessImplTest : public ::testing::Test {
     g_browser_process = stashed_browser_process_;
   }
 
-  // Creates the secondary thread (IO thread).
-  // The UI thread needs to be alive while BrowserProcessImpl is alive, and is
-  // managed separately.
+  // Creates the IO thread (unbound) and task scheduler threads. The UI thread
+  // needs to be alive while BrowserProcessImpl is alive, and is managed
+  // separately.
   void StartSecondaryThreads() {
     base::TaskScheduler::GetInstance()->StartWithDefaultParams();
-    io_thread_->StartIOThread();
+
+    io_thread_ = std::make_unique<content::TestBrowserThread>(
+        content::BrowserThread::IO);
+    io_thread_->StartIOThreadUnregistered();
   }
 
-  // Initializes the IO thread delegate and starts the ServiceManager.
+  // Binds the IO thread to BrowserThread::IO and starts the ServiceManager.
   void Initialize() {
-    io_thread_->InitIOThreadDelegate();
+    io_thread_->RegisterAsBrowserThread();
 
     // TestServiceManagerContext creation requires the task scheduler to be
     // started.

content/browser/browser_main_loop.cc

@@ -15,6 +15,7 @@
 #include "base/base_switches.h"
 #include "base/bind.h"
 #include "base/command_line.h"
+#include "base/debug/alias.h"
 #include "base/deferred_sequenced_task_runner.h"
 #include "base/feature_list.h"
 #include "base/location.h"
@@ -37,6 +38,7 @@
 #include "base/synchronization/waitable_event.h"
 #include "base/system_monitor/system_monitor.h"
 #include "base/task_scheduler/initialization_util.h"
+#include "base/threading/thread.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/time.h"
@@ -54,6 +56,7 @@
 #include "components/viz/service/display_embedder/compositing_mode_reporter_impl.h"
 #include "components/viz/service/display_embedder/server_shared_bitmap_manager.h"
 #include "components/viz/service/frame_sinks/frame_sink_manager_impl.h"
+#include "content/browser/browser_process_sub_thread.h"
 #include "content/browser/browser_thread_impl.h"
 #include "content/browser/child_process_security_policy_impl.h"
 #include "content/browser/compositor/gpu_process_transport_factory.h"
@@ -369,10 +372,11 @@ void OnStoppedStartupTracing(const base::FilePath& trace_file) {
 MSVC_DISABLE_OPTIMIZE()
 MSVC_PUSH_DISABLE_WARNING(4748)
 
-NOINLINE void ResetThread_IO(std::unique_ptr<BrowserProcessSubThread> thread) {
-  volatile int inhibit_comdat = __LINE__;
-  ALLOW_UNUSED_LOCAL(inhibit_comdat);
-  thread.reset();
+NOINLINE void ResetThread_IO(
+    std::unique_ptr<BrowserProcessSubThread> io_thread) {
+  const int line_number = __LINE__;
+  io_thread.reset();
+  base::debug::Alias(&line_number);
 }
 
 MSVC_POP_WARNING()
@@ -994,11 +998,13 @@ int BrowserMainLoop::CreateThreads() {
         *task_scheduler_init_params.get());
   }
 
-  // |io_thread_| is created by |PostMainMessageLoopStart()|, but its
-  // full initialization is deferred until this point because it requires
-  // several dependencies we don't want to depend on so early in startup.
+  // The thread used for BrowserThread::IO is created in
+  // |PostMainMessageLoopStart()|, but it's only tagged as BrowserThread::IO
+  // here in order to prevent any code from statically posting to it before
+  // CreateThreads() (as such maintaining the invariant that PreCreateThreads()
+  // et al. "happen-before" BrowserThread::IO is "brought up").
   DCHECK(io_thread_);
-  io_thread_->InitIOThreadDelegate();
+  io_thread_->RegisterAsBrowserThread();
 
   created_threads_ = true;
   return result_code_;
@@ -1236,9 +1242,12 @@ void BrowserMainLoop::InitializeMainThread() {
   TRACE_EVENT0("startup", "BrowserMainLoop::InitializeMainThread");
   base::PlatformThread::SetName("CrBrowserMain");
 
-  // Register the main thread by instantiating it, but don't call any methods.
-  main_thread_.reset(
-      new BrowserThreadImpl(BrowserThread::UI, base::MessageLoop::current()));
+  // Register the main thread. The main thread's task runner should already have
+  // been initialized in MainMessageLoopStart() (or before if
+  // MessageLoop::current() was externally provided).
+  DCHECK(base::ThreadTaskRunnerHandle::IsSet());
+  main_thread_.reset(new BrowserThreadImpl(
+      BrowserThread::UI, base::ThreadTaskRunnerHandle::Get()));
 }
 
 int BrowserMainLoop::BrowserThreadsStarted() {
@@ -1410,7 +1419,7 @@ int BrowserMainLoop::BrowserThreadsStarted() {
         "startup",
         "BrowserMainLoop::BrowserThreadsStarted::InitUserInputMonitor");
     user_input_monitor_ = media::UserInputMonitor::Create(
-        io_thread_->task_runner(), main_thread_->task_runner());
+        io_thread_->task_runner(), base::ThreadTaskRunnerHandle::Get());
   }
 
   {
@@ -1560,9 +1569,10 @@ void BrowserMainLoop::InitializeIOThread() {
   options.priority = base::ThreadPriority::DISPLAY;
 #endif
 
-  io_thread_.reset(new BrowserProcessSubThread(BrowserThread::IO));
+  io_thread_ = std::make_unique<BrowserProcessSubThread>(BrowserThread::IO);
+
   if (!io_thread_->StartWithOptions(options))
-    LOG(FATAL) << "Failed to start the browser thread: IO";
+    LOG(FATAL) << "Failed to start BrowserThread::IO";
 }
 
 void BrowserMainLoop::InitializeMojo() {

content/browser/browser_main_loop.h

@@ -13,7 +13,6 @@
 #include "base/memory/ref_counted.h"
 #include "base/timer/timer.h"
 #include "build/build_config.h"
-#include "content/browser/browser_process_sub_thread.h"
 #include "content/public/browser/browser_main_runner.h"
 #include "media/media_buildflags.h"
 #include "mojo/public/cpp/bindings/binding_set.h"
@@ -92,6 +91,7 @@ class HostFrameSinkManager;
 namespace content {
 class BrowserMainParts;
 class BrowserOnlineStateObserver;
+class BrowserProcessSubThread;
 class BrowserThreadImpl;
 class LoaderDelegateImpl;
 class MediaStreamManager;
@@ -243,7 +243,10 @@ class CONTENT_EXPORT BrowserMainLoop {
 
   void MainMessageLoopRun();
 
+  // Initializes |io_thread_|. It will not be promoted to BrowserThread::IO
+  // until CreateThreads().
   void InitializeIOThread();
+
   void InitializeMojo();
   base::FilePath GetStartupTraceFileName(
       const base::CommandLine& command_line) const;
@@ -285,6 +288,7 @@ class CONTENT_EXPORT BrowserMainLoop {
   std::unique_ptr<base::MessageLoop> main_message_loop_;
 
   // Members initialized in |PostMainMessageLoopStart()| -----------------------
+  std::unique_ptr<BrowserProcessSubThread> io_thread_;
   std::unique_ptr<base::SystemMonitor> system_monitor_;
   std::unique_ptr<base::PowerMonitor> power_monitor_;
   std::unique_ptr<base::HighResolutionTimerManager> hi_res_timer_manager_;
@@ -335,9 +339,6 @@ class CONTENT_EXPORT BrowserMainLoop {
       gpu_data_manager_visual_proxy_;
 #endif
 
-  // Members initialized in |CreateThreads()| ----------------------------------
-  std::unique_ptr<BrowserProcessSubThread> io_thread_;
-
   // Members initialized in |BrowserThreadsStarted()| --------------------------
   std::unique_ptr<ServiceManagerContext> service_manager_context_;
   std::unique_ptr<mojo::edk::ScopedIPCSupport> mojo_ipc_support_;

content/browser/browser_main_loop_unittest.cc

@@ -9,6 +9,7 @@
 #include "base/sys_info.h"
 #include "base/task_scheduler/task_scheduler.h"
 #include "base/test/scoped_command_line.h"
+#include "content/browser/browser_thread_impl.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/common/main_function_params.h"

content/browser/browser_main_runner.cc

@@ -16,6 +16,7 @@
 #include "base/run_loop.h"
 #include "base/sampling_heap_profiler/sampling_heap_profiler.h"
 #include "base/strings/string_number_conversions.h"
+#include "base/synchronization/atomic_flag.h"
 #include "base/time/time.h"
 #include "base/trace_event/heap_profiler_allocation_context_tracker.h"
 #include "base/trace_event/trace_event.h"

content/browser/browser_process_sub_thread.cc

@@ -4,66 +4,166 @@
 
 #include "content/browser/browser_process_sub_thread.h"
 
-#include "base/debug/leak_tracker.h"
+#include "base/compiler_specific.h"
+#include "base/debug/alias.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/trace_event/memory_dump_manager.h"
-#include "build/build_config.h"
 #include "content/browser/browser_child_process_host_impl.h"
+#include "content/browser/browser_thread_impl.h"
 #include "content/browser/gpu/browser_gpu_memory_buffer_manager.h"
 #include "content/browser/notification_service_impl.h"
+#include "content/public/browser/browser_thread_delegate.h"
 #include "net/url_request/url_fetcher.h"
 #include "net/url_request/url_request.h"
 
+#if defined(OS_ANDROID)
+#include "base/android/jni_android.h"
+#endif
+
 #if defined(OS_WIN)
 #include "base/win/scoped_com_initializer.h"
 #endif
 
 namespace content {
 
-BrowserProcessSubThread::BrowserProcessSubThread(BrowserThread::ID identifier)
-    : BrowserThreadImpl(identifier) {}
+namespace {
+BrowserThreadDelegate* g_io_thread_delegate = nullptr;
+}  // namespace
 
-BrowserProcessSubThread::BrowserProcessSubThread(
-    BrowserThread::ID identifier,
-    base::MessageLoop* message_loop)
-    : BrowserThreadImpl(identifier, message_loop) {}
+// static
+void BrowserThread::SetIOThreadDelegate(BrowserThreadDelegate* delegate) {
+  // |delegate| can only be set/unset while BrowserThread::IO isn't up.
+  DCHECK(!BrowserThread::IsThreadInitialized(BrowserThread::IO));
+  // and it cannot be set twice.
+  DCHECK(!g_io_thread_delegate || !delegate);
+
+  g_io_thread_delegate = delegate;
+}
+
+BrowserProcessSubThread::BrowserProcessSubThread(BrowserThread::ID identifier)
+    : base::Thread(BrowserThreadImpl::GetThreadName(identifier)),
+      identifier_(identifier) {
+  // Not bound to creation thread.
+  DETACH_FROM_THREAD(browser_thread_checker_);
+}
 
 BrowserProcessSubThread::~BrowserProcessSubThread() {
   Stop();
 }
 
+void BrowserProcessSubThread::RegisterAsBrowserThread() {
+  DCHECK(IsRunning());
+
+  DCHECK(!browser_thread_);
+  browser_thread_.reset(new BrowserThreadImpl(identifier_, task_runner()));
+
+  // Unretained(this) is safe as |this| outlives its underlying thread.
+  task_runner()->PostTask(
+      FROM_HERE,
+      base::BindOnce(
+          &BrowserProcessSubThread::CompleteInitializationOnBrowserThread,
+          Unretained(this)));
+}
+
+void BrowserProcessSubThread::AllowBlockingForTesting() {
+  DCHECK(!IsRunning());
+  is_blocking_allowed_for_testing_ = true;
+}
+
 void BrowserProcessSubThread::Init() {
+  DCHECK_CALLED_ON_VALID_THREAD(browser_thread_checker_);
+
 #if defined(OS_WIN)
-  com_initializer_.reset(new base::win::ScopedCOMInitializer());
+  com_initializer_ = std::make_unique<base::win::ScopedCOMInitializer>();
 #endif
 
-  notification_service_.reset(new NotificationServiceImpl());
+  if (!is_blocking_allowed_for_testing_) {
+    base::DisallowBlocking();
+    base::DisallowBaseSyncPrimitives();
+  }
+}
+
+void BrowserProcessSubThread::Run(base::RunLoop* run_loop) {
+  DCHECK_CALLED_ON_VALID_THREAD(browser_thread_checker_);
 
-  BrowserThreadImpl::Init();
+#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
 
-  if (BrowserThread::CurrentlyOn(BrowserThread::IO)) {
-    // Though this thread is called the "IO" thread, it actually just routes
-    // messages around; it shouldn't be allowed to perform any blocking disk
-    // I/O.
-    base::ThreadRestrictions::SetIOAllowed(false);
-    base::ThreadRestrictions::DisallowWaiting();
+  switch (identifier_) {
+    case BrowserThread::UI:
+      // The main thread is usually promoted as the UI thread and doesn't go
+      // through Run() but some tests do run a separate UI thread.
+      UIThreadRun(run_loop);
+      break;
+    case BrowserThread::IO:
+      IOThreadRun(run_loop);
+      return;
+    case BrowserThread::ID_COUNT:
+      NOTREACHED();
+      break;
   }
 }
 
 void BrowserProcessSubThread::CleanUp() {
+  DCHECK_CALLED_ON_VALID_THREAD(browser_thread_checker_);
+
+  // Run extra cleanup if this thread represents BrowserThread::IO.
   if (BrowserThread::CurrentlyOn(BrowserThread::IO))
-    IOThreadPreCleanUp();
+    IOThreadCleanUp();
 
-  BrowserThreadImpl::CleanUp();
+  if (identifier_ == BrowserThread::IO && g_io_thread_delegate)
+    g_io_thread_delegate->CleanUp();
 
   notification_service_.reset();
 
 #if defined(OS_WIN)
   com_initializer_.reset();
 #endif
+
+  browser_thread_.reset();
 }
 
-void BrowserProcessSubThread::IOThreadPreCleanUp() {
+void BrowserProcessSubThread::CompleteInitializationOnBrowserThread() {
+  DCHECK_CALLED_ON_VALID_THREAD(browser_thread_checker_);
+
+  notification_service_ = std::make_unique<NotificationServiceImpl>();
+
+  if (identifier_ == BrowserThread::IO && g_io_thread_delegate) {
+    // Allow blocking calls while initializing the IO thread.
+    base::ScopedAllowBlocking allow_blocking_for_init;
+    g_io_thread_delegate->Init();
+  }
+}
+
+// We disable optimizations for Run specifications so the compiler doesn't merge
+// them all together.
+MSVC_DISABLE_OPTIMIZE()
+MSVC_PUSH_DISABLE_WARNING(4748)
+
+void BrowserProcessSubThread::UIThreadRun(base::RunLoop* run_loop) {
+  const int line_number = __LINE__;
+  Thread::Run(run_loop);
+  base::debug::Alias(&line_number);
+}
+
+void BrowserProcessSubThread::IOThreadRun(base::RunLoop* run_loop) {
+  const int line_number = __LINE__;
+  Thread::Run(run_loop);
+  base::debug::Alias(&line_number);
+}
+
+MSVC_POP_WARNING()
+MSVC_ENABLE_OPTIMIZE();
+
+void BrowserProcessSubThread::IOThreadCleanUp() {
+  DCHECK_CALLED_ON_VALID_THREAD(browser_thread_checker_);
+
   // Kill all things that might be holding onto
   // net::URLRequest/net::URLRequestContexts.
 

content/browser/browser_process_sub_thread.h

@@ -8,8 +8,9 @@
 #include <memory>
 
 #include "base/macros.h"
+#include "base/threading/thread.h"
+#include "base/threading/thread_checker.h"
 #include "build/build_config.h"
-#include "content/browser/browser_thread_impl.h"
 #include "content/common/content_export.h"
 #include "content/public/browser/browser_thread.h"
 
@@ -28,29 +29,57 @@ class NotificationService;
 namespace content {
 
 // ----------------------------------------------------------------------------
-// BrowserProcessSubThread
-//
-// This simple thread object is used for the specialized threads that the
-// BrowserProcess spins up.
+// A BrowserProcessSubThread is a physical thread backing a BrowserThread.
 //
 // Applications must initialize the COM library before they can call
 // COM library functions other than CoGetMalloc and memory allocation
 // functions, so this class initializes COM for those users.
-class CONTENT_EXPORT BrowserProcessSubThread : public BrowserThreadImpl {
+class CONTENT_EXPORT BrowserProcessSubThread : public base::Thread {
  public:
+  // Constructs a BrowserProcessSubThread for |identifier|.
   explicit BrowserProcessSubThread(BrowserThread::ID identifier);
-  BrowserProcessSubThread(BrowserThread::ID identifier,
-                          base::MessageLoop* message_loop);
   ~BrowserProcessSubThread() override;
 
+  // Registers this thread to represent |identifier_| in the browser_thread.h
+  // API. This thread must already be running when this is called. This can only
+  // be called once per BrowserProcessSubThread instance.
+  void RegisterAsBrowserThread();
+
+  // Ideally there wouldn't be a special blanket allowance to block the
+  // BrowserThreads in tests but TestBrowserThreadImpl previously bypassed
+  // BrowserProcessSubThread and hence wasn't subject to ThreadRestrictions...
+  // Flipping that around in favor of explicit scoped allowances would be
+  // preferable but a non-trivial amount of work. Can only be called before
+  // starting this BrowserProcessSubThread.
+  void AllowBlockingForTesting();
+
  protected:
   void Init() override;
+  void Run(base::RunLoop* run_loop) override;
   void CleanUp() override;
 
  private:
-  // These methods encapsulate cleanup that needs to happen on the IO thread
-  // before we call the embedder's CleanUp function.
-  void IOThreadPreCleanUp();
+  // Second Init() phase that must happen on this thread but can only happen
+  // after it's promoted to a BrowserThread in |RegisterAsBrowserThread()|.
+  void CompleteInitializationOnBrowserThread();
+
+  // These methods merely forwards to Thread::Run() but are useful to identify
+  // which BrowserThread this represents in stack traces.
+  void UIThreadRun(base::RunLoop* run_loop);
+  void IOThreadRun(base::RunLoop* run_loop);
+
+  // This method encapsulates cleanup that needs to happen on the IO thread.
+  void IOThreadCleanUp();
+
+  const BrowserThread::ID identifier_;
+
+  // BrowserThreads are not allowed to do file I/O nor wait on synchronization
+  // primivives except when explicitly allowed in tests.
+  bool is_blocking_allowed_for_testing_ = false;
+
+  // The BrowserThread registration for this |identifier_|, initialized in
+  // RegisterAsBrowserThread().
+  std::unique_ptr<BrowserThreadImpl> browser_thread_;
 
 #if defined (OS_WIN)
   std::unique_ptr<base::win::ScopedCOMInitializer> com_initializer_;
@@ -59,6 +88,8 @@ class CONTENT_EXPORT BrowserProcessSubThread : public BrowserThreadImpl {
   // Each specialized thread has its own notification service.
   std::unique_ptr<NotificationService> notification_service_;
 
+  THREAD_CHECKER(browser_thread_checker_);
+
   DISALLOW_COPY_AND_ASSIGN(BrowserProcessSubThread);
 };
 

content/browser/browser_thread_impl.h

@@ -5,47 +5,31 @@
 #ifndef CONTENT_BROWSER_BROWSER_THREAD_IMPL_H_
 #define CONTENT_BROWSER_BROWSER_THREAD_IMPL_H_
 
-#include "base/callback.h"
+#include "base/memory/scoped_refptr.h"
 #include "base/single_thread_task_runner.h"
-#include "base/threading/thread.h"
-#include "base/time/time.h"
 #include "content/common/content_export.h"
 #include "content/public/browser/browser_thread.h"
 
-namespace base {
-class MessageLoop;
-class RunLoop;
-}
-
-namespace base {
-class Location;
-}
-
 namespace content {
 
+class BrowserMainLoop;
+class BrowserProcessSubThread;
+class TestBrowserThread;
+
+// BrowserThreadImpl is a scoped object which maps a SingleThreadTaskRunner to a
+// BrowserThread::ID. On ~BrowserThreadImpl() that ID enters a SHUTDOWN state
+// (in which BrowserThread::IsThreadInitialized() returns false) but the mapping
+// isn't undone to avoid shutdown races (the task runner is free to stop
+// accepting tasks however).
+//
 // Very few users should use this directly. To mock BrowserThreads, tests should
 // use TestBrowserThreadBundle instead.
-class CONTENT_EXPORT BrowserThreadImpl : public BrowserThread,
-                                         public base::Thread {
+class CONTENT_EXPORT BrowserThreadImpl : public BrowserThread {
  public:
-  // Construct a BrowserThreadImpl with the supplied identifier.  It is an error
-  // to construct a BrowserThreadImpl that already exists.
-  explicit BrowserThreadImpl(BrowserThread::ID identifier);
+  ~BrowserThreadImpl();
 
-  // Special constructor for the main (UI) thread and unittests. If a
-  // |message_loop| is provied, we use a dummy thread here since the main
-  // thread already exists.
-  BrowserThreadImpl(BrowserThread::ID identifier,
-                    base::MessageLoop* message_loop);
-  ~BrowserThreadImpl() override;
-
-  bool Start();
-  bool StartWithOptions(const Options& options);
-  bool StartAndWaitForTesting();
-  // Called only by the BrowserThread::IO thread to initialize its
-  // BrowserThreadDelegate after the thread is created. See
-  // https://crbug.com/729596.
-  void InitIOThreadDelegate();
+  // Returns the thread name for |identifier|.
+  static const char* GetThreadName(BrowserThread::ID identifier);
 
   // Resets globals for |identifier|. Used in tests to clear global state that
   // would otherwise leak to the next test. Globals are not otherwise fully
@@ -55,35 +39,19 @@ class CONTENT_EXPORT BrowserThreadImpl : public BrowserThread,
   // |identifier|.
   static void ResetGlobalsForTesting(BrowserThread::ID identifier);
 
- protected:
-  void Init() override;
-  void Run(base::RunLoop* run_loop) override;
-  void CleanUp() override;
-
  private:
-  // We implement all the functionality of the public BrowserThread
-  // functions, but state is stored in the BrowserThreadImpl to keep
-  // the API cleaner. Therefore make BrowserThread a friend class.
-  friend class BrowserThread;
-
-  // The following are unique function names that makes it possible to tell
-  // the thread id from the callstack alone in crash dumps.
-  void UIThreadRun(base::RunLoop* run_loop);
-  void ProcessLauncherThreadRun(base::RunLoop* run_loop);
-  void IOThreadRun(base::RunLoop* run_loop);
-
-  static bool PostTaskHelper(BrowserThread::ID identifier,
-                             const base::Location& from_here,
-                             base::OnceClosure task,
-                             base::TimeDelta delay,
-                             bool nestable);
-
-  // Common initialization code for the constructors.
-  void Initialize();
-
-  // For testing.
-  friend class ContentTestSuiteBaseListener;
-  friend class TestBrowserThreadBundle;
+  // Restrict instantiation to BrowserProcessSubThread as it performs important
+  // initialization that shouldn't be bypassed (except by BrowserMainLoop for
+  // the main thread).
+  friend class BrowserProcessSubThread;
+  friend class BrowserMainLoop;
+  // TestBrowserThread is also allowed to construct this when instantiating fake
+  // threads.
+  friend class TestBrowserThread;
+
+  // Binds |identifier| to |task_runner| for the browser_thread.h API.
+  BrowserThreadImpl(BrowserThread::ID identifier,
+                    scoped_refptr<base::SingleThreadTaskRunner> task_runner);
 
   // The identifier of this thread.  Only one thread can exist with a given
   // identifier at a given time.

content/browser/browser_thread_unittest.cc

@@ -12,6 +12,7 @@
 #include "base/sequenced_task_runner_helpers.h"
 #include "base/single_thread_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "content/browser/browser_process_sub_thread.h"
 #include "content/browser/browser_thread_impl.h"
 #include "content/public/test/test_browser_thread.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -31,17 +32,22 @@ class BrowserThreadTest : public testing::Test {
 
  protected:
   void SetUp() override {
-    ui_thread_.reset(new BrowserThreadImpl(BrowserThread::UI));
-    io_thread_.reset(new BrowserThreadImpl(BrowserThread::IO));
+    ui_thread_ = std::make_unique<BrowserProcessSubThread>(BrowserThread::UI);
     ui_thread_->Start();
-    io_thread_->Start();
+
+    io_thread_ = std::make_unique<BrowserProcessSubThread>(BrowserThread::IO);
+    base::Thread::Options io_options;
+    io_options.message_loop_type = base::MessageLoop::TYPE_IO;
+    io_thread_->StartWithOptions(io_options);
+
+    ui_thread_->RegisterAsBrowserThread();
+    io_thread_->RegisterAsBrowserThread();
   }
 
   void TearDown() override {
-    StopUIThread();
-    io_thread_->Stop();
-    ui_thread_ = nullptr;
-    io_thread_ = nullptr;
+    io_thread_.reset();
+    ui_thread_.reset();
+
     BrowserThreadImpl::ResetGlobalsForTesting(BrowserThread::UI);
     BrowserThreadImpl::ResetGlobalsForTesting(BrowserThread::IO);
   }
@@ -73,8 +79,8 @@ class BrowserThreadTest : public testing::Test {
   };
 
  private:
-  std::unique_ptr<BrowserThreadImpl> ui_thread_;
-  std::unique_ptr<BrowserThreadImpl> io_thread_;
+  std::unique_ptr<BrowserProcessSubThread> ui_thread_;
+  std::unique_ptr<BrowserProcessSubThread> io_thread_;
   // It's kind of ugly to make this mutable - solely so we can post the Quit
   // Task from Release(). This should be fixed.
   mutable base::MessageLoop loop_;

content/public/browser/browser_thread.h

@@ -172,9 +172,11 @@ class CONTENT_EXPORT BrowserThread {
   // thread.  To DCHECK this, use the DCHECK_CURRENTLY_ON() macro above.
   static bool CurrentlyOn(ID identifier) WARN_UNUSED_RESULT;
 
+  // Deprecated: This is equivalent to IsThreadInitialized().
   // Callable on any thread.  Returns whether the threads message loop is valid.
   // If this returns false it means the thread is in the process of shutting
   // down.
+  // TODO(gab): Replace callers with IsThreadInitialized().
   static bool IsMessageLoopValid(ID identifier) WARN_UNUSED_RESULT;
 
   // If the current message loop is one of the known threads, returns true and
@@ -188,16 +190,12 @@ class CONTENT_EXPORT BrowserThread {
 
   // Sets the delegate for BrowserThread::IO.
   //
-  // This only supports the IO thread as it doesn't work for potentially
-  // redirected threads (ref. http://crbug.com/653916) and also doesn't make
-  // sense for the UI thread.
-  //
   // Only one delegate may be registered at a time. The delegate may be
   // unregistered by providing a nullptr pointer.
   //
-  // If the caller unregisters the delegate before CleanUp has been called, it
-  // must perform its own locking to ensure the delegate is not deleted while
-  // unregistering.
+  // The delegate can only be registered through this call before
+  // BrowserThreadImpl(BrowserThread::IO) is created and unregistered after
+  // it was destroyed and its underlying thread shutdown.
   static void SetIOThreadDelegate(BrowserThreadDelegate* delegate);
 
   // Use these templates in conjunction with RefCountedThreadSafe or scoped_ptr

content/public/browser/browser_thread_delegate.h

@@ -5,22 +5,20 @@
 #ifndef CONTENT_PUBLIC_BROWSER_BROWSER_THREAD_DELEGATE_H_
 #define CONTENT_PUBLIC_BROWSER_BROWSER_THREAD_DELEGATE_H_
 
+#include "content/common/content_export.h"
+
 namespace content {
 
-// BrowserThread::SetDelegate was deprecated, this is now only used by
-// BrowserThread::SetIOThreadDelegate.
-//
-// When registered as such, it will schedule to run Init() before the message
-// loop begins and receive a CleanUp call right after the message loop ends (and
-// before the BrowserThread has done its own clean-up).
+// A Delegate for content embedders to perform extra initialization/cleanup on
+// BrowserThread::IO.
 class BrowserThreadDelegate {
  public:
   virtual ~BrowserThreadDelegate() = default;
 
-  // Called prior to starting the message loop
+  // Called prior to completing initialization of BrowserThread::IO.
   virtual void Init() = 0;
 
-  // Called just after the message loop ends.
+  // Called during teardown of BrowserThread::IO.
   virtual void CleanUp() = 0;
 };
 

content/public/test/test_browser_thread.cc

@@ -9,71 +9,32 @@
 #include "base/message_loop/message_loop.h"
 #include "base/threading/thread.h"
 #include "build/build_config.h"
+#include "content/browser/browser_process_sub_thread.h"
 #include "content/browser/browser_thread_impl.h"
-#include "content/browser/notification_service_impl.h"
-
-#if defined(OS_WIN)
-#include "base/win/scoped_com_initializer.h"
-#endif
 
 namespace content {
 
-class TestBrowserThreadImpl : public BrowserThreadImpl {
- public:
-  explicit TestBrowserThreadImpl(BrowserThread::ID identifier)
-      : BrowserThreadImpl(identifier) {}
-
-  TestBrowserThreadImpl(BrowserThread::ID identifier,
-                        base::MessageLoop* message_loop)
-      : BrowserThreadImpl(identifier, message_loop) {}
-
-  ~TestBrowserThreadImpl() override { Stop(); }
-
-  void Init() override {
-#if defined(OS_WIN)
-    com_initializer_ = std::make_unique<base::win::ScopedCOMInitializer>();
-#endif
-
-    notification_service_ = std::make_unique<NotificationServiceImpl>();
-    BrowserThreadImpl::Init();
-  }
-
-  void CleanUp() override {
-    BrowserThreadImpl::CleanUp();
-    notification_service_.reset();
-#if defined(OS_WIN)
-    com_initializer_.reset();
-#endif
-  }
-
- private:
-#if defined(OS_WIN)
-  std::unique_ptr<base::win::ScopedCOMInitializer> com_initializer_;
-#endif
-
-  std::unique_ptr<NotificationService> notification_service_;
-
-  DISALLOW_COPY_AND_ASSIGN(TestBrowserThreadImpl);
-};
-
 TestBrowserThread::TestBrowserThread(BrowserThread::ID identifier)
-    : impl_(new TestBrowserThreadImpl(identifier)), identifier_(identifier) {}
+    : identifier_(identifier),
+      real_thread_(std::make_unique<BrowserProcessSubThread>(identifier_)) {
+  real_thread_->AllowBlockingForTesting();
+}
 
 TestBrowserThread::TestBrowserThread(BrowserThread::ID identifier,
                                      base::MessageLoop* message_loop)
-    : impl_(new TestBrowserThreadImpl(identifier, message_loop)),
-      identifier_(identifier) {}
+    : identifier_(identifier),
+      fake_thread_(
+          new BrowserThreadImpl(identifier_, message_loop->task_runner())) {}
 
 TestBrowserThread::~TestBrowserThread() {
   // The upcoming BrowserThreadImpl::ResetGlobalsForTesting() call requires that
-  // |impl_| have triggered the shutdown phase for its BrowserThread::ID. This
-  // either happens when the thread is stopped (if real) or destroyed (when fake
-  // -- i.e. using an externally provided MessageLoop).
-  impl_.reset();
+  // |identifier_| have completed its SHUTDOWN phase.
+  real_thread_.reset();
+  fake_thread_.reset();
 
-  // Resets BrowserThreadImpl's globals so that |impl_| is no longer bound to
-  // |identifier_|. This is fine since the underlying MessageLoop has already
-  // been flushed and deleted in Stop(). In the case of an externally provided
+  // Resets BrowserThreadImpl's globals so that |identifier_| is no longer
+  // bound. This is fine since the underlying MessageLoop has already been
+  // flushed and deleted above. In the case of an externally provided
   // MessageLoop however, this means that TaskRunners obtained through
   // |BrowserThreadImpl::GetTaskRunnerForThread(identifier_)| will no longer
   // recognize their BrowserThreadImpl for RunsTasksInCurrentSequence(). This
@@ -87,30 +48,34 @@ TestBrowserThread::~TestBrowserThread() {
   BrowserThreadImpl::ResetGlobalsForTesting(identifier_);
 }
 
-bool TestBrowserThread::Start() {
-  return impl_->Start();
+void TestBrowserThread::Start() {
+  CHECK(real_thread_->Start());
+  RegisterAsBrowserThread();
 }
 
-bool TestBrowserThread::StartAndWaitForTesting() {
-  return impl_->StartAndWaitForTesting();
+void TestBrowserThread::StartAndWaitForTesting() {
+  CHECK(real_thread_->StartAndWaitForTesting());
+  RegisterAsBrowserThread();
 }
 
-bool TestBrowserThread::StartIOThread() {
+void TestBrowserThread::StartIOThread() {
+  StartIOThreadUnregistered();
+  RegisterAsBrowserThread();
+}
+
+void TestBrowserThread::StartIOThreadUnregistered() {
   base::Thread::Options options;
   options.message_loop_type = base::MessageLoop::TYPE_IO;
-  return impl_->StartWithOptions(options);
+  CHECK(real_thread_->StartWithOptions(options));
 }
 
-void TestBrowserThread::InitIOThreadDelegate() {
-  impl_->InitIOThreadDelegate();
+void TestBrowserThread::RegisterAsBrowserThread() {
+  real_thread_->RegisterAsBrowserThread();
 }
 
 void TestBrowserThread::Stop() {
-  impl_->Stop();
-}
-
-bool TestBrowserThread::IsRunning() {
-  return impl_->IsRunning();
+  if (real_thread_)
+    real_thread_->Stop();
 }
 
 }  // namespace content

content/public/test/test_browser_thread.h

@@ -12,20 +12,27 @@
 
 namespace base {
 class MessageLoop;
+class Thread;
 }
 
 namespace content {
 
-class TestBrowserThreadImpl;
+class BrowserProcessSubThread;
+class BrowserThreadImpl;
 
 // DEPRECATED: use TestBrowserThreadBundle instead. See http://crbug.com/272091
 // A BrowserThread for unit tests; this lets unit tests in chrome/ create
 // BrowserThread instances.
 class TestBrowserThread {
  public:
+  // Constructs a TestBrowserThread with a |real_thread_| and starts it (with a
+  // MessageLoopForIO if |identifier == BrowserThread::IO|.
   explicit TestBrowserThread(BrowserThread::ID identifier);
+
+  // Constructs a TestBrowserThread based on |message_loop| (no |real_thread_|).
   TestBrowserThread(BrowserThread::ID identifier,
                     base::MessageLoop* message_loop);
+
   ~TestBrowserThread();
 
   // We provide a subset of the capabilities of the Thread interface
@@ -34,29 +41,35 @@ class TestBrowserThread {
   // interface.
 
   // Starts the thread with a generic message loop.
-  bool Start();
+  void Start();
 
   // Starts the thread with a generic message loop and waits for the
   // thread to run.
-  bool StartAndWaitForTesting();
+  void StartAndWaitForTesting();
 
   // Starts the thread with an IOThread message loop.
-  bool StartIOThread();
+  void StartIOThread();
 
-  // Initializes the BrowserThreadDelegate.
-  void InitIOThreadDelegate();
+  // Together these are the same as StartIOThread(). They can be called in
+  // phases to test binding BrowserThread::IO after its underlying thread was
+  // started.
+  void StartIOThreadUnregistered();
+  void RegisterAsBrowserThread();
 
-  // Stops the thread.
+  // Stops the thread, no-op if this is not a real thread.
   void Stop();
 
-  // Returns true if the thread is running.
-  bool IsRunning();
-
  private:
-  std::unique_ptr<TestBrowserThreadImpl> impl_;
-
   const BrowserThread::ID identifier_;
 
+  // A real thread which represents |identifier_| when constructor #1 is used
+  // (null otherwise).
+  std::unique_ptr<BrowserProcessSubThread> real_thread_;
+
+  // Binds |identifier_| to |message_loop| when constructor #2 is used (null
+  // otherwise).
+  std::unique_ptr<BrowserThreadImpl> fake_thread_;
+
   DISALLOW_COPY_AND_ASSIGN(TestBrowserThread);
 };