Make base::Timer sequence-friendly.

Make base::Timer sequence-friendly.

This CL was also going to fix a race condition in the existing implementation when SetTaskRunner() was used but the test-only failures caused by tests having long-lived 
with the status quo are proving hard than expected to fix. The race is no worse than
before by making this code sequence-friendly (and generally healthier). Will fix
race in a follow-up CL.

Of note in this CL:
 - Patch set 1 is https://codereview.chromium.org/1433373003
   so looking at diff from 1 might be less work (especially for tests).
 - (not true anymore, postponed to race fix CL:) 
   The Timer's delayed task now always lives on the sequence it was started from
   (and even SetTaskRunner() was used, a task is posted to it when the delay
    expires instead of having the Timer's delayed task live on it -- this solves
    the aforementioned race condition).
   - This required adapting tests for MediaCodecLoop and UploadProgressTracker.

BUG=587199, 552633, 678592, 684640, 675631
Review-Url: https://codereview.chromium.org/2491613004
Cr-Commit-Position: refs/heads/master@{#476317}
Committed: https://chromium.googlesource.com/chromium/src/+/4e844bb5f686dbd3a062c6aaf83c8a1df171a08a

[gab, 2016-11-10 23:53:31]

Description was changed from

==========
Make Timer sequence-friendly and thread-safe.

BUG=587199
==========

to

==========
Make Timer sequence-friendly and thread-safe.

BUG=587199, 552633
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_tsan_rel_ng
==========

[gab, 2016-11-11 00:40:56]

Description was changed from

==========
Make Timer sequence-friendly and thread-safe.

BUG=587199, 552633
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_tsan_rel_ng
==========

to

==========
Make Timer sequence-friendly and thread-safe.

BUG=587199, 552633, 653394
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_tsan_rel_ng
==========

[gab, 2016-11-15 17:29:24]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-15 17:29:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gab, 2016-11-15 17:31:15]

Description was changed from

==========
Make Timer sequence-friendly and thread-safe.

BUG=587199, 552633, 653394
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_tsan_rel_ng
==========

to

==========
Make base::Timer sequence-safe.

Fixes a race condition in the previous implementation.

Of note in this CL:
 - Patch set 1 is https://codereview.chromium.org/1433373003
   so looking at diff from 1 might be less work (especially for tests).
 - Timer is now 100% sequence-affine.
 - Anything that needs to be handled on the destination task runner
   is handled by BaseTimerTaskInternal (which is also 100% sequence-
   affine on the destination sequence).
 - When origin == destination sequences, the two communicate synchronously
   as much as possible (to keep the characteristics of the previous
implementation
   without having to introduce locks).
 - Communications back-and-forth are WeakPtr bound which allows cancellation
   without reaching in each other's internals as before.
 - BaseTimerTaskInternal::CleanupTrigger is a trick to redo something which was
   previously supported directly by ~BaseTimerTaskInternal() (but it's no longer
   base::Owned() by Run() so something that actually is is required).
   - It is required to pass TimerTest.NonRepeatMessageLoopDeath
   - The modifications to sequenced_worker_pool.cc are required by it (or the
     WeakPtr is unhappy being dereferenced in the destruction of the Run()
Closure).

BUG=587199, 552633, 653394
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_tsan_rel_ng
==========

[commit-bot: I haz the power, 2016-11-15 17:33:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-15 17:33:11]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[gab, 2016-11-15 17:33:14]

Description was changed from

==========
Make base::Timer sequence-safe.

Fixes a race condition in the previous implementation.

Of note in this CL:
 - Patch set 1 is https://codereview.chromium.org/1433373003
   so looking at diff from 1 might be less work (especially for tests).
 - Timer is now 100% sequence-affine.
 - Anything that needs to be handled on the destination task runner
   is handled by BaseTimerTaskInternal (which is also 100% sequence-
   affine on the destination sequence).
 - When origin == destination sequences, the two communicate synchronously
   as much as possible (to keep the characteristics of the previous
implementation
   without having to introduce locks).
 - Communications back-and-forth are WeakPtr bound which allows cancellation
   without reaching in each other's internals as before.
 - BaseTimerTaskInternal::CleanupTrigger is a trick to redo something which was
   previously supported directly by ~BaseTimerTaskInternal() (but it's no longer
   base::Owned() by Run() so something that actually is is required).
   - It is required to pass TimerTest.NonRepeatMessageLoopDeath
   - The modifications to sequenced_worker_pool.cc are required by it (or the
     WeakPtr is unhappy being dereferenced in the destruction of the Run()
Closure).

BUG=587199, 552633, 653394
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_tsan_rel_ng
==========

to

==========
Make base::Timer sequence-safe.

Fixes a race condition in the previous implementation.

Of note in this CL:
 - Patch set 1 is https://codereview.chromium.org/1433373003
   so looking at diff from 1 might be less work (especially for tests).
 - Timer is now 100% sequence-affine.
 - Anything that needs to be handled on the destination task runner
   is handled by BaseTimerTaskInternal (which is also 100% sequence-
   affine on the destination sequence).
 - When origin == destination sequences, the two communicate synchronously
   as much as possible (to keep the characteristics of the previous
implementation
   without having to introduce locks).
 - Communications back-and-forth are WeakPtr bound which allows cancellation
   without reaching in each other's internals as before.
 - BaseTimerTaskInternal::CleanupTrigger is a trick to redo something which was
   previously supported directly by ~BaseTimerTaskInternal() (but it's no longer
   base::Owned() by Run() so something that actually is is required).
   - It is required to pass TimerTest.NonRepeatMessageLoopDeath
   - The modifications to sequenced_worker_pool.cc are required by it (or the
     WeakPtr is unhappy being dereferenced in the destruction of the Run()
Closure).
 - Tests haven't changed (merely modernized towards
RunLoop/TaskRunners/WaitableEvents
   to ease a few things).

BUG=587199, 552633, 653394
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_tsan_rel_ng
==========

[gab, 2016-11-15 17:51:58]

Description was changed from

==========
Make base::Timer sequence-safe.

Fixes a race condition in the previous implementation.

Of note in this CL:
 - Patch set 1 is https://codereview.chromium.org/1433373003
   so looking at diff from 1 might be less work (especially for tests).
 - Timer is now 100% sequence-affine.
 - Anything that needs to be handled on the destination task runner
   is handled by BaseTimerTaskInternal (which is also 100% sequence-
   affine on the destination sequence).
 - When origin == destination sequences, the two communicate synchronously
   as much as possible (to keep the characteristics of the previous
implementation
   without having to introduce locks).
 - Communications back-and-forth are WeakPtr bound which allows cancellation
   without reaching in each other's internals as before.
 - BaseTimerTaskInternal::CleanupTrigger is a trick to redo something which was
   previously supported directly by ~BaseTimerTaskInternal() (but it's no longer
   base::Owned() by Run() so something that actually is is required).
   - It is required to pass TimerTest.NonRepeatMessageLoopDeath
   - The modifications to sequenced_worker_pool.cc are required by it (or the
     WeakPtr is unhappy being dereferenced in the destruction of the Run()
Closure).
 - Tests haven't changed (merely modernized towards
RunLoop/TaskRunners/WaitableEvents
   to ease a few things).

BUG=587199, 552633, 653394
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_tsan_rel_ng
==========

to

==========
Make base::Timer sequence-safe.

Fixes a race condition in the previous implementation.

Essentially a full re-write of timer.cc.

Of note in this CL:
 - Patch set 1 is https://codereview.chromium.org/1433373003
   so looking at diff from 1 might be less work (especially for tests).
 - Timer is now 100% sequence-affine.
 - Anything that needs to be handled on the destination task runner
   is handled by BaseTimerTaskInternal (which is also 100% sequence-
   affine on the destination sequence).
 - When origin == destination sequences, the two communicate synchronously
   as much as possible (to keep the characteristics of the previous
implementation
   without having to introduce locks).
 - Communications back-and-forth are WeakPtr bound which allows cancellation
   without reaching in each other's internals as before.
 - BaseTimerTaskInternal::CleanupTrigger is a trick to redo something which was
   previously supported directly by ~BaseTimerTaskInternal() (but it's no longer
   base::Owned() by Run() so something that actually is is required).
   - It is required to pass TimerTest.NonRepeatMessageLoopDeath
   - The modifications to sequenced_worker_pool.cc are required by it (or the
     WeakPtr is unhappy being dereferenced in the destruction of the Run()
Closure).
 - Tests haven't changed (merely modernized towards
RunLoop/TaskRunners/WaitableEvents
   to ease a few things).

BUG=587199, 552633, 653394
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_tsan_rel_ng
==========

[gab, 2016-11-15 17:53:33]

gab@chromium.org changed reviewers:
+ danakj@chromium.org

[gab, 2016-11-15 17:53:34]

Dana PTAL (bots are red because I need to rebase on
https://codereview.chromium.org/2484023002 -- and sadly the newly introduced
TickClock from TestMockTimeTaskRunner uses a ThreadChecker, sigh! -- but I'd
like your take on this while I dig further!).

CC fdoray/robliao/jsbell if you care to take a look as well.

Another thing which I know is broken in the current impl:
alarm_timer_chromeos.cc uses protected methods to directly tweak the members of
Timer... I'll need to add support for that.

Cheers!
Gab

[gab, 2016-11-16 16:55:58]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-16 16:56:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-16 17:21:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-16 17:21:24]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[gab, 2016-11-16 17:33:23]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-16 17:33:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gab, 2016-11-17 17:38:53]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-17 17:39:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-17 18:06:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-17 18:06:01]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[danakj, 2016-11-21 23:49:52]

To be sure.. if u revert all changes to the unit tests, they all pass? Maybe
that is a silly question and I need to read stuff as I'm just starting this
review finally. But it would give me a lot more confidence if that were the
case.

[danakj, 2016-11-22 00:09:12]

Why isn't the stuff in sequenced_worker_pool.cc a separate CL? It seems pretty
nice on its own?

[gab, 2016-11-22 20:37:24]

Description was changed from

==========
Make base::Timer sequence-safe.

Fixes a race condition in the previous implementation.

Essentially a full re-write of timer.cc.

Of note in this CL:
 - Patch set 1 is https://codereview.chromium.org/1433373003
   so looking at diff from 1 might be less work (especially for tests).
 - Timer is now 100% sequence-affine.
 - Anything that needs to be handled on the destination task runner
   is handled by BaseTimerTaskInternal (which is also 100% sequence-
   affine on the destination sequence).
 - When origin == destination sequences, the two communicate synchronously
   as much as possible (to keep the characteristics of the previous
implementation
   without having to introduce locks).
 - Communications back-and-forth are WeakPtr bound which allows cancellation
   without reaching in each other's internals as before.
 - BaseTimerTaskInternal::CleanupTrigger is a trick to redo something which was
   previously supported directly by ~BaseTimerTaskInternal() (but it's no longer
   base::Owned() by Run() so something that actually is is required).
   - It is required to pass TimerTest.NonRepeatMessageLoopDeath
   - The modifications to sequenced_worker_pool.cc are required by it (or the
     WeakPtr is unhappy being dereferenced in the destruction of the Run()
Closure).
 - Tests haven't changed (merely modernized towards
RunLoop/TaskRunners/WaitableEvents
   to ease a few things).

BUG=587199, 552633, 653394
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_tsan_rel_ng
==========

to

==========
Make base::Timer sequence-safe.

Fixes a race condition in the previous implementation.

Essentially a full re-write of timer.cc.

Of note in this CL:
 - Patch set 1 is https://codereview.chromium.org/1433373003
   so looking at diff from 1 might be less work (especially for tests).
 - Timer is now 100% sequence-affine.
 - Anything that needs to be handled on the destination task runner
   is handled by BaseTimerTaskInternal (which is also 100% sequence-
   affine on the destination sequence).
 - When origin == destination sequences, the two communicate synchronously
   as much as possible (to keep the characteristics of the previous
implementation
   without having to introduce locks).
 - Communications back-and-forth are WeakPtr bound which allows cancellation
   without reaching in each other's internals as before.
 - BaseTimerTaskInternal::CleanupTrigger is a trick to redo something which was
   previously supported directly by ~BaseTimerTaskInternal() (but it's no longer
   base::Owned() by Run() so something that actually is is required).
   - It is required to pass TimerTest.NonRepeatMessageLoopDeath
   - The modifications to sequenced_worker_pool.cc are required by it (or the
     WeakPtr is unhappy being dereferenced in the destruction of the Run()
Closure).
 - Tests haven't changed (merely modernized towards
RunLoop/TaskRunners/WaitableEvents
   to ease a few things).

BUG=587199, 552633
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_tsan_rel_ng
==========

[gab, 2016-11-22 20:42:20]

Description was changed from

==========
Make base::Timer sequence-safe.

Fixes a race condition in the previous implementation.

Essentially a full re-write of timer.cc.

Of note in this CL:
 - Patch set 1 is https://codereview.chromium.org/1433373003
   so looking at diff from 1 might be less work (especially for tests).
 - Timer is now 100% sequence-affine.
 - Anything that needs to be handled on the destination task runner
   is handled by BaseTimerTaskInternal (which is also 100% sequence-
   affine on the destination sequence).
 - When origin == destination sequences, the two communicate synchronously
   as much as possible (to keep the characteristics of the previous
implementation
   without having to introduce locks).
 - Communications back-and-forth are WeakPtr bound which allows cancellation
   without reaching in each other's internals as before.
 - BaseTimerTaskInternal::CleanupTrigger is a trick to redo something which was
   previously supported directly by ~BaseTimerTaskInternal() (but it's no longer
   base::Owned() by Run() so something that actually is is required).
   - It is required to pass TimerTest.NonRepeatMessageLoopDeath
   - The modifications to sequenced_worker_pool.cc are required by it (or the
     WeakPtr is unhappy being dereferenced in the destruction of the Run()
Closure).
 - Tests haven't changed (merely modernized towards
RunLoop/TaskRunners/WaitableEvents
   to ease a few things).

BUG=587199, 552633
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_tsan_rel_ng
==========

to

==========
Make base::Timer sequence-safe.

Fixes a race condition in the previous implementation.

Essentially a full re-write of timer.cc.

Of note in this CL:
 - Patch set 1 is https://codereview.chromium.org/1433373003
   so looking at diff from 1 might be less work (especially for tests).
 - Timer is now 100% sequence-affine.
 - Anything that needs to be handled on the destination task runner
   is handled by BaseTimerTaskInternal (which is also 100% sequence-
   affine on the destination sequence).
 - When origin == destination sequences, the two communicate synchronously
   as much as possible (to keep the characteristics of the previous
implementation
   without having to introduce locks).
 - Communications back-and-forth are WeakPtr bound which allows cancellation
   without reaching in each other's internals as before.
 - BaseTimerTaskInternal::CleanupTrigger is a trick to redo something which was
   previously supported directly by ~BaseTimerTaskInternal() (but it's no longer
   base::Owned() by Run() so something that actually is is required).
   - It is required to pass TimerTest.NonRepeatMessageLoopDeath
   - The modifications to sequenced_worker_pool.cc are required by it (or the
     WeakPtr is unhappy being dereferenced in the destruction of the Run()
Closure).

BUG=587199, 552633
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_tsan_rel_ng
==========

[gab, 2016-11-22 21:05:24]

> To be sure.. if u revert all changes to the unit tests, they all pass? Maybe
> that is a silly question and I need to read stuff as I'm just starting this
> review finally. But it would give me a lot more confidence if that were the
> case.

Good question, yes modulo a tweak for a missing task runner handle, extracted
test refactor to https://codereview.chromium.org/2514293004/ to make that clear.

> Why isn't the stuff in sequenced_worker_pool.cc a separate CL? It seems pretty
> nice on its own?

Because I kind of used timer.cc's usage as the unit test for that "feature" of
sequenced_worker_pool.cc... I realized while writing this that these deletions
are racy (it's possible for multiple workers to delete closures from the same
sequence in parallel...) and I figured that it'd be annoying to write a unit
test that works around this (I don't think we should fix it since it's been the
case forever and SWP is being deprecated).

[gab, 2016-11-24 04:21:35]

Patchset #8 (id:140001) has been deleted

[gab, 2016-11-28 04:30:23]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-28 04:30:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-28 04:55:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-28 04:55:08]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[vmpstr, 2016-12-03 01:14:03]

vmpstr@chromium.org changed reviewers:
+ vmpstr@chromium.org

[vmpstr, 2016-12-03 01:14:04]

https://codereview.chromium.org/2491613004/diff/160001/base/threading/sequenc...
File base/threading/sequenced_worker_pool.cc (right):

https://codereview.chromium.org/2491613004/diff/160001/base/threading/sequenc...
base/threading/sequenced_worker_pool.cc:387: void DeleteTheseOutsideLockHelper(
WDYT about just "DeleteWithoutLock" with "tasks_to_delete" as the parameter?

https://codereview.chromium.org/2491613004/diff/160001/base/threading/sequenc...
base/threading/sequenced_worker_pool.cc:1112:
this_worker->reset_running_task_info();
Does this need to happen after every iteration, or can it just happen once
before the function exits?

https://codereview.chromium.org/2491613004/diff/160001/base/timer/timer.cc
File base/timer/timer.cc (right):

https://codereview.chromium.org/2491613004/diff/160001/base/timer/timer.cc#ne...
base/timer/timer.cc:20: // BaseTimerTaskInternal is a simple delegate for
scheduling a callback to Timer
s/simple // :)

Can you also split up the comment into a couple of blocks,

- common functionality.
- non-repeating functionality.
- repeating functionality.

It's a bit unclear which sentences are still referring to non-repeating stuff,
and which ones are a continuation of common description

https://codereview.chromium.org/2491613004/diff/160001/base/timer/timer.cc#ne...
base/timer/timer.cc:26: // such, cancellation is racing with the scheduled
delayed Run() task but so
By race here, do you mean that either Abandon will run or Run will run, but not
both at the same time? If they both can run, this is double delete badness

https://codereview.chromium.org/2491613004/diff/160001/base/timer/timer.cc#ne...
base/timer/timer.cc:46: class CleanupTrigger {
Consider adding RunOnDestroy type of class to base elsewhere, since I know this
can be a pretty common pattern

https://codereview.chromium.org/2491613004/diff/160001/base/timer/timer.cc#ne...
base/timer/timer.cc:75: destination_sequence_checker_.DetachFromSequence();
Can you comment that we need to detatch to ensure that it's bound to the running
sequence? (that's why we do this, right?)

https://codereview.chromium.org/2491613004/diff/160001/base/timer/timer.cc#ne...
base/timer/timer.cc:104: // TODO(gab): Currently rebasing on Now() to match
prior status quo but
Can you file a bug for this and reference it here?

https://codereview.chromium.org/2491613004/diff/160001/base/timer/timer.cc#ne...
base/timer/timer.cc:111: delay_);
was_reset_ = false?

https://codereview.chromium.org/2491613004/diff/160001/base/timer/timer.cc#ne...
base/timer/timer.cc:130: // So overall, either the Reset() came in too late and
will be canceled or
This comment is a bit confusing to me. So does this mean that there is no actual
reset task, right? Since we just postpone the run task? (You reference "and will
be canceled via the WeakPtr", which is the confusing part... what exactly will
be canceled?)

Or is this function called by the reset task created elsewhere? If that's the
case, aren't we already running the reset task since we're in this function?
Which means it won't be canceled?

https://codereview.chromium.org/2491613004/diff/160001/base/timer/timer.cc#ne...
base/timer/timer.cc:143: // Delete self, invalidating WeakPtrs and as such,
pending Run()/Reset()
Here as well, what's a reset task here?

https://codereview.chromium.org/2491613004/diff/160001/base/timer/timer.cc#ne...
base/timer/timer.cc:148: std::unique_ptr<CleanupTrigger> GetCleanupTrigger() {
nit: CreateCleanupTrigger

https://codereview.chromium.org/2491613004/diff/160001/base/timer/timer.cc#ne...
base/timer/timer.cc:238: if (origin_sequence_checker_.CalledOnValidSequence()) {
This is changing code behavior based on whether DCHECKs are on. The
CalledOnValidSequence always returns true if DCHECKs are off... Is this
expected? Can you drop a comment explaining here?

https://codereview.chromium.org/2491613004/diff/160001/base/timer/timer.cc#ne...
base/timer/timer.cc:261: // Do not allow changing the task runner while
something is been scheduled.
s/been //

https://codereview.chromium.org/2491613004/diff/160001/base/timer/timer.cc#ne...
base/timer/timer.cc:276: Reset();
Is this call always just PostNewScheduledTask?

https://codereview.chromium.org/2491613004/diff/160001/base/timer/timer.cc#ne...
base/timer/timer.cc:303: PostNewScheduledTask(delay_);
It's unclear to me if there other call sites other than Start that hit this, can
you comment when this is expected to hit?

[gab, 2016-12-23 20:17:07]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-23 20:17:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gab, 2016-12-23 20:18:21]

The CQ bit was unchecked by gab@chromium.org

[gab, 2016-12-23 20:18:45]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-23 20:18:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gab, 2016-12-23 20:22:12]

Finally got around to this :) -- still need to cleanup unused Timer members and
to find a story for the protected methods used by alarm_timer_chromeos.cc... but
the overall concept is ready for another look!

https://codereview.chromium.org/2491613004/diff/160001/base/threading/sequenc...
File base/threading/sequenced_worker_pool.cc (right):

https://codereview.chromium.org/2491613004/diff/160001/base/threading/sequenc...
base/threading/sequenced_worker_pool.cc:387: void DeleteTheseOutsideLockHelper(
On 2016/12/03 01:14:04, vmpstr wrote:
> WDYT about just "DeleteWithoutLock" with "tasks_to_delete" as the parameter?

Done on https://codereview.chromium.org/2581213002/

https://codereview.chromium.org/2491613004/diff/160001/base/threading/sequenc...
base/threading/sequenced_worker_pool.cc:1112:
this_worker->reset_running_task_info();
On 2016/12/03 01:14:04, vmpstr wrote:
> Does this need to happen after every iteration, or can it just happen once
> before the function exits?

Done on https://codereview.chromium.org/2581213002/

https://codereview.chromium.org/2491613004/diff/160001/base/timer/timer.cc
File base/timer/timer.cc (right):

https://codereview.chromium.org/2491613004/diff/160001/base/timer/timer.cc#ne...
base/timer/timer.cc:20: // BaseTimerTaskInternal is a simple delegate for
scheduling a callback to Timer
On 2016/12/03 01:14:04, vmpstr wrote:
> s/simple // :)

:)

> 
> Can you also split up the comment into a couple of blocks,
> 
> - common functionality.
> - non-repeating functionality.
> - repeating functionality.
> 
> It's a bit unclear which sentences are still referring to non-repeating stuff,
> and which ones are a continuation of common description

Clarified comment.

https://codereview.chromium.org/2491613004/diff/160001/base/timer/timer.cc#ne...
base/timer/timer.cc:26: // such, cancellation is racing with the scheduled
delayed Run() task but so
On 2016/12/03 01:14:04, vmpstr wrote:
> By race here, do you mean that either Abandon will run or Run will run, but
not
> both at the same time? If they both can run, this is double delete badness

I mean that Abandon() is not a delayed task while Run() is, so Run() can come
out ahead of it in the sequence if the delay expires just before Abandon() is
posted.

The two tasks are still sequenced so the race is in their scheduling, not in
their execution.

Clarified.

https://codereview.chromium.org/2491613004/diff/160001/base/timer/timer.cc#ne...
base/timer/timer.cc:46: class CleanupTrigger {
On 2016/12/03 01:14:04, vmpstr wrote:
> Consider adding RunOnDestroy type of class to base elsewhere, since I know
this
> can be a pretty common pattern

Whereelse? Tried to code search for it and didn't find much.

https://codereview.chromium.org/2491613004/diff/160001/base/timer/timer.cc#ne...
base/timer/timer.cc:75: destination_sequence_checker_.DetachFromSequence();
On 2016/12/03 01:14:04, vmpstr wrote:
> Can you comment that we need to detatch to ensure that it's bound to the
running
> sequence? (that's why we do this, right?)

Right, already have a comment to that effect on |destination_sequence_checker_|
(and this is generally always the reason why some classes detach in
constructor). Let me know if you still think one is needed here as well.

https://codereview.chromium.org/2491613004/diff/160001/base/timer/timer.cc#ne...
base/timer/timer.cc:104: // TODO(gab): Currently rebasing on Now() to match
prior status quo but
On 2016/12/03 01:14:04, vmpstr wrote:
> Can you file a bug for this and reference it here?

Done.

https://codereview.chromium.org/2491613004/diff/160001/base/timer/timer.cc#ne...
base/timer/timer.cc:111: delay_);
On 2016/12/03 01:14:04, vmpstr wrote:
> was_reset_ = false?

Done.

https://codereview.chromium.org/2491613004/diff/160001/base/timer/timer.cc#ne...
base/timer/timer.cc:130: // So overall, either the Reset() came in too late and
will be canceled or
On 2016/12/03 01:14:04, vmpstr wrote:
> This comment is a bit confusing to me. So does this mean that there is no
actual
> reset task, right? Since we just postpone the run task? (You reference "and
will
> be canceled via the WeakPtr", which is the confusing part... what exactly will
> be canceled?)
> 
> Or is this function called by the reset task created elsewhere? If that's the
> case, aren't we already running the reset task since we're in this function?
> Which means it won't be canceled?

This whole comment is just meant to say that although Reset() appears complex at
first sight it really isn't because either it never gets to run (per the
WeakPtr) or it merely needs to update |desired_run_time_|. 

I first tweaked it to:
    // Since this message is sent asynchronously, a few things might occur:
    //  - This is a one-shot timer and Run() hasn't occurred:
    //      postpone |desired_run_time_|, Run() will postpone itself when it
    //      runs.
    //  - This is a one-shot timer and Run() already occurred:
    //      Reset() came in too late and will be canceled via the WeakPtr.
    //  - This is a repeating timer and Run() has occurred zero to many times:
    //      postpone the next |desired_run_time_|.
    // So overall, either the Reset() task came in too late (and won't even
    // execute per the WeakPtr) or the |desired_run_time_| merely needs to be
    // postponed.

but then I decided that was probably overkill (i.e. adding confusion to an
otherwise simple outcome) and now made it:

    // Since Reset() is sequenced with Run() (and cancelled via the WeakPtr if
    // Run() is scheduled before it -- i.e. delay expires as Reset() is posted):
    // it is sufficient to merely update |desired_run_time_|.

https://codereview.chromium.org/2491613004/diff/160001/base/timer/timer.cc#ne...
base/timer/timer.cc:143: // Delete self, invalidating WeakPtrs and as such,
pending Run()/Reset()
On 2016/12/03 01:14:04, vmpstr wrote:
> Here as well, what's a reset task here?

Actually it can only cancel "the pending Run() task" (per invariant there's
always exactly one pending Run()).

It doesn't make sense for there to be a pending Reset() in the queue as Timer
doesn't post anything else to this BaseTimerTaskInternal after sending the
Abandon task.

Done.

https://codereview.chromium.org/2491613004/diff/160001/base/timer/timer.cc#ne...
base/timer/timer.cc:148: std::unique_ptr<CleanupTrigger> GetCleanupTrigger() {
On 2016/12/03 01:14:04, vmpstr wrote:
> nit: CreateCleanupTrigger

Done.

https://codereview.chromium.org/2491613004/diff/160001/base/timer/timer.cc#ne...
base/timer/timer.cc:238: if (origin_sequence_checker_.CalledOnValidSequence()) {
On 2016/12/03 01:14:04, vmpstr wrote:
> This is changing code behavior based on whether DCHECKs are on. The
> CalledOnValidSequence always returns true if DCHECKs are off... Is this
> expected? Can you drop a comment explaining here?

No it doesn't because the member is a SequenceCheckerImpl (there's already a
comment there but I'll also add one here to make this clear).

https://codereview.chromium.org/2491613004/diff/160001/base/timer/timer.cc#ne...
base/timer/timer.cc:261: // Do not allow changing the task runner while
something is been scheduled.
On 2016/12/03 01:14:04, vmpstr wrote:
> s/been //

Done.

https://codereview.chromium.org/2491613004/diff/160001/base/timer/timer.cc#ne...
base/timer/timer.cc:276: Reset();
On 2016/12/03 01:14:04, vmpstr wrote:
> Is this call always just PostNewScheduledTask?

Actually yes it is, inlined that instead and added DCHECK(!is_running_) to
PostNewScheduledTask().

https://codereview.chromium.org/2491613004/diff/160001/base/timer/timer.cc#ne...
base/timer/timer.cc:303: PostNewScheduledTask(delay_);
On 2016/12/03 01:14:04, vmpstr wrote:
> It's unclear to me if there other call sites other than Start that hit this,
can
> you comment when this is expected to hit?

The API is a bit of a mess but yes this is required to support an existing use
case, added comment:

    // This is required to handle the following use case (when
    // |retain_user_task_ ==true|): Start() => Stop() => Reset().

[gab, 2016-12-23 20:24:41]

Description was changed from

==========
Make base::Timer sequence-safe.

Fixes a race condition in the previous implementation.

Essentially a full re-write of timer.cc.

Of note in this CL:
 - Patch set 1 is https://codereview.chromium.org/1433373003
   so looking at diff from 1 might be less work (especially for tests).
 - Timer is now 100% sequence-affine.
 - Anything that needs to be handled on the destination task runner
   is handled by BaseTimerTaskInternal (which is also 100% sequence-
   affine on the destination sequence).
 - When origin == destination sequences, the two communicate synchronously
   as much as possible (to keep the characteristics of the previous
implementation
   without having to introduce locks).
 - Communications back-and-forth are WeakPtr bound which allows cancellation
   without reaching in each other's internals as before.
 - BaseTimerTaskInternal::CleanupTrigger is a trick to redo something which was
   previously supported directly by ~BaseTimerTaskInternal() (but it's no longer
   base::Owned() by Run() so something that actually is is required).
   - It is required to pass TimerTest.NonRepeatMessageLoopDeath
   - The modifications to sequenced_worker_pool.cc are required by it (or the
     WeakPtr is unhappy being dereferenced in the destruction of the Run()
Closure).

BUG=587199, 552633
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_tsan_rel_ng
==========

to

==========
Make base::Timer sequence-safe.

Fixes a race condition in the previous implementation.

Essentially a full re-write of timer.cc.

Of note in this CL:
 - Patch set 1 is https://codereview.chromium.org/1433373003
   so looking at diff from 1 might be less work (especially for tests).
 - Timer is now 100% sequence-affine.
 - Anything that needs to be handled on the destination task runner
   is handled by BaseTimerTaskInternal (which is also 100% sequence-
   affine on the destination sequence).
 - When origin == destination sequences, the two communicate synchronously
   as much as possible (to keep the characteristics of the previous
implementation
   without having to introduce locks).
 - Communications back-and-forth are WeakPtr bound which allows cancellation
   without reaching in each other's internals as before.
 - BaseTimerTaskInternal::CleanupTrigger is a trick to redo something which was
   previously supported directly by ~BaseTimerTaskInternal() (but it's no longer
   base::Owned() by Run() so something that actually is is required).
   - It is required to pass TimerTest.NonRepeatMessageLoopDeath

BUG=587199, 552633
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_tsan_rel_ng
==========

[commit-bot: I haz the power, 2016-12-23 20:45:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-23 20:45:23]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[gab, 2017-01-06 16:28:39]

@vmpstr PTAnL, thanks!

[gab, 2017-01-09 18:24:17]

Patchset #11 (id:220001) has been deleted

[gab, 2017-01-12 19:30:13]

Description was changed from

==========
Make base::Timer sequence-safe.

Fixes a race condition in the previous implementation.

Essentially a full re-write of timer.cc.

Of note in this CL:
 - Patch set 1 is https://codereview.chromium.org/1433373003
   so looking at diff from 1 might be less work (especially for tests).
 - Timer is now 100% sequence-affine.
 - Anything that needs to be handled on the destination task runner
   is handled by BaseTimerTaskInternal (which is also 100% sequence-
   affine on the destination sequence).
 - When origin == destination sequences, the two communicate synchronously
   as much as possible (to keep the characteristics of the previous
implementation
   without having to introduce locks).
 - Communications back-and-forth are WeakPtr bound which allows cancellation
   without reaching in each other's internals as before.
 - BaseTimerTaskInternal::CleanupTrigger is a trick to redo something which was
   previously supported directly by ~BaseTimerTaskInternal() (but it's no longer
   base::Owned() by Run() so something that actually is is required).
   - It is required to pass TimerTest.NonRepeatMessageLoopDeath

BUG=587199, 552633
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_tsan_rel_ng
==========

to

==========
Fix TaskRunner race in base::Timer and make it sequence-friendly.

Fixes a race condition in the previous implementation.

Essentially a full re-write of timer.cc.

Of note in this CL:
 - Patch set 1 is https://codereview.chromium.org/1433373003
   so looking at diff from 1 might be less work (especially for tests).
 - Timer is now 100% sequence-affine.
 - Anything that needs to be handled on the destination task runner
   is handled by BaseTimerTaskInternal (which is also 100% sequence-
   affine on the destination sequence).
 - When origin == destination sequences, the two communicate synchronously
   as much as possible (to keep the characteristics of the previous
implementation
   without having to introduce locks).
 - Communications back-and-forth are WeakPtr bound which allows cancellation
   without reaching in each other's internals as before.
 - BaseTimerTaskInternal::CleanupTrigger is a trick to redo something which was
   previously supported directly by ~BaseTimerTaskInternal() (but it's no longer
   base::Owned() by Run() so something that actually is is required).
   - It is required to pass TimerTest.NonRepeatMessageLoopDeath

BUG=587199, 552633
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_tsan_rel_ng
==========

[vmpstr, 2017-01-12 20:04:22]

looks good, just small nits

https://codereview.chromium.org/2491613004/diff/240001/base/timer/timer.cc
File base/timer/timer.cc (right):

https://codereview.chromium.org/2491613004/diff/240001/base/timer/timer.cc#ne...
base/timer/timer.cc:300: if (is_running_) {
I kinda prefer 

if (!is_running_) {
  PostNewScheduledTask(delay_);
  return;
}

...

I find that it makes it a bit more readable

https://codereview.chromium.org/2491613004/diff/240001/base/timer/timer.cc#ne...
base/timer/timer.cc:323: scoped_refptr<SequencedTaskRunner>
destination_task_runner =
Do you need a ref here? Can it be SequencedTaskRunner* with .get() on the rhs

https://codereview.chromium.org/2491613004/diff/240001/base/timer/timer.cc#ne...
base/timer/timer.cc:328: BaseTimerTaskInternal* scheduled_task = new
BaseTimerTaskInternal(
Can you add a comment saying that the task will delete itself?

https://codereview.chromium.org/2491613004/diff/240001/base/timer/timer.cc#ne...
base/timer/timer.cc:343: if (is_running_) {
if (!is_running_)
  return;

...

[gab, 2017-01-13 20:19:03]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-13 20:19:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-13 20:58:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-13 20:58:05]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[gab, 2017-01-13 22:05:10]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-13 22:05:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-13 22:43:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-13 22:43:39]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[gab, 2017-01-14 00:14:10]

Patchset #13 (id:280001) has been deleted

[gab, 2017-01-14 00:16:08]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-14 00:16:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-14 00:50:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-14 00:50:52]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[gab, 2017-01-23 03:58:14]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-23 03:58:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-23 04:32:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-23 04:32:31]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[gab, 2017-01-23 16:50:24]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-23 16:50:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gab, 2017-01-23 17:17:58]

Description was changed from

==========
Fix TaskRunner race in base::Timer and make it sequence-friendly.

Fixes a race condition in the previous implementation.

Essentially a full re-write of timer.cc.

Of note in this CL:
 - Patch set 1 is https://codereview.chromium.org/1433373003
   so looking at diff from 1 might be less work (especially for tests).
 - Timer is now 100% sequence-affine.
 - Anything that needs to be handled on the destination task runner
   is handled by BaseTimerTaskInternal (which is also 100% sequence-
   affine on the destination sequence).
 - When origin == destination sequences, the two communicate synchronously
   as much as possible (to keep the characteristics of the previous
implementation
   without having to introduce locks).
 - Communications back-and-forth are WeakPtr bound which allows cancellation
   without reaching in each other's internals as before.
 - BaseTimerTaskInternal::CleanupTrigger is a trick to redo something which was
   previously supported directly by ~BaseTimerTaskInternal() (but it's no longer
   base::Owned() by Run() so something that actually is is required).
   - It is required to pass TimerTest.NonRepeatMessageLoopDeath

BUG=587199, 552633
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_tsan_rel_ng
==========

to

==========
Fix TaskRunner race in base::Timer and make it sequence-friendly.

Fixes a race condition in the previous implementation.

Essentially a full re-write of timer.cc.

Of note in this CL:
 - Patch set 1 is https://codereview.chromium.org/1433373003
   so looking at diff from 1 might be less work (especially for tests).
 - Timer is now 100% sequence-affine.
 - Anything that needs to be handled on the destination task runner
   is handled by BaseTimerTaskInternal (which is also 100% sequence-
   affine on the destination sequence).
 - When origin == destination sequences, the two communicate synchronously
   as much as possible (to keep the characteristics of the previous
implementation
   without having to introduce locks).
 - Communications back-and-forth are WeakPtr bound which allows cancellation
   without reaching in each other's internals as before.
 - BaseTimerTaskInternal::CleanupTrigger is a trick to redo something which was
   previously supported directly by ~BaseTimerTaskInternal() (but it's no longer
   base::Owned() by Run() so something that actually is is required).
   - It is required to pass TimerTest.NonRepeatMessageLoopDeath

BUG=587199, 552633, 678592
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_tsan_rel_ng
==========

[gab, 2017-01-23 17:22:30]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-23 17:22:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-23 17:58:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-23 17:58:12]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[gab, 2017-01-23 21:45:45]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-23 21:46:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-23 21:54:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-23 21:54:56]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[gab, 2017-01-24 02:52:38]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-24 02:53:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-24 03:47:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-24 03:47:46]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[gab, 2017-01-24 18:42:05]

Description was changed from

==========
Fix TaskRunner race in base::Timer and make it sequence-friendly.

Fixes a race condition in the previous implementation.

Essentially a full re-write of timer.cc.

Of note in this CL:
 - Patch set 1 is https://codereview.chromium.org/1433373003
   so looking at diff from 1 might be less work (especially for tests).
 - Timer is now 100% sequence-affine.
 - Anything that needs to be handled on the destination task runner
   is handled by BaseTimerTaskInternal (which is also 100% sequence-
   affine on the destination sequence).
 - When origin == destination sequences, the two communicate synchronously
   as much as possible (to keep the characteristics of the previous
implementation
   without having to introduce locks).
 - Communications back-and-forth are WeakPtr bound which allows cancellation
   without reaching in each other's internals as before.
 - BaseTimerTaskInternal::CleanupTrigger is a trick to redo something which was
   previously supported directly by ~BaseTimerTaskInternal() (but it's no longer
   base::Owned() by Run() so something that actually is is required).
   - It is required to pass TimerTest.NonRepeatMessageLoopDeath

BUG=587199, 552633, 678592
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_tsan_rel_ng
==========

to

==========
Fix TaskRunner race in base::Timer and make it sequence-friendly.

Fixes a race condition in the previous implementation.

Essentially a full re-write of timer.cc.

Of note in this CL:
 - Patch set 1 is https://codereview.chromium.org/1433373003
   so looking at diff from 1 might be less work (especially for tests).
 - Timer is now 100% sequence-affine.
 - Anything that needs to be handled on the destination task runner
   is handled by BaseTimerTaskInternal (which is also 100% sequence-
   affine on the destination sequence).
 - When origin == destination sequences, the two communicate synchronously
   as much as possible (to keep the characteristics of the previous
implementation
   without having to introduce locks).
 - Communications back-and-forth are WeakPtr bound which allows cancellation
   without reaching in each other's internals as before.
 - BaseTimerTaskInternal::CleanupTrigger is a trick to redo something which was
   previously supported directly by ~BaseTimerTaskInternal() (but it's no longer
   base::Owned() by Run() so something that actually is is required).
   - It is required to pass TimerTest.NonRepeatMessageLoopDeath

BUG=587199, 552633, 678592, 684640
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_tsan_rel_ng
==========

[gab, 2017-01-24 21:58:50]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-24 21:59:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-24 22:36:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-24 22:36:19]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[gab, 2017-01-25 19:53:14]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-25 19:54:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gab, 2017-01-25 20:08:42]

Description was changed from

==========
Fix TaskRunner race in base::Timer and make it sequence-friendly.

Fixes a race condition in the previous implementation.

Essentially a full re-write of timer.cc.

Of note in this CL:
 - Patch set 1 is https://codereview.chromium.org/1433373003
   so looking at diff from 1 might be less work (especially for tests).
 - Timer is now 100% sequence-affine.
 - Anything that needs to be handled on the destination task runner
   is handled by BaseTimerTaskInternal (which is also 100% sequence-
   affine on the destination sequence).
 - When origin == destination sequences, the two communicate synchronously
   as much as possible (to keep the characteristics of the previous
implementation
   without having to introduce locks).
 - Communications back-and-forth are WeakPtr bound which allows cancellation
   without reaching in each other's internals as before.
 - BaseTimerTaskInternal::CleanupTrigger is a trick to redo something which was
   previously supported directly by ~BaseTimerTaskInternal() (but it's no longer
   base::Owned() by Run() so something that actually is is required).
   - It is required to pass TimerTest.NonRepeatMessageLoopDeath

BUG=587199, 552633, 678592, 684640
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_tsan_rel_ng
==========

to

==========
Fix TaskRunner race in base::Timer and make it sequence-friendly.

Fixes a race condition in the previous implementation when SetTaskRunner()
was used.

Of note in this CL:
 - Patch set 1 is https://codereview.chromium.org/1433373003
   so looking at diff from 1 might be less work (especially for tests).
 - The Timer's delayed task now always lives on the sequence it was started from
   (and even SetTaskRunner() was used, a task is posted to it when the delay
    expires instead of having the Timer's delayed task live on it -- this solves
    the aforementioned race condition).
   - This required adapting tests for MediaCodecLoop and UploadProgressTracker.

BUG=587199, 552633, 678592, 684640
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_tsan_rel_ng
==========

[gab, 2017-01-25 20:14:55]

gab@chromium.org changed reviewers:
+ csharrison@chromium.org, dalecurtis@chromium.org

[gab, 2017-01-25 20:14:56]

@vmpstr: assumptions baked in around the codebase made it hard to fix some tests
with previous impl.

I since then came to the realization that it would be a whole lot simpler
(fixing race) if the delayed task always lived in the origin sequence
(regardless of SetTaskRunner()) and be ran or re-posted when the delay expires
(much less of a rewrite of the whole impl than before :)).

PTAnL, thanks!


@csharrison PTaL at side-effects on UploadProgressTracker's tests.
@dalecurtis PTaL at side-effects on MediaCodecLoop's tests


Thanks!
Gab

https://codereview.chromium.org/2491613004/diff/240001/base/timer/timer.cc
File base/timer/timer.cc (right):

https://codereview.chromium.org/2491613004/diff/240001/base/timer/timer.cc#ne...
base/timer/timer.cc:300: if (is_running_) {
On 2017/01/12 20:04:22, vmpstr wrote:
> I kinda prefer 
> 
> if (!is_running_) {
>   PostNewScheduledTask(delay_);
>   return;
> }
> 
> ...
> 
> I find that it makes it a bit more readable

Agreed, done.

https://codereview.chromium.org/2491613004/diff/240001/base/timer/timer.cc#ne...
base/timer/timer.cc:323: scoped_refptr<SequencedTaskRunner>
destination_task_runner =
On 2017/01/12 20:04:22, vmpstr wrote:
> Do you need a ref here? Can it be SequencedTaskRunner* with .get() on the rhs

I had the same thought and had originally written it as a raw pointer but doing
so crashes because SequencedTaskRunnerHandle::Get() returns a ref as an r-value
(and in the case of SequencedWorkerPool that's actually a temp
SequencedTaskRunner object under the hood).

https://codereview.chromium.org/2491613004/diff/240001/base/timer/timer.cc#ne...
base/timer/timer.cc:328: BaseTimerTaskInternal* scheduled_task = new
BaseTimerTaskInternal(
On 2017/01/12 20:04:22, vmpstr wrote:
> Can you add a comment saying that the task will delete itself?

Done.

https://codereview.chromium.org/2491613004/diff/240001/base/timer/timer.cc#ne...
base/timer/timer.cc:343: if (is_running_) {
On 2017/01/12 20:04:22, vmpstr wrote:
> if (!is_running_)
>   return;
> 
> ...

Done.

[Charlie Harrison, 2017-01-25 20:28:25]

c/b/l lgtm

[DaleCurtis, 2017-01-25 22:33:25]

media/ lgtm

https://codereview.chromium.org/2491613004/diff/470001/media/base/android/med...
File media/base/android/media_codec_loop.h (right):

https://codereview.chromium.org/2491613004/diff/470001/media/base/android/med...
media/base/android/media_codec_loop.h:205:
scoped_refptr<base::SingleThreadTaskRunner> = nullptr);
Is the = nullptr even necessary? I didn't think there were a lot of callsites.

https://codereview.chromium.org/2491613004/diff/470001/media/base/android/med...
File media/base/android/media_codec_loop_unittest.cc (right):

https://codereview.chromium.org/2491613004/diff/470001/media/base/android/med...
media/base/android/media_codec_loop_unittest.cc:189: new
base::TestMockTimeTaskRunner;
()

[vmpstr, 2017-02-01 21:13:11]

lgtm

https://codereview.chromium.org/2491613004/diff/470001/base/timer/timer.cc
File base/timer/timer.cc (right):

https://codereview.chromium.org/2491613004/diff/470001/base/timer/timer.cc#ne...
base/timer/timer.cc:43: // *this will be deleted by the task runner, so Timer
needs to
nit: |this|

https://codereview.chromium.org/2491613004/diff/470001/base/timer/timer.cc#ne...
base/timer/timer.cc:110: if (is_running_) {
This block can be rewritten as

DCHECK(is_running_ || !scheduled_task_);
if (is_running_)
  Stop();


(else block with just a dcheck is kinda awkward)

[gab, 2017-02-07 19:35:26]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-07 19:36:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-07 19:36:18]

There were warnings when CQ was processing your CL:
 * CQ is not running the linux_chromium_tsan_rel_ng trybot, per your
CQ_INCLUDE_TRYBOTS flag request. That bot was already specified

[gab, 2017-02-07 19:43:43]

Description was changed from

==========
Fix TaskRunner race in base::Timer and make it sequence-friendly.

Fixes a race condition in the previous implementation when SetTaskRunner()
was used.

Of note in this CL:
 - Patch set 1 is https://codereview.chromium.org/1433373003
   so looking at diff from 1 might be less work (especially for tests).
 - The Timer's delayed task now always lives on the sequence it was started from
   (and even SetTaskRunner() was used, a task is posted to it when the delay
    expires instead of having the Timer's delayed task live on it -- this solves
    the aforementioned race condition).
   - This required adapting tests for MediaCodecLoop and UploadProgressTracker.

BUG=587199, 552633, 678592, 684640
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_tsan_rel_ng
==========

to

==========
Fix TaskRunner race in base::Timer and make it sequence-friendly.

Fixes a race condition in the previous implementation when SetTaskRunner()
was used.

Of note in this CL:
 - Patch set 1 is https://codereview.chromium.org/1433373003
   so looking at diff from 1 might be less work (especially for tests).
 - The Timer's delayed task now always lives on the sequence it was started from
   (and even SetTaskRunner() was used, a task is posted to it when the delay
    expires instead of having the Timer's delayed task live on it -- this solves
    the aforementioned race condition).
   - This required adapting tests for MediaCodecLoop and UploadProgressTracker.

BUG=587199, 552633, 678592, 684640
==========

[commit-bot: I haz the power, 2017-02-07 20:18:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-07 20:18:40]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[gab, 2017-02-13 19:39:37]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-13 19:40:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gab, 2017-02-13 19:52:05]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-13 19:53:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gab, 2017-02-13 19:55:03]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[gab, 2017-02-13 19:55:12]

Patchset #24 (id:510001) has been deleted

[commit-bot: I haz the power, 2017-02-13 19:56:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gab, 2017-02-13 19:57:00]

Applied comments (sorry for delay, still fighting with tests not happy with the
switch to running the delay from the origin task runner)

https://codereview.chromium.org/2491613004/diff/470001/base/timer/timer.cc
File base/timer/timer.cc (right):

https://codereview.chromium.org/2491613004/diff/470001/base/timer/timer.cc#ne...
base/timer/timer.cc:43: // *this will be deleted by the task runner, so Timer
needs to
On 2017/02/01 21:13:11, vmpstr wrote:
> nit: |this|

Done.

https://codereview.chromium.org/2491613004/diff/470001/base/timer/timer.cc#ne...
base/timer/timer.cc:110: if (is_running_) {
On 2017/02/01 21:13:11, vmpstr wrote:
> This block can be rewritten as
> 
> DCHECK(is_running_ || !scheduled_task_);
> if (is_running_)
>   Stop();
> 
> 
> (else block with just a dcheck is kinda awkward)

Done.

https://codereview.chromium.org/2491613004/diff/470001/media/base/android/med...
File media/base/android/media_codec_loop.h (right):

https://codereview.chromium.org/2491613004/diff/470001/media/base/android/med...
media/base/android/media_codec_loop.h:205:
scoped_refptr<base::SingleThreadTaskRunner> = nullptr);
On 2017/01/25 22:33:25, DaleCurtis wrote:
> Is the = nullptr even necessary? I didn't think there were a lot of callsites.

Guess not, made mandatory.

https://codereview.chromium.org/2491613004/diff/470001/media/base/android/med...
File media/base/android/media_codec_loop_unittest.cc (right):

https://codereview.chromium.org/2491613004/diff/470001/media/base/android/med...
media/base/android/media_codec_loop_unittest.cc:189: new
base::TestMockTimeTaskRunner;
On 2017/01/25 22:33:25, DaleCurtis wrote:
> ()

Why? It's valid C++ without the brackets.

[commit-bot: I haz the power, 2017-02-13 20:46:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-13 20:46:45]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[DaleCurtis, 2017-02-13 21:03:23]

https://codereview.chromium.org/2491613004/diff/470001/media/base/android/med...
File media/base/android/media_codec_loop_unittest.cc (right):

https://codereview.chromium.org/2491613004/diff/470001/media/base/android/med...
media/base/android/media_codec_loop_unittest.cc:189: new
base::TestMockTimeTaskRunner;
On 2017/02/13 at 19:57:00, gab wrote:
> On 2017/01/25 22:33:25, DaleCurtis wrote:
> > ()
> 
> Why? It's valid C++ without the brackets.

Consistency with surrounding code? I thought we had a style guide entry for it,
but I can't find it.

[danakj, 2017-02-13 21:46:46]

On Mon, Feb 13, 2017 at 4:03 PM, <dalecurtis@chromium.org> wrote:

>
> https://codereview.chromium.org/2491613004/diff/470001/
> media/base/android/media_codec_loop_unittest.cc
> File media/base/android/media_codec_loop_unittest.cc (right):
>
> https://codereview.chromium.org/2491613004/diff/470001/
> media/base/android/media_codec_loop_unittest.cc#newcode189
> media/base/android/media_codec_loop_unittest.cc:189: new
> base::TestMockTimeTaskRunner;
> On 2017/02/13 at 19:57:00, gab wrote:
> > On 2017/01/25 22:33:25, DaleCurtis wrote:
> > > ()
> >
> > Why? It's valid C++ without the brackets.
>
> Consistency with surrounding code? I thought we had a style guide entry
> for it, but I can't find it.
>

I usually ask for no (). It's noise other than if you want to initialize a
primitive type to 0.


>
> https://codereview.chromium.org/2491613004/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[gab, 2017-02-27 21:06:58]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-27 21:07:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-27 22:22:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-27 22:22:32]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[gab, 2017-03-02 01:22:44]

Finally managed to land https://codereview.chromium.org/2657013002/ which is
supposed unblock the last failures here *crossing-fingers*!

Taking another stab at CQ :)

[gab, 2017-03-02 01:22:57]

The CQ bit was checked by gab@chromium.org

[gab, 2017-03-02 01:22:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
csharrison@chromium.org, vmpstr@chromium.org, dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/2491613004/#ps550001
(title: "rebase on r453103 and issue 2657013002")

[commit-bot: I haz the power, 2017-03-02 01:23:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-02 02:26:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-02 02:26:18]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[gab, 2017-03-11 23:40:32]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-11 23:40:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-11 23:43:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-11 23:43:10]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[gab, 2017-03-12 00:26:24]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-12 00:26:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-12 01:17:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-12 01:17:03]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[gab, 2017-03-20 14:28:07]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-20 14:28:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-20 14:30:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-20 14:30:27]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[gab, 2017-03-20 14:39:30]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-20 14:39:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-20 15:27:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-20 15:27:48]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[gab, 2017-03-23 18:01:19]

Description was changed from

==========
Fix TaskRunner race in base::Timer and make it sequence-friendly.

Fixes a race condition in the previous implementation when SetTaskRunner()
was used.

Of note in this CL:
 - Patch set 1 is https://codereview.chromium.org/1433373003
   so looking at diff from 1 might be less work (especially for tests).
 - The Timer's delayed task now always lives on the sequence it was started from
   (and even SetTaskRunner() was used, a task is posted to it when the delay
    expires instead of having the Timer's delayed task live on it -- this solves
    the aforementioned race condition).
   - This required adapting tests for MediaCodecLoop and UploadProgressTracker.

BUG=587199, 552633, 678592, 684640
==========

to

==========
Fix TaskRunner race in base::Timer and make it sequence-friendly.

Fixes a race condition in the previous implementation when SetTaskRunner()
was used.

Of note in this CL:
 - Patch set 1 is https://codereview.chromium.org/1433373003
   so looking at diff from 1 might be less work (especially for tests).
 - The Timer's delayed task now always lives on the sequence it was started from
   (and even SetTaskRunner() was used, a task is posted to it when the delay
    expires instead of having the Timer's delayed task live on it -- this solves
    the aforementioned race condition).
   - This required adapting tests for MediaCodecLoop and UploadProgressTracker.

BUG=587199, 552633, 678592, 684640, 675631
==========

[gab, 2017-04-07 21:03:41]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 21:04:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-07 22:04:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-07 22:04:25]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[gab, 2017-04-26 16:47:59]

gab@chromium.org changed reviewers:
+ horo@chromium.org

[gab, 2017-04-26 16:48:00]

Still working on a few more tests but in the meantime

+horo for content/browser/service_worker/service_worker_browsertest.cc

Thanks,
Gab

[horo, 2017-04-27 02:33:12]

https://codereview.chromium.org/2491613004/diff/650001/content/browser/servic...
File content/browser/service_worker/service_worker_browsertest.cc (right):

https://codereview.chromium.org/2491613004/diff/650001/content/browser/servic...
content/browser/service_worker/service_worker_browsertest.cc:1174:
RunOnIOThreadWithDelay(
Keep this as is, and call "version_->timeout_timer_.IsRunning()" in
TimeoutWorkerOnIOThread()

[gab, 2017-04-27 14:56:04]

https://codereview.chromium.org/2491613004/diff/650001/content/browser/servic...
File content/browser/service_worker/service_worker_browsertest.cc (right):

https://codereview.chromium.org/2491613004/diff/650001/content/browser/servic...
content/browser/service_worker/service_worker_browsertest.cc:1174:
RunOnIOThreadWithDelay(
On 2017/04/27 02:33:12, horo wrote:
> Keep this as is, and call "version_->timeout_timer_.IsRunning()" in
> TimeoutWorkerOnIOThread()

RunOnIOThreadWithDelay is an anonymous method, it doesn't have access to
|version_|.

[horo, 2017-04-27 15:49:53]

https://codereview.chromium.org/2491613004/diff/650001/content/browser/servic...
File content/browser/service_worker/service_worker_browsertest.cc (right):

https://codereview.chromium.org/2491613004/diff/650001/content/browser/servic...
content/browser/service_worker/service_worker_browsertest.cc:666:
version_->SimulatePingTimeoutForTesting();
Add EXPECT_TRUE(version_->timeout_timer_.IsRunning());

https://codereview.chromium.org/2491613004/diff/650001/content/browser/servic...
content/browser/service_worker/service_worker_browsertest.cc:1174:
RunOnIOThreadWithDelay(
On 2017/04/27 14:56:04, gab wrote:
> On 2017/04/27 02:33:12, horo wrote:
> > Keep this as is, and call "version_->timeout_timer_.IsRunning()" in
> > TimeoutWorkerOnIOThread()
> 
> RunOnIOThreadWithDelay is an anonymous method, it doesn't have access to
> |version_|.

TimeoutWorkerOnIOThread() have access to |version_|.

[fdoray, 2017-05-10 14:22:56]

fdoray@chromium.org changed reviewers:
+ fdoray@chromium.org

[fdoray, 2017-05-10 14:22:58]

https://codereview.chromium.org/2491613004/diff/650001/base/timer/timer.cc
File base/timer/timer.cc (right):

https://codereview.chromium.org/2491613004/diff/650001/base/timer/timer.cc#ne...
base/timer/timer.cc:126: // Do not allow changing the task runner once something
has been scheduled.
... when the Timer is running.

Because it is possible to change the TaskRunner after Start()+Stop().

https://codereview.chromium.org/2491613004/diff/650001/base/timer/timer.cc#ne...
base/timer/timer.cc:186: // We can use the existing scheduled task if it arrives
before the new
In a world where TaskScheduler is able to ignore canceled tasks, reusing a task
that will run before the delay and posting a second task to wait the remaining
delay from there = one unnecessary wake up. It would have been better to cancel
the existing task and post the second task right away.

Timer would also be simpler without this "optimization".

https://codereview.chromium.org/2491613004/diff/650001/base/timer/timer.cc#ne...
base/timer/timer.cc:207: scheduled_task_ = new BaseTimerTaskInternal(this);
Instead of having a BaseTimerTaskInternal, simply post a
Timer::RunScheduledTask() task bound to a weak ptr to this? That would simplify
the code and would allow TaskScheduler/MessageLoop to know when tasks are
canceled.

https://codereview.chromium.org/2491613004/diff/650001/base/timer/timer.h
File base/timer/timer.h (right):

https://codereview.chromium.org/2491613004/diff/650001/base/timer/timer.h#new...
base/timer/timer.h:35: // Both OneShotTimer and RepeatingTimer also support a
Reset method, which
Observation: PostDelayedTask + WeakPtr or CancelableTaskTracker is equivalent to
OneShotTimer (except they don't provide IsRunning()/GetCurrentDelay()). Should
we provide guidance on which API to use?

https://codereview.chromium.org/2491613004/diff/650001/base/timer/timer.h#new...
base/timer/timer.h:42: // These APIs are not thread safe. Always call from the
same sequenced task
Address thread safety here instead of here + at line 77:

These APIs are not thread safe. All methods must be called from the same
sequence (not necessarily the construction sequence), except for the destructor
and SetTaskRunner() which may be called from any sequence when the timer is not
running (i.e. when Start() has never been called or Stop() has been called since
the last Start()). By default, the scheduled tasks will be run on the same
sequence that the Timer *was started*, but this can be changed prior *to
Start()* via SetTaskRunner().

https://codereview.chromium.org/2491613004/diff/650001/base/timer/timer.h#new...
base/timer/timer.h:63: #include "base/sequence_checker_impl.h"
sequence_checker.h

[gab, 2017-05-26 04:26:58]

Finally looping back to this! Can't land it as-is yet though as I still lack the
ability to RunLoop from a TestMockTimeTaskRunner (almost there!) required to fix
the last few failures this CL highlights.

I'll go ahead and forward the existing test improvements to another CL however.

And I'll also forward the sequences tests as well as making Timer
sequence-friendly to a precursor CLs (the tests will be racy and it will require
a TSAN suppression but this is an existing race that just wasn't caught by
previous tests... so there's no use blocking sequencification on that).

https://codereview.chromium.org/2491613004/diff/650001/base/timer/timer.cc
File base/timer/timer.cc (right):

https://codereview.chromium.org/2491613004/diff/650001/base/timer/timer.cc#ne...
base/timer/timer.cc:126: // Do not allow changing the task runner once something
has been scheduled.
On 2017/05/10 14:22:58, fdoray wrote:
> ... when the Timer is running.
> 
> Because it is possible to change the TaskRunner after Start()+Stop().

Done.

https://codereview.chromium.org/2491613004/diff/650001/base/timer/timer.cc#ne...
base/timer/timer.cc:186: // We can use the existing scheduled task if it arrives
before the new
On 2017/05/10 14:22:58, fdoray wrote:
> In a world where TaskScheduler is able to ignore canceled tasks, reusing a
task
> that will run before the delay and posting a second task to wait the remaining
> delay from there = one unnecessary wake up. It would have been better to
cancel
> the existing task and post the second task right away.
> 
> Timer would also be simpler without this "optimization".

I agree, but that's a behaviour change that belongs in another CL once we do
support that.

https://codereview.chromium.org/2491613004/diff/650001/base/timer/timer.cc#ne...
base/timer/timer.cc:207: scheduled_task_ = new BaseTimerTaskInternal(this);
On 2017/05/10 14:22:58, fdoray wrote:
> Instead of having a BaseTimerTaskInternal, simply post a
> Timer::RunScheduledTask() task bound to a weak ptr to this? That would
simplify
> the code and would allow TaskScheduler/MessageLoop to know when tasks are
> canceled.

Again, this can definitely be improved with WeakPtrs and all, but that isn't the
purpose of this CL.

https://codereview.chromium.org/2491613004/diff/650001/base/timer/timer.h
File base/timer/timer.h (right):

https://codereview.chromium.org/2491613004/diff/650001/base/timer/timer.h#new...
base/timer/timer.h:35: // Both OneShotTimer and RepeatingTimer also support a
Reset method, which
On 2017/05/10 14:22:58, fdoray wrote:
> Observation: PostDelayedTask + WeakPtr or CancelableTaskTracker is equivalent
to
> OneShotTimer (except they don't provide IsRunning()/GetCurrentDelay()). Should
> we provide guidance on which API to use?

Interesting, would have to think about this, suggestions welcome :)

https://codereview.chromium.org/2491613004/diff/650001/base/timer/timer.h#new...
base/timer/timer.h:42: // These APIs are not thread safe. Always call from the
same sequenced task
On 2017/05/10 14:22:58, fdoray wrote:
> Address thread safety here instead of here + at line 77:
> 
> These APIs are not thread safe. All methods must be called from the same
> sequence (not necessarily the construction sequence), except for the
destructor
> and SetTaskRunner() which may be called from any sequence when the timer is
not
> running (i.e. when Start() has never been called or Stop() has been called
since
> the last Start()). By default, the scheduled tasks will be run on the same
> sequence that the Timer *was started*, but this can be changed prior *to
> Start()* via SetTaskRunner().

Done.

https://codereview.chromium.org/2491613004/diff/650001/base/timer/timer.h#new...
base/timer/timer.h:63: #include "base/sequence_checker_impl.h"
On 2017/05/10 14:22:58, fdoray wrote:
> sequence_checker.h

Done.

https://codereview.chromium.org/2491613004/diff/650001/content/browser/servic...
File content/browser/service_worker/service_worker_browsertest.cc (right):

https://codereview.chromium.org/2491613004/diff/650001/content/browser/servic...
content/browser/service_worker/service_worker_browsertest.cc:666:
version_->SimulatePingTimeoutForTesting();
On 2017/04/27 15:49:52, horo wrote:
> Add EXPECT_TRUE(version_->timeout_timer_.IsRunning());

Done.

https://codereview.chromium.org/2491613004/diff/650001/content/browser/servic...
content/browser/service_worker/service_worker_browsertest.cc:1174:
RunOnIOThreadWithDelay(
On 2017/04/27 15:49:52, horo wrote:
> On 2017/04/27 14:56:04, gab wrote:
> > On 2017/04/27 02:33:12, horo wrote:
> > > Keep this as is, and call "version_->timeout_timer_.IsRunning()" in
> > > TimeoutWorkerOnIOThread()
> > 
> > RunOnIOThreadWithDelay is an anonymous method, it doesn't have access to
> > |version_|.
> 
> TimeoutWorkerOnIOThread() have access to |version_|.

Done.

[gab, 2017-05-26 04:32:37]

Patchset #31 (id:670001) has been deleted

[gab, 2017-05-26 05:02:06]

Description was changed from

==========
Fix TaskRunner race in base::Timer and make it sequence-friendly.

Fixes a race condition in the previous implementation when SetTaskRunner()
was used.

Of note in this CL:
 - Patch set 1 is https://codereview.chromium.org/1433373003
   so looking at diff from 1 might be less work (especially for tests).
 - The Timer's delayed task now always lives on the sequence it was started from
   (and even SetTaskRunner() was used, a task is posted to it when the delay
    expires instead of having the Timer's delayed task live on it -- this solves
    the aforementioned race condition).
   - This required adapting tests for MediaCodecLoop and UploadProgressTracker.

BUG=587199, 552633, 678592, 684640, 675631
==========

to

==========
Fix base::Timer sequence-friendly.

This CL was also going to fix a race condition in the existing implementation
when SetTaskRunner() was used but the test-only failures caused by tests having
long-lived 
with the status quo are proving hard than expected to fix. The race is no worse
than
before by making this code sequence-friendly (and generally healthier). Will fix
race in a follow-up CL.

Of note in this CL:
 - Patch set 1 is https://codereview.chromium.org/1433373003
   so looking at diff from 1 might be less work (especially for tests).
 - (not true anymore, postponed to race fix CL:) 
   The Timer's delayed task now always lives on the sequence it was started from
   (and even SetTaskRunner() was used, a task is posted to it when the delay
    expires instead of having the Timer's delayed task live on it -- this solves
    the aforementioned race condition).
   - This required adapting tests for MediaCodecLoop and UploadProgressTracker.

BUG=587199, 552633, 678592, 684640, 675631
==========

[gab, 2017-05-26 05:02:25]

Description was changed from

==========
Fix base::Timer sequence-friendly.

This CL was also going to fix a race condition in the existing implementation
when SetTaskRunner() was used but the test-only failures caused by tests having
long-lived 
with the status quo are proving hard than expected to fix. The race is no worse
than
before by making this code sequence-friendly (and generally healthier). Will fix
race in a follow-up CL.

Of note in this CL:
 - Patch set 1 is https://codereview.chromium.org/1433373003
   so looking at diff from 1 might be less work (especially for tests).
 - (not true anymore, postponed to race fix CL:) 
   The Timer's delayed task now always lives on the sequence it was started from
   (and even SetTaskRunner() was used, a task is posted to it when the delay
    expires instead of having the Timer's delayed task live on it -- this solves
    the aforementioned race condition).
   - This required adapting tests for MediaCodecLoop and UploadProgressTracker.

BUG=587199, 552633, 678592, 684640, 675631
==========

to

==========
Make base::Timer sequence-friendly.

This CL was also going to fix a race condition in the existing implementation
when SetTaskRunner() was used but the test-only failures caused by tests having
long-lived 
with the status quo are proving hard than expected to fix. The race is no worse
than
before by making this code sequence-friendly (and generally healthier). Will fix
race in a follow-up CL.

Of note in this CL:
 - Patch set 1 is https://codereview.chromium.org/1433373003
   so looking at diff from 1 might be less work (especially for tests).
 - (not true anymore, postponed to race fix CL:) 
   The Timer's delayed task now always lives on the sequence it was started from
   (and even SetTaskRunner() was used, a task is posted to it when the delay
    expires instead of having the Timer's delayed task live on it -- this solves
    the aforementioned race condition).
   - This required adapting tests for MediaCodecLoop and UploadProgressTracker.

BUG=587199, 552633, 678592, 684640, 675631
==========

[gab, 2017-05-26 05:10:00]

Decided to re-use this CL as most of it is still good. Merely need to
re-introduce the pre-existing race sadly as landing this is becoming urgent
(blocks TaskScheduler migration) and the race is no worse in the
sequence-friendly world (merely more highlighted by the introduced tests... for
which I added temporary tsan suppressions).

TBR horo for service_worker/

[gab, 2017-05-26 05:10:15]

Description was changed from

==========
Make base::Timer sequence-friendly.

This CL was also going to fix a race condition in the existing implementation
when SetTaskRunner() was used but the test-only failures caused by tests having
long-lived 
with the status quo are proving hard than expected to fix. The race is no worse
than
before by making this code sequence-friendly (and generally healthier). Will fix
race in a follow-up CL.

Of note in this CL:
 - Patch set 1 is https://codereview.chromium.org/1433373003
   so looking at diff from 1 might be less work (especially for tests).
 - (not true anymore, postponed to race fix CL:) 
   The Timer's delayed task now always lives on the sequence it was started from
   (and even SetTaskRunner() was used, a task is posted to it when the delay
    expires instead of having the Timer's delayed task live on it -- this solves
    the aforementioned race condition).
   - This required adapting tests for MediaCodecLoop and UploadProgressTracker.

BUG=587199, 552633, 678592, 684640, 675631
==========

to

==========
Make base::Timer sequence-friendly.

This CL was also going to fix a race condition in the existing implementation
when SetTaskRunner() was used but the test-only failures caused by tests having
long-lived 
with the status quo are proving hard than expected to fix. The race is no worse
than
before by making this code sequence-friendly (and generally healthier). Will fix
race in a follow-up CL.

Of note in this CL:
 - Patch set 1 is https://codereview.chromium.org/1433373003
   so looking at diff from 1 might be less work (especially for tests).
 - (not true anymore, postponed to race fix CL:) 
   The Timer's delayed task now always lives on the sequence it was started from
   (and even SetTaskRunner() was used, a task is posted to it when the delay
    expires instead of having the Timer's delayed task live on it -- this solves
    the aforementioned race condition).
   - This required adapting tests for MediaCodecLoop and UploadProgressTracker.

BUG=587199, 552633, 678592, 684640, 675631
TBR=horo
==========

[gab, 2017-05-26 05:10:25]

The CQ bit was checked by gab@chromium.org

[gab, 2017-05-26 05:10:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
csharrison@chromium.org, vmpstr@chromium.org, dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/2491613004/#ps730001
(title: "reintroduce pre-existing race to land core of this CL which doesn't
worsen the situation")

[commit-bot: I haz the power, 2017-05-26 05:11:10]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gab, 2017-05-26 05:18:30]

Patchset #34 (id:750001) has been deleted

[gab, 2017-05-26 05:19:12]

The CQ bit was checked by gab@chromium.org

[gab, 2017-05-26 05:19:13]

The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org,
csharrison@chromium.org, dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/2491613004/#ps730002
(title: "disable many new sequence checks per the pre-existing race triggering
them...")

[commit-bot: I haz the power, 2017-05-26 05:20:05]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-26 06:07:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-26 06:07:56]

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_...)

[gab, 2017-05-26 15:07:55]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-26 15:08:15]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-26 16:05:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-26 16:05:14]

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_...)

[horo, 2017-05-29 10:38:33]

lgtm

[gab, 2017-05-29 16:54:23]

Description was changed from

==========
Make base::Timer sequence-friendly.

This CL was also going to fix a race condition in the existing implementation
when SetTaskRunner() was used but the test-only failures caused by tests having
long-lived 
with the status quo are proving hard than expected to fix. The race is no worse
than
before by making this code sequence-friendly (and generally healthier). Will fix
race in a follow-up CL.

Of note in this CL:
 - Patch set 1 is https://codereview.chromium.org/1433373003
   so looking at diff from 1 might be less work (especially for tests).
 - (not true anymore, postponed to race fix CL:) 
   The Timer's delayed task now always lives on the sequence it was started from
   (and even SetTaskRunner() was used, a task is posted to it when the delay
    expires instead of having the Timer's delayed task live on it -- this solves
    the aforementioned race condition).
   - This required adapting tests for MediaCodecLoop and UploadProgressTracker.

BUG=587199, 552633, 678592, 684640, 675631
TBR=horo
==========

to

==========
Make base::Timer sequence-friendly.

This CL was also going to fix a race condition in the existing implementation
when SetTaskRunner() was used but the test-only failures caused by tests having
long-lived 
with the status quo are proving hard than expected to fix. The race is no worse
than
before by making this code sequence-friendly (and generally healthier). Will fix
race in a follow-up CL.

Of note in this CL:
 - Patch set 1 is https://codereview.chromium.org/1433373003
   so looking at diff from 1 might be less work (especially for tests).
 - (not true anymore, postponed to race fix CL:) 
   The Timer's delayed task now always lives on the sequence it was started from
   (and even SetTaskRunner() was used, a task is posted to it when the delay
    expires instead of having the Timer's delayed task live on it -- this solves
    the aforementioned race condition).
   - This required adapting tests for MediaCodecLoop and UploadProgressTracker.

BUG=587199, 552633, 678592, 684640, 675631
==========

[gab, 2017-05-31 19:49:52]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[gab, 2017-05-31 19:49:57]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-31 19:50:38]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gab, 2017-05-31 20:23:37]

I'm also opting to revert changes to Stop/Abandon for now. After re-reading this
change yet another time I became leery of unconditionally abandoning task in
Stop(). I thought of a better way but another CL is better suited for that.

[gab, 2017-05-31 20:24:22]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[gab, 2017-05-31 20:24:27]

The CQ bit was checked by gab@chromium.org

[gab, 2017-05-31 20:24:28]

The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org,
csharrison@chromium.org, dalecurtis@chromium.org, horo@chromium.org
Link to the patchset: https://codereview.chromium.org/2491613004/#ps820001
(title: "revert changes to Stop/Abandon behaviour for now")

[commit-bot: I haz the power, 2017-05-31 20:25:15]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gab, 2017-05-31 20:29:53]

The CQ bit was checked by gab@chromium.org to run a CQ dry run

[gab, 2017-05-31 20:29:58]

The CQ bit was checked by gab@chromium.org

[gab, 2017-05-31 20:29:59]

The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org,
csharrison@chromium.org, dalecurtis@chromium.org, horo@chromium.org
Link to the patchset: https://codereview.chromium.org/2491613004/#ps840001
(title: "add back commented out DCHECK in Stop()")

[commit-bot: I haz the power, 2017-05-31 20:30:47]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-31 22:24:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-31 22:24:29]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[gab, 2017-06-01 13:59:56]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2017-06-01 14:00:12]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-01 16:23:39]

CQ is committing da patch.

Bot data: {"patchset_id": 840001, "attempt_start_ts": 1496325596484910,
"parent_rev": "93f44befa96838a3cff744431f2d3c0b05e70be2", "commit_rev":
"4e844bb5f686dbd3a062c6aaf83c8a1df171a08a"}

[commit-bot: I haz the power, 2017-06-01 16:23:51]

Description was changed from

==========
Make base::Timer sequence-friendly.

This CL was also going to fix a race condition in the existing implementation
when SetTaskRunner() was used but the test-only failures caused by tests having
long-lived 
with the status quo are proving hard than expected to fix. The race is no worse
than
before by making this code sequence-friendly (and generally healthier). Will fix
race in a follow-up CL.

Of note in this CL:
 - Patch set 1 is https://codereview.chromium.org/1433373003
   so looking at diff from 1 might be less work (especially for tests).
 - (not true anymore, postponed to race fix CL:) 
   The Timer's delayed task now always lives on the sequence it was started from
   (and even SetTaskRunner() was used, a task is posted to it when the delay
    expires instead of having the Timer's delayed task live on it -- this solves
    the aforementioned race condition).
   - This required adapting tests for MediaCodecLoop and UploadProgressTracker.

BUG=587199, 552633, 678592, 684640, 675631
==========

to

==========
Make base::Timer sequence-friendly.

This CL was also going to fix a race condition in the existing implementation
when SetTaskRunner() was used but the test-only failures caused by tests having
long-lived 
with the status quo are proving hard than expected to fix. The race is no worse
than
before by making this code sequence-friendly (and generally healthier). Will fix
race in a follow-up CL.

Of note in this CL:
 - Patch set 1 is https://codereview.chromium.org/1433373003
   so looking at diff from 1 might be less work (especially for tests).
 - (not true anymore, postponed to race fix CL:) 
   The Timer's delayed task now always lives on the sequence it was started from
   (and even SetTaskRunner() was used, a task is posted to it when the delay
    expires instead of having the Timer's delayed task live on it -- this solves
    the aforementioned race condition).
   - This required adapting tests for MediaCodecLoop and UploadProgressTracker.

BUG=587199, 552633, 678592, 684640, 675631

Review-Url: https://codereview.chromium.org/2491613004
Cr-Commit-Position: refs/heads/master@{#476317}
Committed:
https://chromium.googlesource.com/chromium/src/+/4e844bb5f686dbd3a062c6aaf83c...
==========

[commit-bot: I haz the power, 2017-06-01 16:23:55]

Committed patchset #38 (id:840001) as
https://chromium.googlesource.com/chromium/src/+/4e844bb5f686dbd3a062c6aaf83c...

[findit-for-me, 2017-06-01 17:33:39]

Findit (https://goo.gl/kROfz5) identified this CL at revision 476317 as the
culprit for
failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...

[gab, 2017-06-01 17:58:12]

On 2017/06/01 17:33:39, findit-for-me wrote:
> Findit (https://goo.gl/kROfz5) identified this CL at revision 476317 as the
> culprit for
> failures in the build cycles as shown on:
>
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...

This seems very unlikely to me... looking...

[gab, 2017-06-01 18:09:06]

On 2017/06/01 17:58:12, gab wrote:
> On 2017/06/01 17:33:39, findit-for-me wrote:
> > Findit (https://goo.gl/kROfz5) identified this CL at revision 476317 as the
> > culprit for
> > failures in the build cycles as shown on:
> >
>
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...
> 
> This seems very unlikely to me... looking...

Ah, this CL indeed made TSAN tests fail
(TimerSequenceTest.OneShotTimerUsedAndTaskedOnDifferentPools), but the linked
failed build also had an unrelated content_browsertest failure which I thought
was initially what was being blamed.

This is just a missing TSAN suppression in this CL (same reason I added the
other ones -- the pre-existing race wasn't fixed, the new tests highlight
it...).

[gab, 2017-06-01 18:11:29]

On 2017/06/01 18:09:06, gab wrote:
> On 2017/06/01 17:58:12, gab wrote:
> > On 2017/06/01 17:33:39, findit-for-me wrote:
> > > Findit (https://goo.gl/kROfz5) identified this CL at revision 476317 as
the
> > > culprit for
> > > failures in the build cycles as shown on:
> > >
> >
>
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...
> > 
> > This seems very unlikely to me... looking...
> 
> Ah, this CL indeed made TSAN tests fail
> (TimerSequenceTest.OneShotTimerUsedAndTaskedOnDifferentPools), but the linked
> failed build also had an unrelated content_browsertest failure which I thought
> was initially what was being blamed.
> 
> This is just a missing TSAN suppression in this CL (same reason I added the
> other ones -- the pre-existing race wasn't fixed, the new tests highlight
> it...).

I'm landing the fix right now, no need to rever this.

[gab, 2017-06-01 18:11:33]

On 2017/06/01 18:09:06, gab wrote:
> On 2017/06/01 17:58:12, gab wrote:
> > On 2017/06/01 17:33:39, findit-for-me wrote:
> > > Findit (https://goo.gl/kROfz5) identified this CL at revision 476317 as
the
> > > culprit for
> > > failures in the build cycles as shown on:
> > >
> >
>
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...
> > 
> > This seems very unlikely to me... looking...
> 
> Ah, this CL indeed made TSAN tests fail
> (TimerSequenceTest.OneShotTimerUsedAndTaskedOnDifferentPools), but the linked
> failed build also had an unrelated content_browsertest failure which I thought
> was initially what was being blamed.
> 
> This is just a missing TSAN suppression in this CL (same reason I added the
> other ones -- the pre-existing race wasn't fixed, the new tests highlight
> it...).

I'm landing the fix right now, no need to revert this.