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

cc/scheduler/scheduler_state_machine.cc

 // Copyright 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "cc/scheduler/scheduler_state_machine.h"
 
 #include "base/format_macros.h"
 #include "base/logging.h"
 #include "base/strings/stringprintf.h"
 #include "base/trace_event/trace_event.h"
@@ skipping 393 matching lines @@
 
 bool SchedulerStateMachine::ShouldSendBeginMainFrame() const {
   if (!CouldSendBeginMainFrame())
     return false;
 
   // Do not send begin main frame too many times in a single frame.
   if (send_begin_main_frame_funnel_)
     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;
 
   // Don't send BeginMainFrame early if we are prioritizing the active tree
   // because of impl_latency_takes_priority_.
   if (impl_latency_takes_priority_ &&
       (has_pending_tree_ || active_tree_needs_first_draw_)) {
     return false;
   }
 
   // We should not send BeginMainFrame while we are in
   // BEGIN_IMPL_FRAME_STATE_IDLE since we might have new
   // user input arriving soon.
   // TODO(brianderson): Allow sending BeginMainFrame while idle when the main
   // thread isn't consuming user input.
   if (begin_impl_frame_state_ == BEGIN_IMPL_FRAME_STATE_IDLE &&
       BeginFrameNeeded())
     return false;
 
   // We need a new commit for the forced redraw. This honors the
   // single commit per interval because the result will be swapped to screen.
   if (forced_redraw_state_ == FORCED_REDRAW_STATE_WAITING_FOR_COMMIT)
     return true;
 
   // We shouldn't normally accept commits if there isn't an OutputSurface.
   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;
 
   return true;
 }
 
 bool SchedulerStateMachine::ShouldCommit() const {
   if (commit_state_ != COMMIT_STATE_READY_TO_COMMIT)
     return false;
 
   // We must not finish the commit until the pending tree is free.
   if (has_pending_tree_) {
     DCHECK(settings_.main_frame_before_activation_enabled);
     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;
 }
 
 bool SchedulerStateMachine::ShouldPrepareTiles() const {
   // PrepareTiles only really needs to be called immediately after commit
   // and then periodically after that. Use a funnel to make sure we average
   // one PrepareTiles per BeginImplFrame in the long run.
   if (prepare_tiles_funnel_ > 0)
     return false;
@@ skipping 659 matching lines @@
       static_cast<int>(begin_impl_frame_state_),
       static_cast<int>(commit_state_),
       has_pending_tree_ ? 'T' : 'F',
       pending_tree_is_ready_for_activation_ ? 'T' : 'F',
       active_tree_needs_first_draw_ ? 'T' : 'F',
       max_pending_swaps_,
       pending_swaps_);
 }
 
 }  // namespace cc