Fix PrerenderBrowserTest.AutosigninInPrerenderer flakiness.

Fix PrerenderBrowserTest.AutosigninInPrerenderer flakiness.

The test checked that the prerender is stopped before the page is fully loaded. It's not guaranteed and its unrelated to the test logic.

BUG=660278
Committed: https://crrev.com/44655c02fec42900c8379e49bb83fa8e8b9629ff
Cr-Commit-Position: refs/heads/master@{#428374}

[vasilii, 2016-10-28 09:44:43]

The CQ bit was checked by vasilii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-28 09:44:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vasilii, 2016-10-28 09:46:21]

vasilii@chromium.org changed reviewers:
+ pasko@chromium.org

[vasilii, 2016-10-28 09:46:22]

Hi Egor,

please review.

[commit-bot: I haz the power, 2016-10-28 10:23:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-28 10:23:22]

Dry run: This issue passed the CQ dry run.

[pasko, 2016-10-28 11:39:55]

https://codereview.chromium.org/2458953002/diff/1/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_browsertest.cc (right):

https://codereview.chromium.org/2458953002/diff/1/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_browsertest.cc:3253: DisableLoadEventCheck();
It seems that FINAL_STATUS_APP_TERMINATING can happen as well if:
1. load did not happen
2. the test gets to the end sooner than the auto-signin manage to kill the
webcontents

Wouldn't the problem be solved by 'just' providing expected_number_of_loads=1
toPrerenderTestURL()?

AFAICT the autosignin.html pokes to Credential Manager API after the onload
event. Is that signal strictly ordered against the onload IPC?

Then, if my assumptions about ordering are true, to eliminate the possibility of
FINAL_STATUS_APP_TERMINATING (and make the done_counter check more
deterministic) we would run TestPrerender::WaitForStop() before
EXPECT.*done_counter.

[vasilii, 2016-10-28 11:55:00]

On 2016/10/28 11:39:55, pasko wrote:
>
https://codereview.chromium.org/2458953002/diff/1/chrome/browser/prerender/pr...
> File chrome/browser/prerender/prerender_browsertest.cc (right):
> 
>
https://codereview.chromium.org/2458953002/diff/1/chrome/browser/prerender/pr...
> chrome/browser/prerender/prerender_browsertest.cc:3253:
DisableLoadEventCheck();
> It seems that FINAL_STATUS_APP_TERMINATING can happen as well if:
> 1. load did not happen
> 2. the test gets to the end sooner than the auto-signin manage to kill the
> webcontents
> 
> Wouldn't the problem be solved by 'just' providing expected_number_of_loads=1
> toPrerenderTestURL()?
> 
> AFAICT the autosignin.html pokes to Credential Manager API after the onload
> event. Is that signal strictly ordered against the onload IPC?
> 
> Then, if my assumptions about ordering are true, to eliminate the possibility
of
> FINAL_STATUS_APP_TERMINATING (and make the done_counter check more
> deterministic) we would run TestPrerender::WaitForStop() before
> EXPECT.*done_counter.

expected_number_of_loads=1 doesn't work because the actual number of loads is 0
(at least on my machine).
No need for WaitForStop(). It's done by PrerenderTestURLImpl() in
prerender_browsertest.cc:964 thanks to the fact that
ShouldAbortPrerenderBeforeSwap(FINAL_STATUS_CREDENTIAL_MANAGER_API) returns
true.

[pasko, 2016-10-28 13:46:22]

On 2016/10/28 11:55:00, vasilii wrote:
> On 2016/10/28 11:39:55, pasko wrote:
> >
>
https://codereview.chromium.org/2458953002/diff/1/chrome/browser/prerender/pr...
> > File chrome/browser/prerender/prerender_browsertest.cc (right):
> > 
> >
>
https://codereview.chromium.org/2458953002/diff/1/chrome/browser/prerender/pr...
> > chrome/browser/prerender/prerender_browsertest.cc:3253:
> DisableLoadEventCheck();
> > It seems that FINAL_STATUS_APP_TERMINATING can happen as well if:
> > 1. load did not happen
> > 2. the test gets to the end sooner than the auto-signin manage to kill the
> > webcontents
> > 
> > Wouldn't the problem be solved by 'just' providing
expected_number_of_loads=1
> > toPrerenderTestURL()?
> > 
> > AFAICT the autosignin.html pokes to Credential Manager API after the onload
> > event. Is that signal strictly ordered against the onload IPC?
> > 
> > Then, if my assumptions about ordering are true, to eliminate the
possibility
> of
> > FINAL_STATUS_APP_TERMINATING (and make the done_counter check more
> > deterministic) we would run TestPrerender::WaitForStop() before
> > EXPECT.*done_counter.
> 
> expected_number_of_loads=1 doesn't work because the actual number of loads is
0
> (at least on my machine).

On mine too. Looked more closely: this is because OnCredentialManagerUsed() is
triggered from a mojo message, which can arrive sooner than the page load IPC
(which happened earlier).

Wow, I regret that I did not realize that early enough. This means that we can
get OnCredentialManagerUsed() even before the TestPrrenderContents is created,
and hence we would not destroy it as we originally wanted. It would mean that
we'd loop in either TestPrerender::WaitForCreate() or
TestPrerender::WaitForStop() .. until test timeout.

For production code it is not an issue because the PrerenderWebContents and
WebContents are created together without leaving the UI thread.

To solve the test problem we can create the PrerenderContents synchronously via
a AddPrerenderFromExternalRequest. Let's do it in a followup CL since there is
value of removing this major flake. Alternative suggestions are welcome.

LGTM with the TODO added.

> No need for WaitForStop(). It's done by PrerenderTestURLImpl() in
> prerender_browsertest.cc:964 thanks to the fact that
> ShouldAbortPrerenderBeforeSwap(FINAL_STATUS_CREDENTIAL_MANAGER_API) returns
> true.

Oh, I missed that part. Thank you.

[vasilii, 2016-10-28 14:33:45]

Added a TODO for myself.

[vasilii, 2016-10-28 14:33:51]

The CQ bit was checked by vasilii@chromium.org

[vasilii, 2016-10-28 14:33:51]

The patchset sent to the CQ was uploaded after l-g-t-m from pasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2458953002/#ps20001
(title: "TODO")

[commit-bot: I haz the power, 2016-10-28 14:34:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-28 15:13:24]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-28 15:15:36]

Description was changed from

==========
Fix PrerenderBrowserTest.AutosigninInPrerenderer flakiness.

The test checked that the prerender is stopped before the page is fully loaded.
It's not guaranteed and its unrelated to the test logic.

BUG=660278
==========

to

==========
Fix PrerenderBrowserTest.AutosigninInPrerenderer flakiness.

The test checked that the prerender is stopped before the page is fully loaded.
It's not guaranteed and its unrelated to the test logic.

BUG=660278

Committed: https://crrev.com/44655c02fec42900c8379e49bb83fa8e8b9629ff
Cr-Commit-Position: refs/heads/master@{#428374}
==========

[commit-bot: I haz the power, 2016-10-28 15:15:37]

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