Allow navigation while a JavaScript dialog is up.

Allow navigation while a JavaScript dialog is up.

(It will cancel the dialog.)

BUG=669996
TEST=as in bug

Committed: https://crrev.com/24a1bf068b07d183d456afdb8629e851467829c3
Cr-Commit-Position: refs/heads/master@{#435963}

[Avi (use Gerrit), 2016-12-01 20:09:58]

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

[commit-bot: I haz the power, 2016-12-01 20:10:30]

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), 2016-12-01 20:55:11]

avi@chromium.org changed reviewers:
+ creis@chromium.org, isherman@chromium.org

[Avi (use Gerrit), 2016-12-01 20:55:12]

Charlie, navigation code
Ilya, UMA

[commit-bot: I haz the power, 2016-12-01 21:03:08]

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

[commit-bot: I haz the power, 2016-12-01 21:03:09]

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2016-12-01 21:36:48]

Thanks!  There's some subtlety below, so I wonder if this is something we can
add a test for.

https://codereview.chromium.org/2541173003/diff/1/chrome/browser/ui/javascrip...
File chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc
(right):

https://codereview.chromium.org/2541173003/diff/1/chrome/browser/ui/javascrip...
chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc:272: if
(dialog_ && navigation_handle->IsInMainFrame()) {
nit: No braces?

What's the reason for the IsInMainFrame check?  It probably makes sense, but I
just wanted to confirm.  Maybe it's to prevent OOPIFs (which aren't blocked)
from dismissing the dialog by navigating?

I do wonder if it is a problem, though.  What would happen if you visited
http://csreis.github.io/tests/cross-site-iframe.html, clicked "Go same-site,"
showed a dialog, and went back?  The back navigation would be in a subframe, and
we'd skip this logic.

(Then again, today you can't use the back button while a dialog is showing.  Can
you clarify the repro steps for this bug?)

https://codereview.chromium.org/2541173003/diff/1/chrome/browser/ui/javascrip...
chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc:281: void
JavaScriptDialogTabHelper::DidStartNavigationToPendingEntry(
Sanity check: This is only called for browser-initiated navigations.  Is that
ok, since you're assuming the renderer is blocked?

What if a cross-process window tells this tab to navigate (e.g., window.opener)?
 I guess that will hit the DidStartNavigation case above?  If so, maybe we
should clarify in the comments.

https://codereview.chromium.org/2541173003/diff/1/chrome/browser/ui/javascrip...
chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc:286: if
(dialog_) {
nit: No braces?

Interesting that this doesn't check whether it's a main frame.  If we decide
it's necessary above, should we find a way to do it here as well?  (I'm leaning
towards thinking it isn't necessary, though.)

[Avi (use Gerrit), 2016-12-01 23:17:40]

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

[commit-bot: I haz the power, 2016-12-01 23:18:16]

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), 2016-12-01 23:20:08]

https://codereview.chromium.org/2541173003/diff/1/chrome/browser/ui/javascrip...
File chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc
(right):

https://codereview.chromium.org/2541173003/diff/1/chrome/browser/ui/javascrip...
chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc:272: if
(dialog_ && navigation_handle->IsInMainFrame()) {
On 2016/12/01 21:36:48, Charlie Reis (slow) wrote:
> nit: No braces?

Done.

> What's the reason for the IsInMainFrame check?

Mostly because this patch is to enable the back/forward buttons and the reload
button, and those are main frame navigations. I'm not wedded to it, and it might
make sense.

> I do wonder if it is a problem, though.  What would happen if you visited
> http://csreis.github.io/tests/cross-site-iframe.html, clicked "Go same-site,"
> showed a dialog, and went back?  The back navigation would be in a subframe,
and
> we'd skip this logic.

Oooooh, right. Let's drop it.

> Then again, today you can't use the back button while a dialog is showing.

You can, though, with auto-dismissing dialogs. Give it a try with M56!

https://codereview.chromium.org/2541173003/diff/1/chrome/browser/ui/javascrip...
chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc:281: void
JavaScriptDialogTabHelper::DidStartNavigationToPendingEntry(
On 2016/12/01 21:36:48, Charlie Reis (slow) wrote:
> Sanity check: This is only called for browser-initiated navigations.  Is that
> ok, since you're assuming the renderer is blocked?

This is intended to deal with the user clicking the back/forward buttons and the
reload button, and all of those are browser-initiated. The renderer isn't
initiating anything; it's frozen.

> What if a cross-process window tells this tab to navigate (e.g.,
window.opener)?
> I guess that will hit the DidStartNavigation case above?  If so, maybe we
> should clarify in the comments.

I suppose so? My intent isn't to unblock the dialog in that case, but I'll take
it as a side-effect.

https://codereview.chromium.org/2541173003/diff/1/chrome/browser/ui/javascrip...
chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc:286: if
(dialog_) {
On 2016/12/01 21:36:48, Charlie Reis (slow) wrote:
> nit: No braces?
> 
> Interesting that this doesn't check whether it's a main frame.  If we decide
> it's necessary above, should we find a way to do it here as well?  (I'm
leaning
> towards thinking it isn't necessary, though.)

I'm agreeing that we should drop main frame checks everywhere.

[Charlie Reis, 2016-12-01 23:30:27]

Thanks!  Fix looks good.  Did you see my request for a test?  I think it's
worthwhile if we can.

https://codereview.chromium.org/2541173003/diff/1/chrome/browser/ui/javascrip...
File chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc
(right):

https://codereview.chromium.org/2541173003/diff/1/chrome/browser/ui/javascrip...
chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc:272: if
(dialog_ && navigation_handle->IsInMainFrame()) {
On 2016/12/01 23:20:08, Avi wrote:
> On 2016/12/01 21:36:48, Charlie Reis (slow) wrote:
> > nit: No braces?
> 
> Done.
> 
> > What's the reason for the IsInMainFrame check?
> 
> Mostly because this patch is to enable the back/forward buttons and the reload
> button, and those are main frame navigations. I'm not wedded to it, and it
might
> make sense.
> 
> > I do wonder if it is a problem, though.  What would happen if you visited
> > http://csreis.github.io/tests/cross-site-iframe.html, clicked "Go
same-site,"
> > showed a dialog, and went back?  The back navigation would be in a subframe,
> and
> > we'd skip this logic.
> 
> Oooooh, right. Let's drop it.
> 
> > Then again, today you can't use the back button while a dialog is showing.
> 
> You can, though, with auto-dismissing dialogs. Give it a try with M56!

Cool!  I thought I needed a flag, but yes, it works on Canary.

https://codereview.chromium.org/2541173003/diff/1/chrome/browser/ui/javascrip...
chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc:281: void
JavaScriptDialogTabHelper::DidStartNavigationToPendingEntry(
On 2016/12/01 23:20:07, Avi wrote:
> On 2016/12/01 21:36:48, Charlie Reis (slow) wrote:
> > Sanity check: This is only called for browser-initiated navigations.  Is
that
> > ok, since you're assuming the renderer is blocked?
> 
> This is intended to deal with the user clicking the back/forward buttons and
the
> reload button, and all of those are browser-initiated. The renderer isn't
> initiating anything; it's frozen.

Sounds good.  (Presumably you also care about omnibox navigations, which show
the same bug if they're same-site, and look like they'll fixed by this CL.)

> 
> > What if a cross-process window tells this tab to navigate (e.g.,
> window.opener)?
> > I guess that will hit the DidStartNavigation case above?  If so, maybe we
> > should clarify in the comments.
> 
> I suppose so? My intent isn't to unblock the dialog in that case, but I'll
take
> it as a side-effect.

Ok-- I'll be curious to see if that works.  Agreed it doesn't matter much either
way for this CL.

[commit-bot: I haz the power, 2016-12-02 00:13:14]

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

[commit-bot: I haz the power, 2016-12-02 00:13:15]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2016-12-02 00:53:19]

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

[commit-bot: I haz the power, 2016-12-02 00:53:44]

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), 2016-12-02 00:57:27]

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

[commit-bot: I haz the power, 2016-12-02 00:57:39]

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), 2016-12-02 01:32:26]

https://codereview.chromium.org/2541173003/diff/1/chrome/browser/ui/javascrip...
File chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc
(right):

https://codereview.chromium.org/2541173003/diff/1/chrome/browser/ui/javascrip...
chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc:272: if
(dialog_ && navigation_handle->IsInMainFrame()) {
On 2016/12/01 23:30:26, Charlie Reis (slow) wrote:
> On 2016/12/01 23:20:08, Avi wrote:
> > On 2016/12/01 21:36:48, Charlie Reis (slow) wrote:
> > > nit: No braces?
> > 
> > Done.
> > 
> > > What's the reason for the IsInMainFrame check?
> > 
> > Mostly because this patch is to enable the back/forward buttons and the
reload
> > button, and those are main frame navigations. I'm not wedded to it, and it
> might
> > make sense.
> > 
> > > I do wonder if it is a problem, though.  What would happen if you visited
> > > http://csreis.github.io/tests/cross-site-iframe.html, clicked "Go
> same-site,"
> > > showed a dialog, and went back?  The back navigation would be in a
subframe,
> > and
> > > we'd skip this logic.
> > 
> > Oooooh, right. Let's drop it.
> > 
> > > Then again, today you can't use the back button while a dialog is showing.
> > 
> > You can, though, with auto-dismissing dialogs. Give it a try with M56!
> 
> Cool!  I thought I needed a flag, but yes, it works on Canary.

Acknowledged.

https://codereview.chromium.org/2541173003/diff/1/chrome/browser/ui/javascrip...
chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc:281: void
JavaScriptDialogTabHelper::DidStartNavigationToPendingEntry(
On 2016/12/01 23:30:26, Charlie Reis (slow) wrote:
> Sounds good.  (Presumably you also care about omnibox navigations, which show
> the same bug if they're same-site, and look like they'll fixed by this CL.)

This fixes navigations that happen within the same renderer. New navigations,
including from the Omnibox, will proceed in a non-stuck renderer even without
this patch.

[Ilya Sherman, 2016-12-02 01:48:00]

histograms.xml lgtm

[commit-bot: I haz the power, 2016-12-02 03:21:25]

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

[commit-bot: I haz the power, 2016-12-02 03:21:26]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Charlie Reis, 2016-12-02 05:09:02]

Thanks for the test!  LGTM.

[Avi (use Gerrit), 2016-12-02 15:18:48]

The CQ bit was checked by avi@chromium.org

[commit-bot: I haz the power, 2016-12-02 15:19: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-12-02 17:21:51]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1480691928198220,
"parent_rev": "8b15fcd50b57621221e11a2f52c525023677a312", "commit_rev":
"2b36ccbf8d49ed40f60e73ccbb3fe6dbb20d8218"}

[commit-bot: I haz the power, 2016-12-02 17:22:27]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-02 17:24:45]

Description was changed from

==========
Allow navigation while a JavaScript dialog is up.

(It will cancel the dialog.)

BUG=669996
TEST=as in bug
==========

to

==========
Allow navigation while a JavaScript dialog is up.

(It will cancel the dialog.)

BUG=669996
TEST=as in bug

Committed: https://crrev.com/24a1bf068b07d183d456afdb8629e851467829c3
Cr-Commit-Position: refs/heads/master@{#435963}
==========

[commit-bot: I haz the power, 2016-12-02 17:24:47]

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