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"
+#include "content/public/test/browser_test_utils.h"
 
 namespace content {
 
 RenderFrameHostManager::RenderFrameHostManager(
     FrameTreeNode* frame_tree_node,
     RenderFrameHostDelegate* render_frame_delegate,
     RenderWidgetHostDelegate* render_widget_delegate,
     Delegate* delegate)
     : frame_tree_node_(frame_tree_node),
       delegate_(delegate),
@@ skipping 1438 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.
+    /*if (content::AreAllSitesIsolatedForTesting() ||

[ncarter (slow), 2017/02/21 22:19:53]

It's supposed to be a PRESUBMIT error if you call any function with a name like
ForTest / ForTesting, outside of a unittest/browsertest .cc file.
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md

[davidsac (gone - try alexmos), 2017/03/09 20:21:22]

Done.

+        extensions::IsIsolateExtensionsEnabled()) {  // TODO get this working

[ncarter (slow), 2017/02/21 22:19:53]

This function is not known to content; content doesn't know what extensions are.

[davidsac (gone - try alexmos), 2017/03/09 20:21:22]

Done.

+                                                     // for other modes as well
+      if (url of current parent frame is devtool or about blank) {
+        CHECK(desturl is about blank or devtools or detools extension);
+      } else if (url of current parent frame is devtools
+                     extension) {  // TODO add tests to embed about url or
+                                   // devtools page in devtools extension?
+        if (!dest_url is devtools
+                extension) {  // TODO or is devtools or about blank?
+          return true;
+        }
+      } else {
+        CHECK(false);  // It should not be possible to get here?
+      }
+    }
+    return false;*/

[ncarter (slow), 2017/02/21 22:19:53]

This commented-out code should not be committed.

[davidsac (gone - try alexmos), 2017/03/09 20:21:22]

Done.

+
+    url::Origin origin = url::Origin(dest_url);

[ncarter (slow), 2017/02/21 22:19:53]

What if the origin is unique?

[davidsac (gone - try alexmos), 2017/03/09 20:21:22]

I'm still not entirely sure what you meant by that or if it is an issue, but I
think it maybe have been fixed.  If not, could you please clarify?

+    auto* policy = content::ChildProcessSecurityPolicy::GetInstance();
+    bool is_allowed_origin = policy->HasSpecificPermissionForOrigin(
+        rfh->GetProcess()->GetID(), origin);
+
+    if (rfh->GetLastCommittedURL().SchemeIs(kChromeDevToolsScheme) ||

[ncarter (slow), 2017/02/21 22:19:53]

Why would the last committed URL of the frame matter here?

This scheme check seems like it ought to be done against |dest_url| (or
|origin|) -- i.e., the origin being navigated to -- instead of against the URL
used for the previous navigation.

[davidsac (gone - try alexmos), 2017/03/09 20:21:22]

Done.  This was using a weird strategy to only allow OOPIFs in devtools if they
were in a devtools extension, but that design approach has been abandoned.

+        rfh->GetLastCommittedURL() == GURL(url::kAboutBlankURL)) {

[ncarter (slow), 2017/02/21 22:19:53]

Why can we assume that a frame currently with an "about:blank" URL cannot ever
navigate to an http URL, and need a process transfer?

Any frame can be navigated to "about:blank".

[davidsac (gone - try alexmos), 2017/03/09 20:21:22]

Done.  This was removed after the change in design.

+      CHECK(is_allowed_origin);
+    } else {  // url of current parent frame must be devtools extension
+      // TODO add tests to embed about url or devtools page in devtools
+      // extension?
+      if (!is_allowed_origin) {
+        return true;
+      }
+    }
+
     return false;
+
+    // TODO remove comment
+    // extension/common/extensions.h
+    // childProcessSecurityPolicy::HasSpecificPermissionForOrigin(int child_id,
+    // const url::Origin& origin)
   }
 
   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