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..810b83ec667db6515b063eb7cac521157cec6ffe 100644
--- a/cc/scheduler/scheduler_state_machine.cc
+++ b/cc/scheduler/scheduler_state_machine.cc
@@ -411,6 +411,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;
 
@@ -439,14 +442,24 @@ 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)
+  // We risk deadlock with NPAPI on Windows if we get *this* far ahead of

[sunnyps, 2015/05/14 00:02:01]

How important is this given that NPAPI will be gone soon? Also, is there a bug
for this?

[brianderson, 2015/05/14 01:12:03]

Not super important, but NPAPI won't be completely gone until September.

+  // ourselves. Don't do it.
+  if (pending_swaps_ >= max_pending_swaps_ && has_pending_tree_ &&
+      active_tree_needs_first_draw_)
     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 +476,25 @@ 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 (settings_.impl_side_painting) {
+    // Prioritize drawing the previous commit before finishing the next commit,
+    // but only if the draw is imminent.
+    // Note: We must not block the commit indefinitely when swap throttled and
+    // active_tree_needs_first_draw_ is true, otherwise we could deadlock with
+    // NPAPI plugins on Windows. The commit could be blocking the Browser
+    // UI thread, which prevents it from sending us a swap ack.
+    bool active_tree_first_draw_is_imminent =
+        active_tree_needs_first_draw_ &&
+        begin_impl_frame_state_ != BEGIN_IMPL_FRAME_STATE_IDLE &&
+        pending_swaps_ < max_pending_swaps_;
+    if (active_tree_first_draw_is_imminent)
+      return false;

[sunnyps, 2015/05/14 00:02:01]

This will break webview. There's no concept of an "imminent" draw in webview. A
draw might never come.

[brianderson, 2015/05/14 01:12:03]

I think active_tree_first_draw_is_imminent will almost always be false for
WebView:

At ToT, active_tree_needs_first_draw_ goes false on invalidation, which will
make active_tree_first_draw_is_imminent false.

Additionally, I think we go to the BEGIN_IMPL_FRAME_STATE_IDLE immediately after
the BeginFrame for WebView, which will also make
active_tree_first_draw_is_imminent false.

+  } else {

[sunnyps, 2015/05/14 00:02:01]

nit: else if (...) {

[brianderson, 2015/05/14 01:12:02]

Done.

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