Implement OOPIFs within Devtools Extensions

chrome/browser/devtools/devtools_sanity_browsertest.cc

Index: chrome/browser/devtools/devtools_sanity_browsertest.cc
diff --git a/chrome/browser/devtools/devtools_sanity_browsertest.cc b/chrome/browser/devtools/devtools_sanity_browsertest.cc
index 106ff22e4290dee9e638f1f25b4117405e6bdcde..5161cd81802b5b174e6a8cedf9242229b184abd2 100644
--- a/chrome/browser/devtools/devtools_sanity_browsertest.cc
+++ b/chrome/browser/devtools/devtools_sanity_browsertest.cc
@@ -39,6 +39,7 @@
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_switches.h"
+#include "chrome/common/extensions/extension_process_policy.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/url_constants.h"
 #include "chrome/test/base/in_process_browser_test.h"
@@ -52,6 +53,7 @@
 #include "content/public/browser/devtools_agent_host.h"
 #include "content/public/browser/notification_registrar.h"
 #include "content/public/browser/notification_service.h"
+#include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/render_widget_host.h"
 #include "content/public/browser/render_widget_host_view.h"
@@ -61,6 +63,7 @@
 #include "content/public/browser/worker_service.h"
 #include "content/public/browser/worker_service_observer.h"
 #include "content/public/common/content_switches.h"
+#include "content/public/common/url_constants.h"
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/test_navigation_observer.h"
 #include "extensions/browser/extension_registry.h"
@@ -513,6 +516,81 @@ class DevToolsExtensionTest : public DevToolsSanityTest,
     return GetExtensionByPath(registry->enabled_extensions(), path);
   }
 
+  // TODO(davidsac): should I refactor this with a diff method for each page and
+  // call them individually in each test instead of these if/elses?

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

I might split this into two functions (for the is_devtools_extension=True and
is_devtools_extension=False cases), and then try to simplify or eliminate the
remaining parameters -- see below for some ideas.

For the case of the non-devtools extension, you might just load something off
disk (i.e. add a new directory to chrome/test/data/devtools/extensions, either
forking or extending the simple_content_script extension). All you need is a
web_accessible_resource right?

You only really need to dynamically generate the extension if it's embedding
content that can't be known at compile time.

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

Done.  Kinda.  I had talked to Alex about this earlier, and the general
concensus was that even though it wasn't necessary, enough of the extensions
reused enough of the same files with only slight variations that dynamically
generating them was preferable.  That being said, I took in your suggestions and
put a bit of my own spin on it by passing in a string of the name of the
devtools page to use, or an empty string if it's a regular extension.  Let me
know what you think, but I think this new fix should be preferable to two
separate methods.

+  const Extension* LoadExtensionForTest(
+      const std::string& name,
+      extensions::TestExtensionDir* dir,

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

Having to pass in a TestExtensionDir seems like a burden.

In other places, what we've done instead is to add a
vector<unique_ptr<TestExtensionDir>> test_extension_directories_ to the test
fixture (here that would be class DevToolsExtensionTest), and have the
LoadExtensionForTest function create a dir, and then push it into the vector,
for automatic cleanup later:

https://cs.chromium.org/search/?q=vector.*TestExtensionDir+package:%5Echromiu...

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

Done.

+      bool has_popup_test_page,
+      bool is_devtools_extension,
+      bool should_create_panel_and_pane,
+      const std::string& test_page_url = "test.html") {

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

Could you combine should_create_panel_and_pane and test_page_url? I.e. make the
empty string correspond to the should_create_panel_and_pane=False case?

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

Done.

+    extensions::DictionaryBuilder manifest;
+    manifest.Set("name", name)
+        .Set("version", "1")
+        .Set("manifest_version", 2)
+        .Set("web_accessible_resources",
+             extensions::ListBuilder().Append("test.html").Build());
+    if (is_devtools_extension) {
+      manifest.Set("devtools_page", "devtools.html");
+    }
+
+    dir->WriteManifest(manifest.ToJSON());
+
+    if (is_devtools_extension) {
+      if (should_create_panel_and_pane) {
+        dir->WriteFile(
+            FILE_PATH_LITERAL("devtools.html"),
+            "<html><head><script "
+            "src='devtools.js'></script></head><body></body></html>");
+
+        dir->WriteFile(
+            FILE_PATH_LITERAL("devtools.js"),
+            "chrome.devtools.panels.create('iframe_panel',\n"
+            "    null,\n"
+            "    'panel.html',\n"
+            "    function(panel) {\n"
+            "      "
+            "chrome.devtools.inspectedWindow.eval('console.log("
+            "\"PASS\")');\n"
+            "    }\n"
+            ");\n"
+            "chrome.devtools.panels.elements.createSidebarPane('iframe_pane',\n"
+            "function(sidebar) {\n"
+            "chrome.devtools.inspectedWindow.eval('console.log(\"PASS\")');\n"
+            "sidebar.setPage('panel.html');\n"
+            "});\n");
+
+        dir->WriteFile(FILE_PATH_LITERAL("panel.html"),
+                       "<html><body><iframe src='" + test_page_url +
+                           "'></iframe></body></html>");
+
+      } else {
+        dir->WriteFile(FILE_PATH_LITERAL("devtools.html"),
+                       "<html><head></head><body></body></html>");
+      }
+    }
+    if (has_popup_test_page) {

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

Rather than parameterizing this with a boolean |has_popup_test_page|, it might
be clearer to just write out multiple html files unconditionally:
   - send_done_on_load.html
   - simple.html

Looks like renavigation_test.html could even be done the same way, since you can
call embedded_testserver() from here.

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

Done.

+      dir->WriteFile(
+          FILE_PATH_LITERAL("test.html"),
+          "<html><head><script src='test.js'></script></head><body>This "
+          "is a test</body></html>");
+
+      dir->WriteFile(FILE_PATH_LITERAL("test.js"),
+                     "window.onload = function() {\n"
+                     "  domAutomationController.setAutomationId(0);\n"
+                     "  domAutomationController.send('DONE');\n"
+                     "}\n");
+    } else {
+      dir->WriteFile(FILE_PATH_LITERAL("test.html"),
+                     "<html><head></head><body>This is a test</body></html>");
+    }
+
+    // Install the extension.
+    const Extension* extension = LoadExtensionFromPath(dir->UnpackedPath());
+    return extension;
+  }
+
  private:
   const Extension* GetExtensionByPath(
       const extensions::ExtensionSet& extensions,
@@ -903,17 +981,261 @@ IN_PROC_BROWSER_TEST_F(DevToolsExtensionTest,
   RunTest("waitForTestResultsInConsole", std::string());
 }
 
-// Tests a chrome.devtools extension panel that embeds an http:// iframe.
-IN_PROC_BROWSER_TEST_F(DevToolsExtensionTest, DevToolsExtensionWithHttpIframe) {
+// The following three tests check that http Iframes within the three "devtools
+// pages" (the visible devtools panel for the devtools extension, the sidebar
+// pane page for the devtools extension that is visible in the elemanets panel,
+// and the devtools background page, which is different from the extension's
+// background page)(https://developer.chrome.com/extensions/devtools) are
+// rendered in their own processes and not in the devtools process or the
+// extension's process.
+
+// Tests that http Iframes within the visible devtools panel for the devtools
+// extension are rendered in their own processes and not in the devtools process
+// or the extension's process.  Also tests that data URLs and about blank URLs
+// are rendered in the devtools process, unless an OOPIF is navigated to
+// about:blank, in which case it does not end up back in the devtools process.
+IN_PROC_BROWSER_TEST_F(DevToolsExtensionTest,
+                       HttpIframeInDevToolsExtensionPanel) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+
+  // TODO do I need to add/remove/fix any comments anywhere?
+  std::unique_ptr<extensions::TestExtensionDir> dir(
+      new extensions::TestExtensionDir());
+
+  GURL http_popup_url =
+      embedded_test_server()->GetURL("a.com", "/popup_iframe.html");
+
+  GURL about_blank_url = GURL(url::kAboutBlankURL);
+
+  GURL data_url = GURL("data:text/html,foo");
+
+  // Add the custom page that we want rendered in the extension panel.
+  dir->WriteFile(FILE_PATH_LITERAL("renavigation_test.html"),
+                 "<html><body><iframe src='" + about_blank_url.spec() +
+                     "'></iframe><iframe src='" + data_url.spec() +
+                     "'></iframe><iframe src='" + http_popup_url.spec() +
+                     "'></iframe></body></html>");
+
+  // Install the dynamically-generated extension.
+  const Extension* extension = LoadExtensionForTest(
+      "Devtools Extension", dir.get(), true /* has_popup_test_page */,
+      true /* is_devtools_extension */, true /* should_create_panel_and_pane */,
+      "/renavigation_test.html");
+  ASSERT_TRUE(extension);
+
+  OpenDevToolsWindow(kDebuggerTestPage, false);
+
+  // Wait for the panel extension to finish loading -- it'll output 'PASS'
+  // when it's installed. waitForTestResultsInConsole waits until that 'PASS'.
+  RunTestFunction(window_, "waitForTestResultsInConsole");
+
+  // Now that we know the panel is loaded, switch to it. We'll wait until we
+  // see a 'DONE' message sent from popup_iframe.html, indicating that it
+  // loaded successfully.
+  content::DOMMessageQueue message_queue;
+  std::string message;
+  SwitchToExtensionPanel(window_, extension, "iframe_panel");
+
+  while (true) {
+    ASSERT_TRUE(message_queue.WaitForMessage(&message));
+    if (message == "\"DONE\"")
+      break;
+  }
+
+  std::vector<content::RenderFrameHost*> rfhs =
+      main_web_contents()->GetAllFrames();
+  EXPECT_EQ(7U, rfhs.size());
+
+  content::RenderFrameHost* main_devtools_rfh =
+      main_web_contents()->GetMainFrame();
+  content::RenderFrameHost* devtools_extension_devtools_rfh =
+      ChildFrameAt(main_devtools_rfh, 0);
+  content::RenderFrameHost* devtools_panel_extension_rfh =
+      ChildFrameAt(main_devtools_rfh, 1);
+  content::RenderFrameHost* test_frame_rfh =
+      ChildFrameAt(devtools_panel_extension_rfh, 0);
+  content::RenderFrameHost* about_blank_frame_rfh =
+      ChildFrameAt(test_frame_rfh, 0);
+  content::RenderFrameHost* data_frame_rfh = ChildFrameAt(test_frame_rfh, 1);
+  content::RenderFrameHost* about_blank_http_iframe_rfh =
+      ChildFrameAt(test_frame_rfh, 2);
+
+  EXPECT_TRUE(main_devtools_rfh->GetLastCommittedURL().SchemeIs(
+      content::kChromeDevToolsScheme));
+  EXPECT_EQ(extension->GetResourceURL("/devtools.html"),
+            devtools_extension_devtools_rfh->GetLastCommittedURL());
+  EXPECT_EQ(extension->GetResourceURL("/panel.html"),
+            devtools_panel_extension_rfh->GetLastCommittedURL());
+  EXPECT_EQ(extension->GetResourceURL("/renavigation_test.html"),
+            test_frame_rfh->GetLastCommittedURL());
+  EXPECT_EQ(about_blank_url, about_blank_frame_rfh->GetLastCommittedURL());
+  EXPECT_EQ(data_url, data_frame_rfh->GetLastCommittedURL());
+  EXPECT_EQ(http_popup_url, about_blank_http_iframe_rfh->GetLastCommittedURL());
+
+  if (content::AreAllSitesIsolatedForTesting() ||
+      extensions::IsIsolateExtensionsEnabled()) {
+    EXPECT_TRUE(main_devtools_rfh->GetSiteInstance()->GetSiteURL().SchemeIs(
+        content::kChromeDevToolsScheme));
+    EXPECT_EQ(main_devtools_rfh->GetSiteInstance(),
+              devtools_extension_devtools_rfh->GetSiteInstance());
+    EXPECT_EQ(main_devtools_rfh->GetSiteInstance(),
+              devtools_panel_extension_rfh->GetSiteInstance());
+    EXPECT_EQ(main_devtools_rfh->GetSiteInstance(),
+              test_frame_rfh->GetSiteInstance());
+    EXPECT_EQ(main_devtools_rfh->GetSiteInstance(),
+              about_blank_frame_rfh->GetSiteInstance());
+    EXPECT_EQ(main_devtools_rfh->GetSiteInstance(),
+              data_frame_rfh->GetSiteInstance());
+    EXPECT_EQ(
+        http_popup_url.host(),
+        about_blank_http_iframe_rfh->GetSiteInstance()->GetSiteURL().host());
+
+    std::string about_blank_javascript =
+        "location.href='" + about_blank_url.spec() + "';";
+
+    content::TestNavigationManager manager(main_web_contents(),
+                                           about_blank_url);
+
+    ASSERT_TRUE(content::ExecuteScript(about_blank_http_iframe_rfh,
+                                       about_blank_javascript));
+
+    manager.WaitForNavigationFinished();
+
+    /*LOG(INFO) << "about blank last committed"

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

There's some debugging cruft in this file that you should revisit.

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

Done.

+              << about_blank_http_iframe_rfh->GetLastCommittedURL();
+    LOG(INFO)
+        << "about blank site instance"
+        <<
+    about_blank_http_iframe_rfh->GetSiteInstance()->GetSiteURL().spec();*/
+
+    EXPECT_EQ(about_blank_url,
+              about_blank_http_iframe_rfh->GetLastCommittedURL());
+    EXPECT_EQ(
+        http_popup_url.host(),
+        about_blank_http_iframe_rfh->GetSiteInstance()->GetSiteURL().host());
+
+  } else {
+    // TODO get rid of this? make sure that this test is only ever run in
+    // isolate extensions mode ro site-per-process mode.
+    /*EXPECT_TRUE(main_devtools_rfh->GetSiteInstance()->GetSiteURL().SchemeIs(
+        content::kChromeDevToolsScheme));
+    EXPECT_TRUE(devtools_extension_devtools_rfh->GetSiteInstance()
+                    ->GetSiteURL()
+                    .SchemeIs(content::kChromeDevToolsScheme));
+    EXPECT_TRUE(
+        devtools_panel_extension_rfh->GetSiteInstance()->GetSiteURL().SchemeIs(
+            content::kChromeDevToolsScheme));
+    EXPECT_TRUE(http_iframe_rfh->GetSiteInstance()->GetSiteURL().SchemeIs(
+        content::kChromeDevToolsScheme));*/
+  }
+}
+
+// Tests that http Iframes within the sidebar pane page for the devtools
+// extension that is visible in the elemanets panel are rendered in their own
+// processes and not in the devtools process or the extension's process.
+IN_PROC_BROWSER_TEST_F(DevToolsExtensionTest,
+                       HttpIframeInDevToolsExtensionSideBarPane) {
   ASSERT_TRUE(embedded_test_server()->Start());
 
-  // Our extension must load an URL from the test server, whose port is only
+  // Our extension must load a URL from the test server, whose port is only
   // known at runtime. So, to embed the URL, we must dynamically generate the
   // extension, rather than loading it from static content.
   std::unique_ptr<extensions::TestExtensionDir> dir(
       new extensions::TestExtensionDir());
 
-  extensions::DictionaryBuilder manifest;
+  GURL http_iframe_url =
+      embedded_test_server()->GetURL("a.com", "/popup_iframe.html");
+
+  // Install the dynamically-generated extension.
+  const Extension* extension = LoadExtensionForTest(
+      "Devtools Extension", dir.get(), false /* !has_popup_test_page */,
+      true /* is_devtools_extension */, true /* should_create_panel_and_pane */,
+      http_iframe_url.spec());
+  ASSERT_TRUE(extension);
+
+  OpenDevToolsWindow(kDebuggerTestPage, false);
+
+  // Wait for the panel extension to finish loading -- it'll output 'PASS'
+  // when it's installed. waitForTestResultsInConsole waits until that 'PASS'.
+  RunTestFunction(window_, "waitForTestResultsInConsole");
+
+  // Now that we know the panel is loaded, switch to it.
+  content::DOMMessageQueue message_queue;
+  SwitchToPanel(window_, "elements");
+  // This is a bit of a hack to switch to the sidebar pane in the elements panel
+  // that the Iframe has been added to.  We'll wait until we see a 'DONE'
+  // message sent from popup_iframe.html, indicating that it loaded
+  // successfully.
+  SwitchToPanel(window_, "iframe_pane");
+  std::string message;
+
+  while (true) {
+    ASSERT_TRUE(message_queue.WaitForMessage(&message));
+    if (message == "\"DONE\"")
+      break;
+  }
+
+  std::vector<content::RenderFrameHost*> rfhs =
+      main_web_contents()->GetAllFrames();
+  EXPECT_EQ(4U, rfhs.size());
+
+  content::RenderFrameHost* main_devtools_rfh =
+      main_web_contents()->GetMainFrame();
+  content::RenderFrameHost* devtools_extension_devtools_rfh =
+      ChildFrameAt(main_devtools_rfh, 0);
+  content::RenderFrameHost* devtools_sidebar_pane_extension_rfh =
+      ChildFrameAt(main_devtools_rfh, 1);
+  content::RenderFrameHost* http_iframe_rfh =
+      ChildFrameAt(devtools_sidebar_pane_extension_rfh, 0);
+
+  EXPECT_TRUE(main_devtools_rfh->GetLastCommittedURL().SchemeIs(
+      content::kChromeDevToolsScheme));
+  EXPECT_EQ(extension->GetResourceURL("/devtools.html"),
+            devtools_extension_devtools_rfh->GetLastCommittedURL());
+  EXPECT_EQ(extension->GetResourceURL("/panel.html"),
+            devtools_sidebar_pane_extension_rfh->GetLastCommittedURL());
+  EXPECT_EQ(http_iframe_url, http_iframe_rfh->GetLastCommittedURL());
+
+  if (content::AreAllSitesIsolatedForTesting() ||
+      extensions::IsIsolateExtensionsEnabled()) {
+    EXPECT_TRUE(main_devtools_rfh->GetSiteInstance()->GetSiteURL().SchemeIs(
+        content::kChromeDevToolsScheme));
+    EXPECT_EQ(main_devtools_rfh->GetSiteInstance(),
+              devtools_extension_devtools_rfh->GetSiteInstance());
+    EXPECT_EQ(main_devtools_rfh->GetSiteInstance(),
+              devtools_sidebar_pane_extension_rfh->GetSiteInstance());
+    EXPECT_EQ(http_iframe_url.host(),
+              http_iframe_rfh->GetSiteInstance()->GetSiteURL().host());
+  } else {
+    // TODO get rid of this? make sure that this test is only ever run in
+    // isolate extensions mode ro site-per-process mode.
+    /*EXPECT_TRUE(main_devtools_rfh->GetSiteInstance()->GetSiteURL().SchemeIs(
+        content::kChromeDevToolsScheme));
+    EXPECT_TRUE(devtools_extension_devtools_rfh->GetSiteInstance()
+                    ->GetSiteURL()
+                    .SchemeIs(content::kChromeDevToolsScheme));
+    EXPECT_TRUE(devtools_sidebar_pane_extension_rfh->GetSiteInstance()
+                    ->GetSiteURL()
+                    .SchemeIs(content::kChromeDevToolsScheme));
+    EXPECT_TRUE(http_iframe_rfh->GetSiteInstance()->GetSiteURL().SchemeIs(
+        content::kChromeDevToolsScheme));*/
+  }
+}
+
+// Tests that http Iframes within the devtools background page, which is
+// different from the extension's background page, are rendered in their own
+// processes and not in the devtools process or the extension's process.
+IN_PROC_BROWSER_TEST_F(DevToolsExtensionTest,
+                       HttpIframeInDevToolsExtensionDevtools) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+
+  GURL http_iframe_url =
+      embedded_test_server()->GetURL("a.com", "/popup_iframe.html");
+
+  std::unique_ptr<extensions::TestExtensionDir> dir(
+      new extensions::TestExtensionDir());
+
+  // TODO(davidsac): is this worth refactoring to use |LoadExtensionForTest|?

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

Yes. I think you just pass in http_iframe_url and you're done, right?

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

Done.  It was actually a bit more complicated than that, because this test
embeds the popup iframe in the devtools hextension's devtools.html page, rather
than on the panel.  Basically, refactoring it from my old approach would have
meant adding in a third possible devtools.html page.  Fortunately, this
complexity was handled nicely in the refactoring that you hinted at/suggested. 
Thanks a million!

   dir->WriteManifest(extensions::DictionaryBuilder()
                          .Set("name", "Devtools Panel w/ HTTP Iframe")
                          .Set("version", "1")
@@ -921,31 +1243,271 @@ IN_PROC_BROWSER_TEST_F(DevToolsExtensionTest, DevToolsExtensionWithHttpIframe) {
                          .Set("devtools_page", "devtools.html")
                          .ToJSON());
 
-  dir->WriteFile(
-      FILE_PATH_LITERAL("devtools.html"),
-      "<html><head><script src='devtools.js'></script></head></html>");
+  dir->WriteFile(FILE_PATH_LITERAL("devtools.html"),
+                 "<html><head></head><body><iframe src='" +
+                     http_iframe_url.spec() + "'></iframe></body></html>");
 
-  dir->WriteFile(
-      FILE_PATH_LITERAL("devtools.js"),
-      "chrome.devtools.panels.create('iframe_panel',\n"
-      "    null,\n"
-      "    'panel.html',\n"
-      "    function(panel) {\n"
-      "      chrome.devtools.inspectedWindow.eval('console.log(\"PASS\")');\n"
-      "    }\n"
-      ");\n");
+  // Install the dynamically-generated extension.
+  const Extension* extension = LoadExtensionFromPath(dir->UnpackedPath());
+  ASSERT_TRUE(extension);
 
-  GURL http_iframe =
-      embedded_test_server()->GetURL("a.com", "/popup_iframe.html");
-  dir->WriteFile(FILE_PATH_LITERAL("panel.html"),
-                 "<html><body>Extension panel.<iframe src='" +
-                     http_iframe.spec() + "'></iframe>");
+  OpenDevToolsWindow(kDebuggerTestPage, false);
 
-  // Install the extension.
-  const Extension* extension = LoadExtensionFromPath(dir->UnpackedPath());
+  // Now that we know the panel is loaded, switch to it. We'll wait until we
+  // see a 'DONE' message sent from popup_iframe.html, indicating that it
+  // loaded successfully.
+  content::DOMMessageQueue message_queue;
+  std::string message;
+
+  while (true) {
+    ASSERT_TRUE(message_queue.WaitForMessage(&message));
+    if (message == "\"DONE\"")
+      break;
+  }
+
+  std::vector<content::RenderFrameHost*> rfhs =
+      main_web_contents()->GetAllFrames();
+  EXPECT_EQ(3U, rfhs.size());
+
+  content::RenderFrameHost* main_devtools_rfh =
+      main_web_contents()->GetMainFrame();
+  content::RenderFrameHost* devtools_extension_devtools_rfh =
+      ChildFrameAt(main_devtools_rfh, 0);
+  content::RenderFrameHost* http_iframe_rfh =
+      ChildFrameAt(devtools_extension_devtools_rfh, 0);
+
+  EXPECT_TRUE(main_devtools_rfh->GetLastCommittedURL().SchemeIs(
+      content::kChromeDevToolsScheme));
+  EXPECT_EQ(extension->GetResourceURL("/devtools.html"),
+            devtools_extension_devtools_rfh->GetLastCommittedURL());
+  EXPECT_EQ(http_iframe_url, http_iframe_rfh->GetLastCommittedURL());
+
+  if (content::AreAllSitesIsolatedForTesting() ||
+      extensions::IsIsolateExtensionsEnabled()) {
+    EXPECT_TRUE(main_devtools_rfh->GetSiteInstance()->GetSiteURL().SchemeIs(
+        content::kChromeDevToolsScheme));
+    EXPECT_EQ(main_devtools_rfh->GetSiteInstance(),
+              devtools_extension_devtools_rfh->GetSiteInstance());
+    EXPECT_EQ(http_iframe_url.host(),
+              http_iframe_rfh->GetSiteInstance()->GetSiteURL().host());
+
+  } else {
+    // TODO get rid of this? make sure that this test is only ever run in
+    // isolate extensions mode ro site-per-process mode.
+    /*EXPECT_TRUE(main_devtools_rfh->GetSiteInstance()->GetSiteURL().SchemeIs(
+        content::kChromeDevToolsScheme));
+    EXPECT_TRUE(devtools_extension_devtools_rfh->GetSiteInstance()
+                    ->GetSiteURL()
+                    .SchemeIs(content::kChromeDevToolsScheme));
+    EXPECT_TRUE(http_iframe_rfh->GetSiteInstance()->GetSiteURL().SchemeIs(
+        content::kChromeDevToolsScheme));*/
+  }
+}
+
+// Tests that iframes to a non-devtools extension embedded in a devtools
+// extension will be isolated from devtools and the devtools extension.
+IN_PROC_BROWSER_TEST_F(DevToolsExtensionTest,
+                       NonDevToolsExtensionInDevToolsExtension) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+
+  std::unique_ptr<extensions::TestExtensionDir> dir(
+      new extensions::TestExtensionDir());
+
+  // Install the dynamically-generated non-devtools extension.
+  const Extension* non_devtools_extension = LoadExtensionForTest(
+      "Non-DevTools Extension", dir.get(), true /* has_popup_test_page */,
+      false /* !is_devtools_extension */,
+      false /* !should_create_panel_and_pane */);
+  ASSERT_TRUE(non_devtools_extension);
+
+  GURL non_dt_extension_test_url =
+      non_devtools_extension->GetResourceURL("/test.html");
+
+  std::unique_ptr<extensions::TestExtensionDir> dir2(
+      new extensions::TestExtensionDir());
+
+  // Install the dynamically-generated devtools extension.
+  const Extension* devtools_extension = LoadExtensionForTest(
+      "Devtools Extension", dir2.get(), false /* !has_popup_test_page */,
+      true /* is_devtools_extension */, true /* should_create_panel_and_pane */,
+      non_dt_extension_test_url.spec());
+  ASSERT_TRUE(devtools_extension);
+
+  OpenDevToolsWindow(kDebuggerTestPage, false);
+
+  // Wait for the panel extension to finish loading -- it'll output 'PASS'
+  // when it's installed. waitForTestResultsInConsole waits until that 'PASS'.
+  RunTestFunction(window_, "waitForTestResultsInConsole");
+
+  // Now that we know the panel is loaded, switch to it. We'll wait until we
+  // see a 'DONE' message sent from popup_iframe.html, indicating that it
+  // loaded successfully.
+  content::DOMMessageQueue message_queue;
+  SwitchToExtensionPanel(window_, devtools_extension, "iframe_panel");
+  std::string message;
+
+  while (true) {
+    ASSERT_TRUE(message_queue.WaitForMessage(&message));
+    if (message == "\"DONE\"")
+      break;
+  }
+
+  std::vector<content::RenderFrameHost*> rfhs =
+      main_web_contents()->GetAllFrames();
+  EXPECT_EQ(4U, rfhs.size());
+
+  content::RenderFrameHost* main_devtools_rfh =
+      main_web_contents()->GetMainFrame();
+  content::RenderFrameHost* devtools_extension_devtools_rfh =
+      ChildFrameAt(main_devtools_rfh, 0);
+  content::RenderFrameHost* devtools_extension_panel_rfh =
+      ChildFrameAt(main_devtools_rfh, 1);
+  content::RenderFrameHost* non_devtools_extension_test_rfh =
+      ChildFrameAt(devtools_extension_panel_rfh, 0);
+
+  EXPECT_TRUE(main_devtools_rfh->GetLastCommittedURL().SchemeIs(
+      content::kChromeDevToolsScheme));
+  EXPECT_EQ(devtools_extension->GetResourceURL("/devtools.html"),
+            devtools_extension_devtools_rfh->GetLastCommittedURL());
+  EXPECT_EQ(devtools_extension->GetResourceURL("/panel.html"),
+            devtools_extension_panel_rfh->GetLastCommittedURL());
+  EXPECT_EQ(non_dt_extension_test_url,
+            non_devtools_extension_test_rfh->GetLastCommittedURL());
+
+  if (content::AreAllSitesIsolatedForTesting() ||
+      extensions::IsIsolateExtensionsEnabled()) {
+    // test.html's frame should be in extension b's
+    // process, not in devtools or extension a's process.
+    EXPECT_TRUE(main_devtools_rfh->GetSiteInstance()->GetSiteURL().SchemeIs(
+        content::kChromeDevToolsScheme));
+    EXPECT_EQ(main_devtools_rfh->GetSiteInstance(),
+              devtools_extension_devtools_rfh->GetSiteInstance());
+    EXPECT_EQ(main_devtools_rfh->GetSiteInstance(),
+              devtools_extension_panel_rfh->GetSiteInstance());
+    EXPECT_EQ(non_dt_extension_test_url.GetOrigin(),
+              non_devtools_extension_test_rfh->GetSiteInstance()->GetSiteURL());
+  } else {
+    // TODO get rid of this? make sure that this test is only ever run in
+    // isolate extensions mode ro site-per-process mode. test.html's frame
+    // should be in the devtools process.
+    /*EXPECT_TRUE(main_devtools_rfh->GetSiteInstance()->GetSiteURL().SchemeIs(
+        content::kChromeDevToolsScheme));
+    EXPECT_TRUE(devtools_extension_devtools_rfh->GetSiteInstance()
+                    ->GetSiteURL()
+                    .SchemeIs(content::kChromeDevToolsScheme));
+    EXPECT_TRUE(non_devtools_extension_test_rfh->GetSiteInstance()
+                    ->GetSiteURL()
+                    .SchemeIs(content::kChromeDevToolsScheme));*/
+  }
+}
+
+// Tests that if a devtools extension's devtools page has a subframe to a
+// page for another devtools extension, the subframe is rendered in the devtools
+// process as well.
+IN_PROC_BROWSER_TEST_F(DevToolsExtensionTest,
+                       DevToolsExtensionInDevToolsExtension) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+
+  std::unique_ptr<extensions::TestExtensionDir> dir(
+      new extensions::TestExtensionDir());
+
+  // Install the dynamically-generated extension.
+  const Extension* devtools_b_extension = LoadExtensionForTest(
+      "Devtools Extension B", dir.get(), true /* has_popup_test_page */,
+      true /* is_devtools_extension */,
+      false /* !should_create_panel_and_pane */);
+  ASSERT_TRUE(devtools_b_extension);
+
+  GURL extension_b_test_url =
+      devtools_b_extension->GetResourceURL("/test.html");
+
+  std::unique_ptr<extensions::TestExtensionDir> dir2(
+      new extensions::TestExtensionDir());
+
+  // Install the dynamically-generated extension.
+  const Extension* devtools_a_extension = LoadExtensionForTest(
+      "Devtools Extension A", dir2.get(), false /* !has_popup_test_page */,
+      true /* is_devtools_extension */, true /* should_create_panel_and_pane */,
+      extension_b_test_url.spec());
+  ASSERT_TRUE(devtools_a_extension);
+
+  OpenDevToolsWindow(kDebuggerTestPage, false);
+
+  // Wait for the panel extension to finish loading -- it'll output 'PASS'
+  // when it's installed. waitForTestResultsInConsole waits until that 'PASS'.
+  RunTestFunction(window_, "waitForTestResultsInConsole");
+
+  // Now that we know the panel is loaded, switch to it. We'll wait until we
+  // see a 'DONE' message sent from popup_iframe.html, indicating that it
+  // loaded successfully.
+  content::DOMMessageQueue message_queue;
+  SwitchToExtensionPanel(window_, devtools_a_extension, "iframe_panel");
+  std::string message;
+
+  while (true) {
+    ASSERT_TRUE(message_queue.WaitForMessage(&message));
+    if (message == "\"DONE\"")
+      break;
+  }
+
+  std::vector<content::RenderFrameHost*> rfhs =
+      main_web_contents()->GetAllFrames();
+  EXPECT_EQ(5U, rfhs.size());
+
+  content::RenderFrameHost* main_devtools_rfh =
+      main_web_contents()->GetMainFrame();
+  content::RenderFrameHost* devtools_extension_a_devtools_rfh =
+      ChildFrameAt(main_devtools_rfh, 1);
+  content::RenderFrameHost* devtools_extension_b_devtools_rfh =
+      ChildFrameAt(main_devtools_rfh, 0);
+  content::RenderFrameHost* devtools_extension_a_panel_rfh =
+      ChildFrameAt(main_devtools_rfh, 2);
+  content::RenderFrameHost* devtools_extension_b_test_rfh =
+      ChildFrameAt(devtools_extension_a_panel_rfh, 0);
+
+  // TODO davidsac: the two devtools pages can be flipped in order depending on
+  // the alphabetization of their id.  It doesn't matter, but it makes this test
+  // flaky.

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

How unfortunate. Looks like the source of this nondeterminism is the
ExtensionSet itself.

Easiest way to unflake this might be
https://cs.chromium.org/search/?q=FrameHasSourceUrl+package:%5Echromium$&type=cs

RenderFrameHost* devtools_extension_a_devtools_rfh =
content::FrameMatchingPredicate(
      main_web_contents,
      base::Bind(&content::FrameHasSourceUrl,
                 devtools_a_extension->GetResourceURL("/devtools.html")));
EXPECT_TRUE(devtools_extension_a_devtools_rfh);

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

Done.  Thanks a bunch!  That was very useful.

+  EXPECT_TRUE(main_devtools_rfh->GetLastCommittedURL().SchemeIs(
+      content::kChromeDevToolsScheme));
+  EXPECT_EQ(devtools_a_extension->GetResourceURL("/devtools.html"),
+            devtools_extension_a_devtools_rfh->GetLastCommittedURL());
+  EXPECT_EQ(devtools_b_extension->GetResourceURL("/devtools.html"),
+            devtools_extension_b_devtools_rfh->GetLastCommittedURL());
+  EXPECT_EQ(devtools_a_extension->GetResourceURL("/panel.html"),
+            devtools_extension_a_panel_rfh->GetLastCommittedURL());
+  EXPECT_EQ(extension_b_test_url,
+            devtools_extension_b_test_rfh->GetLastCommittedURL());
+
+  // all frames should be in the devtools process.
+  EXPECT_TRUE(main_devtools_rfh->GetSiteInstance()->GetSiteURL().SchemeIs(
+      content::kChromeDevToolsScheme));
+  EXPECT_EQ(main_devtools_rfh->GetSiteInstance(),
+            devtools_extension_a_devtools_rfh->GetSiteInstance());
+  EXPECT_EQ(main_devtools_rfh->GetSiteInstance(),
+            devtools_extension_b_devtools_rfh->GetSiteInstance());
+  EXPECT_EQ(main_devtools_rfh->GetSiteInstance(),
+            devtools_extension_a_panel_rfh->GetSiteInstance());
+  EXPECT_EQ(main_devtools_rfh->GetSiteInstance(),
+            devtools_extension_b_test_rfh->GetSiteInstance());
+}
+
+// Tests that a devtools extension can still have subframes to itself in a
+// "devtools page" and that they will be rendered within the devtools process as
+// well, not in the extension process.
+IN_PROC_BROWSER_TEST_F(DevToolsExtensionTest, DevToolsExtensionInItself) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+
+  std::unique_ptr<extensions::TestExtensionDir> dir(
+      new extensions::TestExtensionDir());
+
+  // Install the dynamically-generated extension.
+  const Extension* extension = LoadExtensionForTest(
+      "Devtools Extension", dir.get(), true /* has_popup_test_page */,
+      true /* is_devtools_extension */,
+      true /* should_create_panel_and_pane */);
   ASSERT_TRUE(extension);
 
-  // Open a devtools window.
   OpenDevToolsWindow(kDebuggerTestPage, false);
 
   // Wait for the panel extension to finish loading -- it'll output 'PASS'
@@ -958,11 +1520,122 @@ IN_PROC_BROWSER_TEST_F(DevToolsExtensionTest, DevToolsExtensionWithHttpIframe) {
   content::DOMMessageQueue message_queue;
   SwitchToExtensionPanel(window_, extension, "iframe_panel");
   std::string message;
+
   while (true) {
     ASSERT_TRUE(message_queue.WaitForMessage(&message));
     if (message == "\"DONE\"")
       break;
   }
+
+  GURL extension_test_url = extension->GetResourceURL("/test.html");
+
+  std::vector<content::RenderFrameHost*> rfhs =
+      main_web_contents()->GetAllFrames();
+  EXPECT_EQ(4U, rfhs.size());
+
+  content::RenderFrameHost* main_devtools_rfh =
+      main_web_contents()->GetMainFrame();
+  content::RenderFrameHost* devtools_extension_devtools_rfh =
+      ChildFrameAt(main_devtools_rfh, 0);
+  content::RenderFrameHost* devtools_extension_panel_rfh =
+      ChildFrameAt(main_devtools_rfh, 1);
+  content::RenderFrameHost* devtools_extension_test_rfh =
+      ChildFrameAt(devtools_extension_panel_rfh, 0);
+
+  // all frames should be in the devtools process, including test.html's frame
+  EXPECT_TRUE(main_devtools_rfh->GetLastCommittedURL().SchemeIs(
+      content::kChromeDevToolsScheme));
+  EXPECT_EQ(extension->GetResourceURL("/devtools.html"),
+            devtools_extension_devtools_rfh->GetLastCommittedURL());
+  EXPECT_EQ(extension->GetResourceURL("/panel.html"),
+            devtools_extension_panel_rfh->GetLastCommittedURL());
+  EXPECT_EQ(extension_test_url,
+            devtools_extension_test_rfh->GetLastCommittedURL());
+  EXPECT_TRUE(main_devtools_rfh->GetSiteInstance()->GetSiteURL().SchemeIs(
+      content::kChromeDevToolsScheme));
+  EXPECT_EQ(main_devtools_rfh->GetSiteInstance(),
+            devtools_extension_devtools_rfh->GetSiteInstance());
+  EXPECT_EQ(main_devtools_rfh->GetSiteInstance(),
+            devtools_extension_panel_rfh->GetSiteInstance());
+  EXPECT_EQ(main_devtools_rfh->GetSiteInstance(),
+            devtools_extension_test_rfh->GetSiteInstance());
+}
+
+// TODO(davidsac):  Dear code reviewer, should I get rid of this test?  I am
+// strongly leaning towards it.  This tests a very trivial case.  If there is a
+// mistake in the logic, all of the rest of the tests will catch it as well.
+// Tests that a devtools Iframe can still be injected into devtools.
+IN_PROC_BROWSER_TEST_F(DevToolsExtensionTest, DevtoolsInDevTools) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+
+  GURL devtools_url = GURL("chrome-devtools://devtools/bundled/inspector.html");
+
+  OpenDevToolsWindow(kDebuggerTestPage, false);
+
+  std::string javascript =
+      "var devtoolsFrame = document.createElement('iframe');"
+      "document.body.appendChild(devtoolsFrame);"
+      "devtoolsFrame.setAttribute('src', '" +
+      devtools_url.spec() + "');";
+
+  content::RenderFrameHost* main_devtools_rfh =
+      main_web_contents()->GetMainFrame();
+
+  content::TestNavigationManager manager(main_web_contents(), devtools_url);
+
+  ASSERT_TRUE(content::ExecuteScript(main_devtools_rfh, javascript));
+
+  manager.WaitForNavigationFinished();
+  std::vector<content::RenderFrameHost*> rfhs =
+      main_web_contents()->GetAllFrames();
+  EXPECT_EQ(2U, rfhs.size());
+  content::RenderFrameHost* devtools_iframe_rfh =
+      ChildFrameAt(main_devtools_rfh, 0);
+  EXPECT_TRUE(main_devtools_rfh->GetLastCommittedURL().SchemeIs(
+      content::kChromeDevToolsScheme));
+  EXPECT_EQ(devtools_url, devtools_iframe_rfh->GetLastCommittedURL());
+  EXPECT_TRUE(main_devtools_rfh->GetSiteInstance()->GetSiteURL().SchemeIs(
+      content::kChromeDevToolsScheme));
+  EXPECT_EQ(main_devtools_rfh->GetSiteInstance(),
+            devtools_iframe_rfh->GetSiteInstance());
+}
+
+// TODO(davidsac):  Dear code reviewer, should I get rid of this test?  I am
+// strongly leaning towards it.  This is kinda tested even more toroguhly in the
+// |HttpIframeInDevToolsExtensionPanel| test. Tests that a blank Iframe can
+// still be injected into devtools.
+IN_PROC_BROWSER_TEST_F(DevToolsExtensionTest, AboutBlankInDevTools) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+
+  GURL about_blank_url = GURL(url::kAboutBlankURL);
+
+  OpenDevToolsWindow(kDebuggerTestPage, false);
+  std::string javascript =
+      "var blankFrame = document.createElement('iframe');"
+      "document.body.appendChild(blankFrame);"
+      "blankFrame.setAttribute('src', '" +
+      about_blank_url.spec() + "');";
+
+  content::RenderFrameHost* main_devtools_rfh =
+      main_web_contents()->GetMainFrame();
+
+  content::TestNavigationManager manager(main_web_contents(), about_blank_url);
+
+  ASSERT_TRUE(content::ExecuteScript(main_devtools_rfh, javascript));
+
+  manager.WaitForNavigationFinished();
+  std::vector<content::RenderFrameHost*> rfhs =
+      main_web_contents()->GetAllFrames();
+  EXPECT_EQ(2U, rfhs.size());
+  content::RenderFrameHost* about_blank_rfh =
+      ChildFrameAt(main_devtools_rfh, 0);
+  EXPECT_TRUE(main_devtools_rfh->GetLastCommittedURL().SchemeIs(
+      content::kChromeDevToolsScheme));
+  EXPECT_EQ(about_blank_url, about_blank_rfh->GetLastCommittedURL());
+  EXPECT_TRUE(main_devtools_rfh->GetSiteInstance()->GetSiteURL().SchemeIs(
+      content::kChromeDevToolsScheme));
+  EXPECT_EQ(main_devtools_rfh->GetSiteInstance(),
+            about_blank_rfh->GetSiteInstance());
 }
 
 // Some web features, when used from an extension, are subject to browser-side