Expose Timer::AbandonAndStop, so it can safely be destroyed on another thread.

Expose Timer::AbandonAndStop, so it can safely be destroyed on another thread.

BUG=chromium:736268
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2958003002
Cr-Commit-Position: refs/heads/master@{#484507}
Committed: https://chromium.googlesource.com/chromium/src/+/87340af883f06c363febe752245c1ab1a926a234

[ossu-chromium, 2017-06-27 11:30:46]

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

[commit-bot: I haz the power, 2017-06-27 11:30:58]

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-06-27 11:56:28]

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

[commit-bot: I haz the power, 2017-06-27 11:56:28]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[ossu-chromium, 2017-06-27 13:03:43]

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

[commit-bot: I haz the power, 2017-06-27 13:03:49]

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-06-27 13:28:04]

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

[commit-bot: I haz the power, 2017-06-27 13:28:04]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[ossu-chromium, 2017-06-27 15:36:29]

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

[commit-bot: I haz the power, 2017-06-27 15:36:37]

Dry run: CQ is trying da patch.

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

[ossu-chromium, 2017-06-27 15:37:01]

The CQ bit was unchecked by ossu@chromium.org

[ossu-chromium, 2017-06-27 15:37:10]

Patchset #3 (id:40001) has been deleted

[ossu-chromium, 2017-06-27 15:47:56]

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

[ossu-chromium, 2017-06-27 15:47:57]

As I was adding a Timer to a class to do some repeating checks, I stumbled on a
problem where tsan would complain if I destroyed that timer (although stopped)
in the destructor of my class. I initially worked around that by putting the
Timer in an Optional and destroying it directly after stopping, but got some
pushback.

The problem I was seeing was race between BaseTimerTaskInternal::Run and
BaseTimerTaskInternal::Abandon - see the bug. The comment at the top of timer.h
seems to indicate that destroying a stopped Timer should be fine on any thread.

This change makes all Stops also abandon the task, so during destruction,
there's no Abandon call that may interfere with Run.

PTAL!

[gab, 2017-06-27 21:12:23]

https://codereview.chromium.org/2958003002/diff/20001/base/timer/timer.cc
File base/timer/timer.cc (right):

https://codereview.chromium.org/2958003002/diff/20001/base/timer/timer.cc#new...
base/timer/timer.cc:149: origin_sequence_checker_.DetachFromSequence();
This should allow deletion to happen on any sequence.

If abandoning the task is required to happen on sequence you want to have
|retain_user_task_ = false| (see 151) which is the default.

[ossu-chromium, 2017-06-28 14:21:33]

https://codereview.chromium.org/2958003002/diff/20001/base/timer/timer.cc
File base/timer/timer.cc (right):

https://codereview.chromium.org/2958003002/diff/20001/base/timer/timer.cc#new...
base/timer/timer.cc:149: origin_sequence_checker_.DetachFromSequence();
On 2017/06/27 21:12:23, gab (slow Tue-Wed) wrote:
> This should allow deletion to happen on any sequence.
> 
> If abandoning the task is required to happen on sequence you want to have
> |retain_user_task_ = false| (see 151) which is the default.

Detaching the checker works fine - the problem I'm seeing is a race between
lines 40 and 54 in this file: if (timer_) in the scheduled task and timer_ =
nullptr; in Abandon (called in a couple of steps from ~Timer).

I tried only abandoning the scheduled task if retain_user_task_ but
unfortunately, RepeatingTimer retains the user task and the change will do
nothing for my problem.

I'm honestly not sure if I should just destroy my RepeatingTimer on the same
sequence as its tasks run, or if I should try to find a way to fix this race, or
if the race is "fine" and should add a suppression.

[gab, 2017-07-05 02:36:15]

https://codereview.chromium.org/2958003002/diff/20001/base/timer/timer.cc
File base/timer/timer.cc (right):

https://codereview.chromium.org/2958003002/diff/20001/base/timer/timer.cc#new...
base/timer/timer.cc:149: origin_sequence_checker_.DetachFromSequence();
On 2017/06/28 14:21:33, ossu-chromium wrote:
> On 2017/06/27 21:12:23, gab (slow Tue-Wed) wrote:
> > This should allow deletion to happen on any sequence.
> > 
> > If abandoning the task is required to happen on sequence you want to have
> > |retain_user_task_ = false| (see 151) which is the default.
> 
> Detaching the checker works fine - the problem I'm seeing is a race between
> lines 40 and 54 in this file: if (timer_) in the scheduled task and timer_ =
> nullptr; in Abandon (called in a couple of steps from ~Timer).
> 
> I tried only abandoning the scheduled task if retain_user_task_ but
> unfortunately, RepeatingTimer retains the user task and the change will do
> nothing for my problem.
> 
> I'm honestly not sure if I should just destroy my RepeatingTimer on the same
> sequence as its tasks run, or if I should try to find a way to fix this race,
or
> if the race is "fine" and should add a suppression. 

I see, should we just publicly expose AbandonAndStop() for users who truly want
this (and update the erroneous Timer docs you mentioned)? The current
base::Timer impl explicitly tries to avoid trashing by re-using the delayed task
and I'm leery to change that without a thorough analysis.

[ossu-chromium, 2017-07-05 10:58:12]

https://codereview.chromium.org/2958003002/diff/20001/base/timer/timer.cc
File base/timer/timer.cc (right):

https://codereview.chromium.org/2958003002/diff/20001/base/timer/timer.cc#new...
base/timer/timer.cc:149: origin_sequence_checker_.DetachFromSequence();
On 2017/07/05 02:36:14, gab (in-out til july17) wrote:
> On 2017/06/28 14:21:33, ossu-chromium wrote:
> > On 2017/06/27 21:12:23, gab (slow Tue-Wed) wrote:
> > > This should allow deletion to happen on any sequence.
> > > 
> > > If abandoning the task is required to happen on sequence you want to have
> > > |retain_user_task_ = false| (see 151) which is the default.
> > 
> > Detaching the checker works fine - the problem I'm seeing is a race between
> > lines 40 and 54 in this file: if (timer_) in the scheduled task and timer_ =
> > nullptr; in Abandon (called in a couple of steps from ~Timer).
> > 
> > I tried only abandoning the scheduled task if retain_user_task_ but
> > unfortunately, RepeatingTimer retains the user task and the change will do
> > nothing for my problem.
> > 
> > I'm honestly not sure if I should just destroy my RepeatingTimer on the same
> > sequence as its tasks run, or if I should try to find a way to fix this
race,
> or
> > if the race is "fine" and should add a suppression. 
> 
> I see, should we just publicly expose AbandonAndStop() for users who truly
want
> this (and update the erroneous Timer docs you mentioned)? The current
> base::Timer impl explicitly tries to avoid trashing by re-using the delayed
task
> and I'm leery to change that without a thorough analysis.

Sounds good. It's likely that the change in the current patch set would have
unforeseen consequences. I'll make AbandonAndStop public, update the docs and
change my timer to use AbandonAndStop instead.

[ossu-chromium, 2017-07-05 16:06:53]

Description was changed from

==========
Abandon timer task on Stop, so it can safely be destroyed on another thread.

BUG=chromium:736268
==========

to

==========
Abandon timer task on Stop, so it can safely be destroyed on another thread.

BUG=chromium:736268
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[ossu-chromium, 2017-07-05 16:07:52]

Description was changed from

==========
Abandon timer task on Stop, so it can safely be destroyed on another thread.

BUG=chromium:736268
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Expose Timer::AbandonAndStop, so it can safely be destroyed on another thread.

BUG=chromium:736268
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[ossu-chromium, 2017-07-05 16:08:27]

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

[commit-bot: I haz the power, 2017-07-05 16:08:42]

Dry run: CQ is trying da patch.

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

[ossu-chromium, 2017-07-05 16:11:53]

ossu@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[ossu-chromium, 2017-07-05 16:11:54]

I've uploaded a new patch set that moved AbandonAndStop into the public section
and updates the comments with what I believe is correct. I've also updated
AudioInputController to call AbandonAndStop instead of just Stop.

+dalecurtis for AIC review. If this change seems fine, I'll just close the other
CL.

[DaleCurtis, 2017-07-05 16:30:59]

media/ lgtm

[commit-bot: I haz the power, 2017-07-05 18:07:58]

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

[commit-bot: I haz the power, 2017-07-05 18:07:59]

Dry run: This issue passed the CQ dry run.

[gab, 2017-07-05 23:49:36]

lgtm

[ossu-chromium, 2017-07-06 08:57:28]

The CQ bit was checked by ossu@chromium.org

[commit-bot: I haz the power, 2017-07-06 08:57:40]

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-07-06 09:02:17]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1499331448881140,
"parent_rev": "73e2be63fc66559cfaed4b5c598c95efe60cb9cc", "commit_rev":
"87340af883f06c363febe752245c1ab1a926a234"}

[commit-bot: I haz the power, 2017-07-06 09:02:33]

Description was changed from

==========
Expose Timer::AbandonAndStop, so it can safely be destroyed on another thread.

BUG=chromium:736268
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Expose Timer::AbandonAndStop, so it can safely be destroyed on another thread.

BUG=chromium:736268
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2958003002
Cr-Commit-Position: refs/heads/master@{#484507}
Committed:
https://chromium.googlesource.com/chromium/src/+/87340af883f06c363febe752245c...
==========

[commit-bot: I haz the power, 2017-07-06 09:02:34]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/87340af883f06c363febe752245c...