Fix use-after-free in base::Timer::StopAndAbandon()

base/timer/timer_unittest.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 <memory>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
+#include "base/memory/ref_counted.h"
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
 #include "base/sequenced_task_runner.h"
 #include "base/single_thread_task_runner.h"
 #include "base/synchronization/waitable_event.h"
 #include "base/test/sequenced_worker_pool_owner.h"
 #include "base/test/test_mock_time_task_runner.h"
 #include "base/threading/platform_thread.h"
 #include "base/threading/sequenced_task_runner_handle.h"
 #include "base/threading/thread.h"
@@ skipping 523 matching lines @@
     {
       MessageLoop loop;
       a.Start();
       b.Start();
     }  // MessageLoop destructs by falling out of scope.
   }  // OneShotTimers destruct.  SHOULD NOT CRASH, of course.
 
   EXPECT_FALSE(did_run.IsSignaled());
 }
 
+// Ref counted class which owns a Timer. The class passes a reference to itself
+// via the |user_task| parameter in Timer::Start(). |Timer::user_task_| might
+// end up holding the last reference to the class.
+class OneShotSelfOwningTimerTester
+    : public RefCounted<OneShotSelfOwningTimerTester> {
+ public:
+  explicit OneShotSelfOwningTimerTester(bool* did_run) : did_run_(did_run) {}
+
+  void StartTimer() {
+    timer_.Start(FROM_HERE, TimeDelta::FromMilliseconds(10),
+                 base::Bind(&OneShotSelfOwningTimerTester::Run, this));
+  }
+
+ private:
+  friend class RefCounted<OneShotSelfOwningTimerTester>;
+  ~OneShotSelfOwningTimerTester() {}

[gab, 2017/01/12 19:42:54]

= default;

+
+  void Run() {
+    if (did_run_)

[gab, 2017/01/12 19:42:54]

Instead of using |did_run_| simply
  ADD_FAILURE() << "Timer unexpectedly fired.";
here and drop the EXPECT_FALSE in the test's body.

+      *did_run_ = true;
+  }
+
+  OneShotTimer timer_;
+  bool* did_run_;
+
+  DISALLOW_COPY_AND_ASSIGN(OneShotSelfOwningTimerTester);
+};
+
+TEST(TimerTest, MessageLoopShutdownSelfOwningTimer) {
+  // This test verifies that shutdown of the message loop does not cause crashes
+  // if there is a pending timer not yet fired and |Timer::user_task_| owns the
+  // timer. The test may only trigger exceptions if debug heap checking is
+  // enabled.
+
+  bool did_run = false;
+  {
+    MessageLoop loop;
+    {

[gab, 2017/01/12 19:42:54]

Instead of using a nested scope and racily depending on it expiring before the
timer fires, let's std::move(tester)->StartTimer().

+      scoped_refptr<OneShotSelfOwningTimerTester> tester =
+          new OneShotSelfOwningTimerTester(&did_run);
+      tester->StartTimer();
+    }  // |Timer::user_task_| owns sole reference to |tester|.
+  }    // MessageLoop destructs by falling out of scope. SHOULD NOT CRASH.

[gab, 2017/01/12 19:42:54]

Since the goal in this test is to hit ~Timer() before it fires, let's make its
delay much larger than 10ms (with a comment explaining why)

+
+  EXPECT_FALSE(did_run);
+}
+
 void TimerTestCallback() {
 }
 
 TEST(TimerTest, NonRepeatIsRunning) {
   {
     MessageLoop loop;
     Timer timer(false, false);
     EXPECT_FALSE(timer.IsRunning());
     timer.Start(FROM_HERE, TimeDelta::FromDays(1), Bind(&TimerTestCallback));
     EXPECT_TRUE(timer.IsRunning());
@@ skipping 101 matching lines @@
                 Bind(&SetCallbackHappened1));
     timer.Reset();
     // Since Reset happened before task ran, the user_task must not be cleared:
     ASSERT_FALSE(timer.user_task().is_null());
     RunLoop().Run();
     EXPECT_TRUE(g_callback_happened1);
   }
 }
 
 }  // namespace base