Ensure FrameNavigationEntry is fully updated.

content/browser/frame_host/navigation_controller_impl_browsertest.cc

 // Copyright 2014 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 "content/browser/frame_host/navigation_controller_impl.h"
 
 #include <stdint.h>
 #include <utility>
 
 #include "base/bind.h"
@@ skipping 39 matching lines @@
       "var f = document.createElement('iframe');"
       "f.name = 'foo-frame-name';"
       "document.body.appendChild(f);";
 static std::string kAddFrameScript =
       "var f = document.createElement('iframe');"
       "document.body.appendChild(f);";
 static std::string kRemoveFrameScript =
       "var f = document.querySelector('iframe');"
       "f.parentNode.removeChild(f);";
 
+class RenderProcessGoneObserver : public content::WebContentsObserver {

[Charlie Reis, 2016/08/10 17:03:09]

Can we use RenderProcessHostWatcher from browser_test_utils, or is there a
specific need for this one?

[nasko, 2016/08/10 18:08:51]

I tried to find it, but couldn't, so put this one together. Now looking at it
though, it requires waiting for the process to exit, which the test won't do
unless I add extra code to make it exit the process. Maybe I can fold the
functionality of this class into RenderProcessHostWatcher as a follow up CL and
eliminate it from here?

+ public:
+  RenderProcessGoneObserver(content::WebContents* web_contents)
+      : content::WebContentsObserver(web_contents),
+        status_(base::TerminationStatus::TERMINATION_STATUS_MAX_ENUM) {}
+  ~RenderProcessGoneObserver() override {}
+
+  void RenderProcessGone(base::TerminationStatus status) override {
+    status_ = status;
+  }
+
+  base::TerminationStatus status() { return status_; }
+
+  bool is_gone() {
+    return status_ != base::TerminationStatus::TERMINATION_STATUS_MAX_ENUM;
+  }
+
+ private:
+  base::TerminationStatus status_;
+};
+
 }  // namespace
 
 namespace content {
 
 class NavigationControllerBrowserTest : public ContentBrowserTest {
  protected:
   void SetUpOnMainThread() override {
     host_resolver()->AddRule("*", "127.0.0.1");
     ASSERT_TRUE(embedded_test_server()->Start());
   }
@@ skipping 5539 matching lines @@
   EXPECT_EQ(1U, root->child_count());
   if (SiteIsolationPolicy::UseSubframeNavigationEntries()) {
     EXPECT_EQ(1U, nav_entry->root_node()->children.size());
     EXPECT_EQ(tree_node, nav_entry->root_node()->children[0]);
   }
 
   EXPECT_TRUE(ExecuteScript(root, kRemoveFrameScript));
   EXPECT_EQ(0U, root->child_count());
 }
 
+// Test that navigations classified as SAME_PAGE properly update all the
+// members of FrameNavigationEntry. If not, it is possible to get mismatch

[Charlie Reis, 2016/08/10 17:03:09]

nit: a mismatch

[nasko, 2016/08/10 18:08:51]

Done.

+// between the origin and URL of a document as seen in
+// https://crbug.com/630103.
+IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
+                       EnsureSamePageNavigationUpdatesFrameNavigationEntry) {
+  WebContentsImpl* web_contents =
+      static_cast<WebContentsImpl*>(shell()->web_contents());
+  FrameTreeNode* root = web_contents->GetFrameTree()->root();
+
+  // Navigate to a simple page and then perform an in-page navigation.
+  GURL start_url(embedded_test_server()->GetURL("/title1.html"));
+  EXPECT_TRUE(NavigateToURL(shell(), start_url));
+
+  GURL same_page_url(embedded_test_server()->GetURL("/title1.html#foo"));
+  EXPECT_TRUE(NavigateToURL(shell(), same_page_url));
+  EXPECT_EQ(2, web_contents->GetController().GetEntryCount());
+
+  // Replace the URL of the current NavigationEntry with one that will cause
+  // a server redirect when loaded.
+  {
+    GURL redirect_dest_url(
+        embedded_test_server()->GetURL("b.com", "/simple1.html"));
+    TestNavigationObserver observer(web_contents);
+    std::string script = "history.replaceState({}, '', '/server-redirect?" +
+                         redirect_dest_url.spec() + "')";
+    EXPECT_TRUE(ExecuteScript(root, script));
+    observer.Wait();
+  }
+
+  // Simulate the user hitting Enter in the omnibox without changing the URL.
+  {
+    TestNavigationObserver observer(web_contents);
+    web_contents->GetController().LoadURL(web_contents->GetLastCommittedURL(),
+                                          Referrer(), ui::PAGE_TRANSITION_LINK,
+                                          std::string());
+    observer.Wait();
+  }
+
+  // The renderer process has a CHECK to ensure there are no mismatches between
+  // origin and URL, so going back in history should succeed without a process
+  // crash.
+  RenderProcessGoneObserver process_observer(web_contents);
+  {
+    TestNavigationObserver observer(web_contents);
+    web_contents->GetController().GoBack();
+    observer.Wait();
+  }
+  EXPECT_TRUE(!process_observer.is_gone()) << "Exit status: "

[Charlie Reis, 2016/08/10 17:03:09]

Hmm.  It would be nice to verify this beyond avoiding the crash, so that we're
not relying just on the renderer-side CHECK.

Then again, I don't see an easy way to do that.  I was hoping to use
FrameNavigateParamsCapturer to compare the URL and origin in the
FrameNavigateParams.  Unfortunately, we don't appear to have the origin on
FrameNavigateParams-- it's only on FrameHostMsg_DidCommitProvisionalLoad_Params,
which isn't easily captured or exposed.

And I suppose if we didn't have the renderer-side CHECK, we do have the origin
mismatch kill as well, so checking for process death may be ok.

I'm curious how it passes in --site-per-process, though.  Won't the back
navigation be cross-process, causing the current process to exit?  I wonder if
it's just racy, and the process does exit.  We might be able to verify by
tossing in another navigation or script call after this?

[nasko, 2016/08/10 18:08:51]

I've made it use suborigins on the same site, so we don't get cross-process
navigation. In addition, I'm checking that the origin reported by JavaScript
matches the expectation.

[Charlie Reis, 2016/08/10 18:36:16]

Thanks for adding the origin check-- that helps.

I'm not thrilled with the suborigin change, for a few reasons.  It was nice to
have some coverage for the cross-process case (on the Site Isolation FYI bot),
and it's possible the checks could become site-based instead of origin-based in
the future.

There's also still a bit of a flakiness concern for noticing the process exit
without the fix.  It probably wouldn't be an issue if we keep the CHECK in
SendDidCommitProvisionalLoad, but if we later decide the renderer kill is
sufficient and remove the renderer-side check, then process_observer might not
observe the kill before the test exits.

I think I'd suggest relying solely on the origin check (which would fail if the
process was dead), switching back to cross-site redirects, and removing
RenderProcessGoneObserver entirely.  (I think it's too easy for others to use in
a flaky way.)

[nasko, 2016/08/10 19:42:52]

Done.

+                                           << process_observer.status();
+}
+
 }  // namespace content