Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2769693002:
Enable redirection of SequencedWorkerPool to TaskScheduler in TestBrowserThreadBundle. (Closed)
DescriptionEnable redirection of SequencedWorkerPool to TaskScheduler in TestBrowserThreadBundle.
Motivation:
- Redirection of SequencedWorkerPool to TaskScheduler is enabled in
browser tests and on Canary and Dev. It makes sense to enable it
in unit tests too.
- It is not possible to wait until there are no more blocking pool
*and* TaskScheduler tasks (new tasks in the blocking pool during
a FlushForTesting() call on TaskScheduler and vice versa). If
the blocking pool is redirected to TaskScheduler, calling
FlushForTesting() on TaskScheduler guarantees that there are no
more blocking pool *and* TaskScheduler tasks.
BUG=667892
Review-Url: https://codereview.chromium.org/2769693002
Cr-Commit-Position: refs/heads/master@{#475144}
Committed: https://chromium.googlesource.com/chromium/src/+/e1aa28915a28da266d406202a07112d44d6cc018
Patch Set 1 #
Total comments: 3
Patch Set 2 : CR-gab #Patch Set 3 : rebase #Messages
Total messages: 35 (22 generated)
fdoray@chromium.org changed reviewers: + gab@chromium.org, robliao@chromium.org
PTAL
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2769693002/diff/1/content/public/test/test_br... File content/public/test/test_browser_thread_bundle.cc (right): https://codereview.chromium.org/2769693002/diff/1/content/public/test/test_br... content/public/test/test_browser_thread_bundle.cc:63: base::SequencedWorkerPool::EnableForProcess(); Do we need this? Sounds better to have tasks be redirected in a void and explode than lazily instantiating a very late SequencedWorkerPool w/ workers.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
PTAnL https://codereview.chromium.org/2769693002/diff/1/content/public/test/test_br... File content/public/test/test_browser_thread_bundle.cc (right): https://codereview.chromium.org/2769693002/diff/1/content/public/test/test_br... content/public/test/test_browser_thread_bundle.cc:63: base::SequencedWorkerPool::EnableForProcess(); On 2017/03/22 16:23:30, gab wrote: > Do we need this? Sounds better to have tasks be redirected in a void and explode > than lazily instantiating a very late SequencedWorkerPool w/ workers. In TestBrowserThreadBundle::CreateThreads(), we enable redirection of SequencedWorkerPool to TaskScheduler. It is important to disable this redirection here to bring back the process to its original state. Some tests instantiate their own SequencedWorkerPool (e.g. https://cs.chromium.org/chromium/src/components/update_client/update_client_u...). Without this, they will try to redirect tasks to a non-existent TaskScheduler and crash.
lgtm w/ comment https://codereview.chromium.org/2769693002/diff/1/content/public/test/test_br... File content/public/test/test_browser_thread_bundle.cc (right): https://codereview.chromium.org/2769693002/diff/1/content/public/test/test_br... content/public/test/test_browser_thread_bundle.cc:63: base::SequencedWorkerPool::EnableForProcess(); On 2017/04/03 14:57:28, fdoray wrote: > On 2017/03/22 16:23:30, gab wrote: > > Do we need this? Sounds better to have tasks be redirected in a void and > explode > > than lazily instantiating a very late SequencedWorkerPool w/ workers. > > In TestBrowserThreadBundle::CreateThreads(), we enable redirection of > SequencedWorkerPool to TaskScheduler. It is important to disable this > redirection here to bring back the process to its original state. > > Some tests instantiate their own SequencedWorkerPool (e.g. > https://cs.chromium.org/chromium/src/components/update_client/update_client_u...). > Without this, they will try to redirect tasks to a non-existent TaskScheduler > and crash. I see expand comment here then, something like: // Disable redirection of SequencedWorkerPools to TaskScheduler. // This is required in order to reset global state so that tests following this one in this process can still manage their own SequencedWorkerPool without using TestBrowserThreadBundle.
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm++
lgtm
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
fdoray@chromium.org changed reviewers: + sky@chromium.org
sky@: Please take a look.
LGTM
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robliao@chromium.org, gab@chromium.org Link to the patchset: https://codereview.chromium.org/2769693002/#ps40001 (title: "rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1495830359876480,
"parent_rev": "b23d2a4dca1f06ef6d935b2612c2d050fc408338", "commit_rev":
"e1aa28915a28da266d406202a07112d44d6cc018"}
Message was sent while issue was closed.
Description was changed from ========== Enable redirection of SequencedWorkerPool to TaskScheduler in TestBrowserThreadBundle. Motivation: - Redirection of SequencedWorkerPool to TaskScheduler is enabled in browser tests and on Canary and Dev. It makes sense to enable it in unit tests too. - It is not possible to wait until there are no more blocking pool *and* TaskScheduler tasks (new tasks in the blocking pool during a FlushForTesting() call on TaskScheduler and vice versa). If the blocking pool is redirected to TaskScheduler, calling FlushForTesting() on TaskScheduler guarantees that there are no more blocking pool *and* TaskScheduler tasks. BUG=667892 ========== to ========== Enable redirection of SequencedWorkerPool to TaskScheduler in TestBrowserThreadBundle. Motivation: - Redirection of SequencedWorkerPool to TaskScheduler is enabled in browser tests and on Canary and Dev. It makes sense to enable it in unit tests too. - It is not possible to wait until there are no more blocking pool *and* TaskScheduler tasks (new tasks in the blocking pool during a FlushForTesting() call on TaskScheduler and vice versa). If the blocking pool is redirected to TaskScheduler, calling FlushForTesting() on TaskScheduler guarantees that there are no more blocking pool *and* TaskScheduler tasks. BUG=667892 Review-Url: https://codereview.chromium.org/2769693002 Cr-Commit-Position: refs/heads/master@{#475144} Committed: https://chromium.googlesource.com/chromium/src/+/e1aa28915a28da266d406202a071... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/e1aa28915a28da266d406202a071...
Message was sent while issue was closed.
On 2017/05/26 21:47:25, commit-bot: I haz the power wrote: > Committed patchset #3 (id:40001) as > https://chromium.googlesource.com/chromium/src/+/e1aa28915a28da266d406202a071... This seemed to have caused https://bugs.chromium.org/p/chromium/issues/detail?id=727994
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2909393003/ by falken@chromium.org. The reason for reverting is: Caused ServiceWorkerResourceStorageDiskTest.* tests in content_unittests to crash. See https://bugs.chromium.org/p/chromium/issues/detail?id=727994 Reverting locally fixed the crashes, so I'm going to revert this CL.. |
