Implement OOPIFs within Devtools Extensions

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 6de0f24d8ae81b9428617f5287e1fd7fc76a9562..3d844eaf47fcbdb705d8abe7b3cd3105e8d39f58 100644
--- a/content/browser/frame_host/render_frame_host_manager.cc
+++ b/content/browser/frame_host/render_frame_host_manager.cc
@@ -40,6 +40,7 @@
 #include "content/common/frame_owner_properties.h"
 #include "content/common/site_isolation_policy.h"
 #include "content/common/view_messages.h"
+#include "content/public/browser/child_process_security_policy.h"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/render_process_host_observer.h"
 #include "content/public/browser/render_widget_host_iterator.h"
@@ -1337,6 +1338,19 @@ RenderFrameHostManager::DetermineSiteInstanceForURL(
         dest_url.SchemeIs(kChromeUIScheme)) {
       return SiteInstanceDescriptor(parent_site_instance);
     }
+    if (parent_site_instance->GetSiteURL().SchemeIs(kChromeDevToolsScheme)) {

[ncarter (slow), 2017/03/09 23:28:49]

I think using |frame_tree_node_->parent()| is probably correct here.

The other candidate we might consider, though is the SiteInstance of
|frame_tree_node_->root()|, and I'm curious what you and Alex think about that.

The only difference might be in exotic cases, say if you had an http iframe
embed an extension iframe: devtools(extension(a.com(extension))) ? One hopes
that this doesn't happen in practice.

[alexmos, 2017/03/10 21:18:26]

Yes, I think we chatted about using the root here and decided to go with the
parent.  I agree this only matters for cases like
devtools(extension(a.com(extension))), so the question is where we want to put
the innermost extension frame.  I think it's safer to put it in a regular
extension process rather than the devtools process, since otherwise we'd be
letting web content have some control over what gets embedded in the devtools
process, which seems dangerous.  Plus, the web content will presumably only be
able to embed the devtools extension using an <iframe>, which isn't one of the
cases that should get devtools API access according to the docs (those cases are
the devtools_page and the panels/sidebars created there via
chrome.devtools.panels.create and
chrome.devtools.panels.elements.createSidebarPane).  

Andrew - I forget, does this case actually work without your changes today? 
Does the innermost extension frame load?

[davidsac (gone - try alexmos), 2017/03/11 01:51:47]

Yeah, Alex pretty much covered it all.  Even if a web Iframe embedded a devtools
extension page and it were in the devtools process, it would not have access to
the devtools APIs for extensions.  Those permissions are very specific.  As Alex
mentioned, only the extension's devtools page initially has devtools api access,
and it can only grant access to the panel and sidebarpane page.  simply putting
it into the devtools process doesn't really do anything for it, except perhaps
require it to use message passing to communicate with pages in the regular
extension process.

To answer your question, Alex, without my fix at all, whatever is embedded in a
devtools extension page in devtools stays in the devtools process, including
other devtools extension pages.

In sort, I think Alex and I came to a consensus that it would be preferable to
keep devtools extensions pages within web pages in the extension's process at
all times.  Does that sound good?

+      url::Origin origin = url::Origin(dest_url);
+      auto* policy = content::ChildProcessSecurityPolicy::GetInstance();

[ncarter (slow), 2017/03/09 23:28:49]

content:: is unnecessary here; we're already in that namespace.

[davidsac (gone - try alexmos), 2017/03/11 01:51:47]

Done.

+      bool is_devtools_extension =

[ncarter (slow), 2017/03/09 23:28:49]

Technically using 'extension' in this variable name is a layering violation --
//content doesn't know what extensions are. But, the clarity gain here is
worthwhile.

I suggest the compromise of a name like:
  "bool origin_allowed_in_devtools_process"
But mention the 'extension' case in a comment like:
  // Some non-devtools origins (e.g., devtools extensions) have special
permission to
  // stay in the devtools process.

[davidsac (gone - try alexmos), 2017/03/11 01:51:47]

Done.

+          policy->HasSpecificPermissionForOrigin(frame_tree_node_->parent()
+                                                     ->current_frame_host()
+                                                     ->GetProcess()

[ncarter (slow), 2017/03/09 23:28:49]

Instead of calling GetProcess on current_frame_host, you can just call
GetProcess on the SiteInstance, |parent_site_instance|.

These are provably equivalent: RenderFrameHostImpl::GetProcess returns process_,
which is initialized to "process_(site_instance->GetProcess())" in the
RenderFrameHostImpl constructor.

I think using |parent_site_instance| here reads better, since this block of code
is a decision about whether or not we can reuse |parent_site_instance|.

[davidsac (gone - try alexmos), 2017/03/11 01:51:47]

Done.

+                                                     ->GetID(),
+                                                 origin);
+      if (dest_url.SchemeIs(kChromeDevToolsScheme) || is_devtools_extension) {

[ncarter (slow), 2017/03/09 23:28:49]

Since you've already computed |origin|, it's probably better here to check the
scheme of |origin| instead of |dest_url|. The only cases it'll differ are blob
and filesystem URLs, which as far as I know, devtools doesn't use, but we might
as well handle them since it's free.

[davidsac (gone - try alexmos), 2017/03/11 01:51:47]

Done, I think?  I used |origin.scheme()==kChromeDevToolsScheme|.

+        return SiteInstanceDescriptor(parent_site_instance);
+      }
+    }
   }
 
   // If we haven't used our SiteInstance (and thus RVH) yet, then we can use it
@@ -1507,12 +1521,14 @@ bool RenderFrameHostManager::IsRendererTransferNeededForNavigation(
   if (rfh->GetSiteInstance()->GetSiteURL().SchemeIs(kGuestScheme))
     return false;
 
-  // Don't swap processes for extensions embedded in DevTools. See
-  // https://crbug.com/564216.
+  // Don't swap processes for devtools extensions embedded in DevTools, except
+  // for external navigations in iframes. 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;
+    url::Origin origin = url::Origin(dest_url);
+    auto* policy = content::ChildProcessSecurityPolicy::GetInstance();

[ncarter (slow), 2017/03/09 23:28:49]

content:: is unnecessary here, we're already inside "namespace content {}".

[davidsac (gone - try alexmos), 2017/03/11 01:51:47]

Done.

+    bool is_devtools_extension = policy->HasSpecificPermissionForOrigin(
+        rfh->GetProcess()->GetID(), origin);
+    return !(dest_url.SchemeIs(kChromeDevToolsScheme) || is_devtools_extension);

[ncarter (slow), 2017/03/09 23:28:49]

As above, checking the scheme on |origin| instead of |dest_url| will produce
better results, for the [probably irrelevant] corner case of
"blob:chrome-devtools://" URLs.

[davidsac (gone - try alexmos), 2017/03/11 01:51:47]

Done, I think?  I used |origin.scheme()==kChromeDevToolsScheme|.

   }
 
   BrowserContext* context = rfh->GetSiteInstance()->GetBrowserContext();