New issue
Advanced search Search tips

Issue 742303 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 689520



Sign in to add a comment

Make it possible to PostTask to TaskScheduler after it was teared down (required for policy migration off FILE thread)

Project Member Reported by isandrk@chromium.org, Jul 13 2017

Issue description

ChromeUnitTestSuiteInitializer 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
 
Cc: emaxx@chromium.org
Project Member

Comment 2 by bugdroid1@chromium.org, 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

Project Member

Comment 3 by bugdroid1@chromium.org, 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

Comment 4 by gab@chromium.org, Sep 21 2017

Status: Fixed (was: Untriaged)
Summary: Make it possible to PostTask to TaskScheduler after it was teared down (required for policy migration off FILE thread) (was: Add TestBrowserThreadBundle to chrome unit tests)

Sign in to add a comment