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

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
Committed: https://crrev.com/2bcaa68bdea4d8dc89641992b7a0b6570d8d4d26
Cr-Commit-Position: refs/heads/master@{#330808}

[brianderson, 2015-05-13 23:28:46]

brianderson@chromium.org changed reviewers:
+ mithro@mithis.com, sunnyps@chromium.org

[brianderson, 2015-05-13 23:28:47]

Ptal.

This is a replacement for https://codereview.chromium.org/1100943003

[sunnyps, 2015-05-14 00:02:02]

Left a few comments.

https://codereview.chromium.org/1139613003/diff/1/cc/scheduler/scheduler_stat...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/1139613003/diff/1/cc/scheduler/scheduler_stat...
cc/scheduler/scheduler_state_machine.cc:445: // We risk deadlock with NPAPI on
Windows if we get *this* far ahead of
How important is this given that NPAPI will be gone soon? Also, is there a bug
for this?

https://codereview.chromium.org/1139613003/diff/1/cc/scheduler/scheduler_stat...
cc/scheduler/scheduler_state_machine.cc:491: return false;
This will break webview. There's no concept of an "imminent" draw in webview. A
draw might never come.

https://codereview.chromium.org/1139613003/diff/1/cc/scheduler/scheduler_stat...
cc/scheduler/scheduler_state_machine.cc:492: } else {
nit: else if (...) {

https://codereview.chromium.org/1139613003/diff/1/cc/scheduler/scheduler_unit...
File cc/scheduler/scheduler_unittest.cc (right):

https://codereview.chromium.org/1139613003/diff/1/cc/scheduler/scheduler_unit...
cc/scheduler/scheduler_unittest.cc:1382:
Deadlock_NotifyReadyToCommitMakesProgressWhileSwapTrottled) {
Does this allow us to remove commit polling?

https://codereview.chromium.org/1139613003/diff/1/cc/scheduler/scheduler_unit...
cc/scheduler/scheduler_unittest.cc:1483: task_runner().RunPendingTasks();  //
Run posted deadlines.
RunTasksWhile(DeadlinePending).

https://codereview.chromium.org/1139613003/diff/1/cc/scheduler/scheduler_unit...
cc/scheduler/scheduler_unittest.cc:1489:
EXPECT_TRUE(client->HasAction("ScheduledActionSendBeginMainFrame"));
EXPECT_ACTION?

https://codereview.chromium.org/1139613003/diff/1/cc/scheduler/scheduler_unit...
cc/scheduler/scheduler_unittest.cc:1505:
EXPECT_TRUE(client->HasAction("ScheduledActionSendBeginMainFrame"));
EXPECT_ACTION?

https://codereview.chromium.org/1139613003/diff/1/cc/scheduler/scheduler_unit...
cc/scheduler/scheduler_unittest.cc:1521:
Deadlock_CommitMakesProgressWhenIdleAndActiveTreeNeedsFirstDraw) {
I think this test is for the "HavePendingTree" case.

https://codereview.chromium.org/1139613003/diff/1/cc/scheduler/scheduler_unit...
cc/scheduler/scheduler_unittest.cc:1554: task_runner().RunPendingTasks();  //
Run posted deadlines.
RunTasksWhile(DeadlinePending).

https://codereview.chromium.org/1139613003/diff/1/cc/scheduler/scheduler_unit...
cc/scheduler/scheduler_unittest.cc:1560:
EXPECT_TRUE(client->HasAction("ScheduledActionSendBeginMainFrame"));
EXPECT_ACTION?

https://codereview.chromium.org/1139613003/diff/1/cc/scheduler/scheduler_unit...
cc/scheduler/scheduler_unittest.cc:1592:
Deadlock_NoBeginMainFrameWhileSwapTrottledAndHavePendingTree) {
This is the "ActiveTreeNeedsFirstDraw" case.

https://codereview.chromium.org/1139613003/diff/1/cc/scheduler/scheduler_unit...
cc/scheduler/scheduler_unittest.cc:1674:
Deadlock_NoBeginMainFrameWhileSwapTrottledAndActiveTreeNeedsFirstDraw) {
In this case BeginMainFrame is blocked because of a pending tree and not because
of active_tree_needs_first_draw.

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

I'll harden the test expectations in the next patch set. It was easier to hack
on with the looser expectations.

https://codereview.chromium.org/1139613003/diff/1/cc/scheduler/scheduler_stat...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/1139613003/diff/1/cc/scheduler/scheduler_stat...
cc/scheduler/scheduler_state_machine.cc:445: // We risk deadlock with NPAPI on
Windows if we get *this* far ahead of
On 2015/05/14 00:02:01, sunnyps wrote:
> How important is this given that NPAPI will be gone soon? Also, is there a bug
> for this?

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

https://codereview.chromium.org/1139613003/diff/1/cc/scheduler/scheduler_stat...
cc/scheduler/scheduler_state_machine.cc:491: return false;
On 2015/05/14 00:02:01, sunnyps wrote:
> This will break webview. There's no concept of an "imminent" draw in webview.
A
> draw might never come.

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.

https://codereview.chromium.org/1139613003/diff/1/cc/scheduler/scheduler_stat...
cc/scheduler/scheduler_state_machine.cc:492: } else {
On 2015/05/14 00:02:01, sunnyps wrote:
> nit: else if (...) {

Done.

https://codereview.chromium.org/1139613003/diff/1/cc/scheduler/scheduler_unit...
File cc/scheduler/scheduler_unittest.cc (right):

https://codereview.chromium.org/1139613003/diff/1/cc/scheduler/scheduler_unit...
cc/scheduler/scheduler_unittest.cc:1382:
Deadlock_NotifyReadyToCommitMakesProgressWhileSwapTrottled) {
On 2015/05/14 at 00:02:02, sunnyps wrote:
> Does this allow us to remove commit polling?

We can remove commit polling once TextureUploader is removed or NPAPI is
completely gone in September.

Now that I think about it, I think we can already get rid of commit polling -
ever since I moved the BeginFrame suppression logic from the OutputSurface to
the Scheduler swap acks won't suppress BeginFrames. Unfortunately, this problem
will come back when we have unified BeginFrame, so we shouldn't actually remove
it yet.

https://codereview.chromium.org/1139613003/diff/1/cc/scheduler/scheduler_unit...
cc/scheduler/scheduler_unittest.cc:1521:
Deadlock_CommitMakesProgressWhenIdleAndActiveTreeNeedsFirstDraw) {
On 2015/05/14 at 00:02:02, sunnyps wrote:
> I think this test is for the "HavePendingTree" case.

It's also both. Will rename.

https://codereview.chromium.org/1139613003/diff/1/cc/scheduler/scheduler_unit...
cc/scheduler/scheduler_unittest.cc:1592:
Deadlock_NoBeginMainFrameWhileSwapTrottledAndHavePendingTree) {
On 2015/05/14 at 00:02:02, sunnyps wrote:
> This is the "ActiveTreeNeedsFirstDraw" case.

It's both, I will change the name to:
Deadlock_NoBeginMainFrameWhileSwapTrottledAndHavePendingTreeAndActiveTreeNeedsFirstDraw

https://codereview.chromium.org/1139613003/diff/1/cc/scheduler/scheduler_unit...
cc/scheduler/scheduler_unittest.cc:1674:
Deadlock_NoBeginMainFrameWhileSwapTrottledAndActiveTreeNeedsFirstDraw) {
On 2015/05/14 at 00:02:02, sunnyps wrote:
> In this case BeginMainFrame is blocked because of a pending tree and not
because of active_tree_needs_first_draw.

Sorry, my comments in the test were wrong. This test is for impl_side_painting =
false, where we wouldn't have a pending tree. I guess this isn't a supported
configuration for the Renderer anymore - so maybe I should just delete this
test.

[brianderson, 2015-05-18 20:09:41]

Ptal, again.

I split out the tests into two that only apply to NPAPI deadlock cases and one
that applies to general performance and not specifically NPAPI deadlock cases.

[sunnyps, 2015-05-18 22:40:39]

On 2015/05/18 20:09:41, brianderson wrote:
> Ptal, again.
> 
> I split out the tests into two that only apply to NPAPI deadlock cases and one
> that applies to general performance and not specifically NPAPI deadlock cases.

Tests look ok but I wonder if we should allow sending a BeginMainFrame while
there's a pending tree even when we are not swap throttled. If activation isn't
done by the time the main thread is ready to commit, it's going to end up being
blocked in commit. WDYT?

[brianderson, 2015-05-18 22:57:35]

On 2015/05/18 at 22:40:39, sunnyps wrote:
> On 2015/05/18 20:09:41, brianderson wrote:
> > Ptal, again.
> > 
> > I split out the tests into two that only apply to NPAPI deadlock cases and
one
> > that applies to general performance and not specifically NPAPI deadlock
cases.
> 
> Tests look ok but I wonder if we should allow sending a BeginMainFrame while
there's a pending tree even when we are not swap throttled. If activation isn't
done by the time the main thread is ready to commit, it's going to end up being
blocked in commit. WDYT?

main_frame_before_activation_enabled is currently enabled by command line flag
only for that very reason: avoiding main-thread blocking is going to take a lot
of work to avoid.

It's another one of those code paths we should either push forward or delete.
I've definitely seen it help some use cases (and Victor has seen it help Ganesh
in some cases), so I'm inclined to keep it around for testing purposes. If it
complicates the code though (like BeginRetroFrames do), I'd be fine with
deleting it and revisiting it later.

Would it be okay to figure that out in a followup bug/patch?

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

https://codereview.chromium.org/1139613003/diff/80001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/1139613003/diff/80001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:485: // UI thread, which prevents it
from sending us a swap ack.
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?

[brianderson, 2015-05-19 02:00:24]

On 2015/05/19 at 01:45:32, mithro wrote:
>
https://codereview.chromium.org/1139613003/diff/80001/cc/scheduler/scheduler_...
> File cc/scheduler/scheduler_state_machine.cc (right):
> 
>
https://codereview.chromium.org/1139613003/diff/80001/cc/scheduler/scheduler_...
> cc/scheduler/scheduler_state_machine.cc:485: // UI thread, which prevents it
from sending us a swap ack.
> 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?

Yes, that is correct. NPAPI is evil like that.

[mithro-old, 2015-05-19 02:08:01]

Maybe it would be worth moving the logic for detecting this into a
"BrowserMightBeBlockedOnCommit()" or similar method?

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

nit: commit message should start with "cc: "

https://codereview.chromium.org/1139613003/diff/80001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/1139613003/diff/80001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:456: bool just_swapped_in_deadline =
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.

https://codereview.chromium.org/1139613003/diff/80001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:479: if (settings_.impl_side_painting) {
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]

ptal

> Maybe it would be worth moving the logic for detecting this into a
"BrowserMightBeBlockedOnCommit()" or similar method?

Done.

https://codereview.chromium.org/1139613003/diff/80001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/1139613003/diff/80001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:456: bool just_swapped_in_deadline =
On 2015/05/19 02:36:12, sunnyps wrote:
> 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.

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?

https://codereview.chromium.org/1139613003/diff/80001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:479: if (settings_.impl_side_painting) {
On 2015/05/19 02:36:12, sunnyps wrote:
> 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.

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.

[sunnyps, 2015-05-20 19:26:38]

On 2015/05/19 20:06:24, brianderson wrote:
> ptal
> 
> > Maybe it would be worth moving the logic for detecting this into a
> "BrowserMightBeBlockedOnCommit()" or similar method?
> 
> Done.
> 
>
https://codereview.chromium.org/1139613003/diff/80001/cc/scheduler/scheduler_...
> File cc/scheduler/scheduler_state_machine.cc (right):
> 
>
https://codereview.chromium.org/1139613003/diff/80001/cc/scheduler/scheduler_...
> cc/scheduler/scheduler_state_machine.cc:456: bool just_swapped_in_deadline =
> On 2015/05/19 02:36:12, sunnyps wrote:
> > 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.
> 
> 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?

Sure, that's ok.

> 
>
https://codereview.chromium.org/1139613003/diff/80001/cc/scheduler/scheduler_...
> cc/scheduler/scheduler_state_machine.cc:479: if (settings_.impl_side_painting)
{
> On 2015/05/19 02:36:12, sunnyps wrote:
> > 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.
> 
> 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.

The commit could take some amount of time because of the implicit PrepareTiles
call. I thought that was the reason we wanted to prioritize draw over commit.
I'm fine with both options here.

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

LGTM with nits.

https://codereview.chromium.org/1139613003/diff/100001/cc/scheduler/scheduler...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/1139613003/diff/100001/cc/scheduler/scheduler...
cc/scheduler/scheduler_state_machine.cc:413: // BeginMainFrame if we were to
initiate one.
nit: finish a commit if we were to initiate a BeginMainFrame.

https://codereview.chromium.org/1139613003/diff/100001/cc/scheduler/scheduler...
cc/scheduler/scheduler_state_machine.cc:414: // TODO(brianderson): Remove this
once NPAPI support is removed.
nit: Move this TODO to the header.

https://codereview.chromium.org/1139613003/diff/100001/cc/scheduler/scheduler...
cc/scheduler/scheduler_state_machine.cc:452: // TODO(brianderson): Remove this
or move it below the deadlock check.
nit: Why?

https://codereview.chromium.org/1139613003/diff/100001/cc/scheduler/scheduler...
cc/scheduler/scheduler_state_machine.cc:494: } else if
(active_tree_needs_first_draw_) {
nit:
if (!settings_.impl_side_painting && active_tree_needs_first_draw_) {
  return false;
}

https://codereview.chromium.org/1139613003/diff/100001/cc/scheduler/scheduler...
File cc/scheduler/scheduler_unittest.cc (right):

https://codereview.chromium.org/1139613003/diff/100001/cc/scheduler/scheduler...
cc/scheduler/scheduler_unittest.cc:1573: // Start another commit while we still
have an active tree.
nit: while we have a pending tree.

https://codereview.chromium.org/1139613003/diff/100001/cc/scheduler/scheduler...
cc/scheduler/scheduler_unittest.cc:1675: scheduler_->DidSwapBuffersComplete();
nit: Move the DidSwapBuffersComplete call above the NotifyBeginMainFrameStarted
call for consistency.

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

Comments addressed.

> The commit could take some amount of time because of the implicit PrepareTiles
call. I thought that was the reason we wanted to prioritize draw over commit.

That was the intent, but it doesn't really do anything at ToT since we will
never get the NotifyReadyToCommit in a state where we will draw immediately
because NotifyReadyToCommit will never arrive inside the deadline.

If we actually want to defer the commit when the deadline is arriving soon, we
will need to do something more involved. There are tradeoffs, so I'm inclined to
stay away from tackling that until there's data.

https://codereview.chromium.org/1139613003/diff/100001/cc/scheduler/scheduler...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/1139613003/diff/100001/cc/scheduler/scheduler...
cc/scheduler/scheduler_state_machine.cc:413: // BeginMainFrame if we were to
initiate one.
On 2015/05/20 at 19:26:51, sunnyps wrote:
> nit: finish a commit if we were to initiate a BeginMainFrame.

Done.

https://codereview.chromium.org/1139613003/diff/100001/cc/scheduler/scheduler...
cc/scheduler/scheduler_state_machine.cc:414: // TODO(brianderson): Remove this
once NPAPI support is removed.
On 2015/05/20 at 19:26:51, sunnyps wrote:
> nit: Move this TODO to the header.

Done.

https://codereview.chromium.org/1139613003/diff/100001/cc/scheduler/scheduler...
cc/scheduler/scheduler_state_machine.cc:452: // TODO(brianderson): Remove this
or move it below the deadlock check.
On 2015/05/20 at 19:26:50, sunnyps wrote:
> nit: Why?

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

https://codereview.chromium.org/1139613003/diff/100001/cc/scheduler/scheduler...
cc/scheduler/scheduler_state_machine.cc:494: } else if
(active_tree_needs_first_draw_) {
On 2015/05/20 at 19:26:50, sunnyps wrote:
> nit:
> if (!settings_.impl_side_painting && active_tree_needs_first_draw_) {
>   return false;
> }

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

https://codereview.chromium.org/1139613003/diff/100001/cc/scheduler/scheduler...
File cc/scheduler/scheduler_unittest.cc (right):

https://codereview.chromium.org/1139613003/diff/100001/cc/scheduler/scheduler...
cc/scheduler/scheduler_unittest.cc:1573: // Start another commit while we still
have an active tree.
On 2015/05/20 at 19:26:51, sunnyps wrote:
> nit: while we have a pending tree.

Done.

https://codereview.chromium.org/1139613003/diff/100001/cc/scheduler/scheduler...
cc/scheduler/scheduler_unittest.cc:1675: scheduler_->DidSwapBuffersComplete();
On 2015/05/20 at 19:26:51, sunnyps wrote:
> nit: Move the DidSwapBuffersComplete call above the
NotifyBeginMainFrameStarted call for consistency.

Done.

[brianderson, 2015-05-20 19:51:48]

The CQ bit was checked by brianderson@chromium.org

[brianderson, 2015-05-20 19:51:49]

The patchset sent to the CQ was uploaded after l-g-t-m from sunnyps@chromium.org
Link to the patchset: https://codereview.chromium.org/1139613003/#ps120001
(title: "address Sunny's comments")

[commit-bot: I haz the power, 2015-05-20 19:52:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1139613003/120001

[commit-bot: I haz the power, 2015-05-20 21:36:08]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-05-20 21:36:48]

Patchset 7 (id:??) landed as
https://crrev.com/2bcaa68bdea4d8dc89641992b7a0b6570d8d4d26
Cr-Commit-Position: refs/heads/master@{#330808}