Null DisplaySchedulerClient before teardown

Currently, when Display is destroyed, it tears down DisplayScheduler
midway through destruction. This is problematic as DisplayScheduler
can call back into Display in a partially destroyed state.

We could force DisplayScheduler to be destroyed earlier, but this
is still somewhat brittle. Instead we allow DisplayScheduler to have
a null DisplaySchedulerClient and null this before destroying
Display.

Bug: 1012943
Change-Id: I5892ed2746ba29ef5c3c027e9564e52ee6c659ec
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1863674
Auto-Submit: Eric Karl <ericrk@chromium.org>
Reviewed-by: Khushal <khushalsagar@chromium.org>
Commit-Queue: Eric Karl <ericrk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#706492}

components/viz/service/display/display.cc

@@ -218,6 +218,11 @@ Display::~Display() {
       surface_manager_->RemoveObserver(scheduler_.get());
   }
 
+  // Un-register as DisplaySchedulerClient to prevent us from being called in a
+  // partially destructed state.
+  if (scheduler_)
+    scheduler_->SetClient(nullptr);
+
   RunDrawCallbacks();
 }
 

components/viz/service/display/display_scheduler.cc

@@ -212,7 +212,7 @@ bool DisplayScheduler::DrawAndSwap() {
   DCHECK_LT(pending_swaps_, max_pending_swaps_);
   DCHECK(!output_surface_lost_);
 
-  bool success = client_->DrawAndSwap();
+  bool success = client_ && client_->DrawAndSwap();
   if (!success)
     return false;
 
@@ -328,14 +328,15 @@ void DisplayScheduler::OnSurfaceMarkedForDestruction(
 
 bool DisplayScheduler::OnSurfaceDamaged(const SurfaceId& surface_id,
                                         const BeginFrameAck& ack) {
-  bool damaged = client_->SurfaceDamaged(surface_id, ack);
+  bool damaged = client_ && client_->SurfaceDamaged(surface_id, ack);
   ProcessSurfaceDamage(surface_id, ack, damaged);
 
   return damaged;
 }
 
 void DisplayScheduler::OnSurfaceDestroyed(const SurfaceId& surface_id) {
-  client_->SurfaceDestroyed(surface_id);
+  if (client_)
+    client_->SurfaceDestroyed(surface_id);
 }
 
 void DisplayScheduler::OnSurfaceDamageExpected(const SurfaceId& surface_id,
@@ -508,7 +509,8 @@ void DisplayScheduler::DidFinishFrame(bool did_draw) {
   DCHECK(begin_frame_source_);
   begin_frame_source_->DidFinishFrame(this);
   BeginFrameAck ack(current_begin_frame_args_, did_draw);
-  client_->DidFinishFrame(ack);
+  if (client_)
+    client_->DidFinishFrame(ack);
 }
 
 void DisplayScheduler::DidSwapBuffers() {

components/viz/service/display/display_scheduler_unittest.cc

@@ -825,5 +825,22 @@ TEST_F(DisplaySchedulerTest, GpuBusyNotifications) {
   EXPECT_FALSE(fake_begin_frame_source_.RequestCallbackOnGpuAvailable());
 }
 
+TEST_F(DisplaySchedulerTest, OnBeginFrameDeadlineNoClient) {
+  SurfaceId root_surface_id(
+      kArbitraryFrameSinkId,
+      LocalSurfaceId(1, base::UnguessableToken::Create()));
+
+  scheduler_.SetVisible(true);
+  scheduler_.SetNewRootSurface(root_surface_id);
+
+  AdvanceTimeAndBeginFrameForTest({root_surface_id});
+  SurfaceDamaged(root_surface_id);
+
+  // During teardown, we may get a BeginFrameDeadline while |client_| is null.
+  // This should not crash.
+  scheduler_.SetClient(nullptr);
+  scheduler_.BeginFrameDeadlineForTest();
+}
+
 }  // namespace
 }  // namespace viz