Allow navigation while a JavaScript dialog is up.

chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc

Index: chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc
diff --git a/chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc b/chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc
index 228350fa12cad54b97ae342c52aadc781c9b4938..229ac3006a8c73740ba2892c0e4f89ee0b26b117 100644
--- a/chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc
+++ b/chrome/browser/ui/javascript_dialogs/javascript_dialog_tab_helper.cc
@@ -16,6 +16,7 @@
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/common/chrome_features.h"
 #include "components/app_modal/javascript_dialog_manager.h"
+#include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/render_frame_host.h"
 
 DEFINE_WEB_CONTENTS_USER_DATA_KEY(JavaScriptDialogTabHelper);
@@ -66,6 +67,7 @@ enum class JavaScriptDialogTabHelper::DismissalCause {
   TAB_HIDDEN = 4,
   BROWSER_SWITCHED = 5,
   DIALOG_BUTTON_CLICKED = 6,
+  TAB_NAVIGATED = 7,
   MAX,
 };
 
@@ -259,6 +261,33 @@ void JavaScriptDialogTabHelper::WasHidden() {
   }
 }
 
+// This function handles the case where browser-side navigation (PlzNavigate) is
+// enabled. DidStartNavigationToPendingEntry, below, handles the case where
+// PlzNavigate is not enabled. TODO(avi): When the non-PlzNavigate code is
+// removed, remove DidStartNavigationToPendingEntry.
+void JavaScriptDialogTabHelper::DidStartNavigation(
+    content::NavigationHandle* navigation_handle) {
+  // Close the dialog if the user started a new navigation. This allows reloads
+  // and history navigations to proceed.
+  if (dialog_ && navigation_handle->IsInMainFrame()) {

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

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?)

[Avi (use Gerrit), 2016/12/01 23:20:08]

Done.
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.
Oooooh, right. Let's drop it.
You can, though, with auto-dismissing dialogs. Give it a try with M56!

[Charlie Reis, 2016/12/01 23:30:26]

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

[Avi (use Gerrit), 2016/12/02 01:32:26]

Acknowledged.

+    CloseDialog(false, false, base::string16(), DismissalCause::TAB_NAVIGATED);
+  }
+}
+
+// This function handles the case where browser-side navigation (PlzNavigate) is
+// not enabled. DidStartNavigation, above, handles the case where PlzNavigate is
+// enabled. TODO(avi): When the non-PlzNavigate code is removed, remove
+// DidStartNavigationToPendingEntry.
+void JavaScriptDialogTabHelper::DidStartNavigationToPendingEntry(

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

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.

[Avi (use Gerrit), 2016/12/01 23:20:07]

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.
I suppose so? My intent isn't to unblock the dialog in that case, but I'll take
it as a side-effect.

[Charlie Reis, 2016/12/01 23:30:26]

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.)
Ok-- I'll be curious to see if that works.  Agreed it doesn't matter much either
way for this CL.

[Avi (use Gerrit), 2016/12/02 01:32:26]

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.

+    const GURL& url,
+    content::ReloadType reload_type) {
+  // Close the dialog if the user started a new navigation. This allows reloads
+  // and history navigations to proceed.
+  if (dialog_) {

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

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:20:08]

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

+    CloseDialog(false, false, base::string16(), DismissalCause::TAB_NAVIGATED);
+  }
+}
+
 void JavaScriptDialogTabHelper::OnBrowserSetLastActive(Browser* browser) {
   if (dialog_ && !IsWebContentsForemost(web_contents())) {
     CloseDialog(false, false, base::string16(),