New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 672977 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug



Sign in to add a comment

BrowserThreadImpl::StartWithOptions blocks until the underlying thread starts

Project Member Reported by gab@chromium.org, Dec 9 2016

Issue description

Blocking on Thread::Start() was fixed by kinuko@ in r331235.

But looks like it regressed in r408295.

The issue is the call to GetThreadId() which is blocking under the hood.

This appears unintentional as r408295 also introduced BrowserThreadImpl::StartAndWaitForTesting() which is redundant given the normal start also blocks...

I plan to address this as part of https://codereview.chromium.org/2464233002.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Dec 16 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3ee0e445ae07397a077eb3213fc46318af01b43e

commit 3ee0e445ae07397a077eb3213fc46318af01b43e
Author: gab <gab@chromium.org>
Date: Fri Dec 16 17:43:11 2016

Experiment with redirecting all BrowserThreads (but UI/IO) to TaskScheduler

This is part of the migration phase for TaskScheduler, design doc:
https://docs.google.com/document/d/1S2AAeoo1xa_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa5k/edit

Also generalizes BrowserThreadImpl away from the underlying thread's MessageLoop
and id in favor of TaskRunners and BrowserThreadState.

Cool side-effect: BrowserThreadImpl::StartWithOptions() no longer blocks on the
underlying thread starting (it used to via GetThreadId()), it now takes a reference
to its TaskRunner and lets the thread itself start on its own schedule :).
This wasn't previously intended to block (BrowserThreadImpl::StartAndWaitForTesting() is
supposed to be used for that) but had unintentionally regressed in http://crrev.com/408295
because GetThreadId() implicitly waits ( http://crbug.com/672977 ).

Another nice improvement is that ~BrowserThreadImpl() now cleans the BrowserThreadGlobals
associated with it, this has the side-effect to force proper destruction order of
TestBrowserThread in tests which brought forth a few precursor cleanups
( https://crbug.com/653916#c24 ) which will from now on be enforced by this CL.

When redirection is disabled, the logic should be the exact same as before.
 - Threads are brought up.
 - Tasks are posted to their MessageLoop (albeit through their TaskRunner now).
 - On shutdown, threads are joined one by one.

When redirection is enabled, we try to mimic this logic.
 - Redirection TaskRunners are only live (|accepting_tasks|) for the same period that the
   matching thread would be.
 - On shutdown, we block on each TaskRunner's queue being flushed one-by-one
   in the same order as in the no-redirection case (this almost identical to
   real threads join % one slight difference documented in detail in
   BrowserThreadImpl::StopRedirectionOfThreadID()).

BUG= 653916 ,  672977 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_tsan_rel_ng

TEST=
 A) "out\Release\chrome.exe"
    Runs/shuts down the exact same as before.

 B) "out\Release\chrome.exe --force-fieldtrials=BrowserScheduler/Enabled/ --force-fieldtrial-params=BrowserScheduler.Enabled:Background/3;3;1;0;10000/BackgroundFileIO/3;3;1;0;10000/Foreground/3;3;1;0;10000/ForegroundFileIO/3;3;1;0;10000/RedirectSequencedWorkerPools/true/RedirectNonUINonIOBrowserThreads/true"
    Runs/shuts down smoothly and chrome://tracing confirms that redirected BrowserThreads are running on TaskSchedulerWorkers.

Review-Url: https://codereview.chromium.org/2464233002
Cr-Commit-Position: refs/heads/master@{#439139}

[modify] https://crrev.com/3ee0e445ae07397a077eb3213fc46318af01b43e/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/3ee0e445ae07397a077eb3213fc46318af01b43e/chrome/browser/chrome_content_browser_client.h
[modify] https://crrev.com/3ee0e445ae07397a077eb3213fc46318af01b43e/content/browser/browser_main_loop.cc
[modify] https://crrev.com/3ee0e445ae07397a077eb3213fc46318af01b43e/content/browser/browser_main_loop.h
[modify] https://crrev.com/3ee0e445ae07397a077eb3213fc46318af01b43e/content/browser/browser_thread_impl.cc
[modify] https://crrev.com/3ee0e445ae07397a077eb3213fc46318af01b43e/content/browser/browser_thread_impl.h
[modify] https://crrev.com/3ee0e445ae07397a077eb3213fc46318af01b43e/content/public/browser/content_browser_client.cc
[modify] https://crrev.com/3ee0e445ae07397a077eb3213fc46318af01b43e/content/public/browser/content_browser_client.h
[modify] https://crrev.com/3ee0e445ae07397a077eb3213fc46318af01b43e/content/public/test/test_browser_thread.cc
[modify] https://crrev.com/3ee0e445ae07397a077eb3213fc46318af01b43e/content/public/test/test_browser_thread.h
[modify] https://crrev.com/3ee0e445ae07397a077eb3213fc46318af01b43e/content/public/test/test_browser_thread_bundle.cc

Comment 2 by gab@chromium.org, Dec 16 2016

Status: Fixed (was: Started)

Comment 3 by kinuko@chromium.org, Dec 21 2016

Whoa, cool.

Sign in to add a comment