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

base/timer/timer_unittest.cc

Index: base/timer/timer_unittest.cc
diff --git a/base/timer/timer_unittest.cc b/base/timer/timer_unittest.cc
index 7c3c1079d72f54be9585287c4ace5473c9c7b5f2..921b385595e89faf443a9febd009fb9dc60af09b 100644
--- a/base/timer/timer_unittest.cc
+++ b/base/timer/timer_unittest.cc
@@ -13,6 +13,7 @@
 #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"
@@ -556,6 +557,53 @@ TEST(TimerTest, MessageLoopShutdown) {
   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() {
 }