Avoid deadlock with the UI thread and NPAPI in more cases

By doing the following:
1) Avoid deferring commits until draw if not drawing soon.
2) Don't send BeginMainFrame if swap throttling will block commit.

As a side-effect, some of the changes will also help release
the main thread sooner in some cases where we were holding on to
it for too long.

Additional scheduler deadlock tests added to verify existing
and future operation.

This should also enable us to more easily avoid unconditionally
setting active_tree_needs_first_draw_ to false in
SchedulerStateMachine::UpdateStateOnInvalidateOutputSurface
in followup patches if we want to.

BUG=479671, 477082

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

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

cc/scheduler/scheduler_settings.cc

@@ -10,6 +10,7 @@ namespace cc {
 
 SchedulerSettings::SchedulerSettings()
     : use_external_begin_frame_source(false),
+      main_frame_while_swap_throttled_enabled(false),
       main_frame_before_activation_enabled(false),
       impl_side_painting(false),
       timeout_and_draw_when_animation_checkerboards(true),
@@ -28,6 +29,8 @@ SchedulerSettings::AsValue() const {
       new base::trace_event::TracedValue();
   state->SetBoolean("use_external_begin_frame_source",
                     use_external_begin_frame_source);
+  state->SetBoolean("main_frame_while_swap_throttled_enabled",
+                    main_frame_while_swap_throttled_enabled);
   state->SetBoolean("main_frame_before_activation_enabled",
                     main_frame_before_activation_enabled);
   state->SetBoolean("impl_side_painting", impl_side_painting);

cc/scheduler/scheduler_settings.h

@@ -24,6 +24,7 @@ class CC_EXPORT SchedulerSettings {
   ~SchedulerSettings();
 
   bool use_external_begin_frame_source;
+  bool main_frame_while_swap_throttled_enabled;
   bool main_frame_before_activation_enabled;
   bool impl_side_painting;
   bool timeout_and_draw_when_animation_checkerboards;

cc/scheduler/scheduler_state_machine.cc

@@ -402,6 +402,19 @@ bool SchedulerStateMachine::CouldSendBeginMainFrame() const {
   return true;
 }
 
+bool SchedulerStateMachine::SendingBeginMainFrameMightCauseDeadlock() const {
+  // NPAPI is the only case where the UI thread makes synchronous calls to the
+  // Renderer main thread. During that synchronous call, we may not get a
+  // SwapAck for the UI thread, which may prevent BeginMainFrame's from
+  // completing if there's enough back pressure. If the BeginMainFrame can't
+  // make progress, the Renderer can't service the UI thread's synchronous call
+  // and we have deadlock.
+  // This returns true if there's too much backpressure to finish a commit
+  // if we were to initiate a BeginMainFrame.
+  return has_pending_tree_ && active_tree_needs_first_draw_ &&
+         pending_swaps_ >= max_pending_swaps_;
+}
+
 bool SchedulerStateMachine::ShouldSendBeginMainFrame() const {
   if (!CouldSendBeginMainFrame())
     return false;
@@ -411,6 +424,9 @@ bool SchedulerStateMachine::ShouldSendBeginMainFrame() const {
     return false;
 
   // Only send BeginMainFrame when there isn't another commit pending already.
+  // Other parts of the state machine indirectly defer the BeginMainFrame
+  // by transitioning to WAITING commit states rather than going
+  // immediately to IDLE.
   if (commit_state_ != COMMIT_STATE_IDLE)
     return false;
 
@@ -432,6 +448,9 @@ bool SchedulerStateMachine::ShouldSendBeginMainFrame() const {
 
   // We need a new commit for the forced redraw. This honors the
   // single commit per interval because the result will be swapped to screen.
+  // TODO(brianderson): Remove this or move it below the
+  // SendingBeginMainFrameMightCauseDeadlock check since  we want to avoid
+  // ever returning true from this method if we might cause deadlock.
   if (forced_redraw_state_ == FORCED_REDRAW_STATE_WAITING_FOR_COMMIT)
     return true;
 
@@ -439,14 +458,22 @@ bool SchedulerStateMachine::ShouldSendBeginMainFrame() const {
   if (!HasInitializedOutputSurface())
     return false;
 
-  // SwapAck throttle the BeginMainFrames unless we just swapped.
-  // TODO(brianderson): Remove this restriction to improve throughput.
-  bool just_swapped_in_deadline =
-      begin_impl_frame_state_ == BEGIN_IMPL_FRAME_STATE_INSIDE_DEADLINE &&
-      did_perform_swap_in_last_draw_;
-  if (pending_swaps_ >= max_pending_swaps_ && !just_swapped_in_deadline)
+  // Make sure the BeginMainFrame can finish eventually if we start it.
+  if (SendingBeginMainFrameMightCauseDeadlock())
     return false;
 
+  if (!settings_.main_frame_while_swap_throttled_enabled) {
+    // SwapAck throttle the BeginMainFrames unless we just swapped to
+    // potentially improve impl-thread latency over main-thread throughput.
+    // TODO(brianderson): Remove this restriction to improve throughput or
+    // make it conditional on impl_latency_takes_priority_.
+    bool just_swapped_in_deadline =
+        begin_impl_frame_state_ == BEGIN_IMPL_FRAME_STATE_INSIDE_DEADLINE &&
+        did_perform_swap_in_last_draw_;
+    if (pending_swaps_ >= max_pending_swaps_ && !just_swapped_in_deadline)
+      return false;
+  }
+
   if (skip_begin_main_frame_to_reduce_latency_)
     return false;
 
@@ -463,11 +490,14 @@ bool SchedulerStateMachine::ShouldCommit() const {
     return false;
   }
 
-  // Prioritize drawing the previous commit before finishing the next commit.
-  if (active_tree_needs_first_draw_)
-    return false;
+  // If impl-side-painting, commit to the pending tree as soon as we can.
+  if (settings_.impl_side_painting)
+    return true;
 
-  return true;
+  // If we only have an active tree, it is incorrect to replace it
+  // before we've drawn it when we aren't impl-side-painting.
+  DCHECK(!settings_.impl_side_painting);
+  return !active_tree_needs_first_draw_;
 }
 
 bool SchedulerStateMachine::ShouldPrepareTiles() const {

cc/scheduler/scheduler_state_machine.h

@@ -268,6 +268,9 @@ class CC_EXPORT SchedulerStateMachine {
   // True if we need to force activations to make forward progress.
   bool PendingActivationsShouldBeForced() const;
 
+  // TODO(brianderson): Remove this once NPAPI support is removed.
+  bool SendingBeginMainFrameMightCauseDeadlock() const;
+
   bool ShouldAnimate() const;
   bool ShouldBeginOutputSurfaceCreation() const;
   bool ShouldDraw() const;

cc/scheduler/scheduler_state_machine_unittest.cc

@@ -331,10 +331,11 @@ TEST(SchedulerStateMachineTest, MainFrameBeforeActivationEnabled) {
   state.NotifyReadyToCommit();
   EXPECT_ACTION_UPDATE_STATE(SchedulerStateMachine::ACTION_NONE);
 
-  // Verify NotifyReadyToActivate unblocks activation, draw, and
-  // commit in that order.
+  // Verify NotifyReadyToActivate unblocks activation, commit, and
+  // draw in that order.
   state.NotifyReadyToActivate();
   EXPECT_ACTION_UPDATE_STATE(SchedulerStateMachine::ACTION_ACTIVATE_SYNC_TREE);
+  EXPECT_ACTION_UPDATE_STATE(SchedulerStateMachine::ACTION_COMMIT);
   EXPECT_ACTION_UPDATE_STATE(SchedulerStateMachine::ACTION_NONE);
 
   EXPECT_TRUE(state.ShouldTriggerBeginImplFrameDeadlineImmediately());
@@ -344,7 +345,6 @@ TEST(SchedulerStateMachineTest, MainFrameBeforeActivationEnabled) {
       SchedulerStateMachine::ACTION_DRAW_AND_SWAP_IF_POSSIBLE);
   state.DidSwapBuffers();
   state.DidSwapBuffersComplete();
-  EXPECT_ACTION_UPDATE_STATE(SchedulerStateMachine::ACTION_COMMIT);
   EXPECT_ACTION_UPDATE_STATE(SchedulerStateMachine::ACTION_NONE);
   EXPECT_COMMIT_STATE(SchedulerStateMachine::COMMIT_STATE_IDLE);
 }

cc/scheduler/scheduler_unittest.cc

@@ -1380,7 +1380,14 @@ TEST_F(SchedulerTest, NotSkipMainFrameInPreferImplLatencyMode) {
   EXPECT_SCOPED(MainFrameInHighLatencyMode(1, 1, true, true));
 }
 
-TEST_F(SchedulerTest, PollForCommitCompletion) {
+TEST_F(SchedulerTest,
+       Deadlock_NotifyReadyToCommitMakesProgressWhileSwapTrottled) {
+  // NPAPI plugins on Windows block the Browser UI thread on the Renderer main
+  // thread. This prevents the scheduler from receiving any pending swap acks.
+  // This test makes sure that we keep updating the TextureUploader with
+  // DidAnticipatedDrawTimeChange's so that it can make forward progress and
+  // upload all the textures needed for the commit to complete.
+
   // Since we are simulating a long commit, set up a client with draw duration
   // estimates that prevent skipping main frames to get to low latency mode.
   SchedulerClientWithFixedEstimates* client =
@@ -1441,6 +1448,247 @@ TEST_F(SchedulerTest, PollForCommitCompletion) {
   }
 }
 
+TEST_F(
+    SchedulerTest,
+    Deadlock_CommitMakesProgressWhileSwapTrottledAndActiveTreeNeedsFirstDraw) {
+  // NPAPI plugins on Windows block the Browser UI thread on the Renderer main
+  // thread. This prevents the scheduler from receiving any pending swap acks.
+
+  // Since we are simulating a long commit, set up a client with draw duration
+  // estimates that prevent skipping main frames to get to low latency mode.
+  SchedulerClientWithFixedEstimates* client =
+      new SchedulerClientWithFixedEstimates(
+          base::TimeDelta::FromMilliseconds(1),
+          base::TimeDelta::FromMilliseconds(32),
+          base::TimeDelta::FromMilliseconds(32));
+  scheduler_settings_.use_external_begin_frame_source = true;
+  scheduler_settings_.main_frame_while_swap_throttled_enabled = true;
+  scheduler_settings_.impl_side_painting = true;
+  SetUpScheduler(make_scoped_ptr(client).Pass(), true);
+
+  // Disables automatic swap acks so this test can force swap ack throttling
+  // to simulate a blocked Browser ui thread.
+  scheduler_->SetMaxSwapsPending(1);
+  client_->SetAutomaticSwapAck(false);
+
+  // Get a new active tree in main-thread high latency mode and put us
+  // in a swap throttled state.
+  client_->Reset();
+  EXPECT_FALSE(scheduler_->CommitPending());
+  scheduler_->SetNeedsCommit();
+  scheduler_->SetNeedsRedraw();
+  EXPECT_SCOPED(AdvanceFrame());
+  EXPECT_TRUE(scheduler_->CommitPending());
+  EXPECT_TRUE(scheduler_->BeginImplFrameDeadlinePending());
+  task_runner().RunTasksWhile(client_->ImplFrameDeadlinePending(true));
+  EXPECT_FALSE(scheduler_->BeginImplFrameDeadlinePending());
+  scheduler_->NotifyBeginMainFrameStarted();
+  scheduler_->NotifyReadyToCommit();
+  scheduler_->NotifyReadyToActivate();
+  EXPECT_FALSE(scheduler_->CommitPending());
+  EXPECT_ACTION("SetNeedsBeginFrames(true)", client_, 0, 7);
+  EXPECT_ACTION("WillBeginImplFrame", client_, 1, 7);
+  EXPECT_ACTION("ScheduledActionAnimate", client_, 2, 7);
+  EXPECT_ACTION("ScheduledActionSendBeginMainFrame", client_, 3, 7);
+  EXPECT_ACTION("ScheduledActionDrawAndSwapIfPossible", client_, 4, 7);
+  EXPECT_ACTION("ScheduledActionCommit", client_, 5, 7);
+  EXPECT_ACTION("ScheduledActionActivateSyncTree", client_, 6, 7);
+
+  // Make sure that we can finish the next commit even while swap throttled.
+  client_->Reset();
+  EXPECT_FALSE(scheduler_->CommitPending());
+  scheduler_->SetNeedsCommit();
+  EXPECT_SCOPED(AdvanceFrame());
+  scheduler_->NotifyBeginMainFrameStarted();
+  scheduler_->NotifyReadyToCommit();
+  scheduler_->NotifyReadyToActivate();
+  EXPECT_TRUE(scheduler_->BeginImplFrameDeadlinePending());
+  task_runner().RunTasksWhile(client_->ImplFrameDeadlinePending(true));
+  EXPECT_FALSE(scheduler_->BeginImplFrameDeadlinePending());
+  EXPECT_ACTION("WillBeginImplFrame", client_, 0, 5);
+  EXPECT_ACTION("ScheduledActionAnimate", client_, 1, 5);
+  EXPECT_ACTION("ScheduledActionSendBeginMainFrame", client_, 2, 5);
+  EXPECT_ACTION("ScheduledActionCommit", client_, 3, 5);
+  EXPECT_ACTION("ScheduledActionAnimate", client_, 4, 5);
+
+  // Make sure we do not send a BeginMainFrame while swap throttled and
+  // we have both a pending tree and an active tree.
+  client_->Reset();
+  EXPECT_FALSE(scheduler_->CommitPending());
+  scheduler_->SetNeedsCommit();
+  EXPECT_SCOPED(AdvanceFrame());
+  EXPECT_FALSE(scheduler_->CommitPending());
+  task_runner().RunPendingTasks();  // Run posted deadline.
+  EXPECT_ACTION("WillBeginImplFrame", client_, 0, 2);
+  EXPECT_ACTION("ScheduledActionAnimate", client_, 1, 2);
+}
+
+TEST_F(SchedulerTest,
+       Deadlock_NoBeginMainFrameWhileSwapTrottledAndPipelineFull) {
+  // NPAPI plugins on Windows block the Browser UI thread on the Renderer main
+  // thread. This prevents the scheduler from receiving any pending swap acks.
+
+  // This particular test makes sure we do not send a BeginMainFrame while
+  // swap trottled and we have a pending tree and active tree that
+  // still needs to be drawn for the first time.
+
+  // Since we are simulating a long commit, set up a client with draw duration
+  // estimates that prevent skipping main frames to get to low latency mode.
+  SchedulerClientWithFixedEstimates* client =
+      new SchedulerClientWithFixedEstimates(
+          base::TimeDelta::FromMilliseconds(1),
+          base::TimeDelta::FromMilliseconds(32),
+          base::TimeDelta::FromMilliseconds(32));
+  scheduler_settings_.use_external_begin_frame_source = true;
+  scheduler_settings_.main_frame_while_swap_throttled_enabled = true;
+  scheduler_settings_.main_frame_before_activation_enabled = true;
+  scheduler_settings_.impl_side_painting = true;
+  SetUpScheduler(make_scoped_ptr(client).Pass(), true);
+
+  // Disables automatic swap acks so this test can force swap ack throttling
+  // to simulate a blocked Browser ui thread.
+  scheduler_->SetMaxSwapsPending(1);
+  client_->SetAutomaticSwapAck(false);
+
+  // Start a new commit in main-thread high latency mode and hold off on
+  // activation.
+  client_->Reset();
+  EXPECT_FALSE(scheduler_->CommitPending());
+  scheduler_->SetNeedsCommit();
+  scheduler_->SetNeedsRedraw();
+  EXPECT_SCOPED(AdvanceFrame());
+  EXPECT_TRUE(scheduler_->CommitPending());
+  EXPECT_TRUE(scheduler_->BeginImplFrameDeadlinePending());
+  task_runner().RunTasksWhile(client_->ImplFrameDeadlinePending(true));
+  EXPECT_FALSE(scheduler_->BeginImplFrameDeadlinePending());
+  scheduler_->DidSwapBuffersComplete();
+  scheduler_->NotifyBeginMainFrameStarted();
+  scheduler_->NotifyReadyToCommit();
+  EXPECT_FALSE(scheduler_->CommitPending());
+  EXPECT_ACTION("SetNeedsBeginFrames(true)", client_, 0, 6);
+  EXPECT_ACTION("WillBeginImplFrame", client_, 1, 6);
+  EXPECT_ACTION("ScheduledActionAnimate", client_, 2, 6);
+  EXPECT_ACTION("ScheduledActionSendBeginMainFrame", client_, 3, 6);
+  EXPECT_ACTION("ScheduledActionDrawAndSwapIfPossible", client_, 4, 6);
+  EXPECT_ACTION("ScheduledActionCommit", client_, 5, 6);
+
+  // Start another commit while we still have aa pending tree.
+  // Enter a swap throttled state.
+  client_->Reset();
+  EXPECT_FALSE(scheduler_->CommitPending());
+  scheduler_->SetNeedsCommit();
+  scheduler_->SetNeedsRedraw();
+  EXPECT_SCOPED(AdvanceFrame());
+  EXPECT_TRUE(scheduler_->CommitPending());
+  EXPECT_TRUE(scheduler_->BeginImplFrameDeadlinePending());
+  task_runner().RunTasksWhile(client_->ImplFrameDeadlinePending(true));
+  EXPECT_FALSE(scheduler_->BeginImplFrameDeadlinePending());
+  scheduler_->NotifyBeginMainFrameStarted();
+  EXPECT_ACTION("WillBeginImplFrame", client_, 0, 4);
+  EXPECT_ACTION("ScheduledActionAnimate", client_, 1, 4);
+  EXPECT_ACTION("ScheduledActionSendBeginMainFrame", client_, 2, 4);
+  EXPECT_ACTION("ScheduledActionDrawAndSwapIfPossible", client_, 3, 4);
+
+  // Can't commit yet because there's still a pending tree.
+  client_->Reset();
+  scheduler_->NotifyReadyToCommit();
+  EXPECT_NO_ACTION(client_);
+
+  // Activate the pending tree, which also unblocks the commit immediately.
+  client_->Reset();
+  scheduler_->NotifyReadyToActivate();
+  EXPECT_ACTION("ScheduledActionActivateSyncTree", client_, 0, 2);
+  EXPECT_ACTION("ScheduledActionCommit", client_, 1, 2);
+
+  // Make sure we do not send a BeginMainFrame while swap throttled and
+  // we have both a pending tree and an active tree that still needs
+  // it's first draw.
+  client_->Reset();
+  EXPECT_FALSE(scheduler_->CommitPending());
+  scheduler_->SetNeedsCommit();
+  EXPECT_SCOPED(AdvanceFrame());
+  EXPECT_FALSE(scheduler_->CommitPending());
+  task_runner().RunPendingTasks();  // Run posted deadline.
+  EXPECT_ACTION("WillBeginImplFrame", client_, 0, 2);
+  EXPECT_ACTION("ScheduledActionAnimate", client_, 1, 2);
+}
+
+TEST_F(
+    SchedulerTest,
+    CommitMakesProgressWhenIdleAndHasPendingTreeAndActiveTreeNeedsFirstDraw) {
+  // This verifies we don't block commits longer than we need to
+  // for performance reasons - not deadlock reasons.
+
+  // Since we are simulating a long commit, set up a client with draw duration
+  // estimates that prevent skipping main frames to get to low latency mode.
+  SchedulerClientWithFixedEstimates* client =
+      new SchedulerClientWithFixedEstimates(
+          base::TimeDelta::FromMilliseconds(1),
+          base::TimeDelta::FromMilliseconds(32),
+          base::TimeDelta::FromMilliseconds(32));
+  scheduler_settings_.use_external_begin_frame_source = true;
+  scheduler_settings_.main_frame_while_swap_throttled_enabled = true;
+  scheduler_settings_.main_frame_before_activation_enabled = true;
+  scheduler_settings_.impl_side_painting = true;
+  SetUpScheduler(make_scoped_ptr(client).Pass(), true);
+
+  // Disables automatic swap acks so this test can force swap ack throttling
+  // to simulate a blocked Browser ui thread.
+  scheduler_->SetMaxSwapsPending(1);
+  client_->SetAutomaticSwapAck(false);
+
+  // Start a new commit in main-thread high latency mode and hold off on
+  // activation.
+  client_->Reset();
+  EXPECT_FALSE(scheduler_->CommitPending());
+  scheduler_->SetNeedsCommit();
+  scheduler_->SetNeedsRedraw();
+  EXPECT_SCOPED(AdvanceFrame());
+  EXPECT_TRUE(scheduler_->CommitPending());
+  EXPECT_TRUE(scheduler_->BeginImplFrameDeadlinePending());
+  task_runner().RunTasksWhile(client_->ImplFrameDeadlinePending(true));
+  EXPECT_FALSE(scheduler_->BeginImplFrameDeadlinePending());
+  scheduler_->DidSwapBuffersComplete();
+  scheduler_->NotifyBeginMainFrameStarted();
+  scheduler_->NotifyReadyToCommit();
+  EXPECT_FALSE(scheduler_->CommitPending());
+  EXPECT_ACTION("SetNeedsBeginFrames(true)", client_, 0, 6);
+  EXPECT_ACTION("WillBeginImplFrame", client_, 1, 6);
+  EXPECT_ACTION("ScheduledActionAnimate", client_, 2, 6);
+  EXPECT_ACTION("ScheduledActionSendBeginMainFrame", client_, 3, 6);
+  EXPECT_ACTION("ScheduledActionDrawAndSwapIfPossible", client_, 4, 6);
+  EXPECT_ACTION("ScheduledActionCommit", client_, 5, 6);
+
+  // Start another commit while we still have an active tree.
+  client_->Reset();
+  EXPECT_FALSE(scheduler_->CommitPending());
+  scheduler_->SetNeedsCommit();
+  scheduler_->SetNeedsRedraw();
+  EXPECT_SCOPED(AdvanceFrame());
+  EXPECT_TRUE(scheduler_->CommitPending());
+  EXPECT_TRUE(scheduler_->BeginImplFrameDeadlinePending());
+  task_runner().RunTasksWhile(client_->ImplFrameDeadlinePending(true));
+  EXPECT_FALSE(scheduler_->BeginImplFrameDeadlinePending());
+  scheduler_->DidSwapBuffersComplete();
+  scheduler_->NotifyBeginMainFrameStarted();
+  EXPECT_ACTION("WillBeginImplFrame", client_, 0, 4);
+  EXPECT_ACTION("ScheduledActionAnimate", client_, 1, 4);
+  EXPECT_ACTION("ScheduledActionSendBeginMainFrame", client_, 2, 4);
+  EXPECT_ACTION("ScheduledActionDrawAndSwapIfPossible", client_, 3, 4);
+
+  // Can't commit yet because there's still a pending tree.
+  client_->Reset();
+  scheduler_->NotifyReadyToCommit();
+  EXPECT_NO_ACTION(client_);
+
+  // Activate the pending tree, which also unblocks the commit immediately
+  // while we are in an idle state.
+  client_->Reset();
+  scheduler_->NotifyReadyToActivate();
+  EXPECT_ACTION("ScheduledActionActivateSyncTree", client_, 0, 2);
+  EXPECT_ACTION("ScheduledActionCommit", client_, 1, 2);
+}
+
 TEST_F(SchedulerTest, BeginRetroFrame) {
   scheduler_settings_.use_external_begin_frame_source = true;
   SetUpScheduler(true);