Fix a crash from shutdown race in JankMonitor.

During shutdown of JankMonitor, when the monitor timer is fired after
MetricSource has shut down, the timer dereferences the null pointer and
crashes. Fix the crash by nulling out the pointers after the monitor
timer shut down.

Bug: 1015425
Test: JankMonitorShutdownTest.ShutdownRace_TimerFired
Change-Id: Icd9073e4527e0ac01c373fe486d88aa0985a5841
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1868772
Commit-Queue: Chinglin Yu <chinglinyu@chromium.org>
Reviewed-by: Erik Chen <erikchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#709822}

content/browser/scheduler/responsiveness/jank_monitor.cc

@@ -46,16 +46,31 @@ void JankMonitor::RemoveObserver(Observer* observer) {
 void JankMonitor::SetUp() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
-  metric_source_ = CreateMetricSource();
-  metric_source_->SetUp();
+  // Dependencies in SetUp() and Destroy():
+  // * Target thread --(may schedule the timer on)--> Monitor thread.
+  // * Monitor thread --(read/write)--> ThreadExecutionState data members.
+  // * Target thread --(write)--> ThreadExecutionState data members.
 
-  monitor_task_runner_ = base::CreateSequencedTaskRunner({base::ThreadPool()});
+  // ThreadExecutionState data members are created first.
   ui_thread_exec_state_ = std::make_unique<ThreadExecutionState>();
+  io_thread_exec_state_ = std::make_unique<ThreadExecutionState>();
+
+  // Then the monitor thread.
+  monitor_task_runner_ = base::CreateSequencedTaskRunner({base::ThreadPool()});
+
+  // Finally set up the MetricSource.
+  metric_source_ = CreateMetricSource();
+  metric_source_->SetUp();
 }
 
 void JankMonitor::Destroy() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
+  // Destroy() tears down the object in reverse order of SetUp(): destroy the
+  // MetricSource first to silence the WillRun/DidRun callbacks. Then the
+  // monitor timer is stopped. Finally |ui_thread_exec_state_| and
+  // |io_thread_exec_state_| can be safely destroyed.
+
   // This holds a reference to |this| until |metric_source_| finishes async
   // shutdown.
   base::ScopedClosureRunner finish_destroy_metric_source(base::BindOnce(
@@ -76,16 +91,16 @@ void JankMonitor::FinishDestroyMetricSource() {
                                 base::RetainedRef(this)));
 }
 
-void JankMonitor::SetUpOnIOThread() {
-  io_thread_exec_state_ = std::make_unique<ThreadExecutionState>();
-}
+void JankMonitor::SetUpOnIOThread() {}
 
 void JankMonitor::TearDownOnUIThread() {
-  ui_thread_exec_state_ = nullptr;
+  // Don't destroy |ui_thread_exec_state_| yet because it might be used if the
+  // monitor timer runs.
 }
 
 void JankMonitor::TearDownOnIOThread() {
-  io_thread_exec_state_ = nullptr;
+  // Don't destroy |io_thread_exec_state_| yet because it might be used if the
+  // monitor timer fires.
 }
 
 void JankMonitor::WillRunTaskOnUIThread(const base::PendingTask* task) {
@@ -179,6 +194,12 @@ void JankMonitor::DestroyOnMonitorThread() {
 
   timer_.AbandonAndStop();
   timer_running_ = false;
+
+  // This is the last step of shutdown: no tasks will be observed on either IO
+  // or IO thread, and the monitor timer is stopped. It's safe to destroy
+  // |ui_thread_exec_state_| and |io_thread_exec_state_| now.
+  ui_thread_exec_state_ = nullptr;
+  io_thread_exec_state_ = nullptr;
 }
 
 bool JankMonitor::timer_running() const {
@@ -247,6 +268,9 @@ void JankMonitor::NotifyJankStopIfNecessary(const void* opaque_identifier) {
 JankMonitor::ThreadExecutionState::TaskMetadata::~TaskMetadata() = default;
 
 JankMonitor::ThreadExecutionState::ThreadExecutionState() {
+  // Constructor is always on the UI thread. Detach |target_sequence_checker_|
+  // to make it work on IO thread.
+  DETACH_FROM_SEQUENCE(target_sequence_checker_);
   DETACH_FROM_SEQUENCE(monitor_sequence_checker_);
 }
 

content/browser/scheduler/responsiveness/jank_monitor.h

@@ -101,6 +101,7 @@ class CONTENT_EXPORT JankMonitor
   virtual void DestroyOnMonitorThread();
   virtual void FinishDestroyMetricSource();
   virtual std::unique_ptr<MetricSource> CreateMetricSource();
+  virtual void OnCheckJankiness();  // Timer callback.
   bool timer_running() const;
 
  private:
@@ -154,9 +155,6 @@ class CONTENT_EXPORT JankMonitor
   // Stops the timer on inactivity for longer than a threshold.
   void StopTimerIfIdle();
 
-  // Timer callback.
-  void OnCheckJankiness();
-
   // Sends out notifications.
   void OnJankStarted(const void* opaque_identifier);
   void OnJankStopped(const void* opaque_identifier);
@@ -197,7 +195,7 @@ class CONTENT_EXPORT JankMonitor
   // The lock synchronizes access the |observers| from AddObserver(),
   // RemoveObserver(), OnJankStarted() and OnJankStopped().
   base::Lock observers_lock_;
-  base::ObserverList<Observer>::Unchecked observers_;
+  base::ObserverList<Observer, /* check_empty = */ true>::Unchecked observers_;
 
   // Checks some methods are called on the monitor thread.
   SEQUENCE_CHECKER(monitor_sequence_checker_);

content/browser/scheduler/responsiveness/jank_monitor_unittest.cc

@@ -92,6 +92,7 @@ class JankMonitorTest : public testing::Test {
   void TearDown() override {
     if (!monitor_)  // Already teared down.
       return;
+    monitor_->RemoveObserver(&test_observer_);
     monitor_->Destroy();
     task_environment_.RunUntilIdle();
     monitor_ = nullptr;
@@ -116,6 +117,7 @@ TEST_F(JankMonitorTest, LifeCycle) {
   EXPECT_FALSE(monitor_->destroy_on_monitor_thread_called());
 
   // Test that the monitor thread is destroyed.
+  monitor_->RemoveObserver(&test_observer_);
   monitor_->Destroy();
   task_environment_.RunUntilIdle();
   EXPECT_TRUE(monitor_->destroy_on_monitor_thread_called());
@@ -321,7 +323,7 @@ class TestJankMonitorShutdownRace : public JankMonitor {
 };
 
 // Test that shutdown race with the monitor timer doesn't happen.
-TEST(JankMonitorShutdownTest, ShutdownRace) {
+TEST(JankMonitorShutdownTest, ShutdownRace_TimerRestarted) {
   content::BrowserTaskEnvironment task_environment;
 
   // Use WaitableEvent to control the progress of shutdown sequence.
@@ -350,6 +352,66 @@ TEST(JankMonitorShutdownTest, ShutdownRace) {
   EXPECT_FALSE(jank_monitor->timer_running());
 }
 
+class TestJankMonitorShutdownRaceTimerFired : public JankMonitor {
+ public:
+  TestJankMonitorShutdownRaceTimerFired(
+      content::BrowserTaskEnvironment* task_environment)
+      : task_environment_(task_environment) {}
+
+  bool monitor_timer_fired() const { return monitor_timer_fired_; }
+
+ protected:
+  ~TestJankMonitorShutdownRaceTimerFired() override = default;
+
+  std::unique_ptr<MetricSource> CreateMetricSource() override {
+    return std::make_unique<TestMetricSource>(this);
+  }
+
+  void FinishDestroyMetricSource() override {
+    // Forward by 1 ms to trigger the monitor timer. This shouldn't crash even
+    // after MetricSource is destroyed.
+    task_environment_->FastForwardBy(base::TimeDelta::FromMilliseconds(1));
+
+    JankMonitor::FinishDestroyMetricSource();
+  }
+
+  void OnCheckJankiness() override {
+    JankMonitor::OnCheckJankiness();
+    monitor_timer_fired_ = true;
+  }
+
+ private:
+  content::BrowserTaskEnvironment* task_environment_;
+  bool monitor_timer_fired_ = false;
+};
+
+// Test that the monitor timer shouldn't race with shutdown of MetricSource and
+// then crashes.
+TEST(JankMonitorShutdownTest, ShutdownRace_TimerFired) {
+  content::BrowserTaskEnvironment task_environment(
+      base::test::TaskEnvironment::TimeSource::MOCK_TIME);
+
+  scoped_refptr<TestJankMonitorShutdownRaceTimerFired> jank_monitor =
+      base::MakeRefCounted<TestJankMonitorShutdownRaceTimerFired>(
+          &task_environment);
+  jank_monitor->SetUp();
+  task_environment.RunUntilIdle();
+
+  // Fast-forward by 499 ms. This shouldn't trigger the monitor timer.
+  static constexpr base::TimeDelta kCheckInterval =
+      base::TimeDelta::FromMilliseconds(500);
+  task_environment.FastForwardBy(kCheckInterval -
+                                 base::TimeDelta::FromMilliseconds(1));
+
+  EXPECT_FALSE(jank_monitor->monitor_timer_fired());
+
+  jank_monitor->Destroy();
+  task_environment.RunUntilIdle();
+
+  // The timer fires, but we shouldn't crash.
+  EXPECT_TRUE(jank_monitor->monitor_timer_fired());
+}
+
 #undef VALIDATE_TEST_OBSERVER_CALLS
 
 }  // namespace responsiveness.