"Top Document Isolation" mode

content/browser/frame_host/render_frame_host_manager.cc

Index: content/browser/frame_host/render_frame_host_manager.cc
diff --git a/content/browser/frame_host/render_frame_host_manager.cc b/content/browser/frame_host/render_frame_host_manager.cc
index 24f963687a3d53dc423bc7574791d2fec90f38f0..3068ce418a3ab3865f34d3db4f5a6408e05eff24 100644
--- a/content/browser/frame_host/render_frame_host_manager.cc
+++ b/content/browser/frame_host/render_frame_host_manager.cc
@@ -986,9 +986,8 @@ void RenderFrameHostManager::OnDidUpdateOrigin(const url::Origin& origin) {
 RenderFrameHostManager::SiteInstanceDescriptor::SiteInstanceDescriptor(
     BrowserContext* browser_context,
     GURL dest_url,
-    bool related_to_current)
-    : existing_site_instance(nullptr),
-      new_is_related_to_current(related_to_current) {
+    SiteInstanceRelation relation_to_current)
+    : existing_site_instance(nullptr), relation(relation_to_current) {
   new_site_url = SiteInstance::GetSiteForURL(browser_context, dest_url);
 }
 
@@ -1173,13 +1172,16 @@ SiteInstance* RenderFrameHostManager::GetSiteInstanceForNavigation(
 
   SiteInstance* new_instance =
       ConvertToSiteInstance(new_instance_descriptor, candidate_instance);
-
   // If |force_swap| is true, we must use a different SiteInstance than the
   // current one. If we didn't, we would have two RenderFrameHosts in the same
   // SiteInstance and the same frame, resulting in page_id conflicts for their
   // NavigationEntries.
   if (force_swap)
     CHECK_NE(new_instance, current_instance);
+
+  // |new_instance| may be a newly allocated SiteInstance. It is important to
+  // immediately give this new SiteInstance to a RenderViewHost (if it is
+  // different than our current SiteInstance), so that it is ref counted.
   return new_instance;
 }
 
@@ -1212,7 +1214,8 @@ RenderFrameHostManager::DetermineSiteInstanceForURL(
   // If a swap is required, we need to force the SiteInstance AND
   // BrowsingInstance to be different ones, using CreateForURL.
   if (force_browsing_instance_swap)
-    return SiteInstanceDescriptor(browser_context, dest_url, false);
+    return SiteInstanceDescriptor(browser_context, dest_url,
+                                  SiteInstanceRelation::UNRELATED);
 
   // (UGLY) HEURISTIC, process-per-site only:
   //
@@ -1251,7 +1254,8 @@ RenderFrameHostManager::DetermineSiteInstanceForURL(
         RenderProcessHostImpl::GetProcessHostForSite(browser_context, dest_url);
     if (current_instance_impl->HasRelatedSiteInstance(dest_url) ||
         use_process_per_site) {
-      return SiteInstanceDescriptor(browser_context, dest_url, true);
+      return SiteInstanceDescriptor(browser_context, dest_url,
+                                    SiteInstanceRelation::RELATED);
     }
 
     // For extensions, Web UI URLs (such as the new tab page), and apps we do
@@ -1259,20 +1263,23 @@ RenderFrameHostManager::DetermineSiteInstanceForURL(
     // will have a RenderProcessHost of PRIV_NORMAL. Create a new SiteInstance
     // for this URL instead (with the correct process type).
     if (current_instance_impl->HasWrongProcessForURL(dest_url))
-      return SiteInstanceDescriptor(browser_context, dest_url, true);
+      return SiteInstanceDescriptor(browser_context, dest_url,
+                                    SiteInstanceRelation::RELATED);
 
     // View-source URLs must use a new SiteInstance and BrowsingInstance.
     // TODO(nasko): This is the same condition as later in the function. This
     // should be taken into account when refactoring this method as part of
     // http://crbug.com/123007.
     if (dest_is_view_source_mode)
-      return SiteInstanceDescriptor(browser_context, dest_url, false);
+      return SiteInstanceDescriptor(browser_context, dest_url,
+                                    SiteInstanceRelation::UNRELATED);
 
     // If we are navigating from a blank SiteInstance to a WebUI, make sure we
     // create a new SiteInstance.
     if (WebUIControllerFactoryRegistry::GetInstance()->UseWebUIForURL(
             browser_context, dest_url)) {
-      return SiteInstanceDescriptor(browser_context, dest_url, false);
+      return SiteInstanceDescriptor(browser_context, dest_url,
+                                    SiteInstanceRelation::UNRELATED);
     }
 
     // Normally the "site" on the SiteInstance is set lazily when the load
@@ -1322,7 +1329,8 @@ RenderFrameHostManager::DetermineSiteInstanceForURL(
   if (current_entry &&
       current_entry->IsViewSourceMode() != dest_is_view_source_mode &&
       !IsRendererDebugURL(dest_url)) {
-    return SiteInstanceDescriptor(browser_context, dest_url, false);
+    return SiteInstanceDescriptor(browser_context, dest_url,
+                                  SiteInstanceRelation::UNRELATED);
   }
 
   // Use the source SiteInstance in case of data URLs or about:blank pages,
@@ -1334,21 +1342,43 @@ RenderFrameHostManager::DetermineSiteInstanceForURL(
     return SiteInstanceDescriptor(source_instance);
   }
 
-  // Use the current SiteInstance for same site navigations, as long as the
-  // process type is correct.  (The URL may have been installed as an app since
-  // the last time we visited it.)
-  const GURL& current_url = GetCurrentURLForSiteInstance(current_instance_impl);
-  if (SiteInstance::IsSameWebSite(browser_context, current_url, dest_url) &&
-      !current_instance_impl->HasWrongProcessForURL(dest_url)) {
-    return SiteInstanceDescriptor(current_instance_impl);
+  // Use the current SiteInstance for same site navigations.
+  if (IsCurrentlySameSite(render_frame_host_.get(), dest_url))
+    return SiteInstanceDescriptor(render_frame_host_->GetSiteInstance());
+
+  if (SiteIsolationPolicy::IsTopDocumentIsolationEnabled()) {
+    // TODO(nick): Looking at the main frame and openers is required for TDI
+    // mode, but should be safe to enable unconditionally.

[Charlie Reis, 2016/03/25 19:47:12]

I think I agree, but I also agree with guarding it behind the flag for now just
in case.  (I'd like to think about the opener case more closely for other
modes.)

[ncarter (slow), 2016/03/28 22:00:28]

Acknowledged.

+    if (!frame_tree_node_->IsMainFrame()) {
+      RenderFrameHostImpl* main_frame =
+          frame_tree_node_->frame_tree()->root()->current_frame_host();
+      if (IsCurrentlySameSite(main_frame, dest_url))
+        return SiteInstanceDescriptor(main_frame->GetSiteInstance());
+    }
+
+    if (frame_tree_node_->opener()) {
+      RenderFrameHostImpl* opener_frame =
+          frame_tree_node_->opener()->current_frame_host();
+      if (IsCurrentlySameSite(opener_frame, dest_url))
+        return SiteInstanceDescriptor(opener_frame->GetSiteInstance());
+    }
+  }
+
+  if (!frame_tree_node_->IsMainFrame() &&
+      SiteIsolationPolicy::IsTopDocumentIsolationEnabled() &&
+      !SiteInstanceImpl::DoesSiteRequireDedicatedProcess(browser_context,
+                                                         dest_url)) {
+    // This is a cross-site subframe of a non-isolated origin, so place this
+    // frame in the default subframe site instance.
+    return SiteInstanceDescriptor(
+        browser_context, dest_url,
+        SiteInstanceRelation::RELATED_DEFAULT_SUBFRAME);

[Charlie Reis, 2016/03/25 19:47:12]

Yes, I think these changes make sense.

[ncarter (slow), 2016/03/28 22:00:28]

Acknowledged.

   }
 
   // Start the new renderer in a new SiteInstance, but in the current
-  // BrowsingInstance.  It is important to immediately give this new
-  // SiteInstance to a RenderViewHost (if it is different than our current
-  // SiteInstance), so that it is ref counted.  This will happen in
-  // CreateRenderView.
-  return SiteInstanceDescriptor(browser_context, dest_url, true);
+  // BrowsingInstance.
+  return SiteInstanceDescriptor(browser_context, dest_url,
+                                SiteInstanceRelation::RELATED);
 }
 
 bool RenderFrameHostManager::IsRendererTransferNeededForNavigation(
@@ -1377,23 +1407,35 @@ bool RenderFrameHostManager::IsRendererTransferNeededForNavigation(
   // TODO(nasko, nick): These following --site-per-process checks are
   // overly simplistic. Update them to match all the cases
   // considered by DetermineSiteInstanceForURL.
-  if (SiteInstance::IsSameWebSite(rfh->GetSiteInstance()->GetBrowserContext(),
-                                  rfh->GetSiteInstance()->GetSiteURL(),
-                                  dest_url)) {
-    return false;  // The same site, no transition needed.
+  if (IsCurrentlySameSite(rfh, dest_url)) {
+    // The same site, no transition needed for security purposes, and we must
+    // keep the same SiteInstance for correctness of synchronous scripting.
+    return false;
   }
 
   // The sites differ. If either one requires a dedicated process,
   // then a transfer is needed.
-  return rfh->GetSiteInstance()->RequiresDedicatedProcess() ||
-         SiteInstanceImpl::DoesSiteRequireDedicatedProcess(context,
-                                                           effective_url);
+  if (rfh->GetSiteInstance()->RequiresDedicatedProcess() ||
+      SiteInstanceImpl::DoesSiteRequireDedicatedProcess(context,
+                                                        effective_url)) {
+    return true;
+  }
+
+  bool subframe = !frame_tree_node_->IsMainFrame();

[Charlie Reis, 2016/03/25 19:47:12]

nit: is_subframe

(Does this help with clarity?  We use the IsMainFrame() call directly in the
logic above, so I wouldn't feel bad doing the same below.)

[ncarter (slow), 2016/03/28 22:00:28]

I've inlined it -- this was an artifact of an earlier, more complicated
comparison..

FWIW, I'm now passionate about unifying this with DetermineSiteInstanceForURL,
because it seems like it's going to keep evolving.

+  if (SiteIsolationPolicy::IsTopDocumentIsolationEnabled() &&
+      (subframe ||
+       rfh->GetSiteInstance()->is_default_subframe_site_instance())) {
+    // Always attempt a transfer in these cases.
+    return true;
+  }
+
+  return false;
 }
 
 SiteInstance* RenderFrameHostManager::ConvertToSiteInstance(
     const SiteInstanceDescriptor& descriptor,
     SiteInstance* candidate_instance) {
-  SiteInstance* current_instance = render_frame_host_->GetSiteInstance();
+  SiteInstanceImpl* current_instance = render_frame_host_->GetSiteInstance();
 
   // Note: If the |candidate_instance| matches the descriptor, it will already
   // be set to |descriptor.existing_site_instance|.
@@ -1402,9 +1444,12 @@ SiteInstance* RenderFrameHostManager::ConvertToSiteInstance(
 
   // Note: If the |candidate_instance| matches the descriptor,
   // GetRelatedSiteInstance will return it.
-  if (descriptor.new_is_related_to_current)
+  if (descriptor.relation == SiteInstanceRelation::RELATED)
     return current_instance->GetRelatedSiteInstance(descriptor.new_site_url);
 
+  if (descriptor.relation == SiteInstanceRelation::RELATED_DEFAULT_SUBFRAME)
+    return current_instance->GetDefaultSubframeSiteInstance();
+
   // At this point we know an unrelated site instance must be returned. First
   // check if the candidate matches.
   if (candidate_instance &&
@@ -1419,21 +1464,43 @@ SiteInstance* RenderFrameHostManager::ConvertToSiteInstance(
       descriptor.new_site_url);
 }
 
-const GURL& RenderFrameHostManager::GetCurrentURLForSiteInstance(
-    SiteInstance* current_instance) {
+bool RenderFrameHostManager::IsCurrentlySameSite(RenderFrameHostImpl* candidate,
+                                                 const GURL& dest_url) {
+  BrowserContext* browser_context =
+      delegate_->GetControllerForRenderManager().GetBrowserContext();
+
+  // If the process type is incorrect, reject the candidate even if |dest_url|
+  // is same-site.  (The URL may have been installed as an app since
+  // the last time we visited it.)
+  if (candidate->GetSiteInstance()->HasWrongProcessForURL(dest_url))
+    return false;
+
   // Use the current RenderFrameHost's last successful URL if it has one.  This
   // excludes commits of net errors, since net errors do not currently swap
   // processes for transfer navigations.  Thus, we compare against the last
-  // successful commit when deciding whether to swap this time.
-  // (Note: browser-initiated net errors do swap processes, but the frame's last
-  // successful URL will be empty in that case, causing us to fall back to the
-  // SiteInstance's URL below.)
-  if (!render_frame_host_->last_successful_url().is_empty())
-    return render_frame_host_->last_successful_url();
-
-  // Fall back to the SiteInstance's Site URL if the FrameTreeNode doen't have a
-  // current URL.
-  return current_instance->GetSiteURL();
+  // successful commit when deciding whether to swap this time. (Note: browser-
+  // initiated net errors do swap processes, but the frame's last successful URL
+  // will be empty in that case, causing us to fall back to the SiteInstance's
+  // URL below.)
+  if (!candidate->last_successful_url().is_empty()) {
+    if (SiteInstance::IsSameWebSite(browser_context,
+                                    candidate->last_successful_url(), dest_url))
+      return true;
+
+    if (!candidate->GetLastCommittedOrigin().unique() &&

[Charlie Reis, 2016/03/25 19:47:12]

This check takes a while to understand and depends a lot on context (i.e., that
we know dest_url is cross-site from the last successful URL).  Can you add a
comment clarifying what this is for?  

I thought it was so that we keep foo.com in the same process if the previous
page was about:blank in a foo.com origin, but IsSameWebSite kind of accounts for
about:blank.  (Or maybe it doesn't handle that specific case?)  Anyway, I'm a
bit unclear here.

[ncarter (slow), 2016/03/28 22:00:28]

I've clarified the intent here with a comment.

My understanding is that IsSameWebSite handles the dest_url == about:blank case,
but it doesn't have enough info to correctly handle the src_url =="about:blank"
case, even if it wanted to. So that's the main value to this origin check.
Adding this origin check carries some risk of behavior change for non-TDI, but I
actually think the test coverage for the about:blank cases is pretty solid.

[Charlie Reis, 2016/03/29 17:17:12]

Acknowledged.

+        SiteInstance::IsSameWebSite(
+            browser_context,
+            GURL(candidate->GetLastCommittedOrigin().Serialize()), dest_url)) {
+      return true;

[Charlie Reis, 2016/03/25 19:47:12]

Maybe this overall block would be easier to read as:

  if (...has last successful url...) {
    if (...IsSameWebSite...)
      return true;
    return (...origin check...);
  }

  return (...IsSameWebSite...);

[ncarter (slow), 2016/03/28 22:00:28]

I've rearranged the code, hopefully it's better now. I also put in a TODO with
your name on it about eliminating the need for the GetSiteURL() fallback.

[Charlie Reis, 2016/03/29 17:17:12]

Acknowledged.

+    }
+  } else {
+    if (SiteInstance::IsSameWebSite(browser_context,
+                                    candidate->GetSiteInstance()->GetSiteURL(),
+                                    dest_url)) {
+      return true;
+    }
+  }
+  return false;
 }
 
 void RenderFrameHostManager::CreatePendingRenderFrameHost(