cc: Fix overwriting of commit completion event in MFBA mode.

cc: Fix overwriting of commit completion event in MFBA mode.

In main frame before activation (MFBA) mode the following sequence was
problematic:
1. Commit
2. BeginMainFrame
3. StartCommitOnImpl with hold_commit_for_activation=1 (second commit)
4. NotifyReadyToActivate (first commit)
5. Activate (first commit)
6. Commit (second commit)

StartCommitOnImpl will overwrite the completion event and associated
flag that's used in the previous frame's activation. The activation
would then signal the second commit's completion event and that commit
would later crash due to accessing a null completion event.

Added a test specifically for this case and also made the existing test
for hold_commit_for_activation more robust.

R=enne@chromium.org
BUG=613007
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/2237c9b93be8a4f6c106149f714b4b88856c4549
Cr-Commit-Position: refs/heads/master@{#395185}

[sunnyps, 2016-05-20 04:47:11]

Description was changed from

==========
cc: Fix overwriting of commit completion event in MFBA mode.

In main frame before activation (MFBA) mode the following sequence was
problematic:
1. Commit
2. BeginMainFrame
3. StartCommitOnImpl with hold_commit_for_activation=1 (second commit)
4. NotifyReadyToActivate (first commit)
5. Activate (first commit)
6. Commit (second commit)

StartCommitOnImpl will overwrite the completion event and associated
flag that's used in the previous frame's activation. The activation
would then signal the second commit's completion event and that commit
would later crash due to accessing a null completion event.

Added a test specifically for this case and also made the existing test
for hold_commit_for_activation more robust.

R=enne@chromium.org
BUG=613007
==========

to

==========
cc: Fix overwriting of commit completion event in MFBA mode.

In main frame before activation (MFBA) mode the following sequence was
problematic:
1. Commit
2. BeginMainFrame
3. StartCommitOnImpl with hold_commit_for_activation=1 (second commit)
4. NotifyReadyToActivate (first commit)
5. Activate (first commit)
6. Commit (second commit)

StartCommitOnImpl will overwrite the completion event and associated
flag that's used in the previous frame's activation. The activation
would then signal the second commit's completion event and that commit
would later crash due to accessing a null completion event.

Added a test specifically for this case and also made the existing test
for hold_commit_for_activation more robust.

R=enne@chromium.org
BUG=613007
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[sunnyps, 2016-05-20 04:47:49]

PTAL

[enne (OOO), 2016-05-20 18:26:12]

lgtm other than a comment nit, thanks for the test.

https://codereview.chromium.org/2000493002/diff/1/cc/trees/proxy_impl.cc
File cc/trees/proxy_impl.cc (right):

https://codereview.chromium.org/2000493002/diff/1/cc/trees/proxy_impl.cc#newc...
cc/trees/proxy_impl.cc:272: // This commit may be aborted. Store the value for
hold_commit_for_activation
Is this comment true? It seems like one you get past that EarlyOut_NoLayerTree,
you're going to get to ScheduledActionCommit? Maybe the comment can just go
away?

[sunnyps, 2016-05-20 20:49:36]

PTAL

I fixed the data race in the test. There doesn't seem to be a better way for
detecting that the main thread is blocked than checking the completion event.

https://codereview.chromium.org/2000493002/diff/1/cc/trees/proxy_impl.cc
File cc/trees/proxy_impl.cc (right):

https://codereview.chromium.org/2000493002/diff/1/cc/trees/proxy_impl.cc#newc...
cc/trees/proxy_impl.cc:272: // This commit may be aborted. Store the value for
hold_commit_for_activation
On 2016/05/20 18:26:12, enne wrote:
> Is this comment true? It seems like one you get past that
EarlyOut_NoLayerTree,
> you're going to get to ScheduledActionCommit? Maybe the comment can just go
> away?

Done.

[enne (OOO), 2016-05-20 20:58:07]

lgtm

I think checking the completion events seems pretty reasonable to me.  :)

[sunnyps, 2016-05-20 21:46:45]

The CQ bit was checked by sunnyps@chromium.org

[commit-bot: I haz the power, 2016-05-20 21:47:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2000493002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2000493002/20001

[commit-bot: I haz the power, 2016-05-20 22:25:22]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-05-20 22:27:46]

Description was changed from

==========
cc: Fix overwriting of commit completion event in MFBA mode.

In main frame before activation (MFBA) mode the following sequence was
problematic:
1. Commit
2. BeginMainFrame
3. StartCommitOnImpl with hold_commit_for_activation=1 (second commit)
4. NotifyReadyToActivate (first commit)
5. Activate (first commit)
6. Commit (second commit)

StartCommitOnImpl will overwrite the completion event and associated
flag that's used in the previous frame's activation. The activation
would then signal the second commit's completion event and that commit
would later crash due to accessing a null completion event.

Added a test specifically for this case and also made the existing test
for hold_commit_for_activation more robust.

R=enne@chromium.org
BUG=613007
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Fix overwriting of commit completion event in MFBA mode.

In main frame before activation (MFBA) mode the following sequence was
problematic:
1. Commit
2. BeginMainFrame
3. StartCommitOnImpl with hold_commit_for_activation=1 (second commit)
4. NotifyReadyToActivate (first commit)
5. Activate (first commit)
6. Commit (second commit)

StartCommitOnImpl will overwrite the completion event and associated
flag that's used in the previous frame's activation. The activation
would then signal the second commit's completion event and that commit
would later crash due to accessing a null completion event.

Added a test specifically for this case and also made the existing test
for hold_commit_for_activation more robust.

R=enne@chromium.org
BUG=613007
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/2237c9b93be8a4f6c106149f714b4b88856c4549
Cr-Commit-Position: refs/heads/master@{#395185}
==========

[commit-bot: I haz the power, 2016-05-20 22:27:47]

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