Do not create pending NavigationEntry when a transient one is present.

chrome/browser/chrome_navigation_browsertest.cc

Index: chrome/browser/chrome_navigation_browsertest.cc
diff --git a/chrome/browser/chrome_navigation_browsertest.cc b/chrome/browser/chrome_navigation_browsertest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..0c7b2afbfef01d26b8fb83a7d4b178215edbec81
--- /dev/null
+++ b/chrome/browser/chrome_navigation_browsertest.cc
@@ -0,0 +1,128 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/chrome_notification_types.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "chrome/test/base/ui_test_utils.h"
+#include "content/public/browser/navigation_handle.h"
+#include "content/public/browser/notification_service.h"
+#include "content/public/browser/web_contents_observer.h"
+#include "content/public/test/browser_test_utils.h"
+#include "content/public/test/test_navigation_observer.h"
+
+class ChromeNavigationBrowserTest : public InProcessBrowserTest {
+ public:
+  ChromeNavigationBrowserTest() {}
+  ~ChromeNavigationBrowserTest() override {}
+
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    ASSERT_TRUE(embedded_test_server()->Start());
+  }
+
+  void StartServerWithExpiredCert() {
+    https_server_expired_.reset(
+        new net::EmbeddedTestServer(net::EmbeddedTestServer::TYPE_HTTPS));
+    https_server_expired_->SetSSLConfig(net::EmbeddedTestServer::CERT_EXPIRED);
+    https_server_expired_->AddDefaultHandlers(
+        base::FilePath(FILE_PATH_LITERAL("chrome/test/data")));
+    ASSERT_TRUE(https_server_expired_->Start());
+  }
+
+  net::EmbeddedTestServer* https_server_expired() {

[Charlie Reis, 2016/04/13 06:03:37]

nit: expired_https_server

[nasko, 2016/04/13 18:45:30]

Done.

+    return https_server_expired_.get();
+  }
+
+ private:
+  std::unique_ptr<net::EmbeddedTestServer> https_server_expired_;
+
+  DISALLOW_COPY_AND_ASSIGN(ChromeNavigationBrowserTest);
+};
+
+// Helper class to track and allow waiting for navigation start events.
+class DidStartNavigationObserver : public content::WebContentsObserver {
+ public:
+  explicit DidStartNavigationObserver(content::WebContents* web_contents)
+      : content::WebContentsObserver(web_contents),
+        message_loop_runner_(new content::MessageLoopRunner) {}
+  ~DidStartNavigationObserver() override {}
+
+  // Runs a nested message loop and blocks until the full load has
+  // completed.
+  void Wait() { message_loop_runner_->Run(); }
+
+ private:
+  // WebContentsObserver
+  void DidStartNavigation(content::NavigationHandle* handle) override {
+    if (message_loop_runner_->loop_running())
+      message_loop_runner_->Quit();
+  }
+
+  // The MessageLoopRunner used to spin the message loop.
+  scoped_refptr<content::MessageLoopRunner> message_loop_runner_;
+
+  DISALLOW_COPY_AND_ASSIGN(DidStartNavigationObserver);
+};
+
+// Test to verify that navigations are not deleting the transient
+// NavigationEntry when showing an interstitial page and the old renderer
+// process is trying to navigate.

[Charlie Reis, 2016/04/13 06:03:37]

Let's list the bug number here.

[nasko, 2016/04/13 18:45:30]

Done.

+IN_PROC_BROWSER_TEST_F(
+    ChromeNavigationBrowserTest,
+    TransientEntryPreservedOnMultipleNavigationsDuringInterstitial) {
+  StartServerWithExpiredCert();
+
+  GURL setup_url =
+      embedded_test_server()->GetURL("/window_open_and_navigate.html");

[Charlie Reis, 2016/04/13 06:03:37]

Looks like you forgot to add this file, which probably explains the test
failures?

[nasko, 2016/04/13 18:45:30]

Yeah, noticed later, but didn't have a chance to upload a new patch. The last PS
should have it.

+  GURL initial_url = embedded_test_server()->GetURL("/title1.html");
+  GURL error_url(https_server_expired()->GetURL("/ssl/blank_page.html"));
+
+  ui_test_utils::NavigateToURL(browser(), setup_url);
+  content::WebContents* main_web_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  content::WebContents* new_web_contents = nullptr;
+
+  // Call the JavaScript method in the test page, which opens a new window
+  // and stores a handle to it.
+  content::WindowedNotificationObserver tab_added_observer(
+      chrome::NOTIFICATION_TAB_ADDED,
+      content::NotificationService::AllSources());
+  EXPECT_TRUE(content::ExecuteScript(main_web_contents, "openWin();"));
+  tab_added_observer.Wait();
+  new_web_contents = browser()->tab_strip_model()->GetActiveWebContents();
+
+  // Navigate the opened window to a page that will successfully commit and
+  // create a NavigationEntry.
+  {
+    content::TestNavigationObserver observer(new_web_contents);
+    EXPECT_TRUE(content::ExecuteScript(
+        main_web_contents, "navigate('" + initial_url.spec() + "');"));
+    observer.Wait();
+    EXPECT_EQ(new_web_contents->GetLastCommittedURL(), initial_url);

[Charlie Reis, 2016/04/13 06:03:37]

nit: Reverse order (expected, actual), here and below.

[nasko, 2016/04/13 18:45:30]

Done.

+  }
+
+  // Navigate the opened window to a page which will trigger an
+  // interstitial.
+  {
+    content::TestNavigationObserver observer(new_web_contents);
+    EXPECT_TRUE(content::ExecuteScript(
+        main_web_contents, "navigate('" + error_url.spec() + "');"));
+    observer.Wait();
+    EXPECT_EQ(new_web_contents->GetLastCommittedURL(), initial_url);
+    EXPECT_EQ(new_web_contents->GetVisibleURL(), error_url);
+  }
+
+  // Navigate again the opened window to the same page. It should not cause
+  // WebContents::GetVisibleURL to return the last committed one.
+  {
+    DidStartNavigationObserver nav_observer(new_web_contents);
+    EXPECT_TRUE(content::ExecuteScript(
+        main_web_contents, "navigate('" + error_url.spec() + "');"));
+    nav_observer.Wait();

[Charlie Reis, 2016/04/13 06:03:37]

Sanity check: We're not depending on this returning before some later event
happens, right?  I made that mistake in some of my tests and they were flaky,
since the message loop might not quit until more events happen.

Hopefully this is just using DidStartNavigationObserver because that's the last
thing we could observe.

[nasko, 2016/04/13 18:45:30]

No dependency on any other events. I did also discover this behavior when I was
reviewing one of Ken's CLs, so I'm acutely aware now :). Thanks for pointing it
out!
Yes, I need to wait to see the DidStartProvisionalLoad from the navigation.
Because we block the network requests while the interstitial is showing, there
are no other navigation events that will happen from the same RFH. It is also
the reason I couldn't just use TestNavigationObserver, as it expects to see a
commit/end of navigation, which can't happen here.

+  }
+

[Charlie Reis, 2016/04/13 06:03:37]

nit: No blank line here (or move the EXPECTs into the nested braces), since the
comment describing them is in the block above.

[nasko, 2016/04/13 18:45:30]

Done.

+  EXPECT_EQ(new_web_contents->GetVisibleURL(), error_url);

[Charlie Reis, 2016/04/13 06:03:37]

Can you add an EXPECT for new_web_contents->GetController().GetTransientEntry()?

[nasko, 2016/04/13 18:45:29]

Done.

+  EXPECT_FALSE(new_web_contents->IsLoading());
+}