"Top Document Isolation" mode

content/browser/frame_host/render_frame_host_manager.cc

 // Copyright 2013 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/render_frame_host_manager.h"
 
 #include <stddef.h>
 
 #include <algorithm>
 #include <utility>
@@ skipping 968 matching lines @@
 void RenderFrameHostManager::OnDidUpdateOrigin(const url::Origin& origin) {
   for (const auto& pair : proxy_hosts_) {
     pair.second->Send(
         new FrameMsg_DidUpdateOrigin(pair.second->GetRoutingID(), 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);
 }
 
 void RenderFrameHostManager::RenderProcessGone(SiteInstanceImpl* instance) {
   GetRenderFrameProxyHost(instance)->set_render_frame_proxy_created(false);
 }
 
 void RenderFrameHostManager::ActiveFrameCountIsZero(
     SiteInstanceImpl* site_instance) {
   // |site_instance| no longer contains any active RenderFrameHosts, so we don't
@@ skipping 164 matching lines @@
   SiteInstanceDescriptor new_instance_descriptor =
       SiteInstanceDescriptor(current_instance);
   if (ShouldTransitionCrossSite() || force_swap) {
     new_instance_descriptor = DetermineSiteInstanceForURL(
         dest_url, source_instance, current_instance, dest_instance, transition,
         dest_is_restore, dest_is_view_source_mode, force_swap);
   }
 
   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;
 }
 
 RenderFrameHostManager::SiteInstanceDescriptor
 RenderFrameHostManager::DetermineSiteInstanceForURL(
     const GURL& dest_url,
     SiteInstance* source_instance,
     SiteInstance* current_instance,
     SiteInstance* dest_instance,
     ui::PageTransition transition,
@@ skipping 12 matching lines @@
     if (force_browsing_instance_swap) {
       CHECK(!dest_instance->IsRelatedSiteInstance(
                 render_frame_host_->GetSiteInstance()));
     }
     return SiteInstanceDescriptor(dest_instance);
   }
 
   // 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:
   //
   // If this navigation is generated, then it probably corresponds to a search
   // query.  Given that search results typically lead to users navigating to
   // other sites, we don't really want to use the search engine hostname to
   // determine the site instance for this navigation.
   //
   // NOTE: This can be removed once we have a way to transition between
   //       RenderViews in response to a link click.
@@ skipping 18 matching lines @@
     //
     // Also, if the URL should use process-per-site mode and there is an
     // existing process for the site, we should use it.  We can call
     // GetRelatedSiteInstance() for this, which will eagerly set the site and
     // thus use the correct process.
     bool use_process_per_site =
         RenderProcessHost::ShouldUseProcessPerSite(browser_context, dest_url) &&
         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
     // not want to use the |current_instance_impl| if it has no site, since it
     // 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
     // actually commits. This is to support better process sharing in case
     // the site redirects to some other site: we want to use the destination
     // site in the site instance.
     //
     // In the case of session restore, as it loads all the pages immediately
     // we need to set the site first, otherwise after a restore none of the
     // pages would share renderers in process-per-site.
@@ skipping 29 matching lines @@
   }
 
   // View-source URLs must use a new SiteInstance and BrowsingInstance.
   // We don't need a swap when going from view-source to a debug URL like
   // chrome://crash, however.
   // TODO(creis): Refactor this method so this duplicated code isn't needed.
   // See http://crbug.com/123007.
   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,
   // because the content is then controlled and/or scriptable by the source
   // SiteInstance.
   GURL about_blank(url::kAboutBlankURL);
   if (source_instance &&
       (dest_url == about_blank || dest_url.scheme() == url::kDataScheme)) {
     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(
     RenderFrameHostImpl* rfh,
     const GURL& dest_url) {
   // A transfer is not needed if the current SiteInstance doesn't yet have a
   // site.  This is the case for tests that use NavigateToURL.
   if (!rfh->GetSiteInstance()->HasSite())
     return false;
 
   // We do not currently swap processes for navigations in webview tag guests.
   if (rfh->GetSiteInstance()->GetSiteURL().SchemeIs(kGuestScheme))
     return false;
 
   // Don't swap processes for extensions embedded in DevTools. See
   // https://crbug.com/564216.
   if (rfh->GetSiteInstance()->GetSiteURL().SchemeIs(kChromeDevToolsScheme)) {
     // TODO(nick): https://crbug.com/570483 Check to see if |dest_url| is a
     // devtools extension, and swap processes if not.
     return false;
   }
 
   BrowserContext* context = rfh->GetSiteInstance()->GetBrowserContext();
   GURL effective_url = SiteInstanceImpl::GetEffectiveURL(context, dest_url);
 
   // 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|.
   if (descriptor.existing_site_instance)
     return descriptor.existing_site_instance;
 
   // 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 &&
       !current_instance->IsRelatedSiteInstance(candidate_instance) &&
       candidate_instance->GetSiteURL() == descriptor.new_site_url) {
     return candidate_instance;
   }
 
   // Otherwise return a newly created one.
   return SiteInstance::CreateForURL(
       delegate_->GetControllerForRenderManager().GetBrowserContext(),
       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();
+  // 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;
 
-  // Fall back to the SiteInstance's Site URL if the FrameTreeNode doen't have a
-  // current URL.
-  return current_instance->GetSiteURL();
+    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(
     SiteInstance* old_instance,
     SiteInstance* new_instance) {
   int create_render_frame_flags = 0;
   if (delegate_->IsHidden())
     create_render_frame_flags |= CREATE_RF_HIDDEN;
 
   if (pending_render_frame_host_)
@@ skipping 986 matching lines @@
   } else if (pending_render_frame_host_) {
     send_msg(pending_render_frame_host_.get(),
              pending_render_frame_host_->GetRoutingID(), msg);
   }
 
   msg->set_routing_id(render_frame_host_->GetRoutingID());
   render_frame_host_->Send(msg);
 }
 
 }  // namespace content