Fix use-after-free in base::Timer::StopAndAbandon()
A timer may be owned by a ref counted class. If |Timer::user_task_| is a method
in the ref counted class, it is possible for |Timer::user_task_| to hold the
only reference to the ref counted class. This CL changes
Timer::StopAndAbandon() so that Timer::Stop() is called after
Timer::AbandonScheduledTask(). If |Timer::user_task_| holds the only reference
to ref counted class, Timer::Stop() destroys the timer object.
Timer::StopAndAbandon() can be called while |Timer::user_task_| holds the only
reference to the ref counted class if the message loop is shut down.
BUG=678592
TEST=MessageLoopShutdownSelfOwningTimer
Review-Url: https://codereview.chromium.org/2624133004
Cr-Commit-Position: refs/heads/master@{#444132}
CrOS-Libchrome-Original-Commit: 4a286ed6b9406edfb1031ae1d48766f79652a6ca
diff --git a/base/timer/timer_unittest.cc b/base/timer/timer_unittest.cc
index 7c3c107..69338eb 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,50 @@
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:
+ OneShotSelfOwningTimerTester() = default;
+
+ void StartTimer() {
+ // Start timer with long delay in order to test the timer getting destroyed
+ // while a timer task is still pending.
+ timer_.Start(FROM_HERE, TimeDelta::FromDays(1),
+ base::Bind(&OneShotSelfOwningTimerTester::Run, this));
+ }
+
+ private:
+ friend class RefCounted<OneShotSelfOwningTimerTester>;
+ ~OneShotSelfOwningTimerTester() = default;
+
+ void Run() {
+ ADD_FAILURE() << "Timer unexpectedly fired.";
+ }
+
+ OneShotTimer timer_;
+
+ 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.
+
+ MessageLoop loop;
+ scoped_refptr<OneShotSelfOwningTimerTester> tester =
+ new OneShotSelfOwningTimerTester();
+
+ std::move(tester)->StartTimer();
+ // |Timer::user_task_| owns sole reference to |tester|.
+
+ // MessageLoop destructs by falling out of scope. SHOULD NOT CRASH.
+}
+
void TimerTestCallback() {
}