Make it possible to PostTask to TaskScheduler after it was teared down (required for policy migration off FILE thread) |
||
Issue descriptionChromeUnitTestSuiteInitializer is the base of all chrome unit tests and it seems to be responsible for the lifetime of TestingBrowserProcess - TestBrowserThreadBundle is needed somewhere in that layer in chrome unit tests. Link to original discussion: https://groups.google.com/a/chromium.org/forum/#!topic/scheduler-dev/gun5NNf4YSg
,
Sep 20 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cc8362b453ba41cd03f289e17e022eb6353e6946 commit cc8362b453ba41cd03f289e17e022eb6353e6946 Author: Gabriel Charette <gab@chromium.org> Date: Wed Sep 20 21:59:40 2017 Fix TestBrowserThreadBundle lifetime ordering vs ChromeBrowserPolicyConnector. This is a prerequesite to migrate ChromeBrowserPolicyConnector off of BrowserThread::FILE in https://chromium-review.googlesource.com/c/chromium/src/+/668556 i.e.: https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_rel_ng/builds/511298 ChromeBrowserPolicyConnector is lazily instantiated when a callstack gets to TestingBrowserProcess::chrome_browser_policy_connector() which happens in a surprising amount of random places. The reason this worked before is that BrowserThread::GetTaskRunnerForThread(BrowserThread::FILE) returns a lazy proxy which doesn't require that the BrowserThread::FILE actually be up and ready to receive tasks. base::CreateSequencedTaskRunnerWithTraits() however does require a TaskScheduler instance to be up (not necessarily started be at least instantiated, ref. Create/Start TaskScheduler semantics). While TaskScheduler *could* support lazy proxy task runners it has been unnecessary thus far and with this pretty much being the last use case for FILE thread we'd rather not go that route since it's cleaner anyways to have unit tests properly instantiate TestBrowserThreadBundle in the right order. This is mostly a trivial member re-ordering in each file except for RenderViewHostTestHarness which now initializes its TestBrowserThreadBundle at construction instead of SetUp() time so that it's done ahead of its subclasses' members. RenderViewHostTestHarness also drops the explicit ScopedTaskEnvironment member introduced in https://chromium-review.googlesource.com/c/chromium/src/+/575383 as it is not necessary to expose that to RunUntilIdle() (comment added in this CL in TestBrowserThreadBundle to clarify). Bug: 689520 , 742303 Change-Id: I6fb81d45566838dd4d5a8c2c650519ddeeebf8af TBR=jam@chromium.org (see https://chromium-review.googlesource.com/c/chromium/src/+/671635#message-280432e2a1fbd0337d148ba64ad8aead69ab0bb7) Change-Id: I6fb81d45566838dd4d5a8c2c650519ddeeebf8af Reviewed-on: https://chromium-review.googlesource.com/671635 Commit-Queue: Gabriel Charette <gab@chromium.org> Reviewed-by: Gabriel Charette <gab@chromium.org> Reviewed-by: Xiyuan Xia <xiyuan@chromium.org> Cr-Commit-Position: refs/heads/master@{#503262} [modify] https://crrev.com/cc8362b453ba41cd03f289e17e022eb6353e6946/chrome/browser/chromeos/customization/customization_document_unittest.cc [modify] https://crrev.com/cc8362b453ba41cd03f289e17e022eb6353e6946/chrome/browser/chromeos/login/enrollment/enrollment_screen_unittest.cc [modify] https://crrev.com/cc8362b453ba41cd03f289e17e022eb6353e6946/chrome/browser/chromeos/policy/bluetooth_policy_handler_unittest.cc [modify] https://crrev.com/cc8362b453ba41cd03f289e17e022eb6353e6946/chrome/browser/chromeos/policy/network_configuration_updater_unittest.cc [modify] https://crrev.com/cc8362b453ba41cd03f289e17e022eb6353e6946/chrome/browser/chromeos/system/device_disabling_manager_unittest.cc [modify] https://crrev.com/cc8362b453ba41cd03f289e17e022eb6353e6946/chrome/browser/chromeos/ui/low_disk_notification_unittest.cc [modify] https://crrev.com/cc8362b453ba41cd03f289e17e022eb6353e6946/chrome/browser/data_usage/tab_id_annotator_unittest.cc [modify] https://crrev.com/cc8362b453ba41cd03f289e17e022eb6353e6946/chrome/browser/download/download_resource_throttle_unittest.cc [modify] https://crrev.com/cc8362b453ba41cd03f289e17e022eb6353e6946/chrome/browser/extensions/extension_gcm_app_handler_unittest.cc [modify] https://crrev.com/cc8362b453ba41cd03f289e17e022eb6353e6946/chrome/browser/media_galleries/media_file_system_registry_unittest.cc [modify] https://crrev.com/cc8362b453ba41cd03f289e17e022eb6353e6946/chrome/browser/media_galleries/win/mtp_device_delegate_impl_win_unittest.cc [modify] https://crrev.com/cc8362b453ba41cd03f289e17e022eb6353e6946/chrome/browser/profiles/profile_manager_unittest.cc [modify] https://crrev.com/cc8362b453ba41cd03f289e17e022eb6353e6946/chrome/browser/safe_browsing/ui_manager_unittest.cc [modify] https://crrev.com/cc8362b453ba41cd03f289e17e022eb6353e6946/chrome/browser/ssl/ssl_error_handler_unittest.cc [modify] https://crrev.com/cc8362b453ba41cd03f289e17e022eb6353e6946/chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc [modify] https://crrev.com/cc8362b453ba41cd03f289e17e022eb6353e6946/chrome/test/base/chrome_render_view_host_test_harness.cc [modify] https://crrev.com/cc8362b453ba41cd03f289e17e022eb6353e6946/chrome/test/base/chrome_render_view_host_test_harness.h [modify] https://crrev.com/cc8362b453ba41cd03f289e17e022eb6353e6946/components/data_reduction_proxy/content/browser/content_lofi_ui_service_unittest.cc [modify] https://crrev.com/cc8362b453ba41cd03f289e17e022eb6353e6946/content/browser/renderer_host/media/audio_output_authorization_handler_unittest.cc [modify] https://crrev.com/cc8362b453ba41cd03f289e17e022eb6353e6946/content/browser/renderer_host/media/audio_renderer_host_unittest.cc [modify] https://crrev.com/cc8362b453ba41cd03f289e17e022eb6353e6946/content/public/test/test_browser_thread_bundle.h [modify] https://crrev.com/cc8362b453ba41cd03f289e17e022eb6353e6946/content/public/test/test_renderer_host.cc [modify] https://crrev.com/cc8362b453ba41cd03f289e17e022eb6353e6946/content/public/test/test_renderer_host.h
,
Sep 21 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3a32d46d2049a9068e282b8b035d09fb5f93aa40 commit 3a32d46d2049a9068e282b8b035d09fb5f93aa40 Author: Gabriel Charette <gab@chromium.org> Date: Thu Sep 21 02:26:13 2017 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} [modify] https://crrev.com/3a32d46d2049a9068e282b8b035d09fb5f93aa40/base/task_scheduler/scheduler_single_thread_task_runner_manager.cc [modify] https://crrev.com/3a32d46d2049a9068e282b8b035d09fb5f93aa40/base/task_scheduler/scheduler_single_thread_task_runner_manager.h [modify] https://crrev.com/3a32d46d2049a9068e282b8b035d09fb5f93aa40/base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc [modify] https://crrev.com/3a32d46d2049a9068e282b8b035d09fb5f93aa40/base/task_scheduler/scheduler_worker_pool.cc [modify] https://crrev.com/3a32d46d2049a9068e282b8b035d09fb5f93aa40/base/task_scheduler/scheduler_worker_pool.h [modify] https://crrev.com/3a32d46d2049a9068e282b8b035d09fb5f93aa40/base/task_scheduler/scheduler_worker_pool_unittest.cc [modify] https://crrev.com/3a32d46d2049a9068e282b8b035d09fb5f93aa40/base/task_scheduler/task_scheduler.cc [modify] https://crrev.com/3a32d46d2049a9068e282b8b035d09fb5f93aa40/base/task_scheduler/task_scheduler.h [modify] https://crrev.com/3a32d46d2049a9068e282b8b035d09fb5f93aa40/chrome/browser/browser_process_impl_unittest.cc [modify] https://crrev.com/3a32d46d2049a9068e282b8b035d09fb5f93aa40/chrome/browser/io_thread_unittest.cc [modify] https://crrev.com/3a32d46d2049a9068e282b8b035d09fb5f93aa40/chrome/browser/policy/chrome_browser_policy_connector.cc [modify] https://crrev.com/3a32d46d2049a9068e282b8b035d09fb5f93aa40/chrome/test/base/chrome_unit_test_suite.cc
,
Sep 21 2017
|
||
►
Sign in to add a comment |
||
Comment 1 by isandrk@chromium.org
, Jul 13 2017