New issue
Advanced search Search tips

Issue 724077 link

Starred by 1 user

Issue metadata

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

Blocking:
issue 708584
issue 689520



Sign in to add a comment

Add a ScopedTaskEnvironment mode that prevents any work from happening until explicitly RunUntilIdle()

Project Member Reported by gab@chromium.org, May 18 2017

Issue description

Having TaskScheduler start automatically in the background prevents tests that do

<assert state before FILE tasks run>
base::RunLoop().RunUntilIdle();  
<assert state after FILE tasks run>

from being migrated to TaskScheduler as the before state becomes racy.

TaskSchedulers under ScopedTaskEnvironment shouldn't process tasks until ScopedTaskEnvironment::RunUntilIdle() is invoked (or TaskScheduler::FlushForTesting()). Note that this isn't worse than before as the run calls had to block until idle already anyways to prevent the "after state" access from being race.
 

Comment 1 by gab@chromium.org, May 18 2017

Summary: ScopedTaskEnvironment have a mode that prevents any work from happening until explicitly RunUntilIdle() (was: ScopedTaskEnvironment should prevent any work from happening until explicitly RunUntilIdle())
We discussed that it's not possible to do this for all ScopedTaskEnvironments (it would break many tests that merely base::PostTaskAndReply() with a RunLoop::QuitClosure() and then RunLoop().Run()).

But we can provide an explicit mode on ScopedTaskEnvironment to ask it to behave that way for a given test (overall we agree it's a nicer paradigm, it's just not one the codebase we're migrating from is ready to support at scale without making this migration even larger than it is -- and we don't want that).

Comment 2 by gab@chromium.org, May 18 2017

Summary: Add a ScopedTaskEnvironment mode that prevents any work from happening until explicitly RunUntilIdle() (was: ScopedTaskEnvironment have a mode that prevents any work from happening until explicitly RunUntilIdle())
Project Member

Comment 3 by bugdroid1@chromium.org, May 23 2017

Project Member

Comment 4 by bugdroid1@chromium.org, May 23 2017

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

commit 92ac1582b78a62c5c3d1b49c0b1ef6088a065054
Author: mek <mek@chromium.org>
Date: Tue May 23 17:08:13 2017

Revert of Add ScopedTaskEnvironment::ExecutionControlMode. (patchset #6 id:100001 of https://codereview.chromium.org/2891363005/ )

Reason for revert:
Is causing consistent test failures on the main waterfall in net_unittests URLRequestSimpleJobTest.CancelAfterFirstReadStarted, for example:

https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.mac%2FMac10.9_Tests%2F41620%2F%2B%2Frecipes%2Fsteps%2Fnet_unittests%2F0%2Flogs%2FURLRequestSimpleJobTest.CancelAfterFirstReadStarted%2F0

[1130:771:0523/094643.091773:30621298248173:FATAL:scoped_task_environment.cc(178)] Check failed: !task_queue_empty_closure_.
0   net_unittests                       0x0000000102d4603c base::debug::StackTrace::StackTrace(unsigned long) + 28
1   net_unittests                       0x0000000102d5db70 logging::LogMessage::~LogMessage() + 224
2   net_unittests                       0x000000010302df97 base::test::ScopedTaskEnvironment::RunUntilIdle() + 183
3   net_unittests                       0x000000010205f824 net::URLRequestSimpleJobTest_CancelAfterFirstReadStarted_Test::TestBody() + 260
4   net_unittests                       0x00000001022adf56 testing::Test::Run() + 246
5   net_unittests                       0x00000001022ae9f0 testing::TestInfo::Run() + 288
6   net_unittests                       0x00000001022aef57 testing::TestCase::Run() + 263
7   net_unittests                       0x00000001022b5057 testing::internal::UnitTestImpl::RunAllTests() + 871
8   net_unittests                       0x00000001022b4cc3 testing::UnitTest::Run() + 163
9   net_unittests                       0x00000001030309b3 base::TestSuite::Run() + 163
10  net_unittests                       0x000000010303e176 base::LaunchUnitTests(int, char**, base::Callback<int (), (base::internal::CopyMode)1, (base::internal::RepeatMode)1> const&) + 134
11  net_unittests                       0x0000000101fdf008 main + 440
12  libdyld.dylib                       0x00007fff8d4495fd start + 1
13  ???                                 0x0000000000000007 0x0 + 7

Original issue's description:
> Add ScopedTaskEnvironment::ExecutionControlMode.
>
> This enum controls whether tasks posted within the scope of a
> ScopedTaskEnvironment can run as they are posted or have to wait
> until a call to RunUntilIdle() to run.
>
> BUG= 724077 
> TBR=gab@chromium.org
>
> Review-Url: https://codereview.chromium.org/2891363005
> Cr-Commit-Position: refs/heads/master@{#473925}
> Committed: https://chromium.googlesource.com/chromium/src/+/fc7b5ec521b29a97e63a7cda5921aa5ed793679a

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

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

[modify] https://crrev.com/92ac1582b78a62c5c3d1b49c0b1ef6088a065054/base/task_scheduler/task_scheduler_impl.cc
[modify] https://crrev.com/92ac1582b78a62c5c3d1b49c0b1ef6088a065054/base/task_scheduler/task_scheduler_impl.h
[modify] https://crrev.com/92ac1582b78a62c5c3d1b49c0b1ef6088a065054/base/task_scheduler/task_tracker.cc
[modify] https://crrev.com/92ac1582b78a62c5c3d1b49c0b1ef6088a065054/base/task_scheduler/task_tracker.h
[modify] https://crrev.com/92ac1582b78a62c5c3d1b49c0b1ef6088a065054/base/task_scheduler/task_tracker_posix.h
[modify] https://crrev.com/92ac1582b78a62c5c3d1b49c0b1ef6088a065054/base/test/scoped_task_environment.cc
[modify] https://crrev.com/92ac1582b78a62c5c3d1b49c0b1ef6088a065054/base/test/scoped_task_environment.h
[modify] https://crrev.com/92ac1582b78a62c5c3d1b49c0b1ef6088a065054/base/test/scoped_task_environment_unittest.cc

Project Member

Comment 5 by bugdroid1@chromium.org, May 25 2017

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

commit 38ca4d86dba6022a465b9966e6917a9e004e919b
Author: fdoray <fdoray@chromium.org>
Date: Thu May 25 20:06:51 2017

Do not use ScopedTaskEnvironment::RunUntilIdle() in URLRequestSimpleJobTest.

URLRequestSimpleJobTest.CancelAfterFirstReadStarted doesn't need
ScopedTaskEnvironment::RunUntilIdle() because it doesn't depend on
tasks running in TaskScheduler.

Moving from ScopedTaskEnvironment::RunUntilIdle() to
RunLoop::RunUntilIdle() in this test is important because the
test posts a MessageLoop::QuitWhenIdleClosure() (via TestDelegate),
which is incompatible with changes that we want to make to
ScopedTaskEnvironment::RunUntilIdle().

BUG= 724077 

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

[modify] https://crrev.com/38ca4d86dba6022a465b9966e6917a9e004e919b/net/url_request/url_request_simple_job_unittest.cc

Project Member

Comment 6 by bugdroid1@chromium.org, May 29 2017

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

commit 1f3a7e66d633e18507101436900fa98625d8e987
Author: fdoray <fdoray@chromium.org>
Date: Mon May 29 21:16:56 2017

Add MediaControlsImplTestWithMockScheduler.

Currently, MediaControlsImplTest.ControlsRemainVisibleDuringKeyboardInteraction
does the following:

SetUp()
  InitializePage()
  Creates a DummyPageHolder, which indirectly creates a
  blink::PerformanceMonitor. blink::PerformanceMonitor registers
  ifself as a task time observer with the current
  TaskQueueManager.

Body
  Instantiate
  ScopedTestingPlatformSupport<TestingPlatformSupportWithMockScheduler>.
  This overrides the current TaskQueueManager with a mock.

  InitializePage()
  This deletes the existing DummyPageHolder, which indirectly
  deletes the blink::PerformanceMonitor. Before it is deleted,
  blink::PerformanceMonitor unregisters from the current
  TaskQueueManager, which is unfortunately *not the same* as the one
  to which it was registered. That means that the initial
  TaskQueueManager is left with a pointer to a deleted
  blink::PerformanceMonitor.

TearDownBlinkTestEnvironment()
  We would like to flush the ScopedTaskEnvironment from here but
  we can't because that causes the initial TaskQueueManager to
  access its invalid blink::PerformanceMonitor pointer.

This CL fixes this issue by creating a
MediaControlsImplTestWithMockScheduler which instantiates a
ScopedTestingPlatformSupport<TestingPlatformSupportWithMockScheduler>
before SetUp() to make sure that blink::PerformanceMonitor
registers itself with the mock TaskQueueManager rather than
with the real one.

BUG= 724077 

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

[modify] https://crrev.com/1f3a7e66d633e18507101436900fa98625d8e987/third_party/WebKit/Source/modules/media_controls/MediaControlsImplTest.cpp

Project Member

Comment 7 by bugdroid1@chromium.org, May 29 2017

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

commit b199f1bef099a0b34576f10fd57e713551e39349
Author: fdoray <fdoray@chromium.org>
Date: Mon May 29 23:00:03 2017

Add ScopedTaskEnvironment::ExecutionControlMode.

This enum controls whether tasks posted within the scope of a
ScopedTaskEnvironment can run as they are posted or have to wait
until a call to RunUntilIdle() to run.

BUG= 724077 
TBR=gab@chromium.org

Review-Url: https://codereview.chromium.org/2891363005
Cr-Original-Commit-Position: refs/heads/master@{#473925}
Committed: https://chromium.googlesource.com/chromium/src/+/fc7b5ec521b29a97e63a7cda5921aa5ed793679a
Review-Url: https://codereview.chromium.org/2891363005
Cr-Commit-Position: refs/heads/master@{#475392}

[modify] https://crrev.com/b199f1bef099a0b34576f10fd57e713551e39349/base/task_scheduler/task_scheduler_impl.cc
[modify] https://crrev.com/b199f1bef099a0b34576f10fd57e713551e39349/base/task_scheduler/task_scheduler_impl.h
[modify] https://crrev.com/b199f1bef099a0b34576f10fd57e713551e39349/base/task_scheduler/task_tracker.cc
[modify] https://crrev.com/b199f1bef099a0b34576f10fd57e713551e39349/base/task_scheduler/task_tracker.h
[modify] https://crrev.com/b199f1bef099a0b34576f10fd57e713551e39349/base/task_scheduler/task_tracker_posix.h
[modify] https://crrev.com/b199f1bef099a0b34576f10fd57e713551e39349/base/test/scoped_task_environment.cc
[modify] https://crrev.com/b199f1bef099a0b34576f10fd57e713551e39349/base/test/scoped_task_environment.h
[modify] https://crrev.com/b199f1bef099a0b34576f10fd57e713551e39349/base/test/scoped_task_environment_unittest.cc

Comment 8 by fdoray@chromium.org, Jun 16 2017

Status: Fixed (was: Assigned)

Sign in to add a comment