Fix some flaky tests.

NavigationControllerBrowserTest.RaceCrossOriginNavigationAndSamePageHistoryNavigation:
Since cross_origin_commit_observer.Wait() does not really stop right
after commit, nothing prevents the history nav to commit too during this
wait.

SitePerProcessBrowserTest.NavigateInUnloadHandler:
Navigation to c.com could commit while ExecuteScript was spinning the
loop, then deleted_observer binds to C's RFH and waits forever.

SitePerProcessBrowserTest.RenderViewHostIsNotReusedAfterDelayedSwapOutACK:
During handling of navigation commit in the browser, SwapOut message is
sent to the renderer, which asks its host to shut down;
commit_observer.WaitForCommit may spin the loop until this request is
satisfied, and after return the test accesses dead objects.

Regarding the change in DOMMessageQueue::WaitForMessage:
It was possible for NavigateInUnloadHandler to fail because of this.
With reordering code in the test itself, this change is not really
necessary, but to me it seems right to keep it - this is not the place
where the loop should be spinned always, unconditionally. BTW, I'm
planning to discuss (on chromium-dev) a broader proposal to change
things like this.

BUG=668707
Committed: https://crrev.com/ba2d61725438457920b12f562894b7aa7eb1fea1
Cr-Commit-Position: refs/heads/master@{#434643}

[Alexander Semashko, 2016-11-22 14:51:44]

Description was changed from

==========
Fix some flaky tests.

BUG=
==========

to

==========
NavigationControllerBrowserTest.RaceCrossOriginNavigationAndSamePageHistoryNavigation:
Since cross_origin_commit_observer.Wait() does not really stop right after
commit, nothing prevents the history nav to commit too during this wait.

SitePerProcessBrowserTest.NavigateInUnloadHandler:
Navigation to c.com could commit while ExecuteScript was spinning the loop, then
deleted_observer binds to C's RFH and waits forever.

SitePerProcessBrowserTest.RenderViewHostIsNotReusedAfterDelayedSwapOutACK:
During handling of navigation commit in the browser, SwapOut message is sent to
the renderer, which asks its host to shut down; commit_observer.WaitForCommit
may spin the loop until this request is satisfied, and after return the test
accesses dead objects.

Regarding the change in DOMMessageQueue::WaitForMessage:
It was possible for NavigateInUnloadHandler to fail because of this.
With reordering code in the test itself, this change is not really
necessary, but to me it seems right to keep it - this is not the place
where the loop should be spinned always, unconditionally. BTW, I'm
planning to discuss (on chromium-dev) a broader proposal to change
things like this.

BUG=
==========

[Alexander Semashko, 2016-11-22 14:51:44]

ahest@yandex-team.ru changed reviewers:
+ nasko@chromium.org

[Alexander Semashko, 2016-11-22 14:58:47]

Description was changed from

==========
NavigationControllerBrowserTest.RaceCrossOriginNavigationAndSamePageHistoryNavigation:
Since cross_origin_commit_observer.Wait() does not really stop right after
commit, nothing prevents the history nav to commit too during this wait.

SitePerProcessBrowserTest.NavigateInUnloadHandler:
Navigation to c.com could commit while ExecuteScript was spinning the loop, then
deleted_observer binds to C's RFH and waits forever.

SitePerProcessBrowserTest.RenderViewHostIsNotReusedAfterDelayedSwapOutACK:
During handling of navigation commit in the browser, SwapOut message is sent to
the renderer, which asks its host to shut down; commit_observer.WaitForCommit
may spin the loop until this request is satisfied, and after return the test
accesses dead objects.

Regarding the change in DOMMessageQueue::WaitForMessage:
It was possible for NavigateInUnloadHandler to fail because of this.
With reordering code in the test itself, this change is not really
necessary, but to me it seems right to keep it - this is not the place
where the loop should be spinned always, unconditionally. BTW, I'm
planning to discuss (on chromium-dev) a broader proposal to change
things like this.

BUG=
==========

to

==========
NavigationControllerBrowserTest.RaceCrossOriginNavigationAndSamePageHistoryNavigation:
Since cross_origin_commit_observer.Wait() does not really stop right
after commit, nothing prevents the history nav to commit too during this
wait.

SitePerProcessBrowserTest.NavigateInUnloadHandler:
Navigation to c.com could commit while ExecuteScript was spinning the
loop, then deleted_observer binds to C's RFH and waits forever.

SitePerProcessBrowserTest.RenderViewHostIsNotReusedAfterDelayedSwapOutACK:
During handling of navigation commit in the browser, SwapOut message is
sent to the renderer, which asks its host to shut down;
commit_observer.WaitForCommit may spin the loop until this request is
satisfied, and after return the test accesses dead objects.

Regarding the change in DOMMessageQueue::WaitForMessage:
It was possible for NavigateInUnloadHandler to fail because of this.
With reordering code in the test itself, this change is not really
necessary, but to me it seems right to keep it - this is not the place
where the loop should be spinned always, unconditionally. BTW, I'm
planning to discuss (on chromium-dev) a broader proposal to change
things like this.

BUG=
==========

[nasko, 2016-11-23 17:48:28]

nasko@chromium.org changed reviewers:
+ nick@chromium.org

[nasko, 2016-11-23 17:48:29]

Thanks for this CL! Adding nick@, who I think will be interested in also looking
at it.

https://codereview.chromium.org/2523583003/diff/1/content/browser/site_per_pr...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2523583003/diff/1/content/browser/site_per_pr...
content/browser/site_per_process_browsertest.cc:8361:
root->child_at(0)->child_at(0)->current_frame_host());
I agree that moving this code makes sense. However, I don't follow your CL
comment about deleted_observer binding to the "c.com" RenderFrameHost. Even if
the navigation commits for c.com, there is no way for the observer to bind to
that RFH, as it will be root->child_at(0) and not
root->child_at(0)->child_at(0). What is possible is for the
root->child_at(0)->child_at(0) to be null of that happens.

https://codereview.chromium.org/2523583003/diff/1/content/public/test/test_ut...
File content/public/test/test_utils.h (right):

https://codereview.chromium.org/2523583003/diff/1/content/public/test/test_ut...
content/public/test/test_utils.h:101: MessageLoopRunner(QuitMode mode =
QuitMode::LOOSE);
Instead of making the quit type be captured for the lifetime of the runner and
be a property of it, why not have a different method to quit immediately? It
will be even more obvious at the calling site what the desired behavior is.
Something like QuitImmediately, ImmediateQuit, or something else if you have
better ideas.

https://codereview.chromium.org/2523583003/diff/1/content/test/content_browse...
File content/test/content_browser_test_utils_internal.cc (right):

https://codereview.chromium.org/2523583003/diff/1/content/test/content_browse...
content/test/content_browser_test_utils_internal.cc:383: new
MessageLoopRunner(MessageLoopRunner::QuitMode::IMMEDIATE)) {
I imagine all test helpers waiting for commit will likely expect this type of
behavior.

[Alexander Semashko, 2016-11-23 18:38:08]

https://codereview.chromium.org/2523583003/diff/1/content/browser/site_per_pr...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2523583003/diff/1/content/browser/site_per_pr...
content/browser/site_per_process_browsertest.cc:8361:
root->child_at(0)->child_at(0)->current_frame_host());
On 2016/11/23 17:48:29, nasko wrote:
> I agree that moving this code makes sense. However, I don't follow your CL
> comment about deleted_observer binding to the "c.com" RenderFrameHost. Even if
> the navigation commits for c.com, there is no way for the observer to bind to
> that RFH, as it will be root->child_at(0) and not
> root->child_at(0)->child_at(0). What is possible is for the
> root->child_at(0)->child_at(0) to be null of that happens.

c.com is loaded in the third-level frame, because we navigate the child of
root->child_at(0).

https://codereview.chromium.org/2523583003/diff/1/content/public/test/test_ut...
File content/public/test/test_utils.h (right):

https://codereview.chromium.org/2523583003/diff/1/content/public/test/test_ut...
content/public/test/test_utils.h:101: MessageLoopRunner(QuitMode mode =
QuitMode::LOOSE);
On 2016/11/23 17:48:29, nasko wrote:
> Instead of making the quit type be captured for the lifetime of the runner and
> be a property of it, why not have a different method to quit immediately? It
> will be even more obvious at the calling site what the desired behavior is.
> Something like QuitImmediately, ImmediateQuit, or something else if you have
> better ideas.

I thought the current way would be clearer; also there is no real difference
since MessageLoopRunner cannot be run more than once.

https://codereview.chromium.org/2523583003/diff/1/content/test/content_browse...
File content/test/content_browser_test_utils_internal.cc (right):

https://codereview.chromium.org/2523583003/diff/1/content/test/content_browse...
content/test/content_browser_test_utils_internal.cc:383: new
MessageLoopRunner(MessageLoopRunner::QuitMode::IMMEDIATE)) {
On 2016/11/23 17:48:29, nasko wrote:
> I imagine all test helpers waiting for commit will likely expect this type of
> behavior.

I'm planning to look into other helper classes a bit later, once we agree on
what to do with MessageLoopRunner globally (please see the discussion on
chromium-dev).
In this CL I only focused on tests that have shown flakiness on our bots.

[nasko, 2016-11-24 00:00:44]

LGTM with one comment on the quit type naming.

https://codereview.chromium.org/2523583003/diff/1/content/browser/site_per_pr...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/2523583003/diff/1/content/browser/site_per_pr...
content/browser/site_per_process_browsertest.cc:8361:
root->child_at(0)->child_at(0)->current_frame_host());
On 2016/11/23 18:38:08, Alexander Semashko wrote:
> On 2016/11/23 17:48:29, nasko wrote:
> > I agree that moving this code makes sense. However, I don't follow your CL
> > comment about deleted_observer binding to the "c.com" RenderFrameHost. Even
if
> > the navigation commits for c.com, there is no way for the observer to bind
to
> > that RFH, as it will be root->child_at(0) and not
> > root->child_at(0)->child_at(0). What is possible is for the
> > root->child_at(0)->child_at(0) to be null of that happens.
> 
> c.com is loaded in the third-level frame, because we navigate the child of
> root->child_at(0).

Ah, yes!

https://codereview.chromium.org/2523583003/diff/1/content/public/test/test_ut...
File content/public/test/test_utils.h (right):

https://codereview.chromium.org/2523583003/diff/1/content/public/test/test_ut...
content/public/test/test_utils.h:101: MessageLoopRunner(QuitMode mode =
QuitMode::LOOSE);
On 2016/11/23 18:38:08, Alexander Semashko wrote:
> On 2016/11/23 17:48:29, nasko wrote:
> > Instead of making the quit type be captured for the lifetime of the runner
and
> > be a property of it, why not have a different method to quit immediately? It
> > will be even more obvious at the calling site what the desired behavior is.
> > Something like QuitImmediately, ImmediateQuit, or something else if you have
> > better ideas.
> 
> I thought the current way would be clearer; also there is no real difference
> since MessageLoopRunner cannot be run more than once.

lukasza@ pointed out a different argument in favor of using the constructor - we
won't need to change any of the callsites for Quit and can mass-convert tests by
changing the default type at one place.

However, let's try to match the type name to what it does - it is a deferred
quit, so instead of LOOSE can we use DEFERRED? "LOOSE" is too loose (pun
indented) of a definition.

https://codereview.chromium.org/2523583003/diff/1/content/test/content_browse...
File content/test/content_browser_test_utils_internal.cc (right):

https://codereview.chromium.org/2523583003/diff/1/content/test/content_browse...
content/test/content_browser_test_utils_internal.cc:383: new
MessageLoopRunner(MessageLoopRunner::QuitMode::IMMEDIATE)) {
On 2016/11/23 18:38:08, Alexander Semashko wrote:
> On 2016/11/23 17:48:29, nasko wrote:
> > I imagine all test helpers waiting for commit will likely expect this type
of
> > behavior.
> 
> I'm planning to look into other helper classes a bit later, once we agree on
> what to do with MessageLoopRunner globally (please see the discussion on
> chromium-dev).
> In this CL I only focused on tests that have shown flakiness on our bots.

Acknowledged.

[Alexander Semashko, 2016-11-24 10:31:43]

https://codereview.chromium.org/2523583003/diff/1/content/public/test/test_ut...
File content/public/test/test_utils.h (right):

https://codereview.chromium.org/2523583003/diff/1/content/public/test/test_ut...
content/public/test/test_utils.h:101: MessageLoopRunner(QuitMode mode =
QuitMode::LOOSE);
On 2016/11/24 00:00:43, nasko wrote:
> On 2016/11/23 18:38:08, Alexander Semashko wrote:
> > On 2016/11/23 17:48:29, nasko wrote:
> > > Instead of making the quit type be captured for the lifetime of the runner
> and
> > > be a property of it, why not have a different method to quit immediately?
It
> > > will be even more obvious at the calling site what the desired behavior
is.
> > > Something like QuitImmediately, ImmediateQuit, or something else if you
have
> > > better ideas.
> > 
> > I thought the current way would be clearer; also there is no real difference
> > since MessageLoopRunner cannot be run more than once.
> 
> lukasza@ pointed out a different argument in favor of using the constructor -
we
> won't need to change any of the callsites for Quit and can mass-convert tests
by
> changing the default type at one place.
> 
> However, let's try to match the type name to what it does - it is a deferred
> quit, so instead of LOOSE can we use DEFERRED? "LOOSE" is too loose (pun
> indented) of a definition.

Too loose name could be an additional warning for people :)
Ok, changed to DEFERRED.

[Alexander Semashko, 2016-11-25 08:52:41]

The CQ bit was checked by ahest@yandex-team.ru

[Alexander Semashko, 2016-11-25 08:52:42]

The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2523583003/#ps20001
(title: "Use better name.")

[commit-bot: I haz the power, 2016-11-25 08:52:53]

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-25 10:02:46]

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

[commit-bot: I haz the power, 2016-11-25 10:02:47]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Alexander Semashko, 2016-11-25 12:10:54]

ahest@yandex-team.ru changed reviewers:
+ achuith@chromium.org

[Alexander Semashko, 2016-11-25 13:25:51]

Description was changed from

==========
NavigationControllerBrowserTest.RaceCrossOriginNavigationAndSamePageHistoryNavigation:
Since cross_origin_commit_observer.Wait() does not really stop right
after commit, nothing prevents the history nav to commit too during this
wait.

SitePerProcessBrowserTest.NavigateInUnloadHandler:
Navigation to c.com could commit while ExecuteScript was spinning the
loop, then deleted_observer binds to C's RFH and waits forever.

SitePerProcessBrowserTest.RenderViewHostIsNotReusedAfterDelayedSwapOutACK:
During handling of navigation commit in the browser, SwapOut message is
sent to the renderer, which asks its host to shut down;
commit_observer.WaitForCommit may spin the loop until this request is
satisfied, and after return the test accesses dead objects.

Regarding the change in DOMMessageQueue::WaitForMessage:
It was possible for NavigateInUnloadHandler to fail because of this.
With reordering code in the test itself, this change is not really
necessary, but to me it seems right to keep it - this is not the place
where the loop should be spinned always, unconditionally. BTW, I'm
planning to discuss (on chromium-dev) a broader proposal to change
things like this.

BUG=
==========

to

==========
NavigationControllerBrowserTest.RaceCrossOriginNavigationAndSamePageHistoryNavigation:
Since cross_origin_commit_observer.Wait() does not really stop right
after commit, nothing prevents the history nav to commit too during this
wait.

SitePerProcessBrowserTest.NavigateInUnloadHandler:
Navigation to c.com could commit while ExecuteScript was spinning the
loop, then deleted_observer binds to C's RFH and waits forever.

SitePerProcessBrowserTest.RenderViewHostIsNotReusedAfterDelayedSwapOutACK:
During handling of navigation commit in the browser, SwapOut message is
sent to the renderer, which asks its host to shut down;
commit_observer.WaitForCommit may spin the loop until this request is
satisfied, and after return the test accesses dead objects.

Regarding the change in DOMMessageQueue::WaitForMessage:
It was possible for NavigateInUnloadHandler to fail because of this.
With reordering code in the test itself, this change is not really
necessary, but to me it seems right to keep it - this is not the place
where the loop should be spinned always, unconditionally. BTW, I'm
planning to discuss (on chromium-dev) a broader proposal to change
things like this.

BUG=668707
==========

[Alexander Semashko, 2016-11-28 10:04:09]

The CQ bit was checked by ahest@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2016-11-28 10:04:29]

Dry run: 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-28 10:53:42]

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

[commit-bot: I haz the power, 2016-11-28 10:53:42]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Alexander Semashko, 2016-11-28 12:06:13]

The CQ bit was checked by ahest@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2016-11-28 12:06:20]

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

[Alexander Semashko, 2016-11-28 12:09:59]

On 2016/11/25 10:02:47, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

The change in DOMMessageQueue behavior causes more failures than I hoped, better
to handle them separately.

[Alexander Semashko, 2016-11-28 12:10:17]

ahest@yandex-team.ru changed reviewers:
- achuith@chromium.org

[commit-bot: I haz the power, 2016-11-28 12:57:06]

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

[commit-bot: I haz the power, 2016-11-28 12:57:07]

Dry run: This issue passed the CQ dry run.

[Alexander Semashko, 2016-11-28 13:03:04]

The CQ bit was checked by ahest@yandex-team.ru

[Alexander Semashko, 2016-11-28 13:03:04]

The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2523583003/#ps60001
(title: "Undo change in DOMMessageQueue.")

[commit-bot: I haz the power, 2016-11-28 13:03:14]

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-28 13:06:56]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1480338184056780,
"parent_rev": "4cc1120779cf7e20be8209ccaf9f4814d18df0cc", "commit_rev":
"c4eb1b6abd248336efac3874d027cc35b3fff912"}

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

Description was changed from

==========
NavigationControllerBrowserTest.RaceCrossOriginNavigationAndSamePageHistoryNavigation:
Since cross_origin_commit_observer.Wait() does not really stop right
after commit, nothing prevents the history nav to commit too during this
wait.

SitePerProcessBrowserTest.NavigateInUnloadHandler:
Navigation to c.com could commit while ExecuteScript was spinning the
loop, then deleted_observer binds to C's RFH and waits forever.

SitePerProcessBrowserTest.RenderViewHostIsNotReusedAfterDelayedSwapOutACK:
During handling of navigation commit in the browser, SwapOut message is
sent to the renderer, which asks its host to shut down;
commit_observer.WaitForCommit may spin the loop until this request is
satisfied, and after return the test accesses dead objects.

Regarding the change in DOMMessageQueue::WaitForMessage:
It was possible for NavigateInUnloadHandler to fail because of this.
With reordering code in the test itself, this change is not really
necessary, but to me it seems right to keep it - this is not the place
where the loop should be spinned always, unconditionally. BTW, I'm
planning to discuss (on chromium-dev) a broader proposal to change
things like this.

BUG=668707
==========

to

==========
NavigationControllerBrowserTest.RaceCrossOriginNavigationAndSamePageHistoryNavigation:
Since cross_origin_commit_observer.Wait() does not really stop right
after commit, nothing prevents the history nav to commit too during this
wait.

SitePerProcessBrowserTest.NavigateInUnloadHandler:
Navigation to c.com could commit while ExecuteScript was spinning the
loop, then deleted_observer binds to C's RFH and waits forever.

SitePerProcessBrowserTest.RenderViewHostIsNotReusedAfterDelayedSwapOutACK:
During handling of navigation commit in the browser, SwapOut message is
sent to the renderer, which asks its host to shut down;
commit_observer.WaitForCommit may spin the loop until this request is
satisfied, and after return the test accesses dead objects.

Regarding the change in DOMMessageQueue::WaitForMessage:
It was possible for NavigateInUnloadHandler to fail because of this.
With reordering code in the test itself, this change is not really
necessary, but to me it seems right to keep it - this is not the place
where the loop should be spinned always, unconditionally. BTW, I'm
planning to discuss (on chromium-dev) a broader proposal to change
things like this.

BUG=668707
==========

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

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-28 13:09:55]

Description was changed from

==========
NavigationControllerBrowserTest.RaceCrossOriginNavigationAndSamePageHistoryNavigation:
Since cross_origin_commit_observer.Wait() does not really stop right
after commit, nothing prevents the history nav to commit too during this
wait.

SitePerProcessBrowserTest.NavigateInUnloadHandler:
Navigation to c.com could commit while ExecuteScript was spinning the
loop, then deleted_observer binds to C's RFH and waits forever.

SitePerProcessBrowserTest.RenderViewHostIsNotReusedAfterDelayedSwapOutACK:
During handling of navigation commit in the browser, SwapOut message is
sent to the renderer, which asks its host to shut down;
commit_observer.WaitForCommit may spin the loop until this request is
satisfied, and after return the test accesses dead objects.

Regarding the change in DOMMessageQueue::WaitForMessage:
It was possible for NavigateInUnloadHandler to fail because of this.
With reordering code in the test itself, this change is not really
necessary, but to me it seems right to keep it - this is not the place
where the loop should be spinned always, unconditionally. BTW, I'm
planning to discuss (on chromium-dev) a broader proposal to change
things like this.

BUG=668707
==========

to

==========
NavigationControllerBrowserTest.RaceCrossOriginNavigationAndSamePageHistoryNavigation:
Since cross_origin_commit_observer.Wait() does not really stop right
after commit, nothing prevents the history nav to commit too during this
wait.

SitePerProcessBrowserTest.NavigateInUnloadHandler:
Navigation to c.com could commit while ExecuteScript was spinning the
loop, then deleted_observer binds to C's RFH and waits forever.

SitePerProcessBrowserTest.RenderViewHostIsNotReusedAfterDelayedSwapOutACK:
During handling of navigation commit in the browser, SwapOut message is
sent to the renderer, which asks its host to shut down;
commit_observer.WaitForCommit may spin the loop until this request is
satisfied, and after return the test accesses dead objects.

Regarding the change in DOMMessageQueue::WaitForMessage:
It was possible for NavigateInUnloadHandler to fail because of this.
With reordering code in the test itself, this change is not really
necessary, but to me it seems right to keep it - this is not the place
where the loop should be spinned always, unconditionally. BTW, I'm
planning to discuss (on chromium-dev) a broader proposal to change
things like this.

BUG=668707

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

[commit-bot: I haz the power, 2016-11-28 13:09:56]

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

[Alexander Semashko, 2016-11-28 18:33:26]

BTW...
nasko, can you help me to get try job access? Looks like without it I'm going to
have very hard time converting other places that [mis]use deferred quit.

[nasko, 2016-11-28 22:21:42]

On 2016/11/28 18:33:26, Alexander Semashko wrote:
> BTW...
> nasko, can you help me to get try job access? Looks like without it I'm going
to
> have very hard time converting other places that [mis]use deferred quit.

You should have it, try it out and let me know if there are any issues.
Thanks for your help and contributions!

[Alexander Semashko, 2016-11-29 07:41:51]

On 2016/11/28 22:21:42, nasko wrote:
> On 2016/11/28 18:33:26, Alexander Semashko wrote:
> > BTW...
> > nasko, can you help me to get try job access? Looks like without it I'm
going
> to
> > have very hard time converting other places that [mis]use deferred quit.
> 
> You should have it, try it out and let me know if there are any issues.
> Thanks for your help and contributions!

It works. Thank you!

[stgao, 2016-12-02 01:22:54]

Would it be possible that this CL make these two tests flaky?

FindRequestManagerTests/FindRequestManagerTest.AddFrame/1 
FindRequestManagerTests/FindRequestManagerTest.NavigateFrame/1 

https://findit-for-me.appspot.com/waterfall/check-flake?master_name=chromium....

https://findit-for-me.appspot.com/waterfall/check-flake?master_name=chromium....

[Alexander Semashko, 2016-12-02 07:50:36]

On 2016/12/02 01:22:54, stgao (slow on Monday) wrote:
> Would it be possible that this CL make these two tests flaky?
> 
> FindRequestManagerTests/FindRequestManagerTest.AddFrame/1 
> FindRequestManagerTests/FindRequestManagerTest.NavigateFrame/1 
> 
>
https://findit-for-me.appspot.com/waterfall/check-flake?master_name=chromium....
> 
>
https://findit-for-me.appspot.com/waterfall/check-flake?master_name=chromium....

From the first glance, no: these tests do not use neither
TestFrameNavigationObserver nor UrlCommitObserver, and this CL did not change
anything else.
I'll look into it deeper today.

[Charlie Reis, 2016-12-02 21:54:09]

Description was changed from

==========
NavigationControllerBrowserTest.RaceCrossOriginNavigationAndSamePageHistoryNavigation:
Since cross_origin_commit_observer.Wait() does not really stop right
after commit, nothing prevents the history nav to commit too during this
wait.

SitePerProcessBrowserTest.NavigateInUnloadHandler:
Navigation to c.com could commit while ExecuteScript was spinning the
loop, then deleted_observer binds to C's RFH and waits forever.

SitePerProcessBrowserTest.RenderViewHostIsNotReusedAfterDelayedSwapOutACK:
During handling of navigation commit in the browser, SwapOut message is
sent to the renderer, which asks its host to shut down;
commit_observer.WaitForCommit may spin the loop until this request is
satisfied, and after return the test accesses dead objects.

Regarding the change in DOMMessageQueue::WaitForMessage:
It was possible for NavigateInUnloadHandler to fail because of this.
With reordering code in the test itself, this change is not really
necessary, but to me it seems right to keep it - this is not the place
where the loop should be spinned always, unconditionally. BTW, I'm
planning to discuss (on chromium-dev) a broader proposal to change
things like this.

BUG=668707

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

to

==========
NavigationControllerBrowserTest.RaceCrossOriginNavigationAndSamePageHistoryNavigation:
Since cross_origin_commit_observer.Wait() does not really stop right
after commit, nothing prevents the history nav to commit too during this
wait.

SitePerProcessBrowserTest.NavigateInUnloadHandler:
Navigation to c.com could commit while ExecuteScript was spinning the
loop, then deleted_observer binds to C's RFH and waits forever.

SitePerProcessBrowserTest.RenderViewHostIsNotReusedAfterDelayedSwapOutACK:
During handling of navigation commit in the browser, SwapOut message is
sent to the renderer, which asks its host to shut down;
commit_observer.WaitForCommit may spin the loop until this request is
satisfied, and after return the test accesses dead objects.

Regarding the change in DOMMessageQueue::WaitForMessage:
It was possible for NavigateInUnloadHandler to fail because of this.
With reordering code in the test itself, this change is not really
necessary, but to me it seems right to keep it - this is not the place
where the loop should be spinned always, unconditionally. BTW, I'm
planning to discuss (on chromium-dev) a broader proposal to change
things like this.

BUG=668707

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

[stgao, 2016-12-06 05:24:06]

On 2016/12/02 07:50:36, Alexander Semashko wrote:
> On 2016/12/02 01:22:54, stgao (slow on Monday) wrote:
> > Would it be possible that this CL make these two tests flaky?
> > 
> > FindRequestManagerTests/FindRequestManagerTest.AddFrame/1 
> > FindRequestManagerTests/FindRequestManagerTest.NavigateFrame/1 
> > 
> >
>
https://findit-for-me.appspot.com/waterfall/check-flake?master_name=chromium....
> > 
> >
>
https://findit-for-me.appspot.com/waterfall/check-flake?master_name=chromium....
> 
> From the first glance, no: these tests do not use neither
> TestFrameNavigationObserver nor UrlCommitObserver, and this CL did not change
> anything else.
> I'll look into it deeper today.

Thanks for confirming!

[Alexander Semashko, 2016-12-07 14:22:09]

On 2016/12/06 05:24:06, stgao (slow on Monday) wrote:
> On 2016/12/02 07:50:36, Alexander Semashko wrote:
> > On 2016/12/02 01:22:54, stgao (slow on Monday) wrote:
> > > Would it be possible that this CL make these two tests flaky?
> > > 
> > > FindRequestManagerTests/FindRequestManagerTest.AddFrame/1 
> > > FindRequestManagerTests/FindRequestManagerTest.NavigateFrame/1 
> > > 
> > >
> >
>
https://findit-for-me.appspot.com/waterfall/check-flake?master_name=chromium....
> > > 
> > >
> >
>
https://findit-for-me.appspot.com/waterfall/check-flake?master_name=chromium....
> > 
> > From the first glance, no: these tests do not use neither
> > TestFrameNavigationObserver nor UrlCommitObserver, and this CL did not
change
> > anything else.
> > I'll look into it deeper today.
> 
> Thanks for confirming!

Sorry, I was not able to get back to this issues on friday. And it appears that
I was not right - missed the fact that TestFrameNavigationObserver is used in
NavigateFrameToURL from content_browser_test_utils_internal, which is used in
find request manager tests. So it is very likely that this CL made those tests
flaky.

In NavigateFrameToURL the observer waits for DidStopLoading. What this CL did
change is only that no more tasks are processed during this wait. This can be
completely patched out by calling RunAllPendingInMessageLoop after
NavigateFrameToURL, I think, though it may be undesirable.

Unless there is a bug in underlying code, like DidStopLoading firing too early,
I suppose that after the WebContents stop loading something happens that affects
find-in-page operation. It might be expected, or might be a bug, I don't know.
In the first case this should be accounted for in the tests, probably by just
running pending tasks - it is ok if there is no cross-thread or cross-process
dependencies and no delayed tasks. Though, considering that there are other
flaky FindRequestManagerTests, including those that were flaky long before this
CL landed, I guess that there is some more complex condition to wait for, or
just a plain old bug that must be fixed.