Revert "Refactor BrowserThreadImpl, BrowserProcessSubThread, and BrowserMainLoop"

This reverts commit d260e9cf.

Reason for revert: In Windows Canary versions since 67.0.3377.0, where this commit landed, IO thread hang reports have spiked dramatically. It is now the #1 browser crash report on Windows Canary at 33% of reports. I'm speculatively reverting this to see if the crash rate heals.

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=gab@chromium.org,jam@chromium.org
NOPRESUBMIT=true

# Not skipping CQ checks because original CL landed > 1 day ago.
# falken: Skipping presubmit to use deprecated ThreadResrictions::DisallowWaiting().

Bug: 815225, 821034, 729596
Change-Id: I2be97c5d8183497c005ab397c871f625b034d850
Reviewed-on: https://chromium-review.googlesource.com/979752
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#545725}

base/threading/thread.h

@@ -271,9 +271,6 @@ 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,7 +38,6 @@ namespace content {
 class BrowserGpuChannelHostFactory;
 class BrowserGpuMemoryBufferManager;
 class BrowserMainLoop;
-class BrowserProcessSubThread;
 class BrowserShutdownProfileDumper;
 class BrowserSurfaceViewManager;
 class BrowserTestBase;
@@ -212,7 +211,6 @@ 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,9 +31,10 @@ 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_(
-            new BrowserProcessImpl(base::ThreadTaskRunnerHandle::Get().get())) {
+        browser_process_impl_(std::make_unique<BrowserProcessImpl>(
+            base::ThreadTaskRunnerHandle::Get().get())) {
     // Create() and StartWithDefaultParams() TaskScheduler in seperate steps to
     // properly simulate the browser process' lifecycle.
     base::TaskScheduler::Create("BrowserProcessImplTest");
@@ -48,20 +49,17 @@ class BrowserProcessImplTest : public ::testing::Test {
     g_browser_process = stashed_browser_process_;
   }
 
-  // Creates the IO thread (unbound) and task scheduler threads. The UI thread
-  // needs to be alive while BrowserProcessImpl is alive, and is managed
-  // separately.
+  // Creates the secondary thread (IO thread).
+  // The UI thread needs to be alive while BrowserProcessImpl is alive, and is
+  // managed separately.
   void StartSecondaryThreads() {
     base::TaskScheduler::GetInstance()->StartWithDefaultParams();
-
-    io_thread_ = std::make_unique<content::TestBrowserThread>(
-        content::BrowserThread::IO);
-    io_thread_->StartIOThreadUnregistered();
+    io_thread_->StartIOThread();
   }
 
-  // Binds the IO thread to BrowserThread::IO and starts the ServiceManager.
+  // Initializes the IO thread delegate and starts the ServiceManager.
   void Initialize() {
-    io_thread_->RegisterAsBrowserThread();
+    io_thread_->InitIOThreadDelegate();
 
     // TestServiceManagerContext creation requires the task scheduler to be
     // started.

content/browser/browser_main_loop.cc

@@ -37,7 +37,6 @@
 #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"
@@ -55,7 +54,6 @@
 #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"
@@ -373,11 +371,10 @@ void OnStoppedStartupTracing(const base::FilePath& trace_file) {
 MSVC_DISABLE_OPTIMIZE()
 MSVC_PUSH_DISABLE_WARNING(4748)
 
-NOINLINE void ResetThread_IO(
-    std::unique_ptr<BrowserProcessSubThread> io_thread) {
-  volatile int line_number = __LINE__;
-  io_thread.reset();
-  CHECK_GT(line_number, 0);
+NOINLINE void ResetThread_IO(std::unique_ptr<BrowserProcessSubThread> thread) {
+  volatile int inhibit_comdat = __LINE__;
+  ALLOW_UNUSED_LOCAL(inhibit_comdat);
+  thread.reset();
 }
 
 MSVC_POP_WARNING()
@@ -1007,13 +1004,11 @@ int BrowserMainLoop::CreateThreads() {
         *task_scheduler_init_params.get());
   }
 
-  // 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").
+  // |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.
   DCHECK(io_thread_);
-  io_thread_->RegisterAsBrowserThread();
+  io_thread_->InitIOThreadDelegate();
 
   created_threads_ = true;
   return result_code_;
@@ -1251,12 +1246,9 @@ void BrowserMainLoop::InitializeMainThread() {
   TRACE_EVENT0("startup", "BrowserMainLoop::InitializeMainThread");
   base::PlatformThread::SetName("CrBrowserMain");
 
-  // 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()));
+  // Register the main thread by instantiating it, but don't call any methods.
+  main_thread_.reset(
+      new BrowserThreadImpl(BrowserThread::UI, base::MessageLoop::current()));
 }
 
 int BrowserMainLoop::BrowserThreadsStarted() {
@@ -1428,7 +1420,7 @@ int BrowserMainLoop::BrowserThreadsStarted() {
         "startup",
         "BrowserMainLoop::BrowserThreadsStarted::InitUserInputMonitor");
     user_input_monitor_ = media::UserInputMonitor::Create(
-        io_thread_->task_runner(), base::ThreadTaskRunnerHandle::Get());
+        io_thread_->task_runner(), main_thread_->task_runner());
   }
 
   {
@@ -1578,10 +1570,9 @@ void BrowserMainLoop::InitializeIOThread() {
   options.priority = base::ThreadPriority::DISPLAY;
 #endif
 
-  io_thread_ = std::make_unique<BrowserProcessSubThread>(BrowserThread::IO);
-
+  io_thread_.reset(new BrowserProcessSubThread(BrowserThread::IO));
   if (!io_thread_->StartWithOptions(options))
-    LOG(FATAL) << "Failed to start BrowserThread::IO";
+    LOG(FATAL) << "Failed to start the browser thread: IO";
 }
 
 void BrowserMainLoop::InitializeMojo() {

content/browser/browser_main_loop.h

@@ -13,6 +13,7 @@
 #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"
@@ -91,7 +92,6 @@ class HostFrameSinkManager;
 namespace content {
 class BrowserMainParts;
 class BrowserOnlineStateObserver;
-class BrowserProcessSubThread;
 class BrowserThreadImpl;
 class LoaderDelegateImpl;
 class MediaStreamManager;
@@ -243,10 +243,7 @@ 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;
@@ -288,7 +285,6 @@ 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_;
@@ -339,6 +335,9 @@ 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,7 +9,6 @@
 #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,7 +16,6 @@
 #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,165 +4,66 @@
 
 #include "content/browser/browser_process_sub_thread.h"
 
-#include "base/compiler_specific.h"
+#include "base/debug/leak_tracker.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 {
 
-namespace {
-BrowserThreadDelegate* g_io_thread_delegate = nullptr;
-}  // namespace
-
-// 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_);
-}
+    : BrowserThreadImpl(identifier) {}
+
+BrowserProcessSubThread::BrowserProcessSubThread(
+    BrowserThread::ID identifier,
+    base::MessageLoop* message_loop)
+    : BrowserThreadImpl(identifier, message_loop) {}
 
 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_ = std::make_unique<base::win::ScopedCOMInitializer>();
+  com_initializer_.reset(new base::win::ScopedCOMInitializer());
 #endif
 
-  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_);
+  notification_service_.reset(new NotificationServiceImpl());
 
-#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
+  BrowserThreadImpl::Init();
 
-  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;
+  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();
   }
 }
 
 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))
-    IOThreadCleanUp();
+    IOThreadPreCleanUp();
 
-  if (identifier_ == BrowserThread::IO && g_io_thread_delegate)
-    g_io_thread_delegate->CleanUp();
+  BrowserThreadImpl::CleanUp();
 
   notification_service_.reset();
 
 #if defined(OS_WIN)
   com_initializer_.reset();
 #endif
-
-  browser_thread_.reset();
 }
 
-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) {
-  volatile int line_number = __LINE__;
-  Thread::Run(run_loop);
-  CHECK_GT(line_number, 0);
-}
-
-void BrowserProcessSubThread::IOThreadRun(base::RunLoop* run_loop) {
-  volatile int line_number = __LINE__;
-  Thread::Run(run_loop);
-  CHECK_GT(line_number, 0);
-}
-
-MSVC_POP_WARNING()
-MSVC_ENABLE_OPTIMIZE();
-
-void BrowserProcessSubThread::IOThreadCleanUp() {
-  DCHECK_CALLED_ON_VALID_THREAD(browser_thread_checker_);
-
+void BrowserProcessSubThread::IOThreadPreCleanUp() {
   // Kill all things that might be holding onto
   // net::URLRequest/net::URLRequestContexts.
 

content/browser/browser_process_sub_thread.h

@@ -8,9 +8,8 @@
 #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"
 
@@ -29,57 +28,29 @@ class NotificationService;
 namespace content {
 
 // ----------------------------------------------------------------------------
-// A BrowserProcessSubThread is a physical thread backing a BrowserThread.
+// BrowserProcessSubThread
+//
+// This simple thread object is used for the specialized threads that the
+// BrowserProcess spins up.
 //
 // 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 base::Thread {
+class CONTENT_EXPORT BrowserProcessSubThread : public BrowserThreadImpl {
  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:
-  // 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_;
+  // These methods encapsulate cleanup that needs to happen on the IO thread
+  // before we call the embedder's CleanUp function.
+  void IOThreadPreCleanUp();
 
 #if defined (OS_WIN)
   std::unique_ptr<base::win::ScopedCOMInitializer> com_initializer_;
@@ -88,8 +59,6 @@ class CONTENT_EXPORT BrowserProcessSubThread : public base::Thread {
   // 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,31 +5,47 @@
 #ifndef CONTENT_BROWSER_BROWSER_THREAD_IMPL_H_
 #define CONTENT_BROWSER_BROWSER_THREAD_IMPL_H_
 
-#include "base/memory/scoped_refptr.h"
+#include "base/callback.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 content {
+namespace base {
+class MessageLoop;
+class RunLoop;
+}
+
+namespace base {
+class Location;
+}
 
-class BrowserMainLoop;
-class BrowserProcessSubThread;
-class TestBrowserThread;
+namespace content {
 
-// 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 {
+class CONTENT_EXPORT BrowserThreadImpl : public BrowserThread,
+                                         public base::Thread {
  public:
-  ~BrowserThreadImpl();
+  // Construct a BrowserThreadImpl with the supplied identifier.  It is an error
+  // to construct a BrowserThreadImpl that already exists.
+  explicit BrowserThreadImpl(BrowserThread::ID identifier);
 
-  // Returns the thread name for |identifier|.
-  static const char* GetThreadName(BrowserThread::ID identifier);
+  // 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();
 
   // Resets globals for |identifier|. Used in tests to clear global state that
   // would otherwise leak to the next test. Globals are not otherwise fully
@@ -39,19 +55,35 @@ 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:
-  // 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);
+  // 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;
 
   // 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,7 +12,6 @@
 #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"
@@ -32,22 +31,17 @@ class BrowserThreadTest : public testing::Test {
 
  protected:
   void SetUp() override {
-    ui_thread_ = std::make_unique<BrowserProcessSubThread>(BrowserThread::UI);
+    ui_thread_.reset(new BrowserThreadImpl(BrowserThread::UI));
+    io_thread_.reset(new BrowserThreadImpl(BrowserThread::IO));
     ui_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();
+    io_thread_->Start();
   }
 
   void TearDown() override {
-    io_thread_.reset();
-    ui_thread_.reset();
-
+    StopUIThread();
+    io_thread_->Stop();
+    ui_thread_ = nullptr;
+    io_thread_ = nullptr;
     BrowserThreadImpl::ResetGlobalsForTesting(BrowserThread::UI);
     BrowserThreadImpl::ResetGlobalsForTesting(BrowserThread::IO);
   }
@@ -79,8 +73,8 @@ class BrowserThreadTest : public testing::Test {
   };
 
  private:
-  std::unique_ptr<BrowserProcessSubThread> ui_thread_;
-  std::unique_ptr<BrowserProcessSubThread> io_thread_;
+  std::unique_ptr<BrowserThreadImpl> ui_thread_;
+  std::unique_ptr<BrowserThreadImpl> 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,11 +172,9 @@ 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
@@ -190,12 +188,16 @@ 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.
   //
-  // 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.
+  // 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.
   static void SetIOThreadDelegate(BrowserThreadDelegate* delegate);
 
   // Use these templates in conjunction with RefCountedThreadSafe or scoped_ptr

content/public/browser/browser_thread_delegate.h

@@ -5,20 +5,22 @@
 #ifndef CONTENT_PUBLIC_BROWSER_BROWSER_THREAD_DELEGATE_H_
 #define CONTENT_PUBLIC_BROWSER_BROWSER_THREAD_DELEGATE_H_
 
-#include "content/common/content_export.h"
-
 namespace content {
 
-// A Delegate for content embedders to perform extra initialization/cleanup on
-// BrowserThread::IO.
+// 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).
 class BrowserThreadDelegate {
  public:
   virtual ~BrowserThreadDelegate() = default;
 
-  // Called prior to completing initialization of BrowserThread::IO.
+  // Called prior to starting the message loop
   virtual void Init() = 0;
 
-  // Called during teardown of BrowserThread::IO.
+  // Called just after the message loop ends.
   virtual void CleanUp() = 0;
 };
 

content/public/test/test_browser_thread.cc

@@ -9,32 +9,71 @@
 #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)
-    : identifier_(identifier),
-      real_thread_(std::make_unique<BrowserProcessSubThread>(identifier_)) {
-  real_thread_->AllowBlockingForTesting();
-}
+    : impl_(new TestBrowserThreadImpl(identifier)), identifier_(identifier) {}
 
 TestBrowserThread::TestBrowserThread(BrowserThread::ID identifier,
                                      base::MessageLoop* message_loop)
-    : identifier_(identifier),
-      fake_thread_(
-          new BrowserThreadImpl(identifier_, message_loop->task_runner())) {}
+    : impl_(new TestBrowserThreadImpl(identifier, message_loop)),
+      identifier_(identifier) {}
 
 TestBrowserThread::~TestBrowserThread() {
   // The upcoming BrowserThreadImpl::ResetGlobalsForTesting() call requires that
-  // |identifier_| have completed its SHUTDOWN phase.
-  real_thread_.reset();
-  fake_thread_.reset();
+  // |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();
 
-  // 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
+  // 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
   // MessageLoop however, this means that TaskRunners obtained through
   // |BrowserThreadImpl::GetTaskRunnerForThread(identifier_)| will no longer
   // recognize their BrowserThreadImpl for RunsTasksInCurrentSequence(). This
@@ -48,34 +87,30 @@ TestBrowserThread::~TestBrowserThread() {
   BrowserThreadImpl::ResetGlobalsForTesting(identifier_);
 }
 
-void TestBrowserThread::Start() {
-  CHECK(real_thread_->Start());
-  RegisterAsBrowserThread();
+bool TestBrowserThread::Start() {
+  return impl_->Start();
 }
 
-void TestBrowserThread::StartAndWaitForTesting() {
-  CHECK(real_thread_->StartAndWaitForTesting());
-  RegisterAsBrowserThread();
+bool TestBrowserThread::StartAndWaitForTesting() {
+  return impl_->StartAndWaitForTesting();
 }
 
-void TestBrowserThread::StartIOThread() {
-  StartIOThreadUnregistered();
-  RegisterAsBrowserThread();
-}
-
-void TestBrowserThread::StartIOThreadUnregistered() {
+bool TestBrowserThread::StartIOThread() {
   base::Thread::Options options;
   options.message_loop_type = base::MessageLoop::TYPE_IO;
-  CHECK(real_thread_->StartWithOptions(options));
+  return impl_->StartWithOptions(options);
 }
 
-void TestBrowserThread::RegisterAsBrowserThread() {
-  real_thread_->RegisterAsBrowserThread();
+void TestBrowserThread::InitIOThreadDelegate() {
+  impl_->InitIOThreadDelegate();
 }
 
 void TestBrowserThread::Stop() {
-  if (real_thread_)
-    real_thread_->Stop();
+  impl_->Stop();
+}
+
+bool TestBrowserThread::IsRunning() {
+  return impl_->IsRunning();
 }
 
 }  // namespace content

content/public/test/test_browser_thread.h

@@ -12,27 +12,20 @@
 
 namespace base {
 class MessageLoop;
-class Thread;
 }
 
 namespace content {
 
-class BrowserProcessSubThread;
-class BrowserThreadImpl;
+class TestBrowserThreadImpl;
 
 // 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
@@ -41,34 +34,28 @@ class TestBrowserThread {
   // interface.
 
   // Starts the thread with a generic message loop.
-  void Start();
+  bool Start();
 
   // Starts the thread with a generic message loop and waits for the
   // thread to run.
-  void StartAndWaitForTesting();
+  bool StartAndWaitForTesting();
 
   // Starts the thread with an IOThread message loop.
-  void StartIOThread();
+  bool StartIOThread();
 
-  // 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();
+  // Initializes the BrowserThreadDelegate.
+  void InitIOThreadDelegate();
 
-  // Stops the thread, no-op if this is not a real thread.
+  // Stops the thread.
   void Stop();
 
- private:
-  const BrowserThread::ID identifier_;
+  // Returns true if the thread is running.
+  bool IsRunning();
 
-  // A real thread which represents |identifier_| when constructor #1 is used
-  // (null otherwise).
-  std::unique_ptr<BrowserProcessSubThread> real_thread_;
+ private:
+  std::unique_ptr<TestBrowserThreadImpl> impl_;
 
-  // Binds |identifier_| to |message_loop| when constructor #2 is used (null
-  // otherwise).
-  std::unique_ptr<BrowserThreadImpl> fake_thread_;
+  const BrowserThread::ID identifier_;
 
   DISALLOW_COPY_AND_ASSIGN(TestBrowserThread);
 };