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..56454b4ae5f0c9fb7895c50e2621b1691267d9ca 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"
@@ -49,6 +50,7 @@
 #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 {
 
@@ -1507,12 +1509,49 @@ 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.
+    /*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();