-
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: 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}
3a32d46d