Fix potential flakiness in PrerenderBrowserTest.AutosigninInPrerenderer

Fix potential flakiness in PrerenderBrowserTest.AutosigninInPrerenderer

This is a follow-up to https://codereview.chromium.org/2458953002.

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

[vasilii, 2016-11-04 13:09:44]

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

[commit-bot: I haz the power, 2016-11-04 13:10:01]

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

[vasilii, 2016-11-04 13:11:50]

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

[vasilii, 2016-11-04 13:11:51]

Hi Egor,

please review. Honestly I don't understand how the previous test could fail and
why it's not the case in the production code. I'm looking forward to your
clarifications.

[commit-bot: I haz the power, 2016-11-04 13:41:03]

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

[commit-bot: I haz the power, 2016-11-04 13:41:04]

Dry run: This issue passed the CQ dry run.

[pasko, 2016-11-04 18:54:57]

lgtm, thank you

On 2016/11/04 13:11:51, vasilii wrote:
> Hi Egor,
> 
> please review. Honestly I don't understand how the previous test could fail
and
> why it's not the case in the production code. I'm looking forward to your
> clarifications.

My worry appears to be a bit too theoretical. I assume that if
ChromePasswordManagerClient::OnCredentialManagerUsed() is called before
TestPrerender::OnPrerenderCreated(), it will see no prerender_contents and would
not Destroy() it. I thought this could happen because mojo IPC is not
synchronized with chrome IPC. In my short local testing, however, I could not
reproduce the problem easily, which could be because the mojo channel creation
is sequenced after the prerender creation by some other means that I did not
know about, like the order of tasks posted to the UI thread. Still could appear
at some point with more things migrated to mojo, so it seems still nice to
create the PrerenderContents earlier.

[vasilii, 2016-11-07 09:47:36]

On 2016/11/04 18:54:57, pasko wrote:
> lgtm, thank you
> 
> On 2016/11/04 13:11:51, vasilii wrote:
> > Hi Egor,
> > 
> > please review. Honestly I don't understand how the previous test could fail
> and
> > why it's not the case in the production code. I'm looking forward to your
> > clarifications.
> 
> My worry appears to be a bit too theoretical. I assume that if
> ChromePasswordManagerClient::OnCredentialManagerUsed() is called before
> TestPrerender::OnPrerenderCreated(), it will see no prerender_contents and
would
> not Destroy() it. I thought this could happen because mojo IPC is not
> synchronized with chrome IPC. In my short local testing, however, I could not
> reproduce the problem easily, which could be because the mojo channel creation
> is sequenced after the prerender creation by some other means that I did not
> know about, like the order of tasks posted to the UI thread. Still could
appear
> at some point with more things migrated to mojo, so it seems still nice to
> create the PrerenderContents earlier.

The previous test worked like this:
- a testing page has a <link prerender> command.
- PrerenderHostMsg_AddLinkRelPrerender is sent.
- As a result Chrome creates a TestPrerender and WebContents(?) on the UI
thread.
- Only after that some javascript may be executed and handled in
OnCredentialManagerUsed.

Is it correct?

[pasko, 2016-11-07 11:41:48]

On 2016/11/07 09:47:36, vasilii wrote:
> On 2016/11/04 18:54:57, pasko wrote:
> > lgtm, thank you
> > 
> > On 2016/11/04 13:11:51, vasilii wrote:
> > > Hi Egor,
> > > 
> > > please review. Honestly I don't understand how the previous test could
fail
> > and
> > > why it's not the case in the production code. I'm looking forward to your
> > > clarifications.
> > 
> > My worry appears to be a bit too theoretical. I assume that if
> > ChromePasswordManagerClient::OnCredentialManagerUsed() is called before
> > TestPrerender::OnPrerenderCreated(), it will see no prerender_contents and
> would
> > not Destroy() it. I thought this could happen because mojo IPC is not
> > synchronized with chrome IPC. In my short local testing, however, I could
not
> > reproduce the problem easily, which could be because the mojo channel
creation
> > is sequenced after the prerender creation by some other means that I did not
> > know about, like the order of tasks posted to the UI thread. Still could
> appear
> > at some point with more things migrated to mojo, so it seems still nice to
> > create the PrerenderContents earlier.
> 
> The previous test worked like this:
> - a testing page has a <link prerender> command.
> - PrerenderHostMsg_AddLinkRelPrerender is sent.
> - As a result Chrome creates a TestPrerender and WebContents(?) on the UI
> thread.
> - Only after that some javascript may be executed and handled in
> OnCredentialManagerUsed.
> 
> Is it correct?

You are right, thank you and sorry for the silly suggestion! I forgot how
AddPrerenderFromLinkRelPrerender() works and thought that it could be starting
the renderer before registering itself in PrerenderManager. Glad that it is not
the case.

Then this change appears to be not necessary, and makes the test less readable.

I would suggest removing the TODO and clarifying with something like:
// TestPrenderContents is always created before the Autosignin JS can run, so
waiting for PrerenderContents to stop should be reliable.

[vasilii, 2016-11-07 12:26:33]

On 2016/11/07 11:41:48, pasko wrote:
> On 2016/11/07 09:47:36, vasilii wrote:
> > On 2016/11/04 18:54:57, pasko wrote:
> > > lgtm, thank you
> > > 
> > > On 2016/11/04 13:11:51, vasilii wrote:
> > > > Hi Egor,
> > > > 
> > > > please review. Honestly I don't understand how the previous test could
> fail
> > > and
> > > > why it's not the case in the production code. I'm looking forward to
your
> > > > clarifications.
> > > 
> > > My worry appears to be a bit too theoretical. I assume that if
> > > ChromePasswordManagerClient::OnCredentialManagerUsed() is called before
> > > TestPrerender::OnPrerenderCreated(), it will see no prerender_contents and
> > would
> > > not Destroy() it. I thought this could happen because mojo IPC is not
> > > synchronized with chrome IPC. In my short local testing, however, I could
> not
> > > reproduce the problem easily, which could be because the mojo channel
> creation
> > > is sequenced after the prerender creation by some other means that I did
not
> > > know about, like the order of tasks posted to the UI thread. Still could
> > appear
> > > at some point with more things migrated to mojo, so it seems still nice to
> > > create the PrerenderContents earlier.
> > 
> > The previous test worked like this:
> > - a testing page has a <link prerender> command.
> > - PrerenderHostMsg_AddLinkRelPrerender is sent.
> > - As a result Chrome creates a TestPrerender and WebContents(?) on the UI
> > thread.
> > - Only after that some javascript may be executed and handled in
> > OnCredentialManagerUsed.
> > 
> > Is it correct?
> 
> You are right, thank you and sorry for the silly suggestion! I forgot how
> AddPrerenderFromLinkRelPrerender() works and thought that it could be starting
> the renderer before registering itself in PrerenderManager. Glad that it is
not
> the case.
> 
> Then this change appears to be not necessary, and makes the test less
readable.
> 
> I would suggest removing the TODO and clarifying with something like:
> // TestPrenderContents is always created before the Autosignin JS can run, so
> waiting for PrerenderContents to stop should be reliable.

Done.

[vasilii, 2016-11-07 12:26:58]

The CQ bit was checked by vasilii@chromium.org

[vasilii, 2016-11-07 12:26:58]

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/2478163002/#ps20001
(title: "Remove comment")

[commit-bot: I haz the power, 2016-11-07 12:27:10]

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-11-07 13:03:30]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-11-07 13:05:27]

Description was changed from

==========
Fix potential flakiness in PrerenderBrowserTest.AutosigninInPrerenderer

This is a follow-up to https://codereview.chromium.org/2458953002.

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

to

==========
Fix potential flakiness in PrerenderBrowserTest.AutosigninInPrerenderer

This is a follow-up to https://codereview.chromium.org/2458953002.

BUG=660278

Committed: https://crrev.com/cce92f9630364bbab9c37478ae7b24f25cf3ffa1
Cr-Commit-Position: refs/heads/master@{#430258}
==========

[commit-bot: I haz the power, 2016-11-07 13:05:28]

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

[pasko, 2016-11-07 13:59:35]

thanks!