cc: Avoid deadlock with the UI thread and NPAPI in more cases

cc/scheduler/scheduler_state_machine.cc

Index: cc/scheduler/scheduler_state_machine.cc
diff --git a/cc/scheduler/scheduler_state_machine.cc b/cc/scheduler/scheduler_state_machine.cc
index 2fbeca2d8a992d87cd118a09a78eeb5db8b61d3b..b89668d25c5344a6cee44ca8746ed5ef2c4e5483 100644
--- a/cc/scheduler/scheduler_state_machine.cc
+++ b/cc/scheduler/scheduler_state_machine.cc
@@ -402,6 +402,20 @@ 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
+  // BeginMainFrame if we were to initiate one.

[sunnyps, 2015/05/20 19:26:51]

nit: finish a commit if we were to initiate a BeginMainFrame.

[brianderson, 2015/05/20 19:49:47]

Done.

+  // TODO(brianderson): Remove this once NPAPI support is removed.

[sunnyps, 2015/05/20 19:26:51]

nit: Move this TODO to the header.

[brianderson, 2015/05/20 19:49:46]

Done.

+  return has_pending_tree_ && active_tree_needs_first_draw_ &&
+         pending_swaps_ >= max_pending_swaps_;
+}
+
 bool SchedulerStateMachine::ShouldSendBeginMainFrame() const {
   if (!CouldSendBeginMainFrame())
     return false;
@@ -411,6 +425,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 +449,7 @@ 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 deadlock check.

[sunnyps, 2015/05/20 19:26:50]

nit: Why?

[brianderson, 2015/05/20 19:49:47]

Updated comment. We never want to return true from this method if we might
deadlock.

   if (forced_redraw_state_ == FORCED_REDRAW_STATE_WAITING_FOR_COMMIT)
     return true;
 
@@ -439,14 +457,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 will 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,9 +489,13 @@ bool SchedulerStateMachine::ShouldCommit() const {
     return false;
   }
 
-  // Prioritize drawing the previous commit before finishing the next commit.
-  if (active_tree_needs_first_draw_)
+  if (settings_.impl_side_painting) {
+    return true;
+  } else if (active_tree_needs_first_draw_) {

[sunnyps, 2015/05/20 19:26:50]

nit:
if (!settings_.impl_side_painting && active_tree_needs_first_draw_) {
  return false;
}

[brianderson, 2015/05/20 19:49:46]

Changed the organization a bit, but not exactly suggested. Used a
DCHECK(!settings_.impl_side_painting) instead.

+    // If we only have an active tree, it is incorrect to replace it
+    // before we've drawn it.
     return false;
+  }
 
   return true;
 }