commit | 8eb4dff9914d111cbce7cbc756b0c96030151762 | [log] [tgz] |
---|---|---|
author | Gabriel Charette <gab@chromium.org> | Tue Mar 27 14:22:54 2018 |
committer | Commit Bot <commit-bot@chromium.org> | Tue Mar 27 14:22:54 2018 |
tree | e5f7c0021c91ddcd392e5269a925d6407e5ff3f8 | |
parent | 12f9cbe62bb8f6f09782c1ac05bff57bd61f4d51 [diff] |
Reland "Refactor BrowserThreadImpl, BrowserProcessSubThread, and BrowserMainLoop" This is a reland of d260e9cf660f062b203692601cf9b6ccb27f3d1e 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/980793 Reviewed-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}
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 .