cc: Fix for releasing output surface during commit.

In the remote compositor we can get into a state where the LTH on the
client requests a BeginMainFrame and then releases the output surface.
The server will still send a commit but we can't push this commit till
the client initializes a new output surface.

At the same time the scheduler will not request a new output surface
till it clears the pipeline of the previous commit.

This change adds a setting to the Scheduler to allow it to request a new
output surface, while there is a commit pending. The RemoteChannelImpl
queues any protos received if the output is released and process them
when a new output surface is initialized.

BUG=586210
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Review URL: https://codereview.chromium.org/1691143002

Cr-Commit-Position: refs/heads/master@{#379947}

11 files changed, 226 insertions, 11 deletions

diff --git a/cc/scheduler/scheduler_settings.cc b/cc/scheduler/scheduler_settings.cc
index f8dda8e..5b2bbf1 100644
--- a/cc/scheduler/scheduler_settings.cc
+++ b/cc/scheduler/scheduler_settings.cc
@@ -16,9 +16,9 @@ SchedulerSettings::SchedulerSettings()
       timeout_and_draw_when_animation_checkerboards(true),
       using_synchronous_renderer_compositor(false),
       throttle_frame_production(true),
+      abort_commit_before_output_surface_creation(true),
       maximum_number_of_failed_draws_before_draw_is_forced(3),
-      background_frame_interval(base::TimeDelta::FromSeconds(1)) {
-}
+      background_frame_interval(base::TimeDelta::FromSeconds(1)) {}
 
 SchedulerSettings::~SchedulerSettings() {}
 
@@ -42,6 +42,8 @@ SchedulerSettings::AsValue() const {
   state->SetBoolean("throttle_frame_production", throttle_frame_production);
   state->SetInteger("background_frame_interval",
                     background_frame_interval.InMicroseconds());
+  state->SetBoolean("abort_commit_before_output_surface_creation",
+                    abort_commit_before_output_surface_creation);
   return std::move(state);
 }
 
diff --git a/cc/scheduler/scheduler_settings.h b/cc/scheduler/scheduler_settings.h
index 7fd4447..d16e47c 100644
--- a/cc/scheduler/scheduler_settings.h
+++ b/cc/scheduler/scheduler_settings.h
@@ -30,6 +30,7 @@ class CC_EXPORT SchedulerSettings {
   bool timeout_and_draw_when_animation_checkerboards;
   bool using_synchronous_renderer_compositor;
   bool throttle_frame_production;
+  bool abort_commit_before_output_surface_creation;
 
   int maximum_number_of_failed_draws_before_draw_is_forced;
   base::TimeDelta background_frame_interval;
diff --git a/cc/scheduler/scheduler_state_machine.cc b/cc/scheduler/scheduler_state_machine.cc
index 714d64c..266f887 100644
--- a/cc/scheduler/scheduler_state_machine.cc
+++ b/cc/scheduler/scheduler_state_machine.cc
@@ -315,8 +315,14 @@ bool SchedulerStateMachine::ShouldBeginOutputSurfaceCreation() const {
 
   // We only want to start output surface initialization after the
   // previous commit is complete.
-  if (begin_main_frame_state_ != BEGIN_MAIN_FRAME_STATE_IDLE)
+  // We make an exception if the embedder explicitly allows beginning output
+  // surface creation while the previous commit has not been aborted. This
+  // assumes that any state passed from the client during the commit will not be
+  // tied to the output surface.
+  if (begin_main_frame_state_ != BEGIN_MAIN_FRAME_STATE_IDLE &&
+      settings_.abort_commit_before_output_surface_creation) {
     return false;
+  }
 
   // Make sure the BeginImplFrame from any previous OutputSurfaces
   // are complete before creating the new OutputSurface.
@@ -731,7 +737,11 @@ void SchedulerStateMachine::WillBeginOutputSurfaceCreation() {
   // The following DCHECKs make sure we are in the proper quiescent state.
   // The pipeline should be flushed entirely before we start output
   // surface creation to avoid complicated corner cases.
-  DCHECK_EQ(begin_main_frame_state_, BEGIN_MAIN_FRAME_STATE_IDLE);
+
+  // We allow output surface creation while the previous commit has not been
+  // aborted if the embedder explicitly allows it.
+  DCHECK(!settings_.abort_commit_before_output_surface_creation ||
+         begin_main_frame_state_ == BEGIN_MAIN_FRAME_STATE_IDLE);
   DCHECK(!has_pending_tree_);
   DCHECK(!active_tree_needs_first_draw_);
 }
diff --git a/cc/scheduler/scheduler_state_machine_unittest.cc b/cc/scheduler/scheduler_state_machine_unittest.cc
index 15e172e..ee88bfc1 100644
--- a/cc/scheduler/scheduler_state_machine_unittest.cc
+++ b/cc/scheduler/scheduler_state_machine_unittest.cc
@@ -2106,5 +2106,69 @@ TEST(SchedulerStateMachineTest, EarlyOutCommitWantsProactiveBeginFrame) {
   EXPECT_FALSE(state.ProactiveBeginFrameWanted());
 }
 
+TEST(SchedulerStateMachineTest, NoOutputSurfaceCreationWhileCommitPending) {
+  SchedulerSettings settings;
+  StateMachine state(settings);
+  SET_UP_STATE(state);
+
+  // Set up the request for a commit and start a frame.
+  state.SetNeedsBeginMainFrame();
+  state.OnBeginImplFrame();
+  PerformAction(&state, SchedulerStateMachine::ACTION_SEND_BEGIN_MAIN_FRAME);
+
+  // Lose the output surface.
+  state.DidLoseOutputSurface();
+
+  // The scheduler shouldn't trigger the output surface creation till the
+  // previous commit has been cleared.
+  EXPECT_ACTION_UPDATE_STATE(SchedulerStateMachine::ACTION_NONE);
+
+  // Trigger the deadline and ensure that the scheduler does not trigger any
+  // actions until we receive a response for the pending commit.
+  state.OnBeginImplFrameDeadlinePending();
+  state.OnBeginImplFrameDeadline();
+  state.OnBeginImplFrameIdle();
+  EXPECT_ACTION_UPDATE_STATE(SchedulerStateMachine::ACTION_NONE);
+
+  // Abort the commit, since that is what we expect the main thread to do if the
+  // output surface was lost due to a synchronous call from the main thread to
+  // release the output surface.
+  state.NotifyBeginMainFrameStarted();
+  state.BeginMainFrameAborted(
+      CommitEarlyOutReason::ABORTED_OUTPUT_SURFACE_LOST);
+
+  // The scheduler should begin the output surface creation now.
+  EXPECT_ACTION_UPDATE_STATE(
+      SchedulerStateMachine::ACTION_BEGIN_OUTPUT_SURFACE_CREATION);
+}
+
+TEST(SchedulerStateMachineTest, OutputSurfaceCreationWhileCommitPending) {
+  SchedulerSettings settings;
+  settings.abort_commit_before_output_surface_creation = false;
+  StateMachine state(settings);
+  SET_UP_STATE(state);
+
+  // Set up the request for a commit and start a frame.
+  state.SetNeedsBeginMainFrame();
+  state.OnBeginImplFrame();
+  PerformAction(&state, SchedulerStateMachine::ACTION_SEND_BEGIN_MAIN_FRAME);
+
+  // Lose the output surface.
+  state.DidLoseOutputSurface();
+
+  // The scheduler shouldn't trigger the output surface creation till the
+  // previous begin impl frame state is cleared from the pipeline.
+  EXPECT_ACTION_UPDATE_STATE(SchedulerStateMachine::ACTION_NONE);
+
+  // Cycle through the frame stages to clear the scheduler state.
+  state.OnBeginImplFrameDeadlinePending();
+  state.OnBeginImplFrameDeadline();
+  state.OnBeginImplFrameIdle();
+
+  // The scheduler should begin the output surface creation now.
+  EXPECT_ACTION_UPDATE_STATE(
+      SchedulerStateMachine::ACTION_BEGIN_OUTPUT_SURFACE_CREATION);
+}
+
 }  // namespace
 }  // namespace cc
diff --git a/cc/scheduler/scheduler_unittest.cc b/cc/scheduler/scheduler_unittest.cc
index 37fa8e0..ec1a20a 100644
--- a/cc/scheduler/scheduler_unittest.cc
+++ b/cc/scheduler/scheduler_unittest.cc
@@ -3606,6 +3606,61 @@ TEST_F(SchedulerTest, ImplLatencyTakesPriority) {
   EXPECT_FALSE(scheduler_->ImplLatencyTakesPriority());
 }
 
+TEST_F(SchedulerTest, NoOutputSurfaceCreationWhileCommitPending) {
+  SetUpScheduler(true);
+
+  // SetNeedsBeginMainFrame should begin the frame.
+  scheduler_->SetNeedsBeginMainFrame();
+  client_->Reset();
+  EXPECT_SCOPED(AdvanceFrame());
+  EXPECT_ACTION("WillBeginImplFrame", client_, 0, 2);
+  EXPECT_ACTION("ScheduledActionSendBeginMainFrame", client_, 1, 2);
+
+  // Lose the output surface and trigger the deadline.
+  client_->Reset();
+  scheduler_->DidLoseOutputSurface();
+  EXPECT_TRUE(scheduler_->BeginImplFrameDeadlinePending());
+  EXPECT_NO_ACTION(client_);
+
+  // The scheduler should not trigger the output surface creation till the
+  // commit is aborted.
+  task_runner_->RunTasksWhile(client_->ImplFrameDeadlinePending(true));
+  EXPECT_FALSE(scheduler_->BeginImplFrameDeadlinePending());
+  EXPECT_SINGLE_ACTION("SendBeginMainFrameNotExpectedSoon", client_);
+
+  // Abort the commit.
+  client_->Reset();
+  scheduler_->NotifyBeginMainFrameStarted(base::TimeTicks::Now());
+  scheduler_->BeginMainFrameAborted(
+      CommitEarlyOutReason::ABORTED_OUTPUT_SURFACE_LOST);
+  EXPECT_SINGLE_ACTION("ScheduledActionBeginOutputSurfaceCreation", client_);
+}
+
+TEST_F(SchedulerTest, OutputSurfaceCreationWhileCommitPending) {
+  scheduler_settings_.abort_commit_before_output_surface_creation = false;
+  SetUpScheduler(true);
+
+  // SetNeedsBeginMainFrame should begin the frame.
+  scheduler_->SetNeedsBeginMainFrame();
+  client_->Reset();
+  EXPECT_SCOPED(AdvanceFrame());
+  EXPECT_ACTION("WillBeginImplFrame", client_, 0, 2);
+  EXPECT_ACTION("ScheduledActionSendBeginMainFrame", client_, 1, 2);
+
+  // Lose the output surface and trigger the deadline.
+  client_->Reset();
+  scheduler_->DidLoseOutputSurface();
+  EXPECT_TRUE(scheduler_->BeginImplFrameDeadlinePending());
+  EXPECT_NO_ACTION(client_);
+
+  // The scheduler should trigger the output surface creation immediately after
+  // the begin_impl_frame_state_ is cleared.
+  task_runner_->RunTasksWhile(client_->ImplFrameDeadlinePending(true));
+  EXPECT_FALSE(scheduler_->BeginImplFrameDeadlinePending());
+  EXPECT_ACTION("ScheduledActionBeginOutputSurfaceCreation", client_, 0, 2);
+  EXPECT_ACTION("SendBeginMainFrameNotExpectedSoon", client_, 1, 2);
+}
+
 // The three letters appeneded to each version of this test mean the following:s
 // tree_priority: B = both trees same priority; A = active tree priority;
 // scroll_handler_state: H = affects scroll handler; N = does not affect scroll
diff --git a/cc/test/layer_tree_test.cc b/cc/test/layer_tree_test.cc
index 98396ea..0887cc5 100644
--- a/cc/test/layer_tree_test.cc
+++ b/cc/test/layer_tree_test.cc
@@ -1005,6 +1005,7 @@ void LayerTreeTest::CreateRemoteClientHost(
   proto::InitializeImpl initialize_proto = proto.initialize_impl_message();
   LayerTreeSettings settings;
   settings.FromProtobuf(initialize_proto.layer_tree_settings());
+  settings.abort_commit_before_output_surface_creation = false;
   remote_client_layer_tree_host_ = LayerTreeHostForTesting::Create(
       this, mode_, client_.get(), &remote_proto_channel_bridge_.channel_impl,
       nullptr, nullptr, task_graph_runner_.get(), settings,
diff --git a/cc/trees/layer_tree_settings.cc b/cc/trees/layer_tree_settings.cc
index 40606ed..d9062e3 100644
--- a/cc/trees/layer_tree_settings.cc
+++ b/cc/trees/layer_tree_settings.cc
@@ -101,6 +101,7 @@ LayerTreeSettings::LayerTreeSettings()
       image_decode_tasks_enabled(false),
       use_compositor_animation_timelines(true),
       wait_for_beginframe_interval(true),
+      abort_commit_before_output_surface_creation(true),
       use_mouse_wheel_gestures(false),
       max_staging_buffer_usage_in_bytes(32 * 1024 * 1024),
       memory_policy_(64 * 1024 * 1024,
@@ -322,6 +323,8 @@ SchedulerSettings LayerTreeSettings::ToSchedulerSettings() const {
   scheduler_settings.throttle_frame_production = wait_for_beginframe_interval;
   scheduler_settings.background_frame_interval =
       base::TimeDelta::FromSecondsD(1.0 / background_animation_rate);
+  scheduler_settings.abort_commit_before_output_surface_creation =
+      abort_commit_before_output_surface_creation;
   return scheduler_settings;
 }
 
diff --git a/cc/trees/layer_tree_settings.h b/cc/trees/layer_tree_settings.h
index ed7615c..bee3d8e 100644
--- a/cc/trees/layer_tree_settings.h
+++ b/cc/trees/layer_tree_settings.h
@@ -87,6 +87,7 @@ class CC_EXPORT LayerTreeSettings {
   bool image_decode_tasks_enabled;
   bool use_compositor_animation_timelines;
   bool wait_for_beginframe_interval;
+  bool abort_commit_before_output_surface_creation;
   bool use_mouse_wheel_gestures;
   int max_staging_buffer_usage_in_bytes;
   ManagedMemoryPolicy memory_policy_;
diff --git a/cc/trees/remote_channel_impl.cc b/cc/trees/remote_channel_impl.cc
index d8b9319..cc43659 100644
--- a/cc/trees/remote_channel_impl.cc
+++ b/cc/trees/remote_channel_impl.cc
@@ -60,13 +60,20 @@ void RemoteChannelImpl::OnProtoReceived(
   DCHECK(main().started);
   DCHECK(proto->has_to_impl());
 
-  HandleProto(proto->to_impl());
+  // If we don't have an output surface, queue the message and defer processing
+  // it till we initialize a new output surface.
+  if (main().waiting_for_output_surface_initialization) {
+    main().pending_messages.push(proto->to_impl());
+  } else {
+    HandleProto(proto->to_impl());
+  }
 }
 
 void RemoteChannelImpl::HandleProto(
     const proto::CompositorMessageToImpl& proto) {
   DCHECK(task_runner_provider_->IsMainThread());
   DCHECK(proto.has_message_type());
+  DCHECK(!main().waiting_for_output_surface_initialization);
 
   switch (proto.message_type()) {
     case proto::CompositorMessageToImpl::UNKNOWN:
@@ -162,12 +169,18 @@ void RemoteChannelImpl::SetOutputSurface(OutputSurface* output_surface) {
 
 void RemoteChannelImpl::ReleaseOutputSurface() {
   DCHECK(task_runner_provider_->IsMainThread());
-  CompletionEvent completion;
-  DebugScopedSetMainThreadBlocked main_thread_blocked(task_runner_provider_);
-  ImplThreadTaskRunner()->PostTask(
-      FROM_HERE, base::Bind(&ProxyImpl::ReleaseOutputSurfaceOnImpl,
-                            proxy_impl_weak_ptr_, &completion));
-  completion.Wait();
+  DCHECK(!main().waiting_for_output_surface_initialization);
+
+  {
+    CompletionEvent completion;
+    DebugScopedSetMainThreadBlocked main_thread_blocked(task_runner_provider_);
+    ImplThreadTaskRunner()->PostTask(
+        FROM_HERE, base::Bind(&ProxyImpl::ReleaseOutputSurfaceOnImpl,
+                              proxy_impl_weak_ptr_, &completion));
+    completion.Wait();
+  }
+
+  main().waiting_for_output_surface_initialization = true;
 }
 
 void RemoteChannelImpl::SetVisible(bool visible) {
@@ -406,6 +419,15 @@ void RemoteChannelImpl::DidInitializeOutputSurfaceOnMain(
 
   main().renderer_capabilities = capabilities;
   main().layer_tree_host->DidInitializeOutputSurface();
+
+  // If we were waiting for output surface initialization, we might have queued
+  // some messages. Relay them now that a new output surface has been
+  // initialized.
+  main().waiting_for_output_surface_initialization = false;
+  while (!main().pending_messages.empty()) {
+    HandleProto(main().pending_messages.front());
+    main().pending_messages.pop();
+  }
 }
 
 void RemoteChannelImpl::SendMessageProtoOnMain(
@@ -476,6 +498,7 @@ RemoteChannelImpl::MainThreadOnly::MainThreadOnly(
     : layer_tree_host(layer_tree_host),
       remote_proto_channel(remote_proto_channel),
       started(false),
+      waiting_for_output_surface_initialization(false),
       remote_channel_weak_factory(remote_channel_impl) {
   DCHECK(layer_tree_host);
   DCHECK(remote_proto_channel);
diff --git a/cc/trees/remote_channel_impl.h b/cc/trees/remote_channel_impl.h
index f79e447..cfd411b 100644
--- a/cc/trees/remote_channel_impl.h
+++ b/cc/trees/remote_channel_impl.h
@@ -9,6 +9,7 @@
 #include "base/memory/weak_ptr.h"
 #include "cc/base/cc_export.h"
 #include "cc/base/completion_event.h"
+#include "cc/proto/compositor_message_to_impl.pb.h"
 #include "cc/trees/channel_impl.h"
 #include "cc/trees/proxy.h"
 #include "cc/trees/proxy_impl.h"
@@ -103,6 +104,14 @@ class CC_EXPORT RemoteChannelImpl : public ChannelImpl,
 
     bool started;
 
+    // This is set to true if we lost the output surface and can not push any
+    // commits to the impl thread.
+    bool waiting_for_output_surface_initialization;
+
+    // The queue of messages received from the server. The messages are added to
+    // this queue if we are waiting for a new output surface to be initialized.
+    std::queue<proto::CompositorMessageToImpl> pending_messages;
+
     RendererCapabilities renderer_capabilities;
 
     base::WeakPtrFactory<RemoteChannelImpl> remote_channel_weak_factory;
diff --git a/cc/trees/remote_channel_unittest.cc b/cc/trees/remote_channel_unittest.cc
index 4311781..a9c7595 100644
--- a/cc/trees/remote_channel_unittest.cc
+++ b/cc/trees/remote_channel_unittest.cc
@@ -182,4 +182,50 @@ class RemoteChannelTestBeginMainFrameAborted : public RemoteChannelTest {
 
 REMOTE_DIRECT_RENDERER_TEST_F(RemoteChannelTestBeginMainFrameAborted);
 
+class RemoteChannelTestReleaseOutputSurfaceDuringCommit
+    : public RemoteChannelTest {
+ public:
+  RemoteChannelTestReleaseOutputSurfaceDuringCommit()
+      : output_surface_initialized_count_(0), commit_count_(0) {}
+  void BeginChannelTest() override { PostSetNeedsCommitToMainThread(); }
+
+  void DidInitializeOutputSurface() override {
+    ++output_surface_initialized_count_;
+
+    // We should not have any commits at this point. This call runs either after
+    // the first output surface is initialized so no commits should have been
+    // started, or after the output surface was released by the main thread. In
+    // which case, we should have queued the commit and it should only go
+    // through after the new output surface is initialized.
+    EXPECT_EQ(0, commit_count_);
+  }
+
+  void ReceivedBeginMainFrame() override {
+    // Release the output surface before we respond to the BeginMainFrame.
+    SetVisibleOnLayerTreeHost(false);
+    ReleaseOutputSurfaceOnLayerTreeHost();
+    SetVisibleOnLayerTreeHost(true);
+  }
+
+  void StartCommitOnImpl() override {
+    // The commit should go through only when the second output surface is
+    // initialized.
+    EXPECT_EQ(2, output_surface_initialized_count_);
+    EXPECT_EQ(0, commit_count_);
+    ++commit_count_;
+    EndTest();
+  }
+
+  void AfterTest() override {
+    EXPECT_EQ(2, output_surface_initialized_count_);
+    EXPECT_EQ(1, commit_count_);
+  }
+
+  int output_surface_initialized_count_;
+  int commit_count_;
+};
+
+REMOTE_DIRECT_RENDERER_TEST_F(
+    RemoteChannelTestReleaseOutputSurfaceDuringCommit);
+
 }  // namespace cc