Always update the omnibox URL when cancelling via onbeforeunload

content/browser/web_contents/web_contents_impl_browsertest.cc

Index: content/browser/web_contents/web_contents_impl_browsertest.cc
diff --git a/content/browser/web_contents/web_contents_impl_browsertest.cc b/content/browser/web_contents/web_contents_impl_browsertest.cc
index 71c3c4239040f8529b97f593f4c3c8eac826e64a..1641a32c3dc41f7e8539eb02749134ac11cb29b5 100644
--- a/content/browser/web_contents/web_contents_impl_browsertest.cc
+++ b/content/browser/web_contents/web_contents_impl_browsertest.cc
@@ -939,6 +939,148 @@ IN_PROC_BROWSER_TEST_F(WebContentsImplBrowserTest,
 
 namespace {
 
+// Test implementation of WebContentsDelegate that handles and automatically
+// cancels beforeunload dialogs. This class also listens for
+// NavigationStateChanged(INVALIDATE_TYPE_URL) and records the state of
+// GetVisibleURL().
+class DialogDismissingWebContentsDelegate : public JavaScriptDialogManager,
+                                            public WebContentsDelegate {
+ public:
+  DialogDismissingWebContentsDelegate()
+      : message_loop_runner_(new MessageLoopRunner) {}
+  ~DialogDismissingWebContentsDelegate() override {}
+
+  void WaitForDialogDismissed() {
+    message_loop_runner_->Run();
+    message_loop_runner_ = new MessageLoopRunner;
+  }
+
+  // The recent of values of web_contents()->GetVisibleURL(), recorded each time
+  // this WebContentsDelegate gets an INVALIDATE_TYPE_URL event.
+  std::vector<GURL> GetAndClearVisibleUrlInvalidations() {
+    return std::move(visible_url_invalidations_);
+  }
+
+  // WebContentsDelegate
+
+  JavaScriptDialogManager* GetJavaScriptDialogManager(
+      WebContents* source) override {
+    return this;
+  }
+  void NavigationStateChanged(WebContents* web_contents,
+                              InvalidateTypes invalidate_types) override {
+    if (invalidate_types & INVALIDATE_TYPE_URL) {
+      visible_url_invalidations_.push_back(web_contents->GetVisibleURL());
+    }
+  }
+
+  // JavaScriptDialogManager
+
+  void RunJavaScriptDialog(WebContents* web_contents,
+                           const GURL& origin_url,
+                           JavaScriptDialogType dialog_type,
+                           const base::string16& message_text,
+                           const base::string16& default_prompt_text,
+                           const DialogClosedCallback& callback,
+                           bool* did_suppress_message) override {}
+
+  void RunBeforeUnloadDialog(WebContents* web_contents,
+                             bool is_reload,
+                             const DialogClosedCallback& callback) override {
+    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+                            base::Bind(callback, false, base::string16()));
+    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+                            message_loop_runner_->QuitClosure());
+  }
+
+  bool HandleJavaScriptDialog(WebContents* web_contents,
+                              bool accept,
+                              const base::string16* prompt_override) override {
+    return true;
+  }
+
+  void CancelDialogs(WebContents* web_contents, bool reset_state) override {}
+
+ private:
+  std::vector<GURL> visible_url_invalidations_;
+
+  // The MessageLoopRunner used to spin the message loop.
+  scoped_refptr<MessageLoopRunner> message_loop_runner_;
+
+  DISALLOW_COPY_AND_ASSIGN(DialogDismissingWebContentsDelegate);
+};
+
+}  // namespace
+
+// Test that if a BeforeUnload dialog is destroyed due to the commit of a
+// cross-site navigation, it will not reset the loading state.

[Charlie Reis, 2017/06/02 23:57:57]

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?

[ncarter (slow), 2017/06/19 17:45:49]

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.

+IN_PROC_BROWSER_TEST_F(WebContentsImplBrowserTest,
+                       DismissingBeforeUnloadDialogInvalidatesUrl) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+  const GURL kStartURL(embedded_test_server()->GetURL(
+      "foo.com", "/render_frame_host/beforeunload.html"));
+  const GURL kSameSiteURL(
+      embedded_test_server()->GetURL("foo.com", "/title1.html"));
+  const GURL kCrossSiteURL(
+      embedded_test_server()->GetURL("bar.com", "/title1.html"));
+
+  // Navigate to a first web page with a BeforeUnload event listener.
+  EXPECT_TRUE(NavigateToURL(shell(), kStartURL));
+
+  DialogDismissingWebContentsDelegate web_contents_delegate;
+  shell()->web_contents()->SetDelegate(&web_contents_delegate);
+
+  // Send a user gesture (a side effect of ExecuteScript); otherwise the
+  // beforeunload handler won't run.
+  EXPECT_TRUE(ExecuteScript(shell(), ""));

[Charlie Reis, 2017/06/02 23:57:57]

Avi added PrepContentsForBeforeUnloadTest for this (which also disables the hang
monitor to avoid flaky tests due to the 1 second timeout on beforeunload).

[ncarter (slow), 2017/06/19 17:45:49]

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.

+
+  // Start a same-site navigation that is cancelled because of the beforeunload
+  // dialog. INVALIDATE_TYPE_URL should be sent to the delegate after the
+  // cancellation, so that the location bar resets to the original URL.
+  shell()->LoadURL(kSameSiteURL);
+  EXPECT_EQ(kSameSiteURL, shell()->web_contents()->GetVisibleURL());
+  web_contents_delegate.WaitForDialogDismissed();
+  EXPECT_TRUE(WaitForLoadStop(shell()->web_contents()));
+  EXPECT_EQ(std::vector<GURL>{kStartURL},
+            web_contents_delegate.GetAndClearVisibleUrlInvalidations());

[Charlie Reis, 2017/06/02 23:57:57]

Sanity check: Is this the part that failed before, due to the lack of
beforeunload ACK in the same-site case?

[ncarter (slow), 2017/06/19 17:45:49]

Yes.

+
+  // Start a cross-site navigation that is cancelled because of the beforeunload
+  // dialog. INVALIDATE_TYPE_URL should be sent to the delegate after the
+  // cancellation, so that the location bar resets to the original URL.
+  shell()->LoadURL(kCrossSiteURL);
+  EXPECT_EQ(kCrossSiteURL, shell()->web_contents()->GetVisibleURL());
+  web_contents_delegate.WaitForDialogDismissed();
+  EXPECT_TRUE(WaitForLoadStop(shell()->web_contents()));
+  EXPECT_EQ(std::vector<GURL>{kStartURL},
+            web_contents_delegate.GetAndClearVisibleUrlInvalidations());

[Charlie Reis, 2017/06/02 23:57:57]

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:49]

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.

+
+  // Now clear the unload handler and re-try the navigations, which should now
+  // succeed. INVALIDATE_TYPE_URL should happen on commit, with the value of the
+  // new URL.
+  EXPECT_TRUE(ExecuteScript(shell(),
+                            "window.onbeforeunload=function(e){return null;}"));
+  shell()->LoadURL(kSameSiteURL);
+  EXPECT_EQ(kSameSiteURL, shell()->web_contents()->GetVisibleURL());
+  EXPECT_TRUE(WaitForLoadStop(shell()->web_contents()));
+  EXPECT_EQ(kSameSiteURL, shell()->web_contents()->GetVisibleURL());
+  EXPECT_EQ(std::vector<GURL>{kSameSiteURL},
+            web_contents_delegate.GetAndClearVisibleUrlInvalidations());
+
+  shell()->LoadURL(kCrossSiteURL);
+  EXPECT_EQ(kCrossSiteURL, shell()->web_contents()->GetVisibleURL());
+  EXPECT_TRUE(WaitForLoadStop(shell()->web_contents()));
+  EXPECT_EQ(kCrossSiteURL, shell()->web_contents()->GetVisibleURL());
+  EXPECT_EQ(std::vector<GURL>{kCrossSiteURL},
+            web_contents_delegate.GetAndClearVisibleUrlInvalidations());
+
+  // Clear the test delegates.
+  static_cast<WebContentsImpl*>(shell()->web_contents())
+      ->SetJavaScriptDialogManagerForTesting(nullptr);
+  shell()->web_contents()->SetDelegate(shell());
+}
+
+namespace {
+
 class TestJavaScriptDialogManager : public JavaScriptDialogManager,
                                     public WebContentsDelegate {
  public: