Skip to content
  • Gabriel Charette's avatar
    Migrate ChromeBrowserPolicyConnector to TaskScheduler · 3a32d46d
    Gabriel Charette authored
    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: default avatarFrancois Doray <fdoray@chromium.org>
    Reviewed-by: default avatarRobert Liao <robliao@chromium.org>
    Reviewed-by: default avatarLei Zhang <thestig@chromium.org>
    Reviewed-by: default avatarBartosz Fabianowski <bartfab@chromium.org>
    Commit-Queue: Gabriel Charette <gab@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#503312}
    3a32d46d