commit d260e9cf660f062b203692601cf9b6ccb27f3d1e
author Gabriel Charette <gab@chromium.org> Tue Mar 20 18:10:45 2018
committer Commit Bot <commit-bot@chromium.org> Tue Mar 20 18:10:45 2018
tree 859c1d991ef2e1118a977821257777c06957d9d3
parent a54726ff975e09efe95d241091a32954bdd58468

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}