Don't create unnecessary OOPIFs for subframe navigations that don't require a dedicated process.

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 <string>
@@ skipping 1436 matching lines @@
       return SiteInstanceDescriptor(render_frame_host_->GetSiteInstance());
     }
 
     // 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);
   }
 
+  // Subframes should be kept in the parent's SiteInstance unless a dedicated
+  // process is required for either the parent or the subframe's destination
+  // URL.  See https://crbug.com/711006.  Note that this should be
+  // enforced after the TopDocumentIsolation checks above.

[Charlie Reis, 2017/04/14 16:57:05]

I'm slightly wary about this invariant, since it feels like the "requires
dedicated process" logic is just one of many ways we might decide an OOPIF or
cross-process navigation is necessary.  Even the last sentence hints at that,
since TDI doesn't use dedicated processes, so that check has to come first.

I would imagine there are (or will be) other cases as well-- GuestViews may not
fall into this category, but we're considering some future behavior that more
aggressively swaps processes on cross-site main frame navigations, such that
iframes may have to become OOPIFs to script their openers, etc.  That would be
an OOPIF for compatibility rather than because either required a dedicated
process.

Maybe we can rephrase this as more of a heuristic to avoid OOPIFs, as opposed to
sounding like an invariant that we need to satisfy?

[alexmos, 2017/04/17 17:36:02]

Thanks, I agree.  I updated the comment - let me know if the new one is better.

+  if (!frame_tree_node_->IsMainFrame()) {
+    RenderFrameHostImpl* parent =
+        frame_tree_node_->parent()->current_frame_host();
+    bool dest_url_requires_dedicated_process =
+        SiteInstanceImpl::DoesSiteRequireDedicatedProcess(browser_context,
+                                                          dest_url);
+    // Web iframes embedded in DevTools extensions should not reuse the parent
+    // SiteInstance, but DevTools extensions are currently kept in the DevTools
+    // SiteInstance, which is not considered to require a dedicated process.
+    // Work around this by also checking whether the parent's URL requires a
+    // dedicated process.
+    // TODO(alexmos, nick): Remove this once https://crbug.com/706169 is fixed.
+    bool parent_url_requires_dedicated_process =
+        SiteInstanceImpl::DoesSiteRequireDedicatedProcess(
+            browser_context, parent->last_successful_url());
+    if (!parent->GetSiteInstance()->RequiresDedicatedProcess() &&
+        !dest_url_requires_dedicated_process &&
+        !parent_url_requires_dedicated_process) {

[alexmos, 2017/04/14 00:15:46]

Sigh, without this workaround, the devtools extensions tests that Andrew added
broke: the parent (devtools extension) would have a chrome-devtools://
SiteInstance, and dest_url would be http://foo.com/, neither of which requires a
dedicated process, yet we do want the process swap. :(

Not sure what the best way to fix this is.  This one relies on last successful
url still being chrome-extension:// in this case.  An alternate fix could be to
exclude the devtools SiteInstance altogether.  Or, any reason why we couldn't
require a dedicated process for the whole chrome-devtools:// scheme? (That one
is probably better as a separate CL though.)

This workaround would go away once we swap processes for devtools extensions in
706169, so I'm hoping we won't need it for long.

[Charlie Reis, 2017/04/14 16:57:05]

Fun.  Devtools definitely should require a dedicated process, but I imagine that
would force DevTools extensions out of its process, right?  It sounds like that
might be part of how we fix https://crbug.com/706169, at least from a high
level.

The main risk with your current approach is that it could apply to cases other
than DevTools, but I don't see that as likely.  I don't think there are other
cases where a URL says it requires a dedicated process but we don't give it one,
and hopefully there will be no such cases after we fix DevTools.  So this seems
ok to me for the time being.

+      return SiteInstanceDescriptor(parent->GetSiteInstance());
+    }
+  }
+
   // Start the new renderer in a new SiteInstance, but in the current
   // 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
@@ skipping 1341 matching lines @@
           delegate_->IsHidden()) {
     if (delegate_->IsHidden()) {
       render_frame_host_->GetView()->Hide();
     } else {
       render_frame_host_->GetView()->Show();
     }
   }
 }
 
 }  // namespace content