commit | fa239c8c41939589490f62c39321b8c2504b2219 | [log] [tgz] |
---|---|---|
author | Matt Falkenhagen <falken@chromium.org> | Mon Mar 26 04:21:19 2018 |
committer | Commit Bot <commit-bot@chromium.org> | Mon Mar 26 04:21:19 2018 |
tree | 83f54d7265200b99ca721f6e059c0fb9ec6b27b5 | |
parent | fb48059ccdfe83de78d14b889cf734668ded071b [diff] |
Revert "Refactor BrowserThreadImpl, BrowserProcessSubThread, and BrowserMainLoop" This reverts commit d260e9cf660f062b203692601cf9b6ccb27f3d1e. 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}
Chromium is an open-source browser project that aims to build a safer, faster, and more stable way for all users to experience the web.
The project's web site is https://www.chromium.org.
Documentation in the source is rooted in docs/README.md.
Learn how to Get Around the Chromium Source Code Directory Structure .