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..f90636cf275584bd5d5610468d793d46a600b3dc 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
+  // 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 =

[sunnyps, 2015/05/19 02:36:12]

Isn't this just a different way of saying that the main thread is in a high
latency mode? I've always felt this logic is really complicated.

[brianderson, 2015/05/19 20:06:24]

I looked into it and it's not quite the same since we could be in a high latency
mode before the deadline too.

This patch should enable us to get rid of this block of code, I just don't want
to do it as part of this patch.

Mind if we just keep it this way until it gets removed?

+        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,24 @@ 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) {

[sunnyps, 2015/05/19 02:36:12]

Discussed this offline with brianderson:

It seems this logic exists to prevent commit from happening before draw when
both could happen in the same call to NextAction. It might be easier to reorder
draw and commit in NextAction than implement this really complicated logic. That
also has benefits for webview when it gets nested message loops.

[brianderson, 2015/05/19 20:06:24]

I tried reordering draw and commit in NextAction. It had a few side effects, but
nothing that really affected the draw/commit order since we only draw in the
deadline anyway. So I just returned true here in the newest patch and everything
ends up being okay!

Just returning true did affect main_frame_before_activation_enabled (since
activation would unblock the commit immediately before the deadline), but I
think prioritizing the commit over the draw in that case makes sense since
main_frame_before_activation_enabled could cause the main thread to block for a
long time.

+    // 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.

[mithro-old, 2015/05/19 01:45:32]

Is what happening here is that;

  * The browser UI thread is waiting on a commit before it will send a swap ack.
  * The renderer Impl thread is waiting on a swap ack before letting a commit
proceed.

From your comment it sounds like the browser UI thread only blocks waiting on a
commit when there is a NPAPI plugin on Windows?

Is that all correct?

+    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;
+  } else if (active_tree_needs_first_draw_) {
+    // If we only have an active tree, it is incorrect to replace it
+    // before we've drawn it.
     return false;
+  }
 
   return true;
 }