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

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.

Committed: https://crrev.com/3ee0e445ae07397a077eb3213fc46318af01b43e
Cr-Commit-Position: refs/heads/master@{#439139}

[gab, 2016-11-03 17:49:30]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-03 17:50:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-03 18:13:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-03 18:13:32]

Dry run: Try jobs failed on following builders:
  blimp_linux_dbg on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/blimp_linux_dbg...)

[gab, 2016-11-04 16:36:15]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-04 16:36:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-04 16:58:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-04 16:58:11]

Dry run: Try jobs failed on following builders:
  blimp_linux_dbg on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/blimp_linux_dbg...)

[gab, 2016-11-04 20:27:58]

Description was changed from

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

BUG=653916
==========

to

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

Also generalizes BrowserThreadImpl away from the underlying thread's MessageLoop

and id in favor of TaskRunners and BrowserThreadState.

BUG=653916
==========

[gab, 2016-11-04 20:39:11]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-04 20:40:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gab, 2016-11-04 21:07:21]

Description was changed from

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

Also generalizes BrowserThreadImpl away from the underlying thread's MessageLoop

and id in favor of TaskRunners and BrowserThreadState.

BUG=653916
==========

to

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

Also generalizes BrowserThreadImpl away from the underlying thread's MessageLoop

and id in favor of TaskRunners and BrowserThreadState.

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 for the same period that the matching
thread would be live.
 - On shutdown, we block on each TaskRunner's queue being flushed one-by-one
   in the same order as in the no-redirection case.

BUG=653916
==========

[gab, 2016-11-04 21:07:38]

Description was changed from

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

Also generalizes BrowserThreadImpl away from the underlying thread's MessageLoop

and id in favor of TaskRunners and BrowserThreadState.

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 for the same period that the matching
thread would be live.
 - On shutdown, we block on each TaskRunner's queue being flushed one-by-one
   in the same order as in the no-redirection case.

BUG=653916
==========

to

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

Also generalizes BrowserThreadImpl away from the underlying thread's MessageLoop

and id in favor of TaskRunners and BrowserThreadState.

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 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.

BUG=653916
==========

[gab, 2016-11-04 21:13:02]

Description was changed from

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

Also generalizes BrowserThreadImpl away from the underlying thread's MessageLoop

and id in favor of TaskRunners and BrowserThreadState.

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 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.

BUG=653916
==========

to

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

Also generalizes BrowserThreadImpl away from the underlying thread's MessageLoop

and id in favor of TaskRunners and BrowserThreadState.

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 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.

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

[gab, 2016-11-08 15:11:27]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-08 15:11:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-08 15:28:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-08 15:28:37]

Dry run: Try jobs failed on following builders:
  blimp_linux_dbg on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/blimp_linux_dbg...)

[gab, 2016-11-08 15:31:20]

Description was changed from

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

Also generalizes BrowserThreadImpl away from the underlying thread's MessageLoop

and id in favor of TaskRunners and BrowserThreadState.

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 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.

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

to

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

Also generalizes BrowserThreadImpl away from the underlying thread's MessageLoop

and id in favor of TaskRunners and BrowserThreadState.

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 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.

BUG=653916
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.
==========

[gab, 2016-11-08 15:42:49]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-08 15:43:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gab, 2016-11-08 15:47:38]

Description was changed from

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

Also generalizes BrowserThreadImpl away from the underlying thread's MessageLoop

and id in favor of TaskRunners and BrowserThreadState.

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 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.

BUG=653916
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.
==========

to

==========
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_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa...

Also generalizes BrowserThreadImpl away from the underlying thread's MessageLoop

and id in favor of TaskRunners and BrowserThreadState.

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 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.

BUG=653916
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.
==========

[gab, 2016-11-08 15:50:18]

Description was changed from

==========
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_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa...

Also generalizes BrowserThreadImpl away from the underlying thread's MessageLoop

and id in favor of TaskRunners and BrowserThreadState.

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 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.

BUG=653916
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.
==========

to

==========
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_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa...

Also generalizes BrowserThreadImpl away from the underlying thread's MessageLoop

and id in favor of TaskRunners and BrowserThreadState.

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.

BUG=653916
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.
==========

[gab, 2016-11-08 15:50:30]

Description was changed from

==========
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_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa...

Also generalizes BrowserThreadImpl away from the underlying thread's MessageLoop

and id in favor of TaskRunners and BrowserThreadState.

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.

BUG=653916
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.
==========

to

==========
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_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa...

Also generalizes BrowserThreadImpl away from the underlying thread's MessageLoop

and id in favor of TaskRunners and BrowserThreadState.

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.

BUG=653916
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.
==========

[gab, 2016-11-08 15:51:14]

gab@chromium.org changed reviewers:
+ avi@chromium.org

[gab, 2016-11-08 15:51:14]

Avi PTAL, thanks!

[Avi (use Gerrit), 2016-11-08 16:16:15]

Oh boy. This looks reasonable (and fun) but can you get more eyes on this than
just mine? I don't have a ton of threading experience and would appreciate more
coverage.

Mostly minor issues.

https://codereview.chromium.org/2464233002/diff/200001/base/threading/thread.h
File base/threading/thread.h (right):

https://codereview.chromium.org/2464233002/diff/200001/base/threading/thread....
base/threading/thread.h:250: bool using_external_message_loop_ = false;
Data members that are protected are a violation of the style guide
(https://google.github.io/styleguide/cppguide.html#Access_Control). Use accessor
functions.

https://codereview.chromium.org/2464233002/diff/200001/content/browser/browse...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/2464233002/diff/200001/content/browser/browse...
content/browser/browser_main_loop.cc:319: DCHECK(!thread || !task_runner);
This checks that we don't have both.

Do we want to check that we have exactly one? (and below too)

https://codereview.chromium.org/2464233002/diff/200001/content/browser/browse...
File content/browser/browser_main_loop.h (right):

https://codereview.chromium.org/2464233002/diff/200001/content/browser/browse...
content/browser/browser_main_loop.h:276:
std::unique_ptr<BrowserProcessSubThread> db_thread_;
Can you add commentary to this block as to when the "thread_" or "task_runner_"
variants are active?

https://codereview.chromium.org/2464233002/diff/200001/content/browser/browse...
File content/browser/browser_thread_impl.cc (right):

https://codereview.chromium.org/2464233002/diff/200001/content/browser/browse...
content/browser/browser_thread_impl.cc:110: // BrowserThread::ID is associated
but the underlying thread doesn't run yet.
"... underlying thread hasn't run yet."

https://codereview.chromium.org/2464233002/diff/200001/content/browser/browse...
content/browser/browser_thread_impl.cc:126: BrowserThread::ID_COUNT *
sizeof(thread_delegates[0]));
Does xxx = { 0 } syntax suffice to zero these? I'm not a fan of memset. :\

[commit-bot: I haz the power, 2016-11-08 16:47:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-08 16:47:39]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[gab, 2016-11-08 17:09:25]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-08 17:09:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gab, 2016-11-08 18:27:11]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-08 18:27:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-08 19:29:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-08 19:29:21]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[gab, 2016-11-08 20:40:45]

On 2016/11/08 16:16:15, Avi wrote:
> Oh boy. This looks reasonable (and fun) but can you get more eyes on this than
> just mine? I don't have a ton of threading experience and would appreciate
more
> coverage.

Thanks for the initial look. I was going to send it to jam@ but he appears to
have vanished for the next month, he told me that any content/OWNERS would do
but I understand that some are more familiar with some parts than others. Any
idea who in content/OWNERS would be a better fit for this review?

PS: I'm still having trouble with a few tests so please hold off on a deeper
review for now.

https://codereview.chromium.org/2464233002/diff/200001/base/threading/thread.h
File base/threading/thread.h (right):

https://codereview.chromium.org/2464233002/diff/200001/base/threading/thread....
base/threading/thread.h:250: bool using_external_message_loop_ = false;
On 2016/11/08 16:16:15, Avi wrote:
> Data members that are protected are a violation of the style guide
> (https://google.github.io/styleguide/cppguide.html#Access_Control). Use
accessor
> functions.

Done.

https://codereview.chromium.org/2464233002/diff/200001/content/browser/browse...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/2464233002/diff/200001/content/browser/browse...
content/browser/browser_main_loop.cc:319: DCHECK(!thread || !task_runner);
On 2016/11/08 16:16:15, Avi wrote:
> This checks that we don't have both.
> 
> Do we want to check that we have exactly one? (and below too)

Right, made this
DCHECK((thread && !task_runner) || (!thread && task_runner));
instead. Otherwise, to verify inline below would require making the else {} and
else if and having a NOTREACHED() else {} IMO which is uglier I think.

https://codereview.chromium.org/2464233002/diff/200001/content/browser/browse...
File content/browser/browser_main_loop.h (right):

https://codereview.chromium.org/2464233002/diff/200001/content/browser/browse...
content/browser/browser_main_loop.h:276:
std::unique_ptr<BrowserProcessSubThread> db_thread_;
On 2016/11/08 16:16:15, Avi wrote:
> Can you add commentary to this block as to when the "thread_" or
"task_runner_"
> variants are active?

Done.

https://codereview.chromium.org/2464233002/diff/200001/content/browser/browse...
File content/browser/browser_thread_impl.cc (right):

https://codereview.chromium.org/2464233002/diff/200001/content/browser/browse...
content/browser/browser_thread_impl.cc:110: // BrowserThread::ID is associated
but the underlying thread doesn't run yet.
On 2016/11/08 16:16:15, Avi wrote:
> "... underlying thread hasn't run yet."

Done.

https://codereview.chromium.org/2464233002/diff/200001/content/browser/browse...
content/browser/browser_thread_impl.cc:126: BrowserThread::ID_COUNT *
sizeof(thread_delegates[0]));
On 2016/11/08 16:16:15, Avi wrote:
> Does xxx = { 0 } syntax suffice to zero these? I'm not a fan of memset. :\

I just tried = {}; as a member initializer and it seems to work (confirmed with
runtime DCHECKs in concstructor locally -- and in practice the DCHECKs in
SetDelegate() and BrowserThreadImpl() would also fire if this didn't work so I
didn't keep the ones in constructor) :-)!

PS: { 0 } just means "initialize the first entry to zero and default init the
rest", {} means "default init everything" (I was also shocked when I learned
this ;-)).

https://codereview.chromium.org/2464233002/diff/240001/content/browser/browse...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/2464233002/diff/240001/content/browser/browse...
content/browser/browser_main_loop.cc:326:
FlushThreadTaskRunner(std::move(task_runner));
fdoray@ just made me realize offline that although we null out the redirection
before doing the synchronous wait in FlushThreadTaskRunner(), which prevents
BrowserThread::PostTask() to that ID from then on; it's still possible to post
to that thread with ThreadTaskRunnerHandle::Get() -- and thus post something
which will be queued after the Signal(). This is different from the current
MessageLoop world which joins until the queue is idle.

We have an idea to augment TaskScheduler to allow for this use case (e.g.
SchedulerTaskRunner::RunUntilIdle()) and will come back to this CL once that's
been implemented.

[Avi (use Gerrit), 2016-11-08 20:48:17]

I'll OK things for content, but if you know other threads experts (content
owners or not) then definitely get their eyes on this.

[gab, 2016-11-09 16:20:22]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-09 16:20:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-09 16:32:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-09 16:32:38]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[gab, 2016-11-09 21:00:50]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-09 21:01:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gab, 2016-11-09 21:17:34]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-09 21:17:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-09 22:15:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-09 22:15:37]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[gab, 2016-11-11 16:14:36]

Patchset #18 (id:340001) has been deleted

[gab, 2016-11-11 21:33:32]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-11 21:34:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gab, 2016-11-11 21:48:28]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-11 21:49:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-11 22:52:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-11 22:52:04]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[gab, 2016-11-22 23:03:33]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-22 23:04:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-22 23:08:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-22 23:08:18]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[gab, 2016-11-23 03:27:34]

Patchset #23 (id:460001) has been deleted

[gab, 2016-11-23 03:28:42]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-23 03:28:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-23 03:55:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-23 03:55:37]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[gab, 2016-11-28 22:13:17]

Patchset #24 (id:500001) has been deleted

[gab, 2016-11-28 22:13:25]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-28 22:14:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-28 22:19:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-28 22:19:40]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[gab, 2016-11-29 16:38:15]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-29 16:38:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-29 17:07:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-29 17:07:03]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[gab, 2016-11-29 18:07:39]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-29 18:07:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-29 18:52:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-29 18:52:19]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[gab, 2016-11-29 19:56:23]

Description was changed from

==========
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_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa...

Also generalizes BrowserThreadImpl away from the underlying thread's MessageLoop

and id in favor of TaskRunners and BrowserThreadState.

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.

BUG=653916
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.
==========

to

==========
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_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa...

Also generalizes BrowserThreadImpl away from the underlying thread's MessageLoop

and id in favor of TaskRunners and BrowserThreadState.

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.

BUG=653916
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.
==========

[gab, 2016-11-29 19:56:49]

gab@chromium.org changed reviewers:
+ fdoray@chromium.org, robliao@chromium.org

[gab, 2016-11-29 19:56:51]

@fdoray/robliao PTAL at threading logic

@danakj (optional -- I know you're swamped these days but if you find the time,
I'd really like your threading expert eyes on this)

PS: There are a few more red bots but it's just more incorrect destruction order
for TestBrowserThreads (e.g. https://codereview.chromium.org/2535043003/) for a
few things that don't run on Windows and I'll fix those in parallel with the
review.

Thanks!
Gab

[fdoray, 2016-12-01 15:15:22]

https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse...
content/browser/browser_main_loop.cc:327: } else {
You could avoid keeping SingleThreadTaskRunners references in BrowserMainLoop by
changing this code to:

auto task_runner = BrowserThreadImpl::RedirectThreadIDToTaskRunner(
    BrowserThread::DB);
FlushThreadTaskRunner(std::move(task_runner));

(assuming that RedirectThreadIDToTaskRunner() returns the previous TaskRunner
for the provided identifier)

https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse...
content/browser/browser_main_loop.cc:328:
BrowserThreadImpl::RedirectThreadIDToTaskRunner(BrowserThread::DB, nullptr);
Without redirection:
1. Main thread: ~BrowserProcessSubThread
2. Main thread: Thread::Stop()
3. Main thread: A QuitWhenIdle() task is posted to the thread.
4. Browser thread: Runs tasks until there are no more ready tasks.
5. Browser thread: Thread::Cleanup()
6. Browser thread: BrowserThreadImpl::CleanUp() sets the state to SHUTDOWN. No
more tasks can be posted to the thread.
7. Browser thread: Exits, unblocking the join call on the main thread.

With redirection:
1. Main thread: RedirectThreadIDToTaskRunner() sets the state to SHUTDOWN. No
more tasks can be posted to the thread via the BrowserThread API.
2. Main thread: FlushThreadTaskRunner() posts a WaitableEvent::Signal() task to
the browser thread and waits.
3. Browser thread: Runs all tasks that were posted before the call to
FlushThreadTaskRunner() and unblocks the main thread.

Without redirection, we wait for pending tasks to run and then we set the state
to SHUTDOWN. With redirection, we set the state to SHUTDOWN and then we wait for
tasks to run. Could we improve that?

https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse...
content/browser/browser_main_loop.cc:942: // set to the appropriate
|BrowserProcessSubThread*|. And |options| can be
s/|BrowserProcessSubThread*|/BrowserProcessSubThread*

https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse...
content/browser/browser_main_loop.cc:949: // |SingleThreadTaskRunner*| and
|task_runner_traits| can be updated away
s/|SingleThreadTaskRunner*|/SingleThreadTaskRunner*

https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse...
content/browser/browser_main_loop.cc:950: // from its default. |execution_mode|
defaults to |SINGLE_THREADED| as-is
No more |execution_mode|.

https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse...
content/browser/browser_main_loop.cc:957: const bool
redirect_nonUInonIO_browser_threads =
Move outside the loop.

https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse...
content/browser/browser_main_loop.cc:969: task_runner_traits.WithFileIO()
Add WithWait() to allow AssertIOAllowed()?

https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse...
content/browser/browser_main_loop.cc:1081: } else if (task_runner_to_create) {
DCHECK(!thread_to_start);

https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse...
content/browser/browser_main_loop.cc:1244: // Clean up state that lives on or
uses the file_thread_ before it goes
the FILE thread

https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse...
File content/browser/browser_thread_impl.cc (right):

https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse...
content/browser/browser_thread_impl.cc:18: #include
"base/task_scheduler/task_scheduler.h"
Not needed.

https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse...
content/browser/browser_thread_impl.cc:109: // BrowserThread::ID is associated
but the underlying thread hasn't run yet.
The INITIALIZED state is useless. See comment in
BrowserThreadImpl::Initialize().

https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse...
content/browser/browser_thread_impl.cc:111: // BrowserThread::ID is associated
to a live thread.
... is associated to TaskRunner and is accepting tasks.

https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse...
content/browser/browser_thread_impl.cc:113: // BrowserThread::ID is associated
to a thread having been or being shutdown.
... no longer accepts tasks. If it was backed by a BrowserThreadImpl, this
BrowserThreadImpl is being or has been shutdown.

https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse...
content/browser/browser_thread_impl.cc:136: // because |threads| isn't a good
signal per redirected IDs remaining null and
|threads| doesn't exist

https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse...
content/browser/browser_thread_impl.cc:330: // PostTaskHelper() accesses the
message loop while holding this lock.
PostTaskHelper() no longer accesses a MessageLoop. Change to:

// Change the state to SHUTDOWN so that PostTaskHelper stops
// sending tasks to this thread. Do not clear 
// globals.task_runners[identifier_] so that 
// BrowserThread::CurrentlyOn() continues to work.

https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse...
content/browser/browser_thread_impl.cc:346: globals.states[identifier_] =
BrowserThreadState::INITIALIZED;
You can get rid of the INITIALIZED state. Initialize() could be replaced with an
anonymous function that verifies that the identifier it receives is valid.

https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse...
content/browser/browser_thread_impl.cc:435: // threads.
Add:

// Change the state to SHUTDOWN so that PostTaskHelper stops
// sending tasks to this thread. Do not clear 
// globals.task_runners[identifier] so that 
// BrowserThread::CurrentlyOn() continues to work.

https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse...
content/browser/browser_thread_impl.cc:647: void BrowserThread::SetDelegate(ID
identifier,
I would change this to SetIOThreadDelegate() and change
BrowserThreadGlobals::thread_delegates[] ->
BrowserThreadGlobals::io_thread_delegate. The UI thread never supported
delegates. Other threads don't support delegates when they are redirected.

https://codereview.chromium.org/2464233002/diff/580001/content/public/test/te...
File content/public/test/test_browser_thread.h (right):

https://codereview.chromium.org/2464233002/diff/580001/content/public/test/te...
content/public/test/test_browser_thread.h:55: BrowserThread::ID identifier_;
const

[gab, 2016-12-01 23:34:01]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 23:34:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-02 00:25:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-02 00:25:09]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[gab, 2016-12-02 19:30:16]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-02 19:30:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-02 21:11:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-02 21:11:53]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[gab, 2016-12-03 03:14:56]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-03 03:15:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gab, 2016-12-03 03:16:10]

Patchset #30 (id:640001) has been deleted

[commit-bot: I haz the power, 2016-12-03 03:17:41]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[gab, 2016-12-03 03:21:27]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-03 03:21:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gab, 2016-12-03 03:29:16]

Patchset #30 (id:660001) has been deleted

[gab, 2016-12-03 03:30:37]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-03 03:30:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gab, 2016-12-03 03:30:54]

The CQ bit was unchecked by gab@chromium.org

[gab, 2016-12-03 03:31:06]

Patchset #30 (id:680001) has been deleted

[gab, 2016-12-03 03:36:05]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-03 03:36:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-03 04:47:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-03 04:47:28]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[gab, 2016-12-06 21:06:44]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-06 21:07:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-06 22:00:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-06 22:00:47]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[gab, 2016-12-07 19:11:15]

Patchset #33 (id:760001) has been deleted

[gab, 2016-12-07 19:15:27]

@fdoray, comments addressed PTAL.

Also, FYI, with a few more precursor CLs landed I'm now down to almost all bots
passing :)

https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse...
content/browser/browser_main_loop.cc:327: } else {
On 2016/12/01 15:15:21, fdoray wrote:
> You could avoid keeping SingleThreadTaskRunners references in BrowserMainLoop
by
> changing this code to:
> 
> auto task_runner = BrowserThreadImpl::RedirectThreadIDToTaskRunner(
>     BrowserThread::DB);
> FlushThreadTaskRunner(std::move(task_runner));
> 
> (assuming that RedirectThreadIDToTaskRunner() returns the previous TaskRunner
> for the provided identifier)

I was inspired by this and went a step further, the flush is now in
BrowserThreadImpl::StopRedirectionOfThreadID()

https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse...
content/browser/browser_main_loop.cc:328:
BrowserThreadImpl::RedirectThreadIDToTaskRunner(BrowserThread::DB, nullptr);
On 2016/12/01 15:15:21, fdoray wrote:
> Without redirection:
> 1. Main thread: ~BrowserProcessSubThread
> 2. Main thread: Thread::Stop()
> 3. Main thread: A QuitWhenIdle() task is posted to the thread.
> 4. Browser thread: Runs tasks until there are no more ready tasks.
> 5. Browser thread: Thread::Cleanup()
> 6. Browser thread: BrowserThreadImpl::CleanUp() sets the state to SHUTDOWN. No
> more tasks can be posted to the thread.
> 7. Browser thread: Exits, unblocking the join call on the main thread.
> 
> With redirection:
> 1. Main thread: RedirectThreadIDToTaskRunner() sets the state to SHUTDOWN. No
> more tasks can be posted to the thread via the BrowserThread API.
> 2. Main thread: FlushThreadTaskRunner() posts a WaitableEvent::Signal() task
to
> the browser thread and waits.
> 3. Browser thread: Runs all tasks that were posted before the call to
> FlushThreadTaskRunner() and unblocks the main thread.
> 
> Without redirection, we wait for pending tasks to run and then we set the
state
> to SHUTDOWN. With redirection, we set the state to SHUTDOWN and then we wait
for
> tasks to run. Could we improve that?

The thing is we have to change the state to SHUTDOWN in Cleanup() because it
always happens when the thread is stopped (which can happen outside of
~BrowserThreadImpl()) and since I want all state changes to DCHECK the previous
state for documentation it can't also be toggled to SHUTDOWN before invoking
Stop() in ~BrowserThreadImpl() -- and vice-versa, the thread could be SHUTDOWN
before ~BrowserThreadImpl().

Thinking about this some more I think it's fine as-is as:
1) Any tasks for which this matters is already racing in today's world as it has
to be posted from a thread which can't be aware of the main thread's state
(beyond, potentially, the fact that it's past MainMessageLoopRun()).
i.e. such a task already gets denied a %age of the time as it's not always
posted by the time the Join() starts and has therefore no guarantees of being
posted before the Join() completes. So triggering the SHUTDOWN state earlier
merely skews this race towards denying more of these tasks which seems fine to
me.

The WaitableEvent::Signal still matters though as it guarantees all tasks that
were knowingly scheduled ahead of the Join do get to run before the Join
completes.

As discussed further offline, there is still a minor edge case where previously
it was impossible  for a thread lower in BrowserThread::ID to fail to post to a
higher thread. Whereas there is now a race (e.g. it's possible for a
PostTaskAndReply to come back to the "shutdown" thread and run after the "join"
-- before the final TaskScheduler::Shutdown()). We have a solution for this but
won't implement it in this CL as the race only exists behind the experiment and
we'll fix it before making it default on trunk.

Added comments to this effect in BrowserThreadImpl::StopRedirectionToThreadID().

https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse...
content/browser/browser_main_loop.cc:942: // set to the appropriate
|BrowserProcessSubThread*|. And |options| can be
On 2016/12/01 15:15:21, fdoray wrote:
> s/|BrowserProcessSubThread*|/BrowserProcessSubThread*

Done.

https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse...
content/browser/browser_main_loop.cc:949: // |SingleThreadTaskRunner*| and
|task_runner_traits| can be updated away
On 2016/12/01 15:15:21, fdoray wrote:
> s/|SingleThreadTaskRunner*|/SingleThreadTaskRunner*

Done.

https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse...
content/browser/browser_main_loop.cc:950: // from its default. |execution_mode|
defaults to |SINGLE_THREADED| as-is
On 2016/12/01 15:15:21, fdoray wrote:
> No more |execution_mode|.

Done.

https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse...
content/browser/browser_main_loop.cc:957: const bool
redirect_nonUInonIO_browser_threads =
On 2016/12/01 15:15:21, fdoray wrote:
> Move outside the loop.

Done.

https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse...
content/browser/browser_main_loop.cc:969: task_runner_traits.WithFileIO()
On 2016/12/01 15:15:21, fdoray wrote:
> Add WithWait() to allow AssertIOAllowed()?

Assuming you mean "to allow AssertWaitAllowed()". Looks like only UI/IO threads
DisallowWaiting() at the moment so I'll add WithWait() to the other ones for now
with a TODO to look into removing it.

https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse...
content/browser/browser_main_loop.cc:1081: } else if (task_runner_to_create) {
On 2016/12/01 15:15:21, fdoray wrote:
> DCHECK(!thread_to_start);

This is implicit in else block of if (thread_to_start)

https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse...
content/browser/browser_main_loop.cc:1244: // Clean up state that lives on or
uses the file_thread_ before it goes
On 2016/12/01 15:15:21, fdoray wrote:
> the FILE thread

Done.

https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse...
File content/browser/browser_thread_impl.cc (right):

https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse...
content/browser/browser_thread_impl.cc:18: #include
"base/task_scheduler/task_scheduler.h"
On 2016/12/01 15:15:21, fdoray wrote:
> Not needed.

Done.

https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse...
content/browser/browser_thread_impl.cc:109: // BrowserThread::ID is associated
but the underlying thread hasn't run yet.
On 2016/12/01 15:15:22, fdoray wrote:
> The INITIALIZED state is useless. See comment in
> BrowserThreadImpl::Initialize().

It is useful, replied there.

https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse...
content/browser/browser_thread_impl.cc:111: // BrowserThread::ID is associated
to a live thread.
On 2016/12/01 15:15:22, fdoray wrote:
> ... is associated to TaskRunner and is accepting tasks.

Done.

https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse...
content/browser/browser_thread_impl.cc:113: // BrowserThread::ID is associated
to a thread having been or being shutdown.
On 2016/12/01 15:15:21, fdoray wrote:
> ... no longer accepts tasks. If it was backed by a BrowserThreadImpl, this
> BrowserThreadImpl is being or has been shutdown.

Done (without second sentence).

https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse...
content/browser/browser_thread_impl.cc:136: // because |threads| isn't a good
signal per redirected IDs remaining null and
On 2016/12/01 15:15:22, fdoray wrote:
> |threads| doesn't exist

Done.

https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse...
content/browser/browser_thread_impl.cc:330: // PostTaskHelper() accesses the
message loop while holding this lock.
On 2016/12/01 15:15:22, fdoray wrote:
> PostTaskHelper() no longer accesses a MessageLoop. Change to:
> 
> // Change the state to SHUTDOWN so that PostTaskHelper stops
> // sending tasks to this thread. Do not clear 
> // globals.task_runners[identifier_] so that 
> // BrowserThread::CurrentlyOn() continues to work.

Done (slightly reworded).

https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse...
content/browser/browser_thread_impl.cc:346: globals.states[identifier_] =
BrowserThreadState::INITIALIZED;
On 2016/12/01 15:15:21, fdoray wrote:
> You can get rid of the INITIALIZED state. Initialize() could be replaced with
an
> anonymous function that verifies that the identifier it receives is valid.

No, INITIALIZED is required to support BrowserThread::IsThreadInitialized()
versus BrowserThread::IsMessageLoopValid(). I don't know that it ultimately
makes sense to have these two methods but such is the world we're dealing with
and I'd rather have a 1:1 mapping for this CL.

https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse...
content/browser/browser_thread_impl.cc:435: // threads.
On 2016/12/01 15:15:22, fdoray wrote:
> Add:
> 
> // Change the state to SHUTDOWN so that PostTaskHelper stops
> // sending tasks to this thread. Do not clear 
> // globals.task_runners[identifier] so that 
> // BrowserThread::CurrentlyOn() continues to work.

Detailed comments added to BrowserThreadImpl::StopRedirectionOfThreadID().

https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse...
content/browser/browser_thread_impl.cc:647: void BrowserThread::SetDelegate(ID
identifier,
On 2016/12/01 15:15:22, fdoray wrote:
> I would change this to SetIOThreadDelegate() and change
> BrowserThreadGlobals::thread_delegates[] ->
> BrowserThreadGlobals::io_thread_delegate. The UI thread never supported
> delegates. Other threads don't support delegates when they are redirected.

Good call, will do in precursor CL.

https://codereview.chromium.org/2464233002/diff/580001/content/public/test/te...
File content/public/test/test_browser_thread.h (right):

https://codereview.chromium.org/2464233002/diff/580001/content/public/test/te...
content/public/test/test_browser_thread.h:55: BrowserThread::ID identifier_;
On 2016/12/01 15:15:22, fdoray wrote:
> const

Done.

[gab, 2016-12-07 19:15:35]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-07 19:16:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-07 19:19:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-07 19:19:47]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[gab, 2016-12-07 20:01:54]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-07 20:03:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-07 21:24:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-07 21:24:59]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[gab, 2016-12-07 21:54:19]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-07 21:54:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-07 21:57:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-07 21:57:36]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[gab, 2016-12-07 22:11:37]

Patchset #36 (id:840001) has been deleted

[gab, 2016-12-07 22:12:07]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-07 22:12:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gab, 2016-12-07 22:12:32]

Patchset #36 (id:860001) has been deleted

[gab, 2016-12-07 22:13:29]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-07 22:13:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-07 22:17:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-07 22:17:03]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[robliao, 2016-12-08 01:57:17]

Looks reasonable. A few questions and comments.

https://codereview.chromium.org/2464233002/diff/860002/content/browser/browse...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/2464233002/diff/860002/content/browser/browse...
content/browser/browser_main_loop.cc:928: TRACE_EVENT_BEGIN1("startup",
Does it still make sense to do these TRACE_EVENT_BEGIN1's if we're going to
redirect the thread?

https://codereview.chromium.org/2464233002/diff/860002/content/browser/browse...
File content/browser/browser_thread_impl.cc (right):

https://codereview.chromium.org/2464233002/diff/860002/content/browser/browse...
content/browser/browser_thread_impl.cc:411: globals.states[identifier_] =
BrowserThreadState::RUNNING;
In the previous code, if thread creation failed, we hang forever (GetThreadId
appears to wait for ThreadMain to signal its event).

In this case, we mark the thread as running and proceed. Is this okay?

https://codereview.chromium.org/2464233002/diff/860002/content/browser/browse...
content/browser/browser_thread_impl.cc:514: base::SingleThreadTaskRunner*
task_runner =
Since we don't always lock here, it seems like it's possible for a task to get
posted after the flush event signal task above. Maybe we should lock now?

https://codereview.chromium.org/2464233002/diff/860002/content/public/test/te...
File content/public/test/test_browser_thread.cc (right):

https://codereview.chromium.org/2464233002/diff/860002/content/public/test/te...
content/public/test/test_browser_thread.cc:57: // Resets BrowserThreadImpl
globals binding |identifier_| to |impl_|. This is
How does the below set the binding from |identifier_| to |impl_|?

ResetGlobalForTesting looks like it just clears everything.

https://codereview.chromium.org/2464233002/diff/860002/content/public/test/te...
content/public/test/test_browser_thread.cc:65: // should instead use this
shutdown sequence: TestBrowserThread::Stop() ->
Nit: Might be easier to read this as a sequenced list.

[fdoray, 2016-12-08 14:51:46]

lgtm w/ comments

https://codereview.chromium.org/2464233002/diff/860002/content/browser/browse...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/2464233002/diff/860002/content/browser/browse...
content/browser/browser_main_loop.cc:928: TRACE_EVENT_BEGIN1("startup",
On 2016/12/08 01:57:17, robliao wrote:
> Does it still make sense to do these TRACE_EVENT_BEGIN1's if we're going to
> redirect the thread?

Before, StartWithOptions blocked until the thread was started
https://cs.chromium.org/chromium/src/content/browser/browser_thread_impl.cc?l...
Now, it returns immediately
https://codereview.chromium.org/2464233002/diff/860002/content/browser/browse...
. Hence, the duration of this trace event should be very short with or without
redirection.

https://codereview.chromium.org/2464233002/diff/860002/content/browser/browse...
content/browser/browser_main_loop.cc:958: // On Windows, the FILE thread needs
to be have a UI message loop which
needs to be have -> remove be

https://codereview.chromium.org/2464233002/diff/860002/content/browser/browse...
File content/browser/browser_thread_impl.cc (right):

https://codereview.chromium.org/2464233002/diff/860002/content/browser/browse...
content/browser/browser_thread_impl.cc:403: // Although the thread is starting
asynchronously. The MessageLoop is already
Although the thread is starting asynchronously <<,>> the

https://codereview.chromium.org/2464233002/diff/860002/content/browser/browse...
content/browser/browser_thread_impl.cc:411: globals.states[identifier_] =
BrowserThreadState::RUNNING;
On 2016/12/08 01:57:17, robliao wrote:
> In the previous code, if thread creation failed, we hang forever (GetThreadId
> appears to wait for ThreadMain to signal its event).
> 
> In this case, we mark the thread as running and proceed. Is this okay?

This change could make startup faster!

[gab, 2016-12-08 15:29:13]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-08 15:29:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-08 15:32:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-08 15:32:51]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[gab, 2016-12-08 15:48:55]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-08 15:49:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-08 15:53:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-08 15:53:11]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[gab, 2016-12-08 15:54:58]

Description was changed from

==========
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_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa...

Also generalizes BrowserThreadImpl away from the underlying thread's MessageLoop

and id in favor of TaskRunners and BrowserThreadState.

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.

BUG=653916
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.
==========

to

==========
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_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa...

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 :).

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.

BUG=653916
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.
==========

[gab, 2016-12-08 15:56:46]

Description was changed from

==========
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_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa...

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 :).

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.

BUG=653916
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.
==========

to

==========
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_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa...

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 :).

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
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.
==========

[gab, 2016-12-08 16:01:08]

Description was changed from

==========
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_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa...

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 :).

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
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.
==========

to

==========
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_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa...

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.

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
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.
==========

[gab, 2016-12-08 18:49:11]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[gab, 2016-12-08 18:49:42]

Done PTAnL, thanks!

https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse...
File content/browser/browser_thread_impl.cc (right):

https://codereview.chromium.org/2464233002/diff/580001/content/browser/browse...
content/browser/browser_thread_impl.cc:647: void BrowserThread::SetDelegate(ID
identifier,
On 2016/12/07 19:15:27, gab wrote:
> On 2016/12/01 15:15:22, fdoray wrote:
> > I would change this to SetIOThreadDelegate() and change
> > BrowserThreadGlobals::thread_delegates[] ->
> > BrowserThreadGlobals::io_thread_delegate. The UI thread never supported
> > delegates. Other threads don't support delegates when they are redirected.
> 
> Good call, will do in precursor CL.

Done in http://crrev.com/437271 :)

https://codereview.chromium.org/2464233002/diff/860002/content/browser/browse...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/2464233002/diff/860002/content/browser/browse...
content/browser/browser_main_loop.cc:928: TRACE_EVENT_BEGIN1("startup",
On 2016/12/08 14:51:46, fdoray wrote:
> On 2016/12/08 01:57:17, robliao wrote:
> > Does it still make sense to do these TRACE_EVENT_BEGIN1's if we're going to
> > redirect the thread?
> 
> Before, StartWithOptions blocked until the thread was started
>
https://cs.chromium.org/chromium/src/content/browser/browser_thread_impl.cc?l...
> Now, it returns immediately
>
https://codereview.chromium.org/2464233002/diff/860002/content/browser/browse...
> . Hence, the duration of this trace event should be very short with or without
> redirection.

Right, it should be short, but I don't think it hurts to have them. We can strip
them later in the follow-up CL when making this the default. In this CL I'm
merely looking to keep status quo.

https://codereview.chromium.org/2464233002/diff/860002/content/browser/browse...
content/browser/browser_main_loop.cc:958: // On Windows, the FILE thread needs
to be have a UI message loop which
On 2016/12/08 14:51:46, fdoray wrote:
> needs to be have -> remove be

Done.

https://codereview.chromium.org/2464233002/diff/860002/content/browser/browse...
File content/browser/browser_thread_impl.cc (right):

https://codereview.chromium.org/2464233002/diff/860002/content/browser/browse...
content/browser/browser_thread_impl.cc:403: // Although the thread is starting
asynchronously. The MessageLoop is already
On 2016/12/08 14:51:46, fdoray wrote:
> Although the thread is starting asynchronously <<,>> the

Done.

https://codereview.chromium.org/2464233002/diff/860002/content/browser/browse...
content/browser/browser_thread_impl.cc:411: globals.states[identifier_] =
BrowserThreadState::RUNNING;
On 2016/12/08 14:51:46, fdoray wrote:
> On 2016/12/08 01:57:17, robliao wrote:
> > In the previous code, if thread creation failed, we hang forever
(GetThreadId
> > appears to wait for ThreadMain to signal its event).
> > 
> > In this case, we mark the thread as running and proceed. Is this okay?

Yes this is fine because browser_main_loop.cc does this:

      if (!(*thread_to_start)->StartWithOptions(options))
        LOG(FATAL) << "Failed to start the browser thread: id == " << id;

> 
> This change could make startup faster!

Exactly, I was surprised to see that we were blocking on getting the ID here,
the whole point of making the start asynchronous a year or two ago was to avoid
blocking on threads that are launching (i.e. it's fine to let them start async
so long as we make their ML/TaskRunners available the callers can't notice the
difference). Looks like this was an unintentional regression from
http://crrev.com/408295.

So we're back to an asynchronous thread start, yay :).

Updated CL desc to highlight this.

https://codereview.chromium.org/2464233002/diff/860002/content/browser/browse...
content/browser/browser_thread_impl.cc:514: base::SingleThreadTaskRunner*
task_runner =
On 2016/12/08 01:57:17, robliao wrote:
> Since we don't always lock here, it seems like it's possible for a task to get
> posted after the flush event signal task above. Maybe we should lock now?

There is indeed a theoretical race as described in
BrowserThreadImpl::StopRedirectionOfThreadID() because a task posted to a
flushed thread A can theoretically run after a thread B that is meant to die
after thread A and B can thus unexpectedly have been shutdown ahead of a task on
A.

This theoretical race is only there on shutdown when the experiment is enabled.
The better solution long-term is to join IO thread, then
TaskScheduler::Shutdown() then destroy BrowserThreadImpl for UI thread. But this
requires that all other threads be redirected or we still have ordering
problems.

Solving the race in the meantime either requires always locking here (which
breaks my intent of this CL being status quo) or truly mimic'ing status quo by
introducing a Join on task_scheduler::Sequence which is a pain for something we
don't want long-term.

So we figured we were okay with a theoretical shutdown race in the initial
experiment (there's a TODO in BrowserThreadImpl::StopRedirectionOfThreadID() to
fix this before making it default).

https://codereview.chromium.org/2464233002/diff/860002/content/public/test/te...
File content/public/test/test_browser_thread.cc (right):

https://codereview.chromium.org/2464233002/diff/860002/content/public/test/te...
content/public/test/test_browser_thread.cc:57: // Resets BrowserThreadImpl
globals binding |identifier_| to |impl_|. This is
On 2016/12/08 01:57:17, robliao wrote:
> How does the below set the binding from |identifier_| to |impl_|?
> 
> ResetGlobalForTesting looks like it just clears everything.

I meant to say that it clears that very binding, reworded.

https://codereview.chromium.org/2464233002/diff/860002/content/public/test/te...
content/public/test/test_browser_thread.cc:65: // should instead use this
shutdown sequence: TestBrowserThread::Stop() ->
On 2016/12/08 01:57:17, robliao wrote:
> Nit: Might be easier to read this as a sequenced list.

Done.

[commit-bot: I haz the power, 2016-12-08 18:50:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-08 19:11:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-08 19:11:32]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[gab, 2016-12-08 19:16:55]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-08 19:17:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-08 21:05:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-08 21:05:21]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[gab, 2016-12-08 21:46:23]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[gab, 2016-12-08 21:46:36]

The CQ bit was unchecked by gab@chromium.org

[gab, 2016-12-08 21:47:13]

Description was changed from

==========
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_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa...

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.

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
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.
==========

to

==========
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_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa...

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.

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
# Would be nice but bot is too flaky at the moment...
# 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.
==========

[gab, 2016-12-08 21:47:28]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-08 21:48:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-08 23:29:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-08 23:29:20]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[gab, 2016-12-09 15:39:19]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-09 15:39:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-09 15:44:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-09 15:44:15]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[gab, 2016-12-09 18:40:52]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-09 18:41:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gab, 2016-12-09 20:05:51]

The CQ bit was unchecked by gab@chromium.org

[gab, 2016-12-09 20:06:27]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[gab, 2016-12-09 20:06:46]

The CQ bit was unchecked by gab@chromium.org

[gab, 2016-12-09 20:07:14]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-09 20:08:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gab, 2016-12-09 20:14:29]

Description was changed from

==========
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_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa...

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.

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
# Would be nice but bot is too flaky at the moment...
# 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.
==========

to

==========
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_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa...

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.

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
# Would be nice but bot is too flaky at the moment...
# 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.
==========

[gab, 2016-12-09 20:15:18]

Description was changed from

==========
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_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa...

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.

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
# Would be nice but bot is too flaky at the moment...
# 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.
==========

to

==========
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_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa...

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
# Would be nice but bot is too flaky at the moment...
# 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.
==========

[commit-bot: I haz the power, 2016-12-09 21:48:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-09 21:48:37]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[gab, 2016-12-09 23:44:54]

Patchset #42 (id:990001) has been deleted

[gab, 2016-12-09 23:45:33]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-09 23:46:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gab, 2016-12-09 23:46:46]

The CQ bit was unchecked by gab@chromium.org

[gab, 2016-12-09 23:47:23]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-09 23:48:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-10 01:48:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-10 01:48:13]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)

[gab, 2016-12-10 05:19:34]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-10 05:19:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-10 08:10:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-10 08:10:22]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[gab, 2016-12-11 23:20:34]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-11 23:20:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-12 02:14:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-12 02:14:46]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[gab, 2016-12-12 16:16:35]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-12 16:16:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-12 17:47:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-12 17:47:37]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[gab, 2016-12-14 14:08:12]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-14 14:08:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-14 14:56:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-14 14:56:38]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[gab, 2016-12-14 18:08:57]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-14 18:09:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[robliao, 2016-12-14 20:38:13]

lgtm

https://codereview.chromium.org/2464233002/diff/860002/content/browser/browse...
File content/browser/browser_thread_impl.cc (right):

https://codereview.chromium.org/2464233002/diff/860002/content/browser/browse...
content/browser/browser_thread_impl.cc:411: globals.states[identifier_] =
BrowserThreadState::RUNNING;
On 2016/12/08 18:49:41, gab wrote:
> On 2016/12/08 14:51:46, fdoray wrote:
> > On 2016/12/08 01:57:17, robliao wrote:
> > > In the previous code, if thread creation failed, we hang forever
> (GetThreadId
> > > appears to wait for ThreadMain to signal its event).
> > > 
> > > In this case, we mark the thread as running and proceed. Is this okay?
> 
> Yes this is fine because browser_main_loop.cc does this:
> 
>       if (!(*thread_to_start)->StartWithOptions(options))
>         LOG(FATAL) << "Failed to start the browser thread: id == " << id;
> 
> > 
> > This change could make startup faster!
> 
> Exactly, I was surprised to see that we were blocking on getting the ID here,
> the whole point of making the start asynchronous a year or two ago was to
avoid
> blocking on threads that are launching (i.e. it's fine to let them start async
> so long as we make their ML/TaskRunners available the callers can't notice the
> difference). Looks like this was an unintentional regression from
> http://crrev.com/408295.
> 
> So we're back to an asynchronous thread start, yay :).
> 
> Updated CL desc to highlight this.

The state seems a little wonky given that we enter RUNNING even on failure, but
given we check on the outside, I think I can live with that.

[gab, 2016-12-14 20:53:09]

gab@chromium.org changed reviewers:
+ jam@chromium.org

[gab, 2016-12-14 20:53:11]

Thanks, @avi for content (+jam who's had time to come back before everything
this tripped was fixed..!)

[gab, 2016-12-14 20:59:37]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-14 21:00:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gab, 2016-12-14 21:00:34]

Patchset #43 (id:1030001) has been deleted

[gab, 2016-12-14 21:02:08]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-14 21:03:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Avi (use Gerrit), 2016-12-14 21:32:13]

LGTM good luck

[gab, 2016-12-14 21:50:40]

The CQ bit was unchecked by gab@chromium.org

[gab, 2016-12-14 21:51:01]

The CQ bit was checked by gab@chromium.org

[gab, 2016-12-14 21:51:02]

The patchset sent to the CQ was uploaded after l-g-t-m from fdoray@chromium.org,
robliao@chromium.org
Link to the patchset: https://codereview.chromium.org/2464233002/#ps1070001
(title: "nice comment for forced WaitUntilThreadStarted() on Android")

[commit-bot: I haz the power, 2016-12-14 21:51:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gab, 2016-12-14 21:51:45]

gab@chromium.org changed reviewers:
- jam@chromium.org

[commit-bot: I haz the power, 2016-12-15 00:04:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-15 00:04:54]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[gab, 2016-12-15 19:22:50]

Description was changed from

==========
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_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa...

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
# Would be nice but bot is too flaky at the moment...
# 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.
==========

to

==========
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_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa...

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.
==========

[gab, 2016-12-15 23:33:48]

Patchset #46 (id:1110001) has been deleted

[gab, 2016-12-15 23:37:23]

The CQ bit was checked by gab@chromium.org

[gab, 2016-12-15 23:37:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
robliao@chromium.org, avi@chromium.org, fdoray@chromium.org
Link to the patchset: https://codereview.chromium.org/2464233002/#ps1130001
(title: "remove WaitUntilThreadStarted on Android, makes Android test
instrumentation hang, is previous issu…")

[commit-bot: I haz the power, 2016-12-15 23:38:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-16 03:35:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-16 03:35:25]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[gab, 2016-12-16 16:27:19]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2016-12-16 16:27:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-16 17:43:19]

CQ is committing da patch.

Bot data: {"patchset_id": 1130001, "attempt_start_ts": 1481905639694660,
"parent_rev": "26d22903b6eb33b154ee5d8ce232a0fe73f6e52d", "commit_rev":
"46377e449f09ecb45c5e41cc89fce609bf9cbbbc"}

[commit-bot: I haz the power, 2016-12-16 17:43:46]

Description was changed from

==========
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_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa...

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.
==========

to

==========
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_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa...

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
==========

[commit-bot: I haz the power, 2016-12-16 17:43:48]

Committed patchset #46 (id:1130001)

[commit-bot: I haz the power, 2016-12-16 17:46:29]

Description was changed from

==========
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_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa...

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
==========

to

==========
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_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa...

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.

Committed: https://crrev.com/3ee0e445ae07397a077eb3213fc46318af01b43e
Cr-Commit-Position: refs/heads/master@{#439139}
==========

[commit-bot: I haz the power, 2016-12-16 17:46:32]

Patchset 46 (id:??) landed as
https://crrev.com/3ee0e445ae07397a077eb3213fc46318af01b43e
Cr-Commit-Position: refs/heads/master@{#439139}