Do not reset NavigationHandle when navigating same-page

content/browser/frame_host/navigation_controller_impl_browsertest.cc

Index: content/browser/frame_host/navigation_controller_impl_browsertest.cc
diff --git a/content/browser/frame_host/navigation_controller_impl_browsertest.cc b/content/browser/frame_host/navigation_controller_impl_browsertest.cc
index 079902d6041b44b7db2351bb032e40ad9f45f36e..6142346d36edc32f36f67355c728e5c7d9b88d0c 100644
--- a/content/browser/frame_host/navigation_controller_impl_browsertest.cc
+++ b/content/browser/frame_host/navigation_controller_impl_browsertest.cc
@@ -6909,4 +6909,110 @@ IN_PROC_BROWSER_TEST_F(RequestMonitoringNavigationBrowserTest,
                   testing::Key("X-ExtraHeadersVsSubresources"))));
 }
 
+// Test that a same-page navigation does not lead to the deletion of the
+// NavigationHandle for an ongoing different page navigation.
+IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
+                       SamePageNavigationDoesntDeleteNavigationHandle) {
+  const GURL kURL1 = embedded_test_server()->GetURL("/title1.html");
+  const GURL kPushStateURL =
+      embedded_test_server()->GetURL("/title1.html#fragment");
+  const GURL kURL2 = embedded_test_server()->GetURL("/title2.html");
+
+  // Navigate to the initial page.
+  EXPECT_TRUE(NavigateToURL(shell(), kURL1));
+  RenderFrameHostImpl* main_frame =
+      static_cast<WebContentsImpl*>(shell()->web_contents())->GetMainFrame();
+  FrameTreeNode* root = static_cast<WebContentsImpl*>(shell()->web_contents())
+                            ->GetFrameTree()
+                            ->root();
+  EXPECT_FALSE(main_frame->navigation_handle());
+  EXPECT_FALSE(root->navigation_request());
+
+  // Start navigating to the second page.
+  TestNavigationManager manager(shell()->web_contents(), kURL2);
+  shell()->web_contents()->GetController().LoadURL(
+      kURL2, Referrer(), ui::PAGE_TRANSITION_LINK, std::string());
+  EXPECT_TRUE(manager.WaitForRequestStart());
+
+  // This should create a NavigationHandle.
+  if (IsBrowserSideNavigationEnabled()) {
+    EXPECT_TRUE(root->navigation_request());
+  } else {
+    EXPECT_TRUE(main_frame->navigation_handle());

[nasko, 2016/11/09 01:30:22]

Wouldn't this be true even in PlzNavigate mode?

[clamy, 2016/11/09 18:05:52]

No because the NavigationHandle is owned by the NavigationRequest at this point.

+  }
+
+  // The current page does a PushState.
+  std::string push_state =
+      "var stateObj = {}; history.pushState(stateObj, \"title 1\", \"" +

[nasko, 2016/11/09 01:30:22]

nit: No need to declare stateObj, just pass {} as the first param to pushState.

[clamy, 2016/11/09 18:05:52]

Done.

+      kPushStateURL.spec() + "\");";
+  EXPECT_TRUE(ExecuteScript(shell()->web_contents(), push_state));
+  NavigationEntry* last_committed =
+      shell()->web_contents()->GetController().GetLastCommittedEntry();
+  ASSERT_TRUE(last_committed);
+  EXPECT_EQ(kPushStateURL, last_committed->GetURL());
+
+  // This shouldn't affect the ongoing navigation.
+  if (IsBrowserSideNavigationEnabled()) {
+    EXPECT_TRUE(root->navigation_request());
+  } else {
+    EXPECT_TRUE(main_frame->navigation_handle());

[nasko, 2016/11/09 01:30:22]

Why not capture the NavigationHandle before the pushState and ensure it is the
exact same object here?

[clamy, 2016/11/09 18:05:52]

Done.

+  }
+
+  // Let the navigation finish. It should commit successfully.
+  manager.WaitForNavigationFinished();
+  last_committed =
+      shell()->web_contents()->GetController().GetLastCommittedEntry();
+  ASSERT_TRUE(last_committed);
+  EXPECT_EQ(kURL2, last_committed->GetURL());
+}
+
+class NavigationHandleCommitObserver : public WebContentsObserver {

[nasko, 2016/11/09 01:30:22]

It might be useful to use this object in the test above to confirm that the
commits for both navigation handles - the same page one and the delayed
navigation one are properly reporting the expected values.

[clamy, 2016/11/09 18:05:52]

Done.

+ public:
+  NavigationHandleCommitObserver(WebContents* web_contents, const GURL& url)
+      : WebContentsObserver(web_contents),
+        url_(url),
+        has_committed_(false),
+        was_same_page_(false),
+        was_renderer_initiated_(false) {}
+
+  bool has_committed() const { return has_committed_; }
+  bool was_same_page() const { return was_same_page_; }
+  bool was_renderer_initiated() const { return was_renderer_initiated_; }
+
+ private:
+  void DidFinishNavigation(NavigationHandle* handle) override {
+    if (handle->GetURL() != url_)
+      return;
+    has_committed_ = true;
+    was_same_page_ = handle->IsSamePage();
+    was_renderer_initiated_ = handle->IsRendererInitiated();
+  }
+
+  const GURL url_;
+  bool has_committed_;
+  bool was_same_page_;
+  bool was_renderer_initiated_;
+};
+
+// Tests that a same-page browser-initiated navigation is properly reported by
+// the NavigationHandle.
+IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
+                       SamePageBrowserInitiated) {
+  const GURL kURL = embedded_test_server()->GetURL("/title1.html");
+  const GURL kFragmentURL =
+      embedded_test_server()->GetURL("/title1.html#fragment");
+
+  // Navigate to the initial page.
+  EXPECT_TRUE(NavigateToURL(shell(), kURL));
+
+  // Do a browser-initiated fragment navigation.
+  NavigationHandleCommitObserver handle_observer(shell()->web_contents(),
+                                                 kFragmentURL);
+  EXPECT_TRUE(NavigateToURL(shell(), kFragmentURL));
+
+  EXPECT_TRUE(handle_observer.has_committed());
+  EXPECT_TRUE(handle_observer.was_same_page());
+  EXPECT_FALSE(handle_observer.was_renderer_initiated());
+}
+
 }  // namespace content