New issue
Advanced search Search tips

Issue 622400 link

Starred by 7 users
Status: Fixed
Owner:
gab@chromium.org
Closed: May 2018
Cc:
robliao@chromium.org
fdoray@chromium.org
gab@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Feature

Blocked on:
issue 611818
issue 629716
issue 636518
issue 670137
issue 682057
Blocking:
issue 623700
issue 694688
issue 553459
issue 667892



Sign in to add a comment

Experiment with redirecting the SequencedWorkerPool to TaskScheduler in the browser process

Project Member Reported by gab@chromium.org, Jun 22 2016 
https://codereview.chromium.org/2077413009/ will introduce a new API surface on SequencedWorkerPool that allows callers to specify the TaskPriority associated with tasks posted to that pool.

After which SequencedWorkerPool will be tweaked to redirect its tasks/sequences to TaskScheduler if (1) a TaskScheduler instance exists in this process (i.e. in browser process) and (2) an experiment (name TBD) says to do the redirection.

Note: special care will be taken for pools that only have 1 thread (e.g. webcrypto) and were merely used as a way to get access to ShutdownBehavior logic from a single-thread (I think..?). The TaskScheduler will be told to run tasks of such SequencedWorkerPools under ExecutionMode::SINGLE_THREADED instead of ExecutionMode::SEQUENCED/PARALLEL the regular redirection use case.

Comment 1 by eroman@chromium.org, Jun 23 2016 

> Note: special care will be taken for pools that only have 1 thread
> (e.g. webcrypto) and were merely used as a way to get access to
> ShutdownBehavior logic from a single-thread (I think..?). 

The WebCrypto design constraints at the time were needing a sequenced runner, and for tasks to not be joined at shutdown.

Also note that this threadpool is running in the *renderer* process.

Some of those dependencies have since changed, and I am in the process of updating documentation around this and checking the implementation (https://codereview.chromium.org/2088323002/).

The good news is I *believe* we can drop these requirements and go full-blown ExecutionMode::PARALLEL using your new TaskScheduler.

This would be helpful in fact, since right now the single threadedness of WebCrypto is problematic for consumers wanting to do bulk operations and saturate their CPU. But at the same time I was reluctant to simply let web sites have unfettered control and make things unresponsive, and was mulling with crazy ideas like  issue 615133 .

The TaskScheduler seems like it could solve this. Just note that the quantity and types of these tasks are directly controlled by users through Javascript

Feel free to chat with me more about this off-thread or on a different bug. Don't mean to derail this bug.

Cheers.

Comment 2 by gab@chromium.org, Jun 27 2016

Labels: -Type-Bug Type-Feature

Comment 3 by gab@chromium.org, Jul 20 2016

Blockedon: 629716
Project Member

Comment 4 by bugdroid1@chromium.org, Jul 20 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5fc3fafd0a77ca8acd30ddc8cfa17fd669eb9b56

commit 5fc3fafd0a77ca8acd30ddc8cfa17fd669eb9b56
Author: gab <gab@chromium.org>
Date: Wed Jul 20 04:23:04 2016

Add TaskPriority as a parameter to SequencedWorkerPool in preparation for TaskScheduler experiment.

(ref.  http://crbug.com/622400  for experiment details)

The mapping should be as follows:
 - TaskPriority::USER_BLOCKING : the pool runs tasks that are on the blocking path to responding to
                                 a user action.
 - TaskPriority::USER_VISIBLE : the pool runs tasks that must not block (i.e. have visible outcomes)
                                but can be re-ordered behind USER_BLOCKING work.
 - TaskPriority::BACKGROUND : the pool runs non-critical background tasks that should not interfere
                              with foreground work.

Since each pool can only be assigned a single priority (until we do the full migration and each
TaskRunner extracted from it can have its own priority), the highest priority among all tasks
running in that pool should be picked if the pool runs multiple types of tasks.

Note: for usage in unittests I picked USER_VISIBLE since tests shouldn't run at background OS priority
      but it's okay if USER_BLOCKING work gets re-ordered ahead of the test's tasks. A TestTaskScheduler
      will likely soon replace those use cases.

BUG= 553459 ,  622400 

Review-Url: https://codereview.chromium.org/2077413009
Cr-Commit-Position: refs/heads/master@{#406481}

[modify] https://crrev.com/5fc3fafd0a77ca8acd30ddc8cfa17fd669eb9b56/ash/sysui/sysui_application.cc
[modify] https://crrev.com/5fc3fafd0a77ca8acd30ddc8cfa17fd669eb9b56/base/test/sequenced_worker_pool_owner.cc
[modify] https://crrev.com/5fc3fafd0a77ca8acd30ddc8cfa17fd669eb9b56/base/threading/sequenced_worker_pool.cc
[modify] https://crrev.com/5fc3fafd0a77ca8acd30ddc8cfa17fd669eb9b56/base/threading/sequenced_worker_pool.h
[modify] https://crrev.com/5fc3fafd0a77ca8acd30ddc8cfa17fd669eb9b56/base/trace_event/memory_dump_manager_unittest.cc
[modify] https://crrev.com/5fc3fafd0a77ca8acd30ddc8cfa17fd669eb9b56/chrome/service/service_process.cc
[modify] https://crrev.com/5fc3fafd0a77ca8acd30ddc8cfa17fd669eb9b56/components/webcrypto/webcrypto_impl.cc
[modify] https://crrev.com/5fc3fafd0a77ca8acd30ddc8cfa17fd669eb9b56/content/browser/browser_thread_impl.cc
[modify] https://crrev.com/5fc3fafd0a77ca8acd30ddc8cfa17fd669eb9b56/device/bluetooth/bluetooth_task_manager_win.cc
[modify] https://crrev.com/5fc3fafd0a77ca8acd30ddc8cfa17fd669eb9b56/ios/chrome/browser/net/image_fetcher_unittest.mm
[modify] https://crrev.com/5fc3fafd0a77ca8acd30ddc8cfa17fd669eb9b56/ios/web/web_thread_impl.cc
[modify] https://crrev.com/5fc3fafd0a77ca8acd30ddc8cfa17fd669eb9b56/net/disk_cache/blockfile/file_posix.cc
[modify] https://crrev.com/5fc3fafd0a77ca8acd30ddc8cfa17fd669eb9b56/net/disk_cache/simple/simple_backend_impl.cc
[modify] https://crrev.com/5fc3fafd0a77ca8acd30ddc8cfa17fd669eb9b56/net/proxy/dhcp_proxy_script_adapter_fetcher_win_unittest.cc
[modify] https://crrev.com/5fc3fafd0a77ca8acd30ddc8cfa17fd669eb9b56/net/proxy/dhcp_proxy_script_fetcher_win.cc
[modify] https://crrev.com/5fc3fafd0a77ca8acd30ddc8cfa17fd669eb9b56/services/shell/runner/host/child_process_host_unittest.cc
[modify] https://crrev.com/5fc3fafd0a77ca8acd30ddc8cfa17fd669eb9b56/services/shell/standalone/context.cc
[modify] https://crrev.com/5fc3fafd0a77ca8acd30ddc8cfa17fd669eb9b56/tools/gn/header_checker.cc
[modify] https://crrev.com/5fc3fafd0a77ca8acd30ddc8cfa17fd669eb9b56/tools/gn/scheduler.cc
Project Member

Comment 5 by bugdroid1@chromium.org, Aug 1 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c964a852c558c9876e49b1bcea0d98c720b5b300

commit c964a852c558c9876e49b1bcea0d98c720b5b300
Author: gab <gab@chromium.org>
Date: Mon Aug 01 16:39:56 2016

Move EXPECT_DCHECK_DEATH from base/task_scheduler and use it in relevant base/ tests

Extracted from https://codereview.chromium.org/2163023002/

This has the following advantages:
 - Reduces inconsistent usage of #if preprocessors macros for death tests.
 - Enables some tests when DCHECK_ALWAYS_ON in Release builds
   that were previously not (per aforementioned inconsistency)
 - Runs those tests anyways (modulo call expected to die) in non-DCHECK
   builds -- resulting in extra coverage (though sometimes mostly a no-op).

TODO(gab): Try to enable death tests on OS_ANDROID in a follow-up CL and document what breaks.

BUG= 622400 
(relation to that bug is the attempt to open as many tests as possible
to Release+DCHECK_ALWAYS_ON started in https://codereview.chromium.org/2163023002/
as a safety measure for the impl for 622400)

Review-Url: https://codereview.chromium.org/2162053006
Cr-Commit-Position: refs/heads/master@{#408978}

[modify] https://crrev.com/c964a852c558c9876e49b1bcea0d98c720b5b300/base/BUILD.gn
[modify] https://crrev.com/c964a852c558c9876e49b1bcea0d98c720b5b300/base/base.gyp
[modify] https://crrev.com/c964a852c558c9876e49b1bcea0d98c720b5b300/base/bind_unittest.cc
[modify] https://crrev.com/c964a852c558c9876e49b1bcea0d98c720b5b300/base/memory/weak_ptr_unittest.cc
[modify] https://crrev.com/c964a852c558c9876e49b1bcea0d98c720b5b300/base/message_loop/message_pump_io_ios_unittest.cc
[modify] https://crrev.com/c964a852c558c9876e49b1bcea0d98c720b5b300/base/message_loop/message_pump_libevent_unittest.cc
[modify] https://crrev.com/c964a852c558c9876e49b1bcea0d98c720b5b300/base/metrics/field_trial_unittest.cc
[modify] https://crrev.com/c964a852c558c9876e49b1bcea0d98c720b5b300/base/metrics/histogram_unittest.cc
[modify] https://crrev.com/c964a852c558c9876e49b1bcea0d98c720b5b300/base/metrics/persistent_sample_map_unittest.cc
[modify] https://crrev.com/c964a852c558c9876e49b1bcea0d98c720b5b300/base/metrics/sample_map_unittest.cc
[modify] https://crrev.com/c964a852c558c9876e49b1bcea0d98c720b5b300/base/metrics/sample_vector_unittest.cc
[modify] https://crrev.com/c964a852c558c9876e49b1bcea0d98c720b5b300/base/synchronization/atomic_flag_unittest.cc
[modify] https://crrev.com/c964a852c558c9876e49b1bcea0d98c720b5b300/base/task_scheduler/priority_queue_unittest.cc
[modify] https://crrev.com/c964a852c558c9876e49b1bcea0d98c720b5b300/base/task_scheduler/scheduler_lock_unittest.cc
[modify] https://crrev.com/c964a852c558c9876e49b1bcea0d98c720b5b300/base/task_scheduler/scheduler_worker_pool_impl_unittest.cc
[modify] https://crrev.com/c964a852c558c9876e49b1bcea0d98c720b5b300/base/task_scheduler/scheduler_worker_stack_unittest.cc
[modify] https://crrev.com/c964a852c558c9876e49b1bcea0d98c720b5b300/base/task_scheduler/task_tracker_unittest.cc
[delete] https://crrev.com/ae4ee3b128207ec342b216e3ab9c6477a1e3c37d/base/task_scheduler/test_utils.h
[modify] https://crrev.com/c964a852c558c9876e49b1bcea0d98c720b5b300/base/test/gtest_util.h
[modify] https://crrev.com/c964a852c558c9876e49b1bcea0d98c720b5b300/base/threading/non_thread_safe_unittest.cc
[modify] https://crrev.com/c964a852c558c9876e49b1bcea0d98c720b5b300/base/threading/thread_checker_unittest.cc
Project Member

Comment 6 by bugdroid1@chromium.org, Aug 1 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/190f7546ebe213c61874ec8885be95c0f98a84d1

commit 190f7546ebe213c61874ec8885be95c0f98a84d1
Author: gab <gab@chromium.org>
Date: Mon Aug 01 20:03:41 2016

Unify usage of logging/assert macros in base/

* Enables following checks in Release+DCHECK_ALWAYS_ON build (previous DCHECKs behind !NDEBUG):
 - icu_util
 - LazyInstance
 - CancellationFlag

* Unifies ENABLE_DLOG into DCHECK_IS_ON()

* Mass replaces (!NDEBUG || DCHECK_ALWAYS_ON) with DCHECK_IS_ON()

TBR=rkaplow, alokp, boliu
 - rkaplow for chrome/browser/metrics/ side-effects
 - alokp for chromecast/ side-effects
 - boliu for device/power_save_blocker/ side-effects
BUG= 622400 
(grew tired of DCHECKs behind NDEBUG when writing
https://codereview.chromium.org/2135413003/ for https://codereview.chromium.org/2103883004/#msg14)

Review-Url: https://codereview.chromium.org/2163023002
Cr-Commit-Position: refs/heads/master@{#409036}

[modify] https://crrev.com/190f7546ebe213c61874ec8885be95c0f98a84d1/base/android/build_info.cc
[modify] https://crrev.com/190f7546ebe213c61874ec8885be95c0f98a84d1/base/i18n/icu_util.cc
[modify] https://crrev.com/190f7546ebe213c61874ec8885be95c0f98a84d1/base/lazy_instance.h
[modify] https://crrev.com/190f7546ebe213c61874ec8885be95c0f98a84d1/base/logging.h
[modify] https://crrev.com/190f7546ebe213c61874ec8885be95c0f98a84d1/base/memory/singleton.h
[modify] https://crrev.com/190f7546ebe213c61874ec8885be95c0f98a84d1/base/message_loop/incoming_task_queue.cc
[modify] https://crrev.com/190f7546ebe213c61874ec8885be95c0f98a84d1/base/numerics/safe_numerics_unittest.cc
[modify] https://crrev.com/190f7546ebe213c61874ec8885be95c0f98a84d1/base/sequence_checker.h
[modify] https://crrev.com/190f7546ebe213c61874ec8885be95c0f98a84d1/base/task/cancelable_task_tracker_unittest.cc
[modify] https://crrev.com/190f7546ebe213c61874ec8885be95c0f98a84d1/base/task_scheduler/scheduler_worker_pool_impl_unittest.cc
[modify] https://crrev.com/190f7546ebe213c61874ec8885be95c0f98a84d1/base/task_scheduler/task_scheduler_impl_unittest.cc
[modify] https://crrev.com/190f7546ebe213c61874ec8885be95c0f98a84d1/base/threading/non_thread_safe.h
[modify] https://crrev.com/190f7546ebe213c61874ec8885be95c0f98a84d1/base/threading/non_thread_safe_unittest.cc
[modify] https://crrev.com/190f7546ebe213c61874ec8885be95c0f98a84d1/base/threading/platform_thread_win.cc
[modify] https://crrev.com/190f7546ebe213c61874ec8885be95c0f98a84d1/base/threading/thread_checker.h
[modify] https://crrev.com/190f7546ebe213c61874ec8885be95c0f98a84d1/base/threading/thread_checker_unittest.cc
[modify] https://crrev.com/190f7546ebe213c61874ec8885be95c0f98a84d1/base/threading/thread_restrictions.cc
[modify] https://crrev.com/190f7546ebe213c61874ec8885be95c0f98a84d1/base/threading/thread_restrictions.h
[modify] https://crrev.com/190f7546ebe213c61874ec8885be95c0f98a84d1/chrome/browser/metrics/thread_watcher_android.cc
[modify] https://crrev.com/190f7546ebe213c61874ec8885be95c0f98a84d1/chromecast/app/linux/cast_crash_reporter_client_unittest.cc
[modify] https://crrev.com/190f7546ebe213c61874ec8885be95c0f98a84d1/device/power_save_blocker/power_save_blocker_mac.cc
Project Member

Comment 8 by bugdroid1@chromium.org, Aug 3 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ed5ccb96b02293af2f9daa6f3c21b757f843304e

commit ed5ccb96b02293af2f9daa6f3c21b757f843304e
Author: gab <gab@chromium.org>
Date: Wed Aug 03 23:45:45 2016

Remove dependency of SSLPlatformKeyTaskRunner on single-threaded SequencedWorkerPool.

Use a base::Thread bound to a LazyInstance (so it's never joined) instead,
keeping CONTINUE_ON_SHUTDOWN semantics.

Make the API explicitly use SingleThreadTaskRunner for both SSLPlatformKeyTaskRunner
and ThreadedSSLPrivateKey as they appear to have a hard requirement on this being
the case: https://codereview.chromium.org/2077413009/#msg16

Explicitly document that the exposed SingleThreadTaskRunner has CONTINUE_ON_SHUTDOWN
semantics as this is also a critical requirement.

BUG= 622400 

Review-Url: https://codereview.chromium.org/2103883004
Cr-Commit-Position: refs/heads/master@{#409668}

[modify] https://crrev.com/ed5ccb96b02293af2f9daa6f3c21b757f843304e/net/ssl/ssl_platform_key_task_runner.cc
[modify] https://crrev.com/ed5ccb96b02293af2f9daa6f3c21b757f843304e/net/ssl/ssl_platform_key_task_runner.h
[modify] https://crrev.com/ed5ccb96b02293af2f9daa6f3c21b757f843304e/net/ssl/threaded_ssl_private_key.cc
[modify] https://crrev.com/ed5ccb96b02293af2f9daa6f3c21b757f843304e/net/ssl/threaded_ssl_private_key.h

Comment 9 by gab@chromium.org, Aug 10 2016

Blockedon: 636518
Project Member

Comment 10 by bugdroid1@chromium.org, Aug 13 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/620b836c478741d8eec1ef5255f909e6d7e9b649

commit 620b836c478741d8eec1ef5255f909e6d7e9b649
Author: gab <gab@chromium.org>
Date: Sat Aug 13 18:38:29 2016

Move OomKillsMonitor from a single-threaded SequencedWorkerPool to a non-joinable thread.

The semantics are equivalent after this CL:
 - SequencedWorkerPool::CONTINUE_ON_SHUTDOWN => non-joinable thread + leaky instance.
   - Actually, it's better than before because with lack of a Leaky lazy instance,
     there was a join despite the CONTINUE_ON_SHUTDOWN request [1].
 - Ramping worker thread down on destruction through OomKillsMonitorHandle.

[1] https://cs.chromium.org/chromium/src/base/threading/sequenced_worker_pool.cc?rcl=1469791693&l=601

BUG= 622400 

Review-Url: https://codereview.chromium.org/2197753002
Cr-Commit-Position: refs/heads/master@{#411899}

[modify] https://crrev.com/620b836c478741d8eec1ef5255f909e6d7e9b649/components/arc/metrics/arc_metrics_service.cc
[modify] https://crrev.com/620b836c478741d8eec1ef5255f909e6d7e9b649/components/arc/metrics/arc_metrics_service.h
[modify] https://crrev.com/620b836c478741d8eec1ef5255f909e6d7e9b649/components/arc/metrics/oom_kills_monitor.cc
[modify] https://crrev.com/620b836c478741d8eec1ef5255f909e6d7e9b649/components/arc/metrics/oom_kills_monitor.h
Project Member

Comment 12 by bugdroid1@chromium.org, Aug 20 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5a65a6844ec5348feedb041cad8721ab79715395

commit 5a65a6844ec5348feedb041cad8721ab79715395
Author: gab <gab@chromium.org>
Date: Sat Aug 20 18:30:11 2016

Add an experiment to redirect SequencedWorkerPool tasks to TaskScheduler.

The experiment is controlled by a static call for the entire process
which must be made before any SequencedWorkerPool instance is posted
a task (i.e. before it creates a Worker).

The experiment being at run-time, sequenced_worker_pool.cc is now divided
between the two worlds. DCHECKs were added to associate as many methods as
possible to the world they belong to.

FYI, the experiment (and TEST= below) will not actually work until
https://codereview.chromium.org/2237643002/ or a variant of it lands
but this is a stepping stone to getting there and the CLs
themselves can land in any order.

BUG= 622400 
TEST=
A) "out\Debug\chrome.exe" runs without a problem, BlockingPool tasks execute as normal in chrome://tracing
B) "out\Debug\chrome --force-fieldtrials=BrowserScheduler/Enabled/ --force-fieldtrial-params=BrowserScheduler.Enabled:Background/3;3;1;0;10000/BackgroundFileIO/3;3;1;0;10000/Foreground/3;3;1;0;10000/ForegroundFileIO/3;3;1;0;10000/RedirectSequencedWorkerPools/true"
runs without a problem and chrome://tracing shows BlockingPool/CachePool/etc. tasks in the TaskScheduler workers :-)

Review-Url: https://codereview.chromium.org/2241703002
Cr-Commit-Position: refs/heads/master@{#413346}

[modify] https://crrev.com/5a65a6844ec5348feedb041cad8721ab79715395/base/threading/sequenced_worker_pool.cc
[modify] https://crrev.com/5a65a6844ec5348feedb041cad8721ab79715395/base/threading/sequenced_worker_pool.h
[modify] https://crrev.com/5a65a6844ec5348feedb041cad8721ab79715395/chrome/browser/chrome_browser_main.cc
Project Member

Comment 13 by bugdroid1@chromium.org, Aug 27 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fc341fe14e833479aebe4f1395ebbaf1b62d2957

commit fc341fe14e833479aebe4f1395ebbaf1b62d2957
Author: fdoray <fdoray@chromium.org>
Date: Sat Aug 27 23:55:47 2016

Remove calls to SequencedWorkerPool::GetSequenceTokenForCurrentThread().

Remove calls to SequencedWorkerPool::GetSequenceTokenForCurrentThread()
outside of base/. This won't return a valid sequence token when called
from a task forwarded to the new base/task_scheduler.

BUG= 622400 

Review-Url: https://codereview.chromium.org/2282813002
Cr-Commit-Position: refs/heads/master@{#414948}

[modify] https://crrev.com/fc341fe14e833479aebe4f1395ebbaf1b62d2957/chromeos/accelerometer/accelerometer_reader.cc
Project Member

Comment 14 by bugdroid1@chromium.org, Sep 6 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/62f47315fa2bae98943888334e2787ce308659e8

commit 62f47315fa2bae98943888334e2787ce308659e8
Author: fdoray <fdoray@chromium.org>
Date: Tue Sep 06 18:36:37 2016

Remove call to IsRunningSequenceOnCurrentThread() from api_resource_manager.h.

SequencedWorkerPool is being migrated to base/task_scheduler and there
is no equivalent to IsRunningSequenceOnCurrentThread() in
base/task_scheduler.

We believe that always doing work asynchronously is cleaner than using
this pattern:

if (pool->IsRunningSequencedOnCurrentThread(token))
 DoSequencedWork();
else
 pool->PostSequencedWorkerTask(token, FROM_HERE, Bind(&DoSequencedWork));

Developers should us SequenceChecker to verify that tasks run
sequentially. Unlike
SequencedWorkerPool::IsRunningSequenceOnCurrentThread(),
SequenceChecker works everywhere in Chrome (MessageLoop,
SequencedWorkerPool, base/task_scheduler...).

BUG= 622400 

Review-Url: https://codereview.chromium.org/2310593002
Cr-Commit-Position: refs/heads/master@{#416679}

[modify] https://crrev.com/62f47315fa2bae98943888334e2787ce308659e8/extensions/browser/api/api_resource_manager.h
Project Member

Comment 15 by bugdroid1@chromium.org, Sep 7 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ee7a59389604bc254617c71ef53aaf0eaf9c7237

commit ee7a59389604bc254617c71ef53aaf0eaf9c7237
Author: fdoray <fdoray@chromium.org>
Date: Wed Sep 07 18:03:08 2016

Remove calls to IsRunningSequenceOnCurrentThread() from memory_dump_manager_unittest.cc

SequencedWorkerPool is being migrated to base/task_scheduler and there
is no equivalent to IsRunningSequenceOnCurrentThread() in
base/task_scheduler.

BUG= 622400 

Review-Url: https://codereview.chromium.org/2320643002
Cr-Commit-Position: refs/heads/master@{#416994}

[modify] https://crrev.com/ee7a59389604bc254617c71ef53aaf0eaf9c7237/base/trace_event/memory_dump_manager_unittest.cc
Project Member

Comment 16 by bugdroid1@chromium.org, Sep 7 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cec55646e176f883295bb4be37599aa7c0cb0555

commit cec55646e176f883295bb4be37599aa7c0cb0555
Author: fdoray <fdoray@chromium.org>
Date: Wed Sep 07 22:14:59 2016

Remove call to IsRunningSequenceOnCurrentThread() in media_file_system_backend.cc

SequencedWorkerPool is being migrated to base/task_scheduler and there
is no equivalent to IsRunningSequenceOnCurrentThread() in
base/task_scheduler.

Developers should use SequenceChecker to verify that tasks run
sequentially. Unlike
SequencedWorkerPool::IsRunningSequenceOnCurrentThread(),
SequenceChecker works everywhere in Chrome (MessageLoop,
SequencedWorkerPool, base/task_scheduler...).

BUG= 622400 

Review-Url: https://codereview.chromium.org/2318133002
Cr-Commit-Position: refs/heads/master@{#417084}

[modify] https://crrev.com/cec55646e176f883295bb4be37599aa7c0cb0555/chrome/browser/media_galleries/fileapi/file_path_watcher_util.cc
[modify] https://crrev.com/cec55646e176f883295bb4be37599aa7c0cb0555/chrome/browser/media_galleries/fileapi/iapps_data_provider.cc
[modify] https://crrev.com/cec55646e176f883295bb4be37599aa7c0cb0555/chrome/browser/media_galleries/fileapi/itunes_data_provider.cc
[modify] https://crrev.com/cec55646e176f883295bb4be37599aa7c0cb0555/chrome/browser/media_galleries/fileapi/itunes_data_provider_browsertest.cc
[modify] https://crrev.com/cec55646e176f883295bb4be37599aa7c0cb0555/chrome/browser/media_galleries/fileapi/media_file_system_backend.cc
[modify] https://crrev.com/cec55646e176f883295bb4be37599aa7c0cb0555/chrome/browser/media_galleries/fileapi/media_file_system_backend.h
[modify] https://crrev.com/cec55646e176f883295bb4be37599aa7c0cb0555/chrome/browser/media_galleries/fileapi/picasa_data_provider.cc
[modify] https://crrev.com/cec55646e176f883295bb4be37599aa7c0cb0555/chrome/browser/media_galleries/fileapi/picasa_data_provider_browsertest.cc
[modify] https://crrev.com/cec55646e176f883295bb4be37599aa7c0cb0555/chrome/browser/media_galleries/fileapi/safe_iapps_library_parser.cc
[modify] https://crrev.com/cec55646e176f883295bb4be37599aa7c0cb0555/chrome/browser/media_galleries/fileapi/safe_picasa_album_table_reader.cc
[modify] https://crrev.com/cec55646e176f883295bb4be37599aa7c0cb0555/chrome/browser/media_galleries/fileapi/safe_picasa_albums_indexer.cc
[modify] https://crrev.com/cec55646e176f883295bb4be37599aa7c0cb0555/chrome/browser/media_galleries/imported_media_gallery_registry.cc
Project Member

Comment 17 by bugdroid1@chromium.org, Sep 7 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fbfbd92b1292d38c0ef2b3fa7e8ac7b4667ed0dc

commit fbfbd92b1292d38c0ef2b3fa7e8ac7b4667ed0dc
Author: fdoray <fdoray@chromium.org>
Date: Wed Sep 07 22:17:32 2016

Remove calls to IsRunningSequenceOnCurrentThread() from log_private_api_chromeos.cc

SequencedWorkerPool is being migrated to base/task_scheduler and there
is no equivalent to IsRunningSequenceOnCurrentThread() in
base/task_scheduler.

Developers should use SequenceChecker to verify that tasks run
sequentially. Unlike
SequencedWorkerPool::IsRunningSequenceOnCurrentThread(),
SequenceChecker works everywhere in Chrome (MessageLoop,
SequencedWorkerPool, base/task_scheduler...).

Also, we believe that always doing work asynchronously is cleaner
than doing it synchronously part of the time.

BUG= 622400 

Review-Url: https://codereview.chromium.org/2320703002
Cr-Commit-Position: refs/heads/master@{#417087}

[modify] https://crrev.com/fbfbd92b1292d38c0ef2b3fa7e8ac7b4667ed0dc/chrome/browser/extensions/api/log_private/log_private_api.h
[modify] https://crrev.com/fbfbd92b1292d38c0ef2b3fa7e8ac7b4667ed0dc/chrome/browser/extensions/api/log_private/log_private_api_chromeos.cc
Project Member

Comment 18 by bugdroid1@chromium.org, Sep 8 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5e1706c38519a9e467769ef549070dcc5c3cdfe2

commit 5e1706c38519a9e467769ef549070dcc5c3cdfe2
Author: fdoray <fdoray@chromium.org>
Date: Thu Sep 08 12:57:09 2016

Remove calls to IsRunningSequenceOnCurrentThread() from plugin_service_impl.cc

SequencedWorkerPool is being migrated to base/task_scheduler and there
is no equivalent to IsRunningSequenceOnCurrentThread() in
base/task_scheduler.

Developers should use SequenceChecker to verify that tasks run
sequentially. Unlike
SequencedWorkerPool::IsRunningSequenceOnCurrentThread(),
SequenceChecker works everywhere in Chrome (MessageLoop,
SequencedWorkerPool, base/task_scheduler...).

BUG= 622400 

Review-Url: https://codereview.chromium.org/2319623002
Cr-Commit-Position: refs/heads/master@{#417266}

[modify] https://crrev.com/5e1706c38519a9e467769ef549070dcc5c3cdfe2/chrome/browser/download/chrome_download_manager_delegate_unittest.cc
[modify] https://crrev.com/5e1706c38519a9e467769ef549070dcc5c3cdfe2/content/browser/plugin_service_impl.cc
[modify] https://crrev.com/5e1706c38519a9e467769ef549070dcc5c3cdfe2/content/browser/plugin_service_impl.h
Project Member

Comment 19 by bugdroid1@chromium.org, Sep 8 2016

Labels: merge-merged-2854
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5e1706c38519a9e467769ef549070dcc5c3cdfe2

commit 5e1706c38519a9e467769ef549070dcc5c3cdfe2
Author: fdoray <fdoray@chromium.org>
Date: Thu Sep 08 12:57:09 2016

Remove calls to IsRunningSequenceOnCurrentThread() from plugin_service_impl.cc

SequencedWorkerPool is being migrated to base/task_scheduler and there
is no equivalent to IsRunningSequenceOnCurrentThread() in
base/task_scheduler.

Developers should use SequenceChecker to verify that tasks run
sequentially. Unlike
SequencedWorkerPool::IsRunningSequenceOnCurrentThread(),
SequenceChecker works everywhere in Chrome (MessageLoop,
SequencedWorkerPool, base/task_scheduler...).

BUG= 622400 

Review-Url: https://codereview.chromium.org/2319623002
Cr-Commit-Position: refs/heads/master@{#417266}

[modify] https://crrev.com/5e1706c38519a9e467769ef549070dcc5c3cdfe2/chrome/browser/download/chrome_download_manager_delegate_unittest.cc
[modify] https://crrev.com/5e1706c38519a9e467769ef549070dcc5c3cdfe2/content/browser/plugin_service_impl.cc
[modify] https://crrev.com/5e1706c38519a9e467769ef549070dcc5c3cdfe2/content/browser/plugin_service_impl.h
Project Member

Comment 20 by bugdroid1@chromium.org, Sep 8 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1b269e7e22c457144521ff7a34d2599d963502b5

commit 1b269e7e22c457144521ff7a34d2599d963502b5
Author: fdoray <fdoray@chromium.org>
Date: Thu Sep 08 20:38:42 2016

Remove calls to IsRunningSequenceOnCurrentThread() from crash_handler_host_linux.cc

SequencedWorkerPool is being migrated to base/task_scheduler and there
is no equivalent to IsRunningSequenceOnCurrentThread() in
base/task_scheduler.

Developers should use SequenceChecker to verify that tasks run
sequentially. Unlike
SequencedWorkerPool::IsRunningSequenceOnCurrentThread(),
SequenceChecker works everywhere in Chrome (MessageLoop,
SequencedWorkerPool, base/task_scheduler...).

BUG= 622400 

Review-Url: https://codereview.chromium.org/2318123002
Cr-Commit-Position: refs/heads/master@{#417388}

[modify] https://crrev.com/1b269e7e22c457144521ff7a34d2599d963502b5/components/crash/content/browser/crash_handler_host_linux.cc
[modify] https://crrev.com/1b269e7e22c457144521ff7a34d2599d963502b5/components/crash/content/browser/crash_handler_host_linux.h

Comment 21 by gab@chromium.org, Sep 13 2016

Blocking: 623700
Project Member

Comment 22 by bugdroid1@chromium.org, Sep 14 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7cffceeef702204d07f906a905019f650c12e0ba

commit 7cffceeef702204d07f906a905019f650c12e0ba
Author: fdoray <fdoray@chromium.org>
Date: Wed Sep 14 14:20:46 2016

Remove calls to IsRunningSequenceOnCurrentThread() from dom_storage_task_runner.cc

SequencedWorkerPool is being migrated to base/task_scheduler and there
is no equivalent to IsRunningSequenceOnCurrentThread() in
base/task_scheduler.

Developers should use SequenceChecker to verify that tasks run
sequentially. Note: SequenceChecker::CalledOnValidSequence
is a no-op in non-DCHECK builds.

BUG= 622400 

Review-Url: https://codereview.chromium.org/2317253007
Cr-Commit-Position: refs/heads/master@{#418554}

[modify] https://crrev.com/7cffceeef702204d07f906a905019f650c12e0ba/content/browser/dom_storage/dom_storage_area.cc
[modify] https://crrev.com/7cffceeef702204d07f906a905019f650c12e0ba/content/browser/dom_storage/dom_storage_context_impl.cc
[modify] https://crrev.com/7cffceeef702204d07f906a905019f650c12e0ba/content/browser/dom_storage/dom_storage_namespace.cc
[modify] https://crrev.com/7cffceeef702204d07f906a905019f650c12e0ba/content/browser/dom_storage/dom_storage_task_runner.cc
[modify] https://crrev.com/7cffceeef702204d07f906a905019f650c12e0ba/content/browser/dom_storage/dom_storage_task_runner.h
Project Member

Comment 23 by bugdroid1@chromium.org, Sep 14 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d78593a22966b10cd96a9a46d86819d7f9b38d4b

commit d78593a22966b10cd96a9a46d86819d7f9b38d4b
Author: fdoray <fdoray@chromium.org>
Date: Wed Sep 14 14:43:27 2016

Remove calls to IsRunningSequenceOnCurrentThread() in wallpaper API.

SequencedWorkerPool is being migrated to base/task_scheduler and there
is no equivalent to IsRunningSequenceOnCurrentThread() in
base/task_scheduler.

Developers should use SequenceChecker to verify that tasks run
sequentially. Unlike
SequencedWorkerPool::IsRunningSequenceOnCurrentThread(),
SequenceChecker works everywhere in Chrome (MessageLoop,
SequencedWorkerPool, base/task_scheduler...).

BUG= 622400 

Review-Url: https://codereview.chromium.org/2315303002
Cr-Commit-Position: refs/heads/master@{#418560}

[modify] https://crrev.com/d78593a22966b10cd96a9a46d86819d7f9b38d4b/chrome/browser/chromeos/extensions/wallpaper_api.cc
[modify] https://crrev.com/d78593a22966b10cd96a9a46d86819d7f9b38d4b/chrome/browser/chromeos/extensions/wallpaper_api.h
[modify] https://crrev.com/d78593a22966b10cd96a9a46d86819d7f9b38d4b/chrome/browser/chromeos/extensions/wallpaper_private_api.cc
[modify] https://crrev.com/d78593a22966b10cd96a9a46d86819d7f9b38d4b/chrome/browser/chromeos/extensions/wallpaper_private_api.h
[modify] https://crrev.com/d78593a22966b10cd96a9a46d86819d7f9b38d4b/components/wallpaper/wallpaper_manager_base.cc
[modify] https://crrev.com/d78593a22966b10cd96a9a46d86819d7f9b38d4b/components/wallpaper/wallpaper_manager_base.h
Project Member

Comment 24 by bugdroid1@chromium.org, Sep 15 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5cea1b0cf4328831e583fa66f9fa30c8d075d677

commit 5cea1b0cf4328831e583fa66f9fa30c8d075d677
Author: fdoray <fdoray@chromium.org>
Date: Thu Sep 15 19:59:00 2016

Forward return value of PostTaskToTaskScheduler() to PostTask() in SequencedWorkerPool.

With this CL, SequencedWorkerPool::Inner::PostTask (and hence all task
posting APIs associated with SequencedWorkerPool) returns false when
redirection to TaskScheduler is enabled and the posted task definitely
will not run.

BUG= 622400 

Review-Url: https://codereview.chromium.org/2346823002
Cr-Commit-Position: refs/heads/master@{#418939}

[modify] https://crrev.com/5cea1b0cf4328831e583fa66f9fa30c8d075d677/base/threading/sequenced_worker_pool.cc

Comment 25 by robliao@chromium.org, Sep 15 2016

Blockedon: 647389
Project Member

Comment 26 by bugdroid1@chromium.org, Sep 15 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2c56f2de0b3d427b358872fd84535932cff39e86

commit 2c56f2de0b3d427b358872fd84535932cff39e86
Author: fdoray <fdoray@chromium.org>
Date: Thu Sep 15 21:31:19 2016

Make SequencedWorkerPool::IsRunningSequenceOnCurrentThread() private.

This method is not used outside of SequencedWorkerPool itself. Making it private
will prevent external users from using it directly, which will help the deprecation
of SequencedWorkerPool.

SequencedWorkerPool::IsRunningSequenceOnCurrentThread() is still used
by the RunsTasksOnCurrentThread() method of SequencedTaskRunners
returned by a SequencedWokrerPool.

BUG= 622400 

Review-Url: https://codereview.chromium.org/2341063002
Cr-Commit-Position: refs/heads/master@{#418984}

[modify] https://crrev.com/2c56f2de0b3d427b358872fd84535932cff39e86/base/threading/sequenced_task_runner_handle.cc
[modify] https://crrev.com/2c56f2de0b3d427b358872fd84535932cff39e86/base/threading/sequenced_worker_pool.cc
[modify] https://crrev.com/2c56f2de0b3d427b358872fd84535932cff39e86/base/threading/sequenced_worker_pool.h
[modify] https://crrev.com/2c56f2de0b3d427b358872fd84535932cff39e86/base/threading/sequenced_worker_pool_unittest.cc
Project Member

Comment 27 by bugdroid1@chromium.org, Sep 16 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c8f706ab3b7b9b0a10488e5144b843b2648c7c2f

commit c8f706ab3b7b9b0a10488e5144b843b2648c7c2f
Author: fdoray <fdoray@chromium.org>
Date: Fri Sep 16 13:04:32 2016

Remove outdated TODO in SequencedWorkerPool::Inner::IsRunningSequenceOnCurrentThread().

Now that SchedulerSequencedTaskRunner::RunsTasksOnCurrentThread()
returns true iff the current task is part of the
SchedulerSequencedTaskRunner's sequence, the TODO in
SequencedWorkerPool::Inner::IsRunningSequenceOnCurrentThread() does
not apply anymore.

BUG= 622400 

Review-Url: https://codereview.chromium.org/2343803004
Cr-Commit-Position: refs/heads/master@{#419152}

[modify] https://crrev.com/c8f706ab3b7b9b0a10488e5144b843b2648c7c2f/base/threading/sequenced_worker_pool.cc
Project Member

Comment 28 by bugdroid1@chromium.org, Sep 16 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/37e79479d334a1a18cdf5918255f1531bc11afd9

commit 37e79479d334a1a18cdf5918255f1531bc11afd9
Author: fdoray <fdoray@chromium.org>
Date: Fri Sep 16 23:19:50 2016

Test SequencedWorkerPool with redirection to the TaskScheduler.

This CL makes most SequencedWorkerPoolTest a TestWithParam. The parameter
controls whether redirection to TaskScheduler is enabled.

BUG= 622400 

Review-Url: https://codereview.chromium.org/2285633003
Cr-Commit-Position: refs/heads/master@{#419322}

[modify] https://crrev.com/37e79479d334a1a18cdf5918255f1531bc11afd9/base/threading/sequenced_worker_pool.cc
[modify] https://crrev.com/37e79479d334a1a18cdf5918255f1531bc11afd9/base/threading/sequenced_worker_pool.h
[modify] https://crrev.com/37e79479d334a1a18cdf5918255f1531bc11afd9/base/threading/sequenced_worker_pool_unittest.cc
[modify] https://crrev.com/37e79479d334a1a18cdf5918255f1531bc11afd9/chrome/browser/chrome_browser_main.cc
Project Member

Comment 29 by bugdroid1@chromium.org, Sep 22 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0881867ce08db7368ca9f6ea10aa230dc2849227

commit 0881867ce08db7368ca9f6ea10aa230dc2849227
Author: fdoray <fdoray@chromium.org>
Date: Thu Sep 22 16:22:28 2016

Do not flush SequencedWorkerPools in browser tests.

Before this CL, the blocking pool and the simple cache's pool were
flushed at the end of each browser test. That means that every task
posted to these pools ran, regardless of their shutdown behavior.
This flush was introduced by https://codereview.chromium.org/11649032
to prevent leaks between tests (see  https://crbug.com/168415 ).

This flush is not required for browser tests since they each run in
their own process.

Note that in production, BrowserThreads are joined before the
blocking pool. Before this CL, the blocking pool was flushed before
BrowserThreads were joined. With this CL, the behavior of browser
threads is closer to production.

Why is this important? There is no way to flush TaskScheduler and
we would prefer not to implement it if it is not required.

BUG= 622400 

Review-Url: https://codereview.chromium.org/2355093003
Cr-Commit-Position: refs/heads/master@{#420366}

[modify] https://crrev.com/0881867ce08db7368ca9f6ea10aa230dc2849227/content/public/test/content_test_suite_base.cc
[modify] https://crrev.com/0881867ce08db7368ca9f6ea10aa230dc2849227/ios/web/test/web_test_suite.mm
Project Member

Comment 30 by bugdroid1@chromium.org, Sep 26 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fe309b0183048023246ccbefaf0cf4df0f05e29b

commit fe309b0183048023246ccbefaf0cf4df0f05e29b
Author: fdoray <fdoray@chromium.org>
Date: Mon Sep 26 16:18:58 2016

TaskScheduler: Add FlushForTesting().

Many browser tests depend on being able to wait until there are no
pending undelayed tasks in the blocking pool (via
content::RunAllBlockingPoolTasksUntilIdle). This CL brings this
functionality to TaskScheduler.

BUG= 622400 ,  553459 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_tsan_rel_ng

Review-Url: https://codereview.chromium.org/2362253002
Cr-Commit-Position: refs/heads/master@{#420901}

[modify] https://crrev.com/fe309b0183048023246ccbefaf0cf4df0f05e29b/base/task_scheduler/scheduler_worker_unittest.cc
[modify] https://crrev.com/fe309b0183048023246ccbefaf0cf4df0f05e29b/base/task_scheduler/task_scheduler.h
[modify] https://crrev.com/fe309b0183048023246ccbefaf0cf4df0f05e29b/base/task_scheduler/task_scheduler_impl.cc
[modify] https://crrev.com/fe309b0183048023246ccbefaf0cf4df0f05e29b/base/task_scheduler/task_scheduler_impl.h
[modify] https://crrev.com/fe309b0183048023246ccbefaf0cf4df0f05e29b/base/task_scheduler/task_tracker.cc
[modify] https://crrev.com/fe309b0183048023246ccbefaf0cf4df0f05e29b/base/task_scheduler/task_tracker.h
[modify] https://crrev.com/fe309b0183048023246ccbefaf0cf4df0f05e29b/base/task_scheduler/task_tracker_unittest.cc
Project Member

Comment 32 by bugdroid1@chromium.org, Sep 30 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fdb23ec2e44b83f4fc8debbdb43ed5775cb5d92d

commit fdb23ec2e44b83f4fc8debbdb43ed5775cb5d92d
Author: fdoray <fdoray@chromium.org>
Date: Fri Sep 30 15:25:04 2016

Do not call SequencedWorkerPool::GetWorkerPoolForCurrentThread() from CollectEnvironmentData().

SequencedWorkerPool::GetWorkerPoolForCurrentThread() returns nullptr
when called from a SequencedWorkerPool task redirected to the
TaskScheduler.

Note: Code archeology reveals that a reviewer asked to use
SequencedWorkerPool::GetWorkerPoolForCurrentThread() instead of
content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread()
to avoid a dependance on BrowserThread. I think it's better to
temporarily introduce a dependance on BrowserThread with this CL
than to make SequencedWorkerPool::GetWorkerPoolForCurrentThread()
work from SequencedWorkerPool tasks redirected to TaskScheduler
just for this call site.
https://codereview.chromium.org/1505813003/diff/20001/chrome/browser/safe_browsing/incident_reporting/environment_data_collection.cc#newcode68

BUG= 622400 

Review-Url: https://codereview.chromium.org/2379913004
Cr-Commit-Position: refs/heads/master@{#422118}

[modify] https://crrev.com/fdb23ec2e44b83f4fc8debbdb43ed5775cb5d92d/chrome/browser/safe_browsing/incident_reporting/environment_data_collection.cc

Comment 33 by gab@chromium.org, Sep 30 2016 

@fdoray/#32: Sounds like we should hide GetWorkerPoolForCurrentThread() from the public API? It's now only required by STRH. Maybe it should work the other way around? i.e. introduce STRH::RegisterSTRHThread() and have SWP threads register with STRH?

Comment 34 by fdoray@chromium.org, Sep 30 2016 

I'm against introducing STRH::RegisterSTRHThread() since it would only be used by SWP which we plan to deprecate very soon.

Potential solutions:
a. Make GetWorkerPoolForCurrentThread() private and make STRH a friend of SWP.
b. Add a DEPRECATED comment above the description of GetWorkerPoolForCurrentThread.
c. Instantiate a STRH from SWP before running a task (caveat: we would have to instantiate a SequencedTaskRunner whenever we run a task). We could remove the STRH code that is specific to SWP.

Comment 35 by gab@chromium.org, Sep 30 2016 

Let's do b.
Project Member

Comment 36 by bugdroid1@chromium.org, Sep 30 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3be8af75b2f307116aabebc05dd92b5169145f58

commit 3be8af75b2f307116aabebc05dd92b5169145f58
Author: fdoray <fdoray@chromium.org>
Date: Fri Sep 30 22:15:05 2016

Mark SequencedWorkerPool::GetWorkerPoolForCurrentThread() as deprecated.

This methods does not work when SequencedWorkerPool is redirected to
TaskScheduler (always returns nullptr).

The only remaining use is in base/threading/sequenced_task_runner_handle.cc
(which uses another way to get the current SequencedTaskRunner when
redirection to TaskScheduler is enabled).

BUG= 622400 

Review-Url: https://codereview.chromium.org/2383203002
Cr-Commit-Position: refs/heads/master@{#422238}

[modify] https://crrev.com/3be8af75b2f307116aabebc05dd92b5169145f58/base/threading/sequenced_worker_pool.h
Project Member

Comment 37 by bugdroid1@chromium.org, Oct 4 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/387159105445bc8153931a02df261059b95b6a97

commit 387159105445bc8153931a02df261059b95b6a97
Author: fdoray <fdoray@chromium.org>
Date: Tue Oct 04 13:53:39 2016

Allow BLOCK_SHUTDOWN tasks during shutdown with SequencedWorkerPool redirected to TaskScheduler.

Before this CL, any task posted to a SequencedWorkerPool redirected to
TaskScheduler after
SequencedWorkerPool::Shutdown(int max_new_blocking_tasks_after_shutdown)
returned was ignored.

With this CL, |max_new_blocking_tasks_after_shutdown| BLOCK_SHUTDOWN
tasks are allowed to be posted after SequencedWorkerPool::Shutdown
returns.

BUG= 622400 

Review-Url: https://codereview.chromium.org/2389923004
Cr-Commit-Position: refs/heads/master@{#422775}

[modify] https://crrev.com/387159105445bc8153931a02df261059b95b6a97/base/threading/sequenced_worker_pool.cc
[modify] https://crrev.com/387159105445bc8153931a02df261059b95b6a97/base/threading/sequenced_worker_pool_unittest.cc

Comment 38 by gab@chromium.org, Oct 24 2016

Blockedon: 611818

Comment 39 by robliao@chromium.org, Oct 24 2016

Blockedon: -647389
Project Member

Comment 40 by bugdroid1@chromium.org, Oct 25 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9b3162c323b6529cbba17fb419129fea26b75257

commit 9b3162c323b6529cbba17fb419129fea26b75257
Author: fdoray <fdoray@chromium.org>
Date: Tue Oct 25 16:24:51 2016

Enable SequencedWorkerPool to TaskScheduler redirection in testing config.

With this CL, redirection of SequencedWorkerPools to TaskScheduler will
be tested on bots.

Before a similar CL https://codereview.chromium.org/2353973002/ landed,
SequencedWorkerPool tasks didn't generate tracing events. With
redirection to TaskScheduler enabled, the same tasks started to generate
tracing events. That affected the battor.power_cases/cpu_time_percentage_max
graph https://crbug.com/656629 and the CL was reverted. Now that
SequencedWorkerPool tasks generate tracing events
https://codereview.chromium.org/2429863002/, enabling redirection
to TaskScheduler shouldn't affect any performance graph. Note that the
battor.power_cases/cpu_time_percentage_max graph didn't go up when tracing
was enabled in SequencedWorkerPools because of
https://codereview.chromium.org/2420133002/ which reduced the CPU
consumption of a task running in the blocking pool.

BUG= 622400 

Review-Url: https://codereview.chromium.org/2446603002
Cr-Commit-Position: refs/heads/master@{#427375}

[modify] https://crrev.com/9b3162c323b6529cbba17fb419129fea26b75257/testing/variations/fieldtrial_testing_config.json
Project Member

Comment 41 by bugdroid1@chromium.org, Oct 26 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/94e031354f4fbe6b39dbe23072450986e0e4f022

commit 94e031354f4fbe6b39dbe23072450986e0e4f022
Author: gab <gab@chromium.org>
Date: Wed Oct 26 18:18:16 2016

[TaskScheduler] Temporarily co-init all TaskScheduler threads to match SequencedWorkerPool's behaviour.

BUG= 622400 

Review-Url: https://codereview.chromium.org/2435013006
Cr-Commit-Position: refs/heads/master@{#427748}

[modify] https://crrev.com/94e031354f4fbe6b39dbe23072450986e0e4f022/base/task_scheduler/scheduler_worker.cc

Comment 42 by gab@chromium.org, Nov 7 2016

Components: Internals>TaskScheduler
Project Member

Comment 43 by bugdroid1@chromium.org, Nov 8 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5a6dfbbcae66fe80acefd8e1f0d812cdc4551bb6

commit 5a6dfbbcae66fe80acefd8e1f0d812cdc4551bb6
Author: fdoray <fdoray@chromium.org>
Date: Tue Nov 08 15:27:41 2016

Revert of Enable SequencedWorkerPool to TaskScheduler redirection in testing config. (patchset #2 id:20001 of https://codereview.chromium.org/2446603002/ )

Reason for revert:
This caused a 1.1% regression in system_health.memory_mobile. https://crbug.com/661520

Original issue's description:
> Enable SequencedWorkerPool to TaskScheduler redirection in testing config.
>
> With this CL, redirection of SequencedWorkerPools to TaskScheduler will
> be tested on bots.
>
> Before a similar CL https://codereview.chromium.org/2353973002/ landed,
> SequencedWorkerPool tasks didn't generate tracing events. With
> redirection to TaskScheduler enabled, the same tasks started to generate
> tracing events. That affected the battor.power_cases/cpu_time_percentage_max
> graph https://crbug.com/656629 and the CL was reverted. Now that
> SequencedWorkerPool tasks generate tracing events
> https://codereview.chromium.org/2429863002/, enabling redirection
> to TaskScheduler shouldn't affect any performance graph. Note that the
> battor.power_cases/cpu_time_percentage_max graph didn't go up when tracing
> was enabled in SequencedWorkerPools because of
> https://codereview.chromium.org/2420133002/ which reduced the CPU
> consumption of a task running in the blocking pool.
>
> BUG= 622400 
>
> Committed: https://crrev.com/9b3162c323b6529cbba17fb419129fea26b75257
> Cr-Commit-Position: refs/heads/master@{#427375}

TBR=gab@chromium.org,rkaplow@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 622400 , 661520

Review-Url: https://codereview.chromium.org/2488513003
Cr-Commit-Position: refs/heads/master@{#430616}

[modify] https://crrev.com/5a6dfbbcae66fe80acefd8e1f0d812cdc4551bb6/testing/variations/fieldtrial_testing_config.json
Project Member

Comment 44 by bugdroid1@chromium.org, Nov 17 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4606ada249a240e115a08a7c66409e8fdd457ea1

commit 4606ada249a240e115a08a7c66409e8fdd457ea1
Author: fdoray <fdoray@chromium.org>
Date: Thu Nov 17 18:07:28 2016

Disallow posting tasks to SequencedWorkerPools by default.

We have observed crash reports in which SequencedWorkerPool workers
were active while redirection to TaskScheduler was enabled. This can
happen if a task is posted to a SequencedWorkerPool before redirection
to TaskScheduler is enabled. With this CL, DumpWithoutCrashing is
called if a task is posted to a SequencedWorkerPool too early. This
will help us find offending call sites.

BUG= 622400 

Review-Url: https://codereview.chromium.org/2445763002
Cr-Commit-Position: refs/heads/master@{#432913}

[modify] https://crrev.com/4606ada249a240e115a08a7c66409e8fdd457ea1/base/test/launcher/test_launcher.cc
[modify] https://crrev.com/4606ada249a240e115a08a7c66409e8fdd457ea1/base/test/test_suite.cc
[modify] https://crrev.com/4606ada249a240e115a08a7c66409e8fdd457ea1/base/threading/sequenced_worker_pool.cc
[modify] https://crrev.com/4606ada249a240e115a08a7c66409e8fdd457ea1/base/threading/sequenced_worker_pool.h
[modify] https://crrev.com/4606ada249a240e115a08a7c66409e8fdd457ea1/base/threading/sequenced_worker_pool_unittest.cc
[modify] https://crrev.com/4606ada249a240e115a08a7c66409e8fdd457ea1/chrome/browser/chrome_browser_main.cc
[modify] https://crrev.com/4606ada249a240e115a08a7c66409e8fdd457ea1/chrome/service/service_process.cc
[modify] https://crrev.com/4606ada249a240e115a08a7c66409e8fdd457ea1/content/browser/browser_main_loop.cc
[modify] https://crrev.com/4606ada249a240e115a08a7c66409e8fdd457ea1/content/public/test/browser_test_base.cc
[modify] https://crrev.com/4606ada249a240e115a08a7c66409e8fdd457ea1/content/renderer/render_thread_impl.cc
[modify] https://crrev.com/4606ada249a240e115a08a7c66409e8fdd457ea1/content/renderer/render_thread_impl_browsertest.cc
Project Member

Comment 45 by bugdroid1@chromium.org, Nov 17 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8f211d4a78b61ab2b70995c4a0a5d6ae2e24d62e

commit 8f211d4a78b61ab2b70995c4a0a5d6ae2e24d62e
Author: estade <estade@chromium.org>
Date: Thu Nov 17 22:14:25 2016

Revert of Disallow posting tasks to SequencedWorkerPools by default. (patchset #24 id:460001 of https://codereview.chromium.org/2445763002/ )

Reason for revert:
Probable cause of test failures: "cronet_test_instrumentation_apk" on "Android Cronet Builder (dbg)"

https://build.chromium.org/p/chromium.android/builders/Android%20Cronet%20Builder%20%28dbg%29/builds/3581

Original issue's description:
> Disallow posting tasks to SequencedWorkerPools by default.
>
> We have observed crash reports in which SequencedWorkerPool workers
> were active while redirection to TaskScheduler was enabled. This can
> happen if a task is posted to a SequencedWorkerPool before redirection
> to TaskScheduler is enabled. With this CL, DumpWithoutCrashing is
> called if a task is posted to a SequencedWorkerPool too early. This
> will help us find offending call sites.
>
> BUG= 622400 
>
> Committed: https://crrev.com/4606ada249a240e115a08a7c66409e8fdd457ea1
> Cr-Commit-Position: refs/heads/master@{#432913}

TBR=gab@chromium.org,brettw@chromium.org,fdoray@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 622400 

Review-Url: https://codereview.chromium.org/2517443002
Cr-Commit-Position: refs/heads/master@{#432989}

[modify] https://crrev.com/8f211d4a78b61ab2b70995c4a0a5d6ae2e24d62e/base/test/launcher/test_launcher.cc
[modify] https://crrev.com/8f211d4a78b61ab2b70995c4a0a5d6ae2e24d62e/base/test/test_suite.cc
[modify] https://crrev.com/8f211d4a78b61ab2b70995c4a0a5d6ae2e24d62e/base/threading/sequenced_worker_pool.cc
[modify] https://crrev.com/8f211d4a78b61ab2b70995c4a0a5d6ae2e24d62e/base/threading/sequenced_worker_pool.h
[modify] https://crrev.com/8f211d4a78b61ab2b70995c4a0a5d6ae2e24d62e/base/threading/sequenced_worker_pool_unittest.cc
[modify] https://crrev.com/8f211d4a78b61ab2b70995c4a0a5d6ae2e24d62e/chrome/browser/chrome_browser_main.cc
[modify] https://crrev.com/8f211d4a78b61ab2b70995c4a0a5d6ae2e24d62e/chrome/service/service_process.cc
[modify] https://crrev.com/8f211d4a78b61ab2b70995c4a0a5d6ae2e24d62e/content/browser/browser_main_loop.cc
[modify] https://crrev.com/8f211d4a78b61ab2b70995c4a0a5d6ae2e24d62e/content/public/test/browser_test_base.cc
[modify] https://crrev.com/8f211d4a78b61ab2b70995c4a0a5d6ae2e24d62e/content/renderer/render_thread_impl.cc
[modify] https://crrev.com/8f211d4a78b61ab2b70995c4a0a5d6ae2e24d62e/content/renderer/render_thread_impl_browsertest.cc
Project Member

Comment 46 by bugdroid1@chromium.org, Nov 18 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8fd22217395d50b6704109c7f1d4deff59f9a109

commit 8fd22217395d50b6704109c7f1d4deff59f9a109
Author: fdoray <fdoray@chromium.org>
Date: Fri Nov 18 18:13:26 2016

Disallow posting tasks to SequencedWorkerPools by default.

We have observed crash reports in which SequencedWorkerPool workers
were active while redirection to TaskScheduler was enabled. This can
happen if a task is posted to a SequencedWorkerPool before redirection
to TaskScheduler is enabled. With this CL, DumpWithoutCrashing() is
called if a task is posted to a SequencedWorkerPool too early. This
will help us find offending call sites.

A commented DCHECK is also added to find offending call sites. It
will be uncommented in a separate CL to avoid a revert of this CL
in case it fails on the waterfall.

BUG= 622400 

Committed: https://crrev.com/4606ada249a240e115a08a7c66409e8fdd457ea1
Review-Url: https://codereview.chromium.org/2445763002
Cr-Original-Commit-Position: refs/heads/master@{#432913}
Cr-Commit-Position: refs/heads/master@{#433237}

[modify] https://crrev.com/8fd22217395d50b6704109c7f1d4deff59f9a109/base/test/launcher/test_launcher.cc
[modify] https://crrev.com/8fd22217395d50b6704109c7f1d4deff59f9a109/base/test/test_suite.cc
[modify] https://crrev.com/8fd22217395d50b6704109c7f1d4deff59f9a109/base/threading/sequenced_worker_pool.cc
[modify] https://crrev.com/8fd22217395d50b6704109c7f1d4deff59f9a109/base/threading/sequenced_worker_pool.h
[modify] https://crrev.com/8fd22217395d50b6704109c7f1d4deff59f9a109/base/threading/sequenced_worker_pool_unittest.cc
[modify] https://crrev.com/8fd22217395d50b6704109c7f1d4deff59f9a109/chrome/browser/chrome_browser_main.cc
[modify] https://crrev.com/8fd22217395d50b6704109c7f1d4deff59f9a109/chrome/service/service_process.cc
[modify] https://crrev.com/8fd22217395d50b6704109c7f1d4deff59f9a109/components/task_scheduler_util/initialization_util.cc
[modify] https://crrev.com/8fd22217395d50b6704109c7f1d4deff59f9a109/content/browser/browser_main_loop.cc
[modify] https://crrev.com/8fd22217395d50b6704109c7f1d4deff59f9a109/content/public/test/browser_test_base.cc
[modify] https://crrev.com/8fd22217395d50b6704109c7f1d4deff59f9a109/content/renderer/render_thread_impl.cc
[modify] https://crrev.com/8fd22217395d50b6704109c7f1d4deff59f9a109/content/renderer/render_thread_impl_browsertest.cc
Project Member

Comment 47 by bugdroid1@chromium.org, Nov 18 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/47c895a15251dde93c04721480cc9f1172d52cca

commit 47c895a15251dde93c04721480cc9f1172d52cca
Author: fsamuel <fsamuel@chromium.org>
Date: Fri Nov 18 19:37:05 2016

Revert of Disallow posting tasks to SequencedWorkerPools by default. (patchset #25 id:480001 of https://codereview.chromium.org/2445763002/ )

Reason for revert:
chrome --mash crashing.

Original issue's description:
> Disallow posting tasks to SequencedWorkerPools by default.
>
> We have observed crash reports in which SequencedWorkerPool workers
> were active while redirection to TaskScheduler was enabled. This can
> happen if a task is posted to a SequencedWorkerPool before redirection
> to TaskScheduler is enabled. With this CL, DumpWithoutCrashing() is
> called if a task is posted to a SequencedWorkerPool too early. This
> will help us find offending call sites.
>
> A commented DCHECK is also added to find offending call sites. It
> will be uncommented in a separate CL to avoid a revert of this CL
> in case it fails on the waterfall.
>
> BUG= 622400 
>
> Committed: https://crrev.com/4606ada249a240e115a08a7c66409e8fdd457ea1
> Committed: https://crrev.com/8fd22217395d50b6704109c7f1d4deff59f9a109
> Cr-Original-Commit-Position: refs/heads/master@{#432913}
> Cr-Commit-Position: refs/heads/master@{#433237}

TBR=gab@chromium.org,brettw@chromium.org,fdoray@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 622400 

Review-Url: https://codereview.chromium.org/2519543002
Cr-Commit-Position: refs/heads/master@{#433267}

[modify] https://crrev.com/47c895a15251dde93c04721480cc9f1172d52cca/base/test/launcher/test_launcher.cc
[modify] https://crrev.com/47c895a15251dde93c04721480cc9f1172d52cca/base/test/test_suite.cc
[modify] https://crrev.com/47c895a15251dde93c04721480cc9f1172d52cca/base/threading/sequenced_worker_pool.cc
[modify] https://crrev.com/47c895a15251dde93c04721480cc9f1172d52cca/base/threading/sequenced_worker_pool.h
[modify] https://crrev.com/47c895a15251dde93c04721480cc9f1172d52cca/base/threading/sequenced_worker_pool_unittest.cc
[modify] https://crrev.com/47c895a15251dde93c04721480cc9f1172d52cca/chrome/browser/chrome_browser_main.cc
[modify] https://crrev.com/47c895a15251dde93c04721480cc9f1172d52cca/chrome/service/service_process.cc
[modify] https://crrev.com/47c895a15251dde93c04721480cc9f1172d52cca/components/task_scheduler_util/initialization_util.cc
[modify] https://crrev.com/47c895a15251dde93c04721480cc9f1172d52cca/content/browser/browser_main_loop.cc
[modify] https://crrev.com/47c895a15251dde93c04721480cc9f1172d52cca/content/public/test/browser_test_base.cc
[modify] https://crrev.com/47c895a15251dde93c04721480cc9f1172d52cca/content/renderer/render_thread_impl.cc
[modify] https://crrev.com/47c895a15251dde93c04721480cc9f1172d52cca/content/renderer/render_thread_impl_browsertest.cc
Project Member

Comment 48 by bugdroid1@chromium.org, Nov 18 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/47c895a15251dde93c04721480cc9f1172d52cca

commit 47c895a15251dde93c04721480cc9f1172d52cca
Author: fsamuel <fsamuel@chromium.org>
Date: Fri Nov 18 19:37:05 2016

Revert of Disallow posting tasks to SequencedWorkerPools by default. (patchset #25 id:480001 of https://codereview.chromium.org/2445763002/ )

Reason for revert:
chrome --mash crashing.

Original issue's description:
> Disallow posting tasks to SequencedWorkerPools by default.
>
> We have observed crash reports in which SequencedWorkerPool workers
> were active while redirection to TaskScheduler was enabled. This can
> happen if a task is posted to a SequencedWorkerPool before redirection
> to TaskScheduler is enabled. With this CL, DumpWithoutCrashing() is
> called if a task is posted to a SequencedWorkerPool too early. This
> will help us find offending call sites.
>
> A commented DCHECK is also added to find offending call sites. It
> will be uncommented in a separate CL to avoid a revert of this CL
> in case it fails on the waterfall.
>
> BUG= 622400 
>
> Committed: https://crrev.com/4606ada249a240e115a08a7c66409e8fdd457ea1
> Committed: https://crrev.com/8fd22217395d50b6704109c7f1d4deff59f9a109
> Cr-Original-Commit-Position: refs/heads/master@{#432913}
> Cr-Commit-Position: refs/heads/master@{#433237}

TBR=gab@chromium.org,brettw@chromium.org,fdoray@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 622400 

Review-Url: https://codereview.chromium.org/2519543002
Cr-Commit-Position: refs/heads/master@{#433267}

[modify] https://crrev.com/47c895a15251dde93c04721480cc9f1172d52cca/base/test/launcher/test_launcher.cc
[modify] https://crrev.com/47c895a15251dde93c04721480cc9f1172d52cca/base/test/test_suite.cc
[modify] https://crrev.com/47c895a15251dde93c04721480cc9f1172d52cca/base/threading/sequenced_worker_pool.cc
[modify] https://crrev.com/47c895a15251dde93c04721480cc9f1172d52cca/base/threading/sequenced_worker_pool.h
[modify] https://crrev.com/47c895a15251dde93c04721480cc9f1172d52cca/base/threading/sequenced_worker_pool_unittest.cc
[modify] https://crrev.com/47c895a15251dde93c04721480cc9f1172d52cca/chrome/browser/chrome_browser_main.cc
[modify] https://crrev.com/47c895a15251dde93c04721480cc9f1172d52cca/chrome/service/service_process.cc
[modify] https://crrev.com/47c895a15251dde93c04721480cc9f1172d52cca/components/task_scheduler_util/initialization_util.cc
[modify] https://crrev.com/47c895a15251dde93c04721480cc9f1172d52cca/content/browser/browser_main_loop.cc
[modify] https://crrev.com/47c895a15251dde93c04721480cc9f1172d52cca/content/public/test/browser_test_base.cc
[modify] https://crrev.com/47c895a15251dde93c04721480cc9f1172d52cca/content/renderer/render_thread_impl.cc
[modify] https://crrev.com/47c895a15251dde93c04721480cc9f1172d52cca/content/renderer/render_thread_impl_browsertest.cc
Project Member

Comment 49 by bugdroid1@chromium.org, Nov 21 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/50a3834c24c1e4a4a61130db156405240b04b346

commit 50a3834c24c1e4a4a61130db156405240b04b346
Author: fdoray <fdoray@chromium.org>
Date: Mon Nov 21 20:46:04 2016

Disallow posting tasks to SequencedWorkerPools by default.

We have observed crash reports in which SequencedWorkerPool workers
were active while redirection to TaskScheduler was enabled. This can
happen if a task is posted to a SequencedWorkerPool before redirection
to TaskScheduler is enabled. With this CL, DumpWithoutCrashing() is
called if a task is posted to a SequencedWorkerPool too early. This
will help us find offending call sites.

A commented DCHECK is also added to find offending call sites. It
will be uncommented in a separate CL to avoid a revert of this CL
in case it fails on the waterfall.

BUG= 622400 

Committed: https://crrev.com/4606ada249a240e115a08a7c66409e8fdd457ea1
Committed: https://crrev.com/8fd22217395d50b6704109c7f1d4deff59f9a109
Review-Url: https://codereview.chromium.org/2445763002
Cr-Original-Original-Commit-Position: refs/heads/master@{#432913}
Cr-Original-Commit-Position: refs/heads/master@{#433237}
Cr-Commit-Position: refs/heads/master@{#433639}

[modify] https://crrev.com/50a3834c24c1e4a4a61130db156405240b04b346/base/test/launcher/test_launcher.cc
[modify] https://crrev.com/50a3834c24c1e4a4a61130db156405240b04b346/base/test/test_suite.cc
[modify] https://crrev.com/50a3834c24c1e4a4a61130db156405240b04b346/base/threading/sequenced_worker_pool.cc
[modify] https://crrev.com/50a3834c24c1e4a4a61130db156405240b04b346/base/threading/sequenced_worker_pool.h
[modify] https://crrev.com/50a3834c24c1e4a4a61130db156405240b04b346/base/threading/sequenced_worker_pool_unittest.cc
[modify] https://crrev.com/50a3834c24c1e4a4a61130db156405240b04b346/chrome/service/service_process.cc
[modify] https://crrev.com/50a3834c24c1e4a4a61130db156405240b04b346/components/task_scheduler_util/initialization_util.cc
[modify] https://crrev.com/50a3834c24c1e4a4a61130db156405240b04b346/content/browser/browser_main_loop.cc
[modify] https://crrev.com/50a3834c24c1e4a4a61130db156405240b04b346/content/public/test/browser_test_base.cc
[modify] https://crrev.com/50a3834c24c1e4a4a61130db156405240b04b346/content/renderer/render_thread_impl.cc
[modify] https://crrev.com/50a3834c24c1e4a4a61130db156405240b04b346/content/renderer/render_thread_impl_browsertest.cc

Comment 50 by fdoray@chromium.org, Nov 22 2016

Blockedon: 667892

Comment 51 by gab@chromium.org, Nov 22 2016

Blocking: 667892

Comment 52 by gab@chromium.org, Nov 22 2016

Blockedon: -667892

Comment 53 by robliao@chromium.org, Dec 1 2016

Blockedon: 670137
Project Member

Comment 55 by bugdroid1@chromium.org, Dec 19 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/28fb80beda6b171647d037057543f77e5b78e170

commit 28fb80beda6b171647d037057543f77e5b78e170
Author: gab <gab@chromium.org>
Date: Mon Dec 19 20:09:28 2016

Force HostContentSettingsMap to be destroyed on its owning thread.

It uses WeakPtrFactory and as such must be destroyed on its owning thread (UI).

The rest of the changes can then be inferred from the stack trace on the bug:

1) RefcountedKeyedServiceTraits::Destruct() must check for
   ThreadTaskRunnerHandle::IsSet() before invoking TTRH::Get().
  ** UPDATE in PS 5 : Use STR::RunsTasksOnCurrentThread() instead of STRH comparison.

2) Nothing about RefcountedKeyedService's use of its |task_runner| is thread-affine
   => using SequencedTaskRunner instead is inline with our desire to make all
   top-level API use sequence-affinity instead of thread-affinity for thread-safety.

3) Calling SequencedTaskRunnerHandle::IsSet() from the Callback's destructor requires
   the SequencedWorkerPool's task info to be set in that scope
   => brought changes from same requirements in another CL
      @ https://codereview.chromium.org/2491613004/#ps160001
   Note: intentionally not adding tests as this change highlights that this deletion is
   racy in theory (deleting without owning the sequence means potentially deleting
   things from the same SequenceToken in parallel... any good test would fail TSAN and
   I also don't want to address this issue given it's long standing and SWP is being
   replaced by TaskScheduler shortly).

4) Switch to ScopedMockTimeMessageLoopTaskRunner in NotificationPermissionContextTest
   => otherwise LSAN catches a leak in patch set 3 as the HostContentSettingsMap is
      sent for deletion on the wrong task runner and the task is never flushed.
      (ScopedMockTimeMessageLoopTaskRunner ensures the loop is flushed before replacing
       it, both ways).

Precursor requirement to https://codereview.chromium.org/2576243003/ which
for some reason makes this destruction race come to life.

BUG= 674946 ,  665588 ,  622400 
TBR=mukai (NotificationPermissionContextTest side-effects)

Review-Url: https://codereview.chromium.org/2581213002
Cr-Commit-Position: refs/heads/master@{#439534}

[modify] https://crrev.com/28fb80beda6b171647d037057543f77e5b78e170/base/threading/sequenced_worker_pool.cc
[modify] https://crrev.com/28fb80beda6b171647d037057543f77e5b78e170/chrome/browser/notifications/notification_permission_context_unittest.cc
[modify] https://crrev.com/28fb80beda6b171647d037057543f77e5b78e170/components/content_settings/core/browser/cookie_settings_unittest.cc
[modify] https://crrev.com/28fb80beda6b171647d037057543f77e5b78e170/components/content_settings/core/browser/host_content_settings_map.cc
[modify] https://crrev.com/28fb80beda6b171647d037057543f77e5b78e170/components/keyed_service/core/refcounted_keyed_service.cc
[modify] https://crrev.com/28fb80beda6b171647d037057543f77e5b78e170/components/keyed_service/core/refcounted_keyed_service.h
Project Member

Comment 56 by bugdroid1@chromium.org, Dec 19 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/72b1a059ae930ebe1dda804e607f251d3ae94a70

commit 72b1a059ae930ebe1dda804e607f251d3ae94a70
Author: gab <gab@chromium.org>
Date: Mon Dec 19 21:40:50 2016

Add an experiment to redirect DOMStorageTaskRunner to TaskScheduler.

Using a bug as an opportunity to experiment with TaskScheduler (as well
as confirm the issue is what we think it is: a priority inversion where
a sync IPC from the renderer is blocking on a response from the
BlockingPool).

Only logical difference is that TaskScheduler doesn't have a notion of
named sequences and as such every DOMStorageContextWrapper will have its
own primary and commit sequence instead of sharing one with other
DOMStorageContextWrapper. If anything that's a good thing as it allows
multiple profiles to do their DOMStorage ops in parallel? Don't think
this can conflict..?

BUG= 665588 ,  622400 

Review-Url: https://codereview.chromium.org/2576243003
Cr-Commit-Position: refs/heads/master@{#439575}

[modify] https://crrev.com/72b1a059ae930ebe1dda804e607f251d3ae94a70/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/72b1a059ae930ebe1dda804e607f251d3ae94a70/chrome/browser/chrome_content_browser_client.h
[modify] https://crrev.com/72b1a059ae930ebe1dda804e607f251d3ae94a70/content/browser/dom_storage/dom_storage_context_impl.cc
[modify] https://crrev.com/72b1a059ae930ebe1dda804e607f251d3ae94a70/content/browser/dom_storage/dom_storage_context_impl.h
[modify] https://crrev.com/72b1a059ae930ebe1dda804e607f251d3ae94a70/content/browser/dom_storage/dom_storage_context_wrapper.cc
[modify] https://crrev.com/72b1a059ae930ebe1dda804e607f251d3ae94a70/content/browser/dom_storage/dom_storage_task_runner.cc
[modify] https://crrev.com/72b1a059ae930ebe1dda804e607f251d3ae94a70/content/browser/dom_storage/dom_storage_task_runner.h
[modify] https://crrev.com/72b1a059ae930ebe1dda804e607f251d3ae94a70/content/public/browser/content_browser_client.cc
[modify] https://crrev.com/72b1a059ae930ebe1dda804e607f251d3ae94a70/content/public/browser/content_browser_client.h
Project Member

Comment 57 by bugdroid1@chromium.org, Dec 22 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/53a4e4af0525c0112bc6b3d03c9f4d2534212830

commit 53a4e4af0525c0112bc6b3d03c9f4d2534212830
Author: gab <gab@chromium.org>
Date: Thu Dec 22 16:03:34 2016

Enable BrowserScheduler.RedirectSequencedWorkerPools experiment on trunk.

It previously caused a memory regression which might since have been
addressed, trying again...

BUG= 622400 , 661520,  663887 

Review-Url: https://codereview.chromium.org/2597493002
Cr-Commit-Position: refs/heads/master@{#440433}

[modify] https://crrev.com/53a4e4af0525c0112bc6b3d03c9f4d2534212830/testing/variations/fieldtrial_testing_config.json

Comment 58 by gab@chromium.org, Jan 18 2017

Blockedon: 682057

Comment 59 by scottmg@chromium.org, Feb 23 2017

Blocking: 694688
Project Member

Comment 60 by bugdroid1@chromium.org, May 2 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/aaf8a0f0061066008bd43ab77173dc6729d167f2

commit aaf8a0f0061066008bd43ab77173dc6729d167f2
Author: gab <gab@chromium.org>
Date: Tue May 02 22:31:32 2017

Tweak client-side default scheduler configs to match latest server configs (https://goto.google.com/jhzim).

Also, in fieldtrial_testing_config.json:
 - avoid using "Default" as a name for a group that does something
 - add an explanation of why we don't also specify pool init params there
   with reference to client-side code that does this already.

BUG= 622400 

Review-Url: https://codereview.chromium.org/2855673002
Cr-Commit-Position: refs/heads/master@{#468798}

[modify] https://crrev.com/aaf8a0f0061066008bd43ab77173dc6729d167f2/content/browser/browser_main_loop.cc
[modify] https://crrev.com/aaf8a0f0061066008bd43ab77173dc6729d167f2/ios/web/app/web_main_loop.mm
[modify] https://crrev.com/aaf8a0f0061066008bd43ab77173dc6729d167f2/testing/variations/fieldtrial_testing_config.json
Project Member

Comment 61 by bugdroid1@chromium.org, Jun 16 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b58c5ae28893d0a4b897380b106496431bb2c59d

commit b58c5ae28893d0a4b897380b106496431bb2c59d
Author: Gabriel Charette <gab@chromium.org>
Date: Fri Jun 16 16:00:06 2017

Make SequencedWorkerPool->TaskScheduler always on (no more experiment)

This was already the case through config, just cleaning up experimental
code.

Bug:  622400 
Change-Id: I82a942437a47e925f4c253d4d0ebd4595be67dab

TBR=avi@chromium.org

Change-Id: I82a942437a47e925f4c253d4d0ebd4595be67dab
Reviewed-on: https://chromium-review.googlesource.com/533920
Commit-Queue: Gabriel Charette <gab@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Reviewed-by: Francois Doray <fdoray@chromium.org>
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Cr-Commit-Position: refs/heads/master@{#480059}
[modify] https://crrev.com/b58c5ae28893d0a4b897380b106496431bb2c59d/base/threading/sequenced_worker_pool.cc
[modify] https://crrev.com/b58c5ae28893d0a4b897380b106496431bb2c59d/base/threading/sequenced_worker_pool.h
[modify] https://crrev.com/b58c5ae28893d0a4b897380b106496431bb2c59d/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/b58c5ae28893d0a4b897380b106496431bb2c59d/chrome/browser/chrome_content_browser_client.h
[modify] https://crrev.com/b58c5ae28893d0a4b897380b106496431bb2c59d/components/task_scheduler_util/browser/initialization.cc
[modify] https://crrev.com/b58c5ae28893d0a4b897380b106496431bb2c59d/components/task_scheduler_util/browser/initialization.h
[modify] https://crrev.com/b58c5ae28893d0a4b897380b106496431bb2c59d/content/browser/browser_main_loop.cc
[modify] https://crrev.com/b58c5ae28893d0a4b897380b106496431bb2c59d/content/public/browser/content_browser_client.h
[modify] https://crrev.com/b58c5ae28893d0a4b897380b106496431bb2c59d/ios/chrome/browser/web/chrome_web_client.h
[modify] https://crrev.com/b58c5ae28893d0a4b897380b106496431bb2c59d/ios/chrome/browser/web/chrome_web_client.mm
[modify] https://crrev.com/b58c5ae28893d0a4b897380b106496431bb2c59d/ios/web/app/web_main_loop.mm
[modify] https://crrev.com/b58c5ae28893d0a4b897380b106496431bb2c59d/ios/web/public/web_client.h
[modify] https://crrev.com/b58c5ae28893d0a4b897380b106496431bb2c59d/testing/variations/fieldtrial_testing_config.json
Project Member

Comment 62 by bugdroid1@chromium.org, Dec 21 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8eb552ea282a7a465389a9a8b4c8ee8ffd365125

commit 8eb552ea282a7a465389a9a8b4c8ee8ffd365125
Author: Sylvain Defresne <sdefresne@chromium.org>
Date: Thu Dec 21 10:40:53 2017

Remove obsolete task scheduler flags & switches.

Task scheduler is now enabled unconditionally and those switches
and flags are unused (since http://crrev.com/c/533920). Remove
them.

Bug:  622400 
Change-Id: Ic5c3ac981733c4cc7357c6a568e9be8d2dc19c25
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Reviewed-on: https://chromium-review.googlesource.com/833926
Commit-Queue: Sylvain Defresne <sdefresne@chromium.org>
Reviewed-by: Robert Liao <robliao@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Mark Mentovai <mark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#525664}
[modify] https://crrev.com/8eb552ea282a7a465389a9a8b4c8ee8ffd365125/base/BUILD.gn
[delete] https://crrev.com/3ea73474028bc719fc85c79b7d9f6cfa78bcfbf0/base/task_scheduler/switches.cc
[delete] https://crrev.com/3ea73474028bc719fc85c79b7d9f6cfa78bcfbf0/base/task_scheduler/switches.h
[modify] https://crrev.com/8eb552ea282a7a465389a9a8b4c8ee8ffd365125/chrome/browser/about_flags.cc
[modify] https://crrev.com/8eb552ea282a7a465389a9a8b4c8ee8ffd365125/ios/chrome/browser/about_flags.mm

Comment 63 by gab@chromium.org, May 3 2018

Status: Fixed (was: Started)
Experiment is long done and SequencedWorkerPool is no more.
►

Sign in to add a comment