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

base/timer/timer.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/timer/timer.h"
 
 #include <stddef.h>
 
 #include <utility>
 
@@ skipping 14 matching lines @@
  public:
   explicit BaseTimerTaskInternal(Timer* timer)
       : timer_(timer) {
   }
 
   ~BaseTimerTaskInternal() {
     // This task may be getting cleared because the task runner has been
     // destructed.  If so, don't leave Timer with a dangling pointer
     // to this.
     if (timer_)
-      timer_->AbandonAndStop();
+      timer_->Stop();
   }
 
   void Run() {
     // |timer_| is nullptr if we were abandoned.
     if (!timer_)
       return;
 
     // |this| will be deleted by the task runner, so Timer needs to forget us:
     timer_->scheduled_task_ = nullptr;
 
@@ skipping 47 matching lines @@
       is_repeating_(is_repeating),
       retain_user_task_(true),
       tick_clock_(tick_clock),
       is_running_(false) {
   // See comment in other constructor.
   origin_sequence_checker_.DetachFromSequence();
 }
 
 Timer::~Timer() {
   DCHECK(origin_sequence_checker_.CalledOnValidSequence());
-  AbandonAndStop();
+  Stop();
 }
 
 bool Timer::IsRunning() const {
   DCHECK(origin_sequence_checker_.CalledOnValidSequence());
   return is_running_;
 }
 
 TimeDelta Timer::GetCurrentDelay() const {
   DCHECK(origin_sequence_checker_.CalledOnValidSequence());
   return delay_;
@@ skipping 21 matching lines @@
   user_task_ = user_task;
 
   Reset();
 }
 
 void Timer::Stop() {
   // TODO(gab): Enable this when it's no longer called racily from
   // RunScheduledTask(): https://crbug.com/587199.
   // DCHECK(origin_sequence_checker_.CalledOnValidSequence());
 
+  AbandonScheduledTask();
   is_running_ = false;
 
   // It's safe to destroy or restart Timer on another sequence after Stop().
   origin_sequence_checker_.DetachFromSequence();

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

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]

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:14]

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]

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.

 
   if (!retain_user_task_)
     user_task_.Reset();
   // No more member accesses here: |this| could be deleted after freeing
   // |user_task_|.
 }
 
 void Timer::Reset() {
   DCHECK(origin_sequence_checker_.CalledOnValidSequence());
   DCHECK(!user_task_.is_null());
@@ skipping 98 matching lines @@
     PostNewScheduledTask(delay_);
   else
     Stop();
 
   task.Run();
 
   // No more member accesses here: |this| could be deleted at this point.
 }
 
 }  // namespace base