cc: Post the missed BeginFrame to a new stack

When the UI compositor (or any compositor with a Display attached to it
in process - such as layout tests) does SwapBuffers, it submits the
CompositorFrame to the SurfaceFactory.

The SurfaceFactory pokes the DisplayScheduler, which can lead to it
doing the Display::DrawAndSwap immediately, in the same callstack. In
that case, the callback to the SurfaceDisplayOutputSurface (or the
TestDelegatingOutputSurface) that it drew happens inline also, which
was previously calling out to the OutputSurfaceClient.

The problem is, the compositor expects SwapBuffers() to return before
it hears that DidSwapBuffersComplete(). To avoid this, and similar
problems, we prevent DisplayScheduler from running a new BeginFrame
inside an external call to DisplayScheduler::SurfaceDamaged() which
prevents re-entrancy for its clients.

We do this by posting the missed BeginFrame to respond to it on a new
stack (and cancelling it if another BeginFrame comes in the meantime
since it was already posted).

For TestDelegatedOutputSurface synchronous composites, we simply
post the SwapBuffersComplete message to get it on a new call stack.

R=jbauman
BUG=495650
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Review-Url: https://codereview.chromium.org/2183333003
Cr-Commit-Position: refs/heads/master@{#408289}

cc/surfaces/display_scheduler.cc

@@ -6,6 +6,7 @@
 
 #include <vector>
 
+#include "base/auto_reset.h"
 #include "base/stl_util.h"
 #include "base/trace_event/trace_event.h"
 #include "cc/output/output_surface.h"
@@ -17,6 +18,7 @@ DisplayScheduler::DisplayScheduler(BeginFrameSource* begin_frame_source,
                                    int max_pending_swaps)
     : begin_frame_source_(begin_frame_source),
       task_runner_(task_runner),
+      inside_surface_damaged_(false),
       output_surface_lost_(false),
       root_surface_resources_locked_(true),
       inside_begin_frame_deadline_interval_(false),
@@ -82,6 +84,11 @@ void DisplayScheduler::SurfaceDamaged(const SurfaceId& surface_id) {
   TRACE_EVENT1("cc", "DisplayScheduler::SurfaceDamaged", "surface_id",
                surface_id.ToString());
 
+  // We may cause a new BeginFrame to be run inside this method, but to help
+  // avoid being reentrant to the caller of SurfaceDamaged, track when this is
+  // happening with |inside_surface_damaged_|.
+  base::AutoReset<bool>(&inside_surface_damaged_, true);
+
   needs_draw_ = true;
 
   if (surface_id == root_surface_id_) {
@@ -138,6 +145,22 @@ bool DisplayScheduler::OnBeginFrameDerivedImpl(const BeginFrameArgs& args) {
   TRACE_EVENT2("cc", "DisplayScheduler::BeginFrame", "args", args.AsValue(),
                "now", now);
 
+  if (inside_surface_damaged_) {
+    // Repost this so that we don't run a missed BeginFrame on the same
+    // callstack. Otherwise we end up running unexpected scheduler actions
+    // immediately while inside some other action (such as submitting a
+    // CompositorFrame for a SurfaceFactory).
+    DCHECK_EQ(args.type, BeginFrameArgs::MISSED);
+    DCHECK(missed_begin_frame_task_.IsCancelled());
+    missed_begin_frame_task_.Reset(base::Bind(
+        base::IgnoreResult(&DisplayScheduler::OnBeginFrameDerivedImpl),
+        // The CancelableCallback will not run after it is destroyed, which
+        // happens when |this| is destroyed.
+        base::Unretained(this), args));
+    task_runner_->PostTask(FROM_HERE, missed_begin_frame_task_.callback());
+    return true;
+  }
+
   // If we get another BeginFrame before the previous deadline,
   // synchronously trigger the previous deadline before progressing.
   if (inside_begin_frame_deadline_interval_) {
@@ -151,6 +174,12 @@ bool DisplayScheduler::OnBeginFrameDerivedImpl(const BeginFrameArgs& args) {
   inside_begin_frame_deadline_interval_ = true;
   ScheduleBeginFrameDeadline();
 
+  // If we get another BeginFrame before a posted missed frame, just drop the
+  // missed frame. Also if this was the missed frame, drop the Callback inside
+  // it. Do this last because this might be the missed frame and we don't want
+  // to destroy |args| prematurely.
+  missed_begin_frame_task_.Cancel();
+
   return true;
 }
 
@@ -283,6 +312,9 @@ void DisplayScheduler::AttemptDrawAndSwap() {
     if (observing_begin_frame_source_) {
       observing_begin_frame_source_ = false;
       begin_frame_source_->RemoveObserver(this);
+      // A missed BeginFrame may be queued, so drop that too if we're going to
+      // stop listening.
+      missed_begin_frame_task_.Cancel();
     }
   }
 }

cc/surfaces/display_scheduler.h

@@ -68,6 +68,9 @@ class CC_SURFACES_EXPORT DisplayScheduler : public BeginFrameObserverBase {
   base::CancelableClosure begin_frame_deadline_task_;
   base::TimeTicks begin_frame_deadline_task_time_;
 
+  base::CancelableClosure missed_begin_frame_task_;
+  bool inside_surface_damaged_;
+
   bool output_surface_lost_;
   bool root_surface_resources_locked_;
 

cc/test/test_delegating_output_surface.cc

@@ -118,10 +118,12 @@ void TestDelegatingOutputSurface::SwapBuffers(CompositorFrame frame) {
       frame.delegated_frame_data->render_pass_list.back()->output_rect.size();
   display_->Resize(frame_size);
 
+  bool synchronous = !display_->has_scheduler();
+
   surface_factory_->SubmitCompositorFrame(
       delegated_surface_id_, std::move(frame),
       base::Bind(&TestDelegatingOutputSurface::DrawCallback,
-                 weak_ptrs_.GetWeakPtr()));
+                 weak_ptrs_.GetWeakPtr(), synchronous));
 
   for (std::unique_ptr<CopyOutputRequest>& copy_request : copy_requests_)
     surface_factory_->RequestCopyOfSurface(delegated_surface_id_,
@@ -132,8 +134,17 @@ void TestDelegatingOutputSurface::SwapBuffers(CompositorFrame frame) {
     display_->DrawAndSwap();
 }
 
-void TestDelegatingOutputSurface::DrawCallback() {
-  client_->DidSwapBuffersComplete();
+void TestDelegatingOutputSurface::DrawCallback(bool synchronous) {
+  // This is the frame ack to unthrottle the next frame, not actually a notice
+  // that drawing is done.
+  if (synchronous) {
+    // For synchronous draws, this must be posted to a new stack because we are
+    // still the original call to SwapBuffers, and we want to leave that before
+    // saying that it is done.
+    OutputSurface::PostSwapBuffersComplete();
+  } else {
+    client_->DidSwapBuffersComplete();
+  }
 }
 
 void TestDelegatingOutputSurface::ForceReclaimResources() {

cc/test/test_delegating_output_surface.h

@@ -57,7 +57,7 @@ class TestDelegatingOutputSurface : public OutputSurface,
   void DisplaySetMemoryPolicy(const ManagedMemoryPolicy& policy) override;
 
  private:
-  void DrawCallback();
+  void DrawCallback(bool synchronous);
 
   // TODO(danakj): These don't to be stored in unique_ptrs when OutputSurface
   // is owned/destroyed on the compositor thread.