commit 3a32d46d2049a9068e282b8b035d09fb5f93aa40
author Gabriel Charette <gab@chromium.org> Thu Sep 21 02:26:13 2017
committer Commit Bot <commit-bot@chromium.org> Thu Sep 21 02:26:13 2017
tree 9ad682b32bbd1529bf31e781e5765671d73162e0
parent 0ca2565e0ae6f7fd1b5aaf9aca55ac8520ba15c5

Migrate ChromeBrowserPolicyConnector to TaskScheduler

This is a continuity of the work @
https://chromium-review.googlesource.com/c/chromium/src/+/555496
and strips one of the very last usage of BrowserThread::FILE :).

The complexity in this CL arises from a late call to
AsyncPolicyProvider::Shutdown() which uses its TaskRunner's PostTask 
return value to determine whether the DeleteSoon of its 
AsyncPolicyLoader was successful (task environment still up) or failed
(and can be safely deleted on main thread per everything being 
naturally sequenced on main thread after the task environment is gone):
https://cs.chromium.org/chromium/src/components/policy/core/common/async_policy_provider.cc?type=cs&q=AsyncPolicyProvider::Shutdown%5C(%5C)+file:async_policy_provider.cc&sq=package:chromium&l=52

Until this CL, PostTask() on a TaskScheduler provided TaskRunner after
shutdown crashes in unit tests per the ScopedTaskEnvironment being gone
(in production we leak the environment in the shutdown phase for that
very reason; whereas in tests we join everything and then assume no further
calls). AsyncPolicyProvider::Shutdown() is called by
~TestingBrowserProcess() from chrome_unit_test_suite.cc::OnTestEnd()
which happens after the test fixture was destroyed. Furthermore,
a solution involving a custom TestingBrowserProcess call isn't
possible because this doesn't only affect the policy tests but
every chrome unit tests that happens to unintentionally instantiate
the policy service.

This CL gives the task_scheduler TaskRunners the ability
to return false if the environment is already completely gone when
PostTask() is invoked.

The chosen approach also gets rid of need for custom handling of this
use case in chrome_unit_test_suite.cc :).

This CL also removes BrowserProcessImplTest's usage of FILE thread as
it was now only necessary to satisfy ChromeBrowserPolicyConnector's
usage as part of its Lifecycle test :).

Other approaches considered:

A) Have AsyncPolicyProvider use a LazySequencedTaskRunner and update
the code in chrome_unit_test_suite.cc to provide a
ScopedTaskEnvironment. The call to DeleteSoon() would thus generate
a new SequencedTaskRunner in the new ScopedTaskEnvironment and would
be "sequenced" by virtue of an happens-after relationship between the
two environments.
This isn't great because:
  1) it doesn't solve the problem at scale (AsyncPolicyProvider::
     Shutdown() is doing something which is reasonable and shouldn't
     require custom handling).
  2) it's not actually possible as-is because the TaskRunner is passed
     by parameter and as such isn't compatible with
     LazySequencedTaskRunner. This parameter is injected from various
     tricky callsites and this would be a pain to update for this CL.

B) TestBrowserThreadBundle::TaskEnvironmentManager. The
TestingBrowserProcess could register itself on TBTB as a
TaskEnvironmentManager which would have the contract that TBTB would
hand its ScopedTaskEnvironment to it when destroyed instead of
destroying it.
This isn't great because:
  1) It unexpectdely breaks RAII for TBTB.
  2) TestingBrowserProcess can't have a TBTB member however and own
     it for every test (would keep sane RAII properties) as that
     prevents test fixtures from using TBTB::Options to customize their
     environment.
  3) TBTB sometimes doesn't own its ScopedTaskEnvironment (an
     explicit one can be provided by the test environment). This takes
     that solution for a "meh" to a "hell no" as shady RAII properties
     which are only applied in some scenarios are just horrendous.

Bug: 689520, 742303
Change-Id: I87341452cb80b249465894170f6ebd3adcc685de
Reviewed-on: https://chromium-review.googlesource.com/668556
Reviewed-by: Francois Doray <fdoray@chromium.org>
Reviewed-by: Robert Liao <robliao@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Bartosz Fabianowski <bartfab@chromium.org>
Commit-Queue: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#503312}