Simplify AreURLsInPageNavigation

content/browser/frame_host/navigation_controller_impl.cc

Index: content/browser/frame_host/navigation_controller_impl.cc
diff --git a/content/browser/frame_host/navigation_controller_impl.cc b/content/browser/frame_host/navigation_controller_impl.cc
index 6d1c597d7604629df1b636ed8947c099fbc40c5c..5cdeff3c1efc7ee959d4188554ef9262c55481d3 100644
--- a/content/browser/frame_host/navigation_controller_impl.cc
+++ b/content/browser/frame_host/navigation_controller_impl.cc
@@ -104,24 +104,28 @@ void ConfigureEntriesForRestore(
   }
 }
 
-// See NavigationController::IsURLInPageNavigation for how this works and why.
+// There are two general cases where a navigation is in page:
+// 1. A fragment navigation, in which the url is kept the same except for the
+//    reference fragment.
+// 2. A history API navigation (pushState and replaceState). This case is
+//    always in-page, but the urls are not guaranteed to match excluding the
+//    fragment. The relevant spec allows pushState/replaceState to any URL on
+//    the same origin.
+// However, due to reloads, even identical urls are *not* guaranteed to be
+// in-page navigations, we have to trust the renderer almost entirely.
+// The one thing we do know is that cross-origin navigations will *never* be
+// in-page. Therefore, trust the renderer if the URLs are on the same origin,
+// and assume the renderer is malicious if a cross-origin navigation claims to
+// be in-page.
 bool AreURLsInPageNavigation(const GURL& existing_url,
                              const GURL& new_url,
                              bool renderer_says_in_page,
-                             NavigationType navigation_type) {
-  if (existing_url.GetOrigin() == new_url.GetOrigin())
-    return renderer_says_in_page;
-
-  if (!new_url.has_ref()) {
-    // When going back from the ref URL to the non ref one the navigation type
-    // is IN_PAGE.
-    return navigation_type == NAVIGATION_TYPE_IN_PAGE;
-  }
-
-  url::Replacements<char> replacements;
-  replacements.ClearRef();
-  return existing_url.ReplaceComponents(replacements) ==
-      new_url.ReplaceComponents(replacements);
+                             RenderFrameHost* rfh) {
+  bool is_same_origin = existing_url.is_empty() ||

[Nate Chapin, 2014/06/06 22:45:23]

Declaring a navigation from an empty url same-origin handles the case of
history.pushState in a frame's initial empty document. blink allows this, though
it's not clear to me whether that's correct.

[nasko, 2014/06/09 18:49:43]

Isn't the initial empty document with "about:blank" URL (vs empty string)?

[Nate Chapin, 2014/06/09 18:53:49]

It is (most of the time, there's a special case in blink's Document
constructor), but I don't think blink reports the initial empty document's
commit to the the browser process.

[nasko, 2014/06/09 18:59:24]

Ok, though I think we would have to start reporting it when session history
moves to the browser.

[Nate Chapin, 2014/06/09 19:02:54]

Huh, interesting. I would have thought that the initial empty document wouldn't
necessarily need to be declared to the browser process, though if it was
scripted or navigated to about:blank, then it would.

[nasko, 2014/06/09 20:41:28]

When I discussed this with Charlie, we found that having all cases reported to
the browser process might make the logic more streamlined. In any case, I
mentioned it as a FYI, no action item here for this CL.

+                        existing_url.GetOrigin() == new_url.GetOrigin();
+  if (!is_same_origin && renderer_says_in_page)
+      rfh->GetProcess()->ReceivedBadMessage();
+  return is_same_origin && renderer_says_in_page;
 }
 
 // Determines whether or not we should be carrying over a user agent override
@@ -766,8 +770,8 @@ bool NavigationControllerImpl::RendererDidNavigate(
   details->type = ClassifyNavigation(rfh, params);
 
   // is_in_page must be computed before the entry gets committed.
-  details->is_in_page = IsURLInPageNavigation(
-      params.url, params.was_within_same_page, details->type);
+  details->is_in_page = AreURLsInPageNavigation(rfh->GetLastCommittedURL(),

[Nate Chapin, 2014/06/06 22:45:23]

Unlike other uses of the in-page logic, this callsite does not guarantee that it
was the main frame that was navigated. Use the RenderFrameHost's state to
determine the existing url to ensure we correctly handle subframe navigations.

[nasko, 2014/06/09 18:49:43]

GetLastCommittedURL on RFH worries me a bit. It comes from the frame tree node,
which we save for estimating process allocation and overhead of site isolation.
It looks we always set it on commit, but it was never used for such decisions
before.

[Nate Chapin, 2014/06/09 18:53:49]

Fair enough. Is there an alternate way to get the committing frame's previous
url without adding it to the DidCommitProvisionaLoad IPC?

[nasko, 2014/06/09 18:59:24]

It is already there:
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/fra...

[Nate Chapin, 2014/06/09 19:02:54]

I see the URL being navigated to, the base url, and the start of the redirect
chain, but not the url that we're leaving. Am I missing something?

[nasko, 2014/06/09 20:41:28]

Duh, I misread what you meant. You are correct. Looking at it, we do set it
always on commit, so I think it is fine to use and we can revisit this if needed
in the future.

+      params.url, params.was_within_same_page, rfh);
 
   switch (details->type) {
     case NAVIGATION_TYPE_NEW_PAGE:
@@ -986,8 +990,7 @@ NavigationType NavigationControllerImpl::ClassifyNavigation(
   // navigations that don't actually navigate, but it can happen when there is
   // an encoding override (it always sends a navigation request).
   if (AreURLsInPageNavigation(existing_entry->GetURL(), params.url,
-                              params.was_within_same_page,
-                              NAVIGATION_TYPE_UNKNOWN)) {
+                              params.was_within_same_page, rfh)) {
     return NAVIGATION_TYPE_IN_PAGE;
   }
 
@@ -1253,10 +1256,10 @@ int NavigationControllerImpl::GetIndexOfEntry(
 bool NavigationControllerImpl::IsURLInPageNavigation(
     const GURL& url,
     bool renderer_says_in_page,
-    NavigationType navigation_type) const {
+    RenderFrameHost* rfh) const {
   NavigationEntry* last_committed = GetLastCommittedEntry();
   return last_committed && AreURLsInPageNavigation(
-      last_committed->GetURL(), url, renderer_says_in_page, navigation_type);
+      last_committed->GetURL(), url, renderer_says_in_page, rfh);
 }
 
 void NavigationControllerImpl::CopyStateFrom(