Implement OOPIFs within Devtools Extensions

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 22 matching lines @@
 #include "content/browser/frame_host/render_frame_proxy_host.h"
 #include "content/browser/renderer_host/render_process_host_impl.h"
 #include "content/browser/renderer_host/render_view_host_factory.h"
 #include "content/browser/renderer_host/render_view_host_impl.h"
 #include "content/browser/site_instance_impl.h"
 #include "content/browser/webui/web_ui_controller_factory_registry.h"
 #include "content/common/frame_messages.h"
 #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"
 #include "content/public/browser/render_widget_host_view.h"
 #include "content/public/browser/user_metrics.h"
 #include "content/public/common/browser_side_navigation_policy.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/common/referrer.h"
 #include "content/public/common/url_constants.h"
 
@@ skipping 1277 matching lines @@
   // chrome://settings, which currently has multiple "cross-site" subframes that
   // don't need isolation.
   if (SiteIsolationPolicy::AreCrossProcessFramesPossible() &&
       !frame_tree_node_->IsMainFrame()) {
     SiteInstance* parent_site_instance =
         frame_tree_node_->parent()->current_frame_host()->GetSiteInstance();
     if (parent_site_instance->GetSiteURL().SchemeIs(kChromeUIScheme) &&
         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
   // for this entry.  We won't commit the SiteInstance to this site until the
   // navigation commits (in DidNavigate), unless the navigation entry was
   // restored or it's a Web UI as described below.
   if (!current_instance_impl->HasSite()) {
     // If we've already created a SiteInstance for our destination, we don't
     // want to use this unused SiteInstance; use the existing one.  (We don't
     // do this check if the current_instance has a site, because for now, we
@@ skipping 150 matching lines @@
     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.
+  // 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();
   // TODO(nasko, nick): These following --site-per-process checks are
   // overly simplistic. Update them to match all the cases
   // considered by DetermineSiteInstanceForURL.
   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;
@@ skipping 1256 matching lines @@
                                              resolved_url)) {
     DCHECK(!dest_instance ||
            dest_instance == render_frame_host_->GetSiteInstance());
     return false;
   }
 
   return true;
 }
 
 }  // namespace content