Fix races in tests waiting for beforeunload dialogs.

Fix races in tests waiting for beforeunload dialogs.

BUG=707342, 130411
TEST=no flakes
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2787393003
Cr-Commit-Position: refs/heads/master@{#462293}
Committed: https://chromium.googlesource.com/chromium/src/+/3c70a0d266c02d7e91fc5a2e5649247367a8fecc

[Avi (use Gerrit), 2017-03-31 20:23:49]

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

[Avi (use Gerrit), 2017-03-31 20:23:49]

Description was changed from

==========
Fix hangs in tests waiting for beforeunload dialogs.

BUG=707342
TEST=no flakes
==========

to

==========
Fix hangs in tests waiting for beforeunload dialogs.

BUG=707342
TEST=no flakes
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2017-03-31 20:24:17]

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

[Avi (use Gerrit), 2017-03-31 21:01:24]

Description was changed from

==========
Fix hangs in tests waiting for beforeunload dialogs.

BUG=707342
TEST=no flakes
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fix hangs in tests waiting for beforeunload dialogs.

BUG=707342, 130411
TEST=no flakes
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2017-03-31 21:09:02]

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

[commit-bot: I haz the power, 2017-03-31 21:09:03]

Dry run: 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_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Avi (use Gerrit), 2017-03-31 21:12:30]

Description was changed from

==========
Fix hangs in tests waiting for beforeunload dialogs.

BUG=707342, 130411
TEST=no flakes
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fix races in tests waiting for beforeunload dialogs.

BUG=707342, 130411
TEST=no flakes
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Avi (use Gerrit), 2017-03-31 21:36:44]

Description was changed from

==========
Fix races in tests waiting for beforeunload dialogs.

BUG=707342, 130411
TEST=no flakes
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fix races in tests waiting for beforeunload dialogs.

BUG=707342, 130411, 655628
TEST=no flakes
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Avi (use Gerrit), 2017-03-31 21:40:06]

Description was changed from

==========
Fix races in tests waiting for beforeunload dialogs.

BUG=707342, 130411, 655628
TEST=no flakes
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fix races in tests waiting for beforeunload dialogs.

BUG=707342, 130411
TEST=no flakes
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Avi (use Gerrit), 2017-03-31 21:45:00]

Description was changed from

==========
Fix races in tests waiting for beforeunload dialogs.

BUG=707342, 130411
TEST=no flakes
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fix races in tests waiting for beforeunload dialogs.

BUG=707342, 130411, 655628
TEST=no flakes
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Avi (use Gerrit), 2017-03-31 21:45:00]

avi@chromium.org changed reviewers:
+ clamy@chromium.org, jam@chromium.org

[Avi (use Gerrit), 2017-03-31 21:46:13]

Description was changed from

==========
Fix races in tests waiting for beforeunload dialogs.

BUG=707342, 130411, 655628
TEST=no flakes
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fix races in tests waiting for beforeunload dialogs.

BUG=707342, 130411
TEST=no flakes
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Avi (use Gerrit), 2017-03-31 21:53:53]

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

[commit-bot: I haz the power, 2017-03-31 21:54:22]

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

[Avi (use Gerrit), 2017-03-31 22:12:43]

avi@chromium.org changed reviewers:
- clamy@chromium.org

[commit-bot: I haz the power, 2017-04-01 00:12:40]

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

[commit-bot: I haz the power, 2017-04-01 00:12:41]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Avi (use Gerrit), 2017-04-01 20:49:42]

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

[commit-bot: I haz the power, 2017-04-01 20:49:54]

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, 2017-04-01 21:28:17]

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

[commit-bot: I haz the power, 2017-04-01 21:28:18]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2017-04-03 19:56:11]

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

[commit-bot: I haz the power, 2017-04-03 19:56:51]

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, 2017-04-03 20:34:39]

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

[commit-bot: I haz the power, 2017-04-03 20:34:40]

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

[Avi (use Gerrit), 2017-04-04 20:37:41]

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

[commit-bot: I haz the power, 2017-04-04 20:38:25]

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

[Avi (use Gerrit), 2017-04-04 20:40:04]

John, please take a look.

[commit-bot: I haz the power, 2017-04-04 21:59:51]

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

[commit-bot: I haz the power, 2017-04-04 21:59:52]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2017-04-05 19:11:14]

avi@chromium.org changed reviewers:
+ thakis@chromium.org
- jam@chromium.org

[Avi (use Gerrit), 2017-04-05 19:11:14]

Nico, can you take this?

[Nico, 2017-04-05 19:26:00]

Two questions:

1. Shouldn't the beforeunload timer be disabled before navigations to it instead
of after?
2. This is a medium amount of copy-pasta does it make sense to try and
consolidate it into some "NavigateToOnUnloadURL()" function?

https://codereview.chromium.org/2787393003/diff/60001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_browsertest.cc (right):

https://codereview.chromium.org/2787393003/diff/60001/chrome/browser/ui/brows...
chrome/browser/ui/browser_browsertest.cc:838:
contents->GetMainFrame()->DisableBeforeUnloadHangMonitorForTesting();
Shouldn't you do this before the navigation to the beforeunload page right
above? (question applies to everywhere in this file)

[Avi (use Gerrit), 2017-04-05 19:42:40]

On 2017/04/05 19:26:00, Nico wrote:
> 1. Shouldn't the beforeunload timer be disabled before navigations to it
instead
> of after?

I don't see why it would need to be. It would work either way.

When you load a page with a beforeunload handler, you need to disable the timer
before you *leave* that page. Loading that page is fine. So I let the pages load
and immediately disable the timer afterwards.

> 2. This is a medium amount of copy-pasta does it make sense to try and
> consolidate it into some "NavigateToOnUnloadURL()" function?

Every test has some variation of a flavor of beforeunload. I'm not going to
second-guess the contents of the pages loaded by these tests. (Also, we're
talking beforeunload, not unload.)

[Nico, 2017-04-05 20:02:53]

lgtm

[Avi (use Gerrit), 2017-04-05 23:00:22]

The CQ bit was checked by avi@chromium.org

[commit-bot: I haz the power, 2017-04-05 23:00:54]

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, 2017-04-06 00:31:46]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491433222320710,
"parent_rev": "c467f3db3f1e4d2aef973373740d7213e6660246", "commit_rev":
"3c70a0d266c02d7e91fc5a2e5649247367a8fecc"}

[commit-bot: I haz the power, 2017-04-06 00:35:56]

Description was changed from

==========
Fix races in tests waiting for beforeunload dialogs.

BUG=707342, 130411
TEST=no flakes
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Fix races in tests waiting for beforeunload dialogs.

BUG=707342, 130411
TEST=no flakes
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2787393003
Cr-Commit-Position: refs/heads/master@{#462293}
Committed:
https://chromium.googlesource.com/chromium/src/+/3c70a0d266c02d7e91fc5a2e5649...
==========

[commit-bot: I haz the power, 2017-04-06 00:35:58]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/3c70a0d266c02d7e91fc5a2e5649...