Make unload tests stable.

Make unload tests stable.

During unload tab has only one second to respond. If machine is busy doing
other tests renderer host might think that tab is unresponsive and close it
while test expecting the tab to react.

You can easily reproduce the failure by setting
RenderViewHostImpl::kUnloadTimeoutMS = 1

There are other tests that fail with that modification
(ex. UnloadTest.BrowserCloseBeforeUnloadCancel) but they are not flacky and I kept them intact.

BUG=519646
R=creis@chromium.org,sky@chromium.org

Committed: https://crrev.com/812bb854380007efc5852037af26632797053e90
Cr-Commit-Position: refs/heads/master@{#395693}

[Mikhail Goncharov, 2016-05-21 15:51:18]

Description was changed from

==========
Make unload tests stable.

During unload tab has only one second to respond. If machine is busy doing
other tests renderer host might think that tab is unresponsive and close it
while test expecting the tab to react.

You can easily reproduce the failure by setting
RenderViewHostImpl::kUnloadTimeoutMS = 1

There are other test that fail with that modification
(ex. UnloadTest.BrowserCloseBeforeUnloadCancel) but they are not flacky
and I kept them intact.

There is no OWNERS right now so I am adding reviewers of
https://codereview.chromium.org/1968933002/ which is related.

I will revisit this CL after 1968933002 merged as it introduced new
tests to fix.

BUG=519646
R=creis@chromium.org,sky@chromium.org
==========

to

==========
Make unload tests stable.

During unload tab has only one second to respond. If machine is busy doing
other tests renderer host might think that tab is unresponsive and close it
while test expecting the tab to react.

You can easily reproduce the failure by setting
RenderViewHostImpl::kUnloadTimeoutMS = 1

There are other tests that fail with that modification
(ex. UnloadTest.BrowserCloseBeforeUnloadCancel) but they are not flacky
and I kept them intact.

There is no OWNERS right now so I am adding reviewers of
https://codereview.chromium.org/1968933002/ which is related.

I will revisit this CL after 1968933002 merged as it introduced new
tests to fix.

BUG=519646
R=creis@chromium.org,sky@chromium.org
==========

[Charlie Reis, 2016-05-23 17:03:30]

Looks pretty good to me!  Just a few nits about comments below, and let's rebase
once https://codereview.chromium.org/1968933002/ lands to fix that one as well.

In the meantime, I'll send for a dry run to see if the bots are happy with it. 
Thanks for the quick work on it.

https://codereview.chromium.org/2005663002/diff/1/chrome/browser/lifetime/bro...
File chrome/browser/lifetime/browser_close_manager_browsertest.cc (left):

https://codereview.chromium.org/2005663002/diff/1/chrome/browser/lifetime/bro...
chrome/browser/lifetime/browser_close_manager_browsertest.cc:581: 
nit: Let's leave this blank line, just to make the test easier to read in
blocks.

https://codereview.chromium.org/2005663002/diff/1/chrome/browser/lifetime/bro...
File chrome/browser/lifetime/browser_close_manager_browsertest.cc (right):

https://codereview.chromium.org/2005663002/diff/1/chrome/browser/lifetime/bro...
chrome/browser/lifetime/browser_close_manager_browsertest.cc:69: void
AcceptClose() {
Please add comments to these saying that they require calling DisableHangMonitor
on the relevant WebContents or Browser before trying to close it, to avoid
flakiness.

https://codereview.chromium.org/2005663002/diff/1/chrome/browser/lifetime/bro...
chrome/browser/lifetime/browser_close_manager_browsertest.cc:255: // for that
web content.
Please mention that this must be called before any close action when using
AcceptClose or CancelClose, to ensure the timeout does not prevent the dialog
from appearing.

https://codereview.chromium.org/2005663002/diff/1/chrome/browser/lifetime/bro...
chrome/browser/lifetime/browser_close_manager_browsertest.cc:425:
DisableHangMonitor(browsers_[0]->tab_strip_model()->GetWebContentsAt(1));
nit: Move this line after this block of NavigateToURL and add a comment:

Disable the hang monitor in the tab that is not expected to hang, so that the
dialog is guaranteed to show.

https://codereview.chromium.org/2005663002/diff/1/chrome/browser/lifetime/bro...
chrome/browser/lifetime/browser_close_manager_browsertest.cc:460:
DisableHangMonitor(browsers_[1]);
Same as above.

https://codereview.chromium.org/2005663002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/2005663002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_impl.cc:920:
hang_monitor_timeout_.reset();
Yeah, I think this should be sufficient for this test.  We also have
RenderFrameHostImpl::DisableSwapOutTimerForTesting, but I don't think that's
necessary here because the swapout/unload event won't ever show dialogs.

nit: Maybe change the name from ForTest() to ForTesting() to be consistent (and
since there's a lot more of the latter in the codebase).

https://codereview.chromium.org/2005663002/diff/1/content/public/browser/rend...
File content/public/browser/render_widget_host.h (right):

https://codereview.chromium.org/2005663002/diff/1/content/public/browser/rend...
content/public/browser/render_widget_host.h:222: // as under load this widget
might not respond in time.
nit: Let's rephrase the second sentence:

This avoids flakiness in tests that need to observe things like beforeunload
dialogs, which could fail if the timeout skips the dialog.

[Charlie Reis, 2016-05-23 17:03:45]

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

[commit-bot: I haz the power, 2016-05-23 17:04:10]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2005663002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2005663002/1

[commit-bot: I haz the power, 2016-05-23 19:12:04]

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

[commit-bot: I haz the power, 2016-05-23 19:12:05]

Dry run: This issue passed the CQ dry run.

[Mikhail Goncharov, 2016-05-24 04:41:37]

Description was changed from

==========
Make unload tests stable.

During unload tab has only one second to respond. If machine is busy doing
other tests renderer host might think that tab is unresponsive and close it
while test expecting the tab to react.

You can easily reproduce the failure by setting
RenderViewHostImpl::kUnloadTimeoutMS = 1

There are other tests that fail with that modification
(ex. UnloadTest.BrowserCloseBeforeUnloadCancel) but they are not flacky
and I kept them intact.

There is no OWNERS right now so I am adding reviewers of
https://codereview.chromium.org/1968933002/ which is related.

I will revisit this CL after 1968933002 merged as it introduced new
tests to fix.

BUG=519646
R=creis@chromium.org,sky@chromium.org
==========

to

==========
Make unload tests stable.

During unload tab has only one second to respond. If machine is busy doing
other tests renderer host might think that tab is unresponsive and close it
while test expecting the tab to react.

You can easily reproduce the failure by setting
RenderViewHostImpl::kUnloadTimeoutMS = 1

There are other tests that fail with that modification
(ex. UnloadTest.BrowserCloseBeforeUnloadCancel) but they are not flacky and I
kept them intact.

BUG=519646
R=creis@chromium.org,sky@chromium.org
==========

[Mikhail Goncharov, 2016-05-24 04:43:21]

On 2016/05/23 17:03:30, Charlie Reis (slow to reply) wrote:
> Looks pretty good to me!  Just a few nits about comments below, and let's
rebase
> once https://codereview.chromium.org/1968933002/ lands to fix that one as
well.
> 
> In the meantime, I'll send for a dry run to see if the bots are happy with it.

> Thanks for the quick work on it.
> 
>
https://codereview.chromium.org/2005663002/diff/1/chrome/browser/lifetime/bro...
> File chrome/browser/lifetime/browser_close_manager_browsertest.cc (left):
> 
>
https://codereview.chromium.org/2005663002/diff/1/chrome/browser/lifetime/bro...
> chrome/browser/lifetime/browser_close_manager_browsertest.cc:581: 
> nit: Let's leave this blank line, just to make the test easier to read in
> blocks.
> 
>
https://codereview.chromium.org/2005663002/diff/1/chrome/browser/lifetime/bro...
> File chrome/browser/lifetime/browser_close_manager_browsertest.cc (right):
> 
>
https://codereview.chromium.org/2005663002/diff/1/chrome/browser/lifetime/bro...
> chrome/browser/lifetime/browser_close_manager_browsertest.cc:69: void
> AcceptClose() {
> Please add comments to these saying that they require calling
DisableHangMonitor
> on the relevant WebContents or Browser before trying to close it, to avoid
> flakiness.
> 
>
https://codereview.chromium.org/2005663002/diff/1/chrome/browser/lifetime/bro...
> chrome/browser/lifetime/browser_close_manager_browsertest.cc:255: // for that
> web content.
> Please mention that this must be called before any close action when using
> AcceptClose or CancelClose, to ensure the timeout does not prevent the dialog
> from appearing.
> 
>
https://codereview.chromium.org/2005663002/diff/1/chrome/browser/lifetime/bro...
> chrome/browser/lifetime/browser_close_manager_browsertest.cc:425:
> DisableHangMonitor(browsers_[0]->tab_strip_model()->GetWebContentsAt(1));
> nit: Move this line after this block of NavigateToURL and add a comment:
> 
> Disable the hang monitor in the tab that is not expected to hang, so that the
> dialog is guaranteed to show.
> 
>
https://codereview.chromium.org/2005663002/diff/1/chrome/browser/lifetime/bro...
> chrome/browser/lifetime/browser_close_manager_browsertest.cc:460:
> DisableHangMonitor(browsers_[1]);
> Same as above.
> 
>
https://codereview.chromium.org/2005663002/diff/1/content/browser/renderer_ho...
> File content/browser/renderer_host/render_widget_host_impl.cc (right):
> 
>
https://codereview.chromium.org/2005663002/diff/1/content/browser/renderer_ho...
> content/browser/renderer_host/render_widget_host_impl.cc:920:
> hang_monitor_timeout_.reset();
> Yeah, I think this should be sufficient for this test.  We also have
> RenderFrameHostImpl::DisableSwapOutTimerForTesting, but I don't think that's
> necessary here because the swapout/unload event won't ever show dialogs.
> 
> nit: Maybe change the name from ForTest() to ForTesting() to be consistent
(and
> since there's a lot more of the latter in the codebase).
> 
>
https://codereview.chromium.org/2005663002/diff/1/content/public/browser/rend...
> File content/public/browser/render_widget_host.h (right):
> 
>
https://codereview.chromium.org/2005663002/diff/1/content/public/browser/rend...
> content/public/browser/render_widget_host.h:222: // as under load this widget
> might not respond in time.
> nit: Let's rephrase the second sentence:
> 
> This avoids flakiness in tests that need to observe things like beforeunload
> dialogs, which could fail if the timeout skips the dialog.

Done + rebased CL to master,

[Charlie Reis, 2016-05-24 19:14:14]

creis@chromium.org changed reviewers:
+ wez@chromium.org

[Charlie Reis, 2016-05-24 19:14:14]

LGTM, thanks!

wez@: Can you stamp for blimp/?
sky@: Can you stamp for chrome/?  (I'll add the nav folks as owners to this test
as well, to avoid this in the future.)

[Charlie Reis, 2016-05-24 19:14:21]

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

[commit-bot: I haz the power, 2016-05-24 19:14:52]

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

[sky, 2016-05-24 20:44:51]

Rubber stamp LGTM

[Wez, 2016-05-24 20:48:07]

blimp/ LGTM

[Charlie Reis, 2016-05-24 20:55:38]

The CQ bit was unchecked by creis@chromium.org

[Charlie Reis, 2016-05-24 20:55:44]

The CQ bit was checked by creis@chromium.org

[commit-bot: I haz the power, 2016-05-24 20:56:03]

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

[commit-bot: I haz the power, 2016-05-24 21:10:55]

Description was changed from

==========
Make unload tests stable.

During unload tab has only one second to respond. If machine is busy doing
other tests renderer host might think that tab is unresponsive and close it
while test expecting the tab to react.

You can easily reproduce the failure by setting
RenderViewHostImpl::kUnloadTimeoutMS = 1

There are other tests that fail with that modification
(ex. UnloadTest.BrowserCloseBeforeUnloadCancel) but they are not flacky and I
kept them intact.

BUG=519646
R=creis@chromium.org,sky@chromium.org
==========

to

==========
Make unload tests stable.

During unload tab has only one second to respond. If machine is busy doing
other tests renderer host might think that tab is unresponsive and close it
while test expecting the tab to react.

You can easily reproduce the failure by setting
RenderViewHostImpl::kUnloadTimeoutMS = 1

There are other tests that fail with that modification
(ex. UnloadTest.BrowserCloseBeforeUnloadCancel) but they are not flacky and I
kept them intact.

BUG=519646
R=creis@chromium.org,sky@chromium.org
==========

[commit-bot: I haz the power, 2016-05-24 21:10:57]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-05-24 21:13:43]

Description was changed from

==========
Make unload tests stable.

During unload tab has only one second to respond. If machine is busy doing
other tests renderer host might think that tab is unresponsive and close it
while test expecting the tab to react.

You can easily reproduce the failure by setting
RenderViewHostImpl::kUnloadTimeoutMS = 1

There are other tests that fail with that modification
(ex. UnloadTest.BrowserCloseBeforeUnloadCancel) but they are not flacky and I
kept them intact.

BUG=519646
R=creis@chromium.org,sky@chromium.org
==========

to

==========
Make unload tests stable.

During unload tab has only one second to respond. If machine is busy doing
other tests renderer host might think that tab is unresponsive and close it
while test expecting the tab to react.

You can easily reproduce the failure by setting
RenderViewHostImpl::kUnloadTimeoutMS = 1

There are other tests that fail with that modification
(ex. UnloadTest.BrowserCloseBeforeUnloadCancel) but they are not flacky and I
kept them intact.

BUG=519646
R=creis@chromium.org,sky@chromium.org

Committed: https://crrev.com/812bb854380007efc5852037af26632797053e90
Cr-Commit-Position: refs/heads/master@{#395693}
==========

[commit-bot: I haz the power, 2016-05-24 21:13:44]

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