Always update the omnibox URL when cancelling via onbeforeunload

Always update the omnibox URL when cancelling via onbeforeunload

The problem was that browser-initiated, same-site navigations that are
cancelled via a beforeunload dialog didn't send a INVALIDATE_TYPE_URL
invalidation to the Browser, because this invalidation was done as
result of the beforeunload ACK, and we never got a beforeunload ACK in
that case in non-PlzNavigate.

PlzNavigate did not have this problem.

We already cleared the pending entry -- in WebContentsImpl::OnDialogClosed,
just before we send the response to the RunJavaScriptDialog result. The fix
is to invalidate the omnibox at the same moment.

BUG=721184
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2919593007
Cr-Commit-Position: refs/heads/master@{#480598}
Committed: https://chromium.googlesource.com/chromium/src/+/bcce9cb42a1578ba95ead8b64e66101a8ae5e40b

[ncarter (slow), 2017-06-01 20:28:59]

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

[ncarter (slow), 2017-06-01 20:29:00]

Description was changed from

==========
Fix

The problem was that browser-initated, same-site navigations that are
cancelled via a beforeunload dialog didn't send a INVALIDATE_TYPE_URL
invalidation, because this invalidation was done as result of the unload
ACK, and we never got an unload ACK in that case.

We already cleared the pending entry -- in WebContentsImpl::OnDialogClosed,
just before we send the response to the RunJavaScriptDialog result. The fix
is to invalidate the omnibox at the same moment.

BUG=721184
==========

to

==========
Fix

The problem was that browser-initated, same-site navigations that are
cancelled via a beforeunload dialog didn't send a INVALIDATE_TYPE_URL
invalidation, because this invalidation was done as result of the unload
ACK, and we never got an unload ACK in that case.

We already cleared the pending entry -- in WebContentsImpl::OnDialogClosed,
just before we send the response to the RunJavaScriptDialog result. The fix
is to invalidate the omnibox at the same moment.

BUG=721184
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2017-06-01 20:29:36]

Dry run: CQ is trying da patch.

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

[ncarter (slow), 2017-06-01 20:49:17]

Description was changed from

==========
Fix

The problem was that browser-initated, same-site navigations that are
cancelled via a beforeunload dialog didn't send a INVALIDATE_TYPE_URL
invalidation, because this invalidation was done as result of the unload
ACK, and we never got an unload ACK in that case.

We already cleared the pending entry -- in WebContentsImpl::OnDialogClosed,
just before we send the response to the RunJavaScriptDialog result. The fix
is to invalidate the omnibox at the same moment.

BUG=721184
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Always update the omnibox URL when cancelling via onbeforeunload

The problem was that browser-initated, same-site navigations that are
cancelled via a beforeunload dialog didn't send a INVALIDATE_TYPE_URL
invalidation, because this invalidation was done as result of the unload
ACK, and we never got an unload ACK in that case.

We already cleared the pending entry -- in WebContentsImpl::OnDialogClosed,
just before we send the response to the RunJavaScriptDialog result. The fix
is to invalidate the omnibox at the same moment.

BUG=721184
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[ncarter (slow), 2017-06-01 20:50:01]

Description was changed from

==========
Always update the omnibox URL when cancelling via onbeforeunload

The problem was that browser-initated, same-site navigations that are
cancelled via a beforeunload dialog didn't send a INVALIDATE_TYPE_URL
invalidation, because this invalidation was done as result of the unload
ACK, and we never got an unload ACK in that case.

We already cleared the pending entry -- in WebContentsImpl::OnDialogClosed,
just before we send the response to the RunJavaScriptDialog result. The fix
is to invalidate the omnibox at the same moment.

BUG=721184
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Always update the omnibox URL when cancelling via onbeforeunload

The problem was that browser-initated, same-site navigations that are
cancelled via a beforeunload dialog didn't send a INVALIDATE_TYPE_URL
invalidation to the Browser, because this invalidation was done as
result of the unload ACK, and we never got an unload ACK in that case
in non-PlzNavigate.

PlzNavigate did not have this problem.

We already cleared the pending entry -- in WebContentsImpl::OnDialogClosed,
just before we send the response to the RunJavaScriptDialog result. The fix
is to invalidate the omnibox at the same moment.

BUG=721184
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[ncarter (slow), 2017-06-01 20:51:44]

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

[commit-bot: I haz the power, 2017-06-01 20:52:27]

Dry run: CQ is trying da patch.

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

[ncarter (slow), 2017-06-01 20:53:29]

The CQ bit was unchecked by nick@chromium.org

[ncarter (slow), 2017-06-01 20:55:47]

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

[commit-bot: I haz the power, 2017-06-01 20:56:26]

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-06-01 21:28:35]

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

[commit-bot: I haz the power, 2017-06-01 21:28:36]

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

[ncarter (slow), 2017-06-01 22:11:44]

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

[commit-bot: I haz the power, 2017-06-01 22:12:41]

Dry run: CQ is trying da patch.

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

[ncarter (slow), 2017-06-01 22:22:43]

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

[commit-bot: I haz the power, 2017-06-01 22:23:43]

Dry run: CQ is trying da patch.

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

[ncarter (slow), 2017-06-01 23:08:06]

nick@chromium.org changed reviewers:
+ clamy@chromium.org, creis@chromium.org

[ncarter (slow), 2017-06-01 23:08:07]

Charlie & Camille -- please review

https://codereview.chromium.org/2919593007/diff/70001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/2919593007/diff/70001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1737: void
NavigationControllerImpl::DiscardNonCommittedEntries() {
I debated whether to go with the approach you see here, or to just add a new
call to NotifyNavigationStateChanged from
WebContentsImpl::OnDialogClosed.

I put it here because the proper place to send a notification that a value has
changed, is from the class that owns the value. And since we already do a
notification here for the transient case, so doing it for the pending case seems
to make sense too.

However, because DiscardNonCommittedEntries called from several places (it's
content/public, actually), it's possible that this approach will either cause
double-invalidation. One case I've thought about is this one:
https://cs.chromium.org/chromium/src/components/omnibox/browser/omnibox_edit_...

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

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

[commit-bot: I haz the power, 2017-06-02 00:06:05]

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2017-06-02 17:01:52]

Description was changed from

==========
Always update the omnibox URL when cancelling via onbeforeunload

The problem was that browser-initated, same-site navigations that are
cancelled via a beforeunload dialog didn't send a INVALIDATE_TYPE_URL
invalidation to the Browser, because this invalidation was done as
result of the unload ACK, and we never got an unload ACK in that case
in non-PlzNavigate.

PlzNavigate did not have this problem.

We already cleared the pending entry -- in WebContentsImpl::OnDialogClosed,
just before we send the response to the RunJavaScriptDialog result. The fix
is to invalidate the omnibox at the same moment.

BUG=721184
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Always update the omnibox URL when cancelling via onbeforeunload

The problem was that browser-initiated, same-site navigations that are
cancelled via a beforeunload dialog didn't send a INVALIDATE_TYPE_URL
invalidation to the Browser, because this invalidation was done as
result of the beforeunload ACK, and we never got a beforeunload ACK in
that case in non-PlzNavigate.

PlzNavigate did not have this problem.

We already cleared the pending entry -- in WebContentsImpl::OnDialogClosed,
just before we send the response to the RunJavaScriptDialog result. The fix
is to invalidate the omnibox at the same moment.

BUG=721184
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Charlie Reis, 2017-06-02 23:57:58]

LGTM with some sanity check questions in the test.  Thanks for the cleanup along
with the fix!

https://codereview.chromium.org/2919593007/diff/70001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/2919593007/diff/70001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1737: void
NavigationControllerImpl::DiscardNonCommittedEntries() {
On 2017/06/01 23:08:07, ncarter (slow) wrote:
> I debated whether to go with the approach you see here, or to just add a new
> call to NotifyNavigationStateChanged from
> WebContentsImpl::OnDialogClosed.
> 
> I put it here because the proper place to send a notification that a value has
> changed, is from the class that owns the value. And since we already do a
> notification here for the transient case, so doing it for the pending case
seems
> to make sense too.
> 
> However, because DiscardNonCommittedEntries called from several places (it's
> content/public, actually), it's possible that this approach will either cause
> double-invalidation. One case I've thought about is this one:
>
https://cs.chromium.org/chromium/src/components/omnibox/browser/omnibox_edit_...

I like the approach you've taken.  Can you update the declaration comment in
navigation_controller.h?

(I think the OmniboxEditModel::OnEscapeKeyPressed case should be ok...)

https://codereview.chromium.org/2919593007/diff/70001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl_browsertest.cc (right):

https://codereview.chromium.org/2919593007/diff/70001/content/browser/web_con...
content/browser/web_contents/web_contents_impl_browsertest.cc:1016: //
cross-site navigation, it will not reset the loading state.
The test covers both same-site and cross-site, and my impression was that the
current bug was in the same-site case.  Is that right?

https://codereview.chromium.org/2919593007/diff/70001/content/browser/web_con...
content/browser/web_contents/web_contents_impl_browsertest.cc:1035:
EXPECT_TRUE(ExecuteScript(shell(), ""));
Avi added PrepContentsForBeforeUnloadTest for this (which also disables the hang
monitor to avoid flaky tests due to the 1 second timeout on beforeunload).

https://codereview.chromium.org/2919593007/diff/70001/content/browser/web_con...
content/browser/web_contents/web_contents_impl_browsertest.cc:1045:
web_contents_delegate.GetAndClearVisibleUrlInvalidations());
Sanity check: Is this the part that failed before, due to the lack of
beforeunload ACK in the same-site case?

https://codereview.chromium.org/2919593007/diff/70001/content/browser/web_con...
content/browser/web_contents/web_contents_impl_browsertest.cc:1055:
web_contents_delegate.GetAndClearVisibleUrlInvalidations());
Is this just for completeness, or did this cross-site case fail before your fix
as well?  (My impression is that it was handled in
https://codereview.chromium.org/6904039 and covered by
BrowserTest.CancelBeforeUnloadResetsURL, but I could certainly be wrong.)

[ncarter (slow), 2017-06-19 17:45:50]

https://codereview.chromium.org/2919593007/diff/70001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl_browsertest.cc (right):

https://codereview.chromium.org/2919593007/diff/70001/content/browser/web_con...
content/browser/web_contents/web_contents_impl_browsertest.cc:1016: //
cross-site navigation, it will not reset the loading state.
On 2017/06/02 23:57:57, Charlie Reis (slow) wrote:
> The test covers both same-site and cross-site, and my impression was that the
> current bug was in the same-site case.  Is that right?

The coverage of the cross-process case was to make sure it doesn't regress when
fixing the in-process case.

Specifically the test is designed to fail if double-notification occurs in
either path; this was a bug I encountered in the cross-process case while
developing the change, and which no existing test seems to be sensitive to.

https://codereview.chromium.org/2919593007/diff/70001/content/browser/web_con...
content/browser/web_contents/web_contents_impl_browsertest.cc:1035:
EXPECT_TRUE(ExecuteScript(shell(), ""));
On 2017/06/02 23:57:57, Charlie Reis (slow) wrote:
> Avi added PrepContentsForBeforeUnloadTest for this (which also disables the
hang
> monitor to avoid flaky tests due to the 1 second timeout on beforeunload).

I was hoping that disabling the hang monitor timeout wouldn't be necessary,
since this unload handler is fast, but it's probably better to be paranoid.

https://codereview.chromium.org/2919593007/diff/70001/content/browser/web_con...
content/browser/web_contents/web_contents_impl_browsertest.cc:1045:
web_contents_delegate.GetAndClearVisibleUrlInvalidations());
On 2017/06/02 23:57:57, Charlie Reis (slow) wrote:
> Sanity check: Is this the part that failed before, due to the lack of
> beforeunload ACK in the same-site case?

Yes.

https://codereview.chromium.org/2919593007/diff/70001/content/browser/web_con...
content/browser/web_contents/web_contents_impl_browsertest.cc:1055:
web_contents_delegate.GetAndClearVisibleUrlInvalidations());
On 2017/06/02 23:57:57, Charlie Reis (slow) wrote:
> Is this just for completeness, or did this cross-site case fail before your
fix
> as well?  (My impression is that it was handled in
> https://codereview.chromium.org/6904039 and covered by
> BrowserTest.CancelBeforeUnloadResetsURL, but I could certainly be wrong.)

See above. This coverage different from CancelBeforeUnloadResetsURL, because
it's (1) a test of the content-layer INVALIDATE_TYPE_URL contract, vs the
end-to-end URL behavior, and (2) it's sensitive to double-notification bugs,
which (from what I can tell) BrowserTest.CancelBeforeUnloadResetsURL is not.

[ncarter (slow), 2017-06-19 18:14:19]

https://codereview.chromium.org/2919593007/diff/70001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/2919593007/diff/70001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1737: void
NavigationControllerImpl::DiscardNonCommittedEntries() {
On 2017/06/02 23:57:57, Charlie Reis (slow) wrote:
> On 2017/06/01 23:08:07, ncarter (slow) wrote:
> > I debated whether to go with the approach you see here, or to just add a new
> > call to NotifyNavigationStateChanged from
> > WebContentsImpl::OnDialogClosed.
> > 
> > I put it here because the proper place to send a notification that a value
has
> > changed, is from the class that owns the value. And since we already do a
> > notification here for the transient case, so doing it for the pending case
> seems
> > to make sense too.
> > 
> > However, because DiscardNonCommittedEntries called from several places (it's
> > content/public, actually), it's possible that this approach will either
cause
> > double-invalidation. One case I've thought about is this one:
> >
>
https://cs.chromium.org/chromium/src/components/omnibox/browser/omnibox_edit_...
> 
> I like the approach you've taken.  Can you update the declaration comment in
> navigation_controller.h?
> 
> (I think the OmniboxEditModel::OnEscapeKeyPressed case should be ok...)

Done.

[ncarter (slow), 2017-06-19 18:14:24]

The CQ bit was checked by nick@chromium.org

[ncarter (slow), 2017-06-19 18:14:24]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org
Link to the patchset: https://codereview.chromium.org/2919593007/#ps90001
(title: "Update comment in navigation_controller.h")

[commit-bot: I haz the power, 2017-06-19 18:14:37]

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-06-19 18:16:40]

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

[commit-bot: I haz the power, 2017-06-19 18:16:41]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ncarter (slow), 2017-06-19 20:28:45]

The CQ bit was checked by nick@chromium.org

[ncarter (slow), 2017-06-19 20:28:46]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org
Link to the patchset: https://codereview.chromium.org/2919593007/#ps110001
(title: "Merge remote-tracking branch 'origin/master' into spoof")

[commit-bot: I haz the power, 2017-06-19 20:29:03]

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-06-19 22:17:12]

CQ is committing da patch.

Bot data: {"patchset_id": 110001, "attempt_start_ts": 1497904125772360,
"parent_rev": "14aec2a6a42986423efbbdb09b5f7d1be2b3d92f", "commit_rev":
"bcce9cb42a1578ba95ead8b64e66101a8ae5e40b"}

[commit-bot: I haz the power, 2017-06-19 22:17:24]

Description was changed from

==========
Always update the omnibox URL when cancelling via onbeforeunload

The problem was that browser-initiated, same-site navigations that are
cancelled via a beforeunload dialog didn't send a INVALIDATE_TYPE_URL
invalidation to the Browser, because this invalidation was done as
result of the beforeunload ACK, and we never got a beforeunload ACK in
that case in non-PlzNavigate.

PlzNavigate did not have this problem.

We already cleared the pending entry -- in WebContentsImpl::OnDialogClosed,
just before we send the response to the RunJavaScriptDialog result. The fix
is to invalidate the omnibox at the same moment.

BUG=721184
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Always update the omnibox URL when cancelling via onbeforeunload

The problem was that browser-initiated, same-site navigations that are
cancelled via a beforeunload dialog didn't send a INVALIDATE_TYPE_URL
invalidation to the Browser, because this invalidation was done as
result of the beforeunload ACK, and we never got a beforeunload ACK in
that case in non-PlzNavigate.

PlzNavigate did not have this problem.

We already cleared the pending entry -- in WebContentsImpl::OnDialogClosed,
just before we send the response to the RunJavaScriptDialog result. The fix
is to invalidate the omnibox at the same moment.

BUG=721184
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2919593007
Cr-Commit-Position: refs/heads/master@{#480598}
Committed:
https://chromium.googlesource.com/chromium/src/+/bcce9cb42a1578ba95ead8b64e66...
==========

[commit-bot: I haz the power, 2017-06-19 22:17:25]

Committed patchset #7 (id:110001) as
https://chromium.googlesource.com/chromium/src/+/bcce9cb42a1578ba95ead8b64e66...

[pdr., 2017-06-20 00:38:40]

On 2017/06/19 at 22:17:25, commit-bot wrote:
> Committed patchset #7 (id:110001) as
https://chromium.googlesource.com/chromium/src/+/bcce9cb42a1578ba95ead8b64e66...

I think this patch caused a failure:
https://bugs.chromium.org/p/chromium/issues/detail?id=734835

[pdr., 2017-06-20 00:56:37]

A revert of this CL (patchset #7 id:110001) has been created in
https://codereview.chromium.org/2952483002/ by pdr@chromium.org.

The reason for reverting is: This caused failures on
browser_side_navigation_content_browsertests
(WebContentsImplBrowserTest.DismissingBeforeUnloadDialogInvalidatesUrl). See:
https://crbug.com/734835 for links to the builders..