Implement OOPIFs within Devtools Extensions

chrome/browser/devtools/devtools_sanity_browsertest.cc

 // Copyright (c) 2012 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 <stddef.h>
 
 #include <memory>
+#include <vector>
 
 #include "base/bind.h"
 #include "base/cancelable_callback.h"
 #include "base/command_line.h"
 #include "base/compiler_specific.h"
 #include "base/files/file_path.h"
 #include "base/location.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/memory/ref_counted.h"
@@ skipping 14 matching lines @@
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/test_extension_dir.h"
 #include "chrome/browser/extensions/unpacked_installer.h"
 #include "chrome/browser/lifetime/application_lifetime.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_commands.h"
 #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"
 #include "chrome/test/base/test_chrome_web_ui_controller_factory.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "components/app_modal/javascript_app_modal_dialog.h"
 #include "components/app_modal/native_app_modal_dialog.h"
 #include "components/prefs/pref_service.h"
 #include "content/public/browser/child_process_data.h"
 #include "content/public/browser/content_browser_client.h"
 #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"
 #include "content/public/browser/url_data_source.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_ui_controller.h"
 #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"
 #include "extensions/browser/extension_registry_observer.h"
 #include "extensions/browser/extension_system.h"
 #include "extensions/browser/notification_types.h"
 #include "extensions/browser/test_extension_registry_observer.h"
 #include "extensions/common/switches.h"
 #include "extensions/common/value_builder.h"
 #include "net/dns/mock_host_resolver.h"
@@ skipping 420 matching lines @@
     extensions::TestExtensionRegistryObserver observer(registry);
     extensions::UnpackedInstaller::Create(service)->Load(path);
     observer.WaitForExtensionLoaded();
 
     if (!WaitForExtensionViewsToLoad())
       return nullptr;
 
     return GetExtensionByPath(registry->enabled_extensions(), path);
   }
 
+  // Loads a dynamically generated extension populated with a bunch of test
+  // pages. |name| is the extension name to use in the manifest.
+  // |devtools_page|, if non-empty, indicates which test page should be be
+  // listed as a devtools_page in the manifest.  If |devtools_page| is empty, a
+  // non-devtools extension is created instead. |panel_iframe_src| controls the
+  // src= attribute of the <iframe> element in the 'panel.html' test page.
+  const Extension* LoadExtensionForTest(const std::string& name,
+                                        const std::string& devtools_page,
+                                        const std::string& panel_iframe_src) {
+    test_extension_dirs_.push_back(
+        base::MakeUnique<extensions::TestExtensionDir>());
+    extensions::TestExtensionDir* dir = test_extension_dirs_.back().get();
+
+    extensions::DictionaryBuilder manifest;
+    manifest.Set("name", name)
+        .Set("version", "1")
+        .Set("manifest_version", 2)
+        // simple_test_page.html is currently the only page referenced outside
+        // of its own extension in the tests
+        .Set("web_accessible_resources",
+             extensions::ListBuilder().Append("simple_test_page.html").Build());
+    if (!devtools_page.empty()) {

[alexmos, 2017/03/20 22:52:51]

nit: no {}

[davidsac (gone - try alexmos), 2017/03/23 01:22:34]

Done.

+      manifest.Set("devtools_page", devtools_page);
+    }
+
+    dir->WriteManifest(manifest.ToJSON());
+
+    GURL http_popup_url =

[alexmos, 2017/03/20 22:52:50]

Since you're using it from an <iframe>, this isn't really a popup.  Perhaps
rename to http_frame_url?

[davidsac (gone - try alexmos), 2017/03/23 01:22:35]

Done.

+        embedded_test_server()->GetURL("a.com", "/popup_iframe.html");
+
+    dir->WriteFile(FILE_PATH_LITERAL("web_devtools_page.html"),

[alexmos, 2017/03/20 22:52:51]

Maybe add a comment before this saying that only one of these devtools pages
will ends up being used, and different tests use |devtools_page| to specify
which one.

[davidsac (gone - try alexmos), 2017/03/23 01:22:36]

Done.

+                   "<html><head></head><body><iframe src='" +

[alexmos, 2017/03/20 22:52:50]

nit: no need for <head></head>, here and similarly below

[davidsac (gone - try alexmos), 2017/03/23 01:22:34]

Done.

+                       http_popup_url.spec() + "'></iframe></body></html>");
+
+    dir->WriteFile(FILE_PATH_LITERAL("simple_devtools_page.html"),
+                   "<html><head></head><body></body></html>");
+
+    dir->WriteFile(
+        FILE_PATH_LITERAL("panel_devtools_page.html"),
+        "<html><head><script "
+        "src='panel_devtools_page.js'></script></head><body></body></html>");
+
+    dir->WriteFile(
+        FILE_PATH_LITERAL("panel_devtools_page.js"),
+        "chrome.devtools.panels.create('iframe_panel',\n"
+        "    null,\n"
+        "    'panel.html',\n"
+        "    function(panel) {\n"
+        "      "
+        "chrome.devtools.inspectedWindow.eval('console.log("

[alexmos, 2017/03/20 22:52:51]

nit: fix the indent here and remove the preceding blank line.  Same for indent
in createSidebarPane

[davidsac (gone - try alexmos), 2017/03/23 01:22:36]

Done.

+        "\"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='" + panel_iframe_src +
+                       "'></iframe></body></html>");
+
+    dir->WriteFile(FILE_PATH_LITERAL("simple_test_page.html"),
+                   "<html><head></head><body>This is a test</body></html>");
+
+    GURL about_blank_url = GURL(url::kAboutBlankURL);
+    GURL web_url = embedded_test_server()->GetURL("a.com", "/title3.html");
+    GURL data_url = GURL("data:text/html,foo");
+
+    dir->WriteFile(FILE_PATH_LITERAL("renavigation_test_page.html"),

[alexmos, 2017/03/20 22:52:50]

In the tests, this seems to be used for a lot more than renavigation.  Perhaps
use a more generic name, e.g., page_with_three_frames?  

[davidsac (gone - try alexmos), 2017/03/23 01:22:35]

Done.

+                   "<html><body><iframe src='" + about_blank_url.spec() +
+                       "'></iframe><iframe src='" + data_url.spec() +
+                       "'></iframe><iframe src='" + web_url.spec() +
+                       "'></iframe></body></html>");
+
+    // Install the extension.
+    const Extension* extension = LoadExtensionFromPath(dir->UnpackedPath());
+    return extension;
+  }
+
  private:
   const Extension* GetExtensionByPath(
       const extensions::ExtensionSet& extensions,
       const base::FilePath& path) {
     base::FilePath extension_path = base::MakeAbsoluteFilePath(path);
     EXPECT_TRUE(!extension_path.empty());
     for (const scoped_refptr<const Extension>& extension : extensions) {
       if (extension->path() == extension_path) {
         return extension.get();
       }
@@ skipping 32 matching lines @@
   }
 
   void Observe(int type,
                const content::NotificationSource& source,
                const content::NotificationDetails& details) override {
     DCHECK_EQ(extensions::NOTIFICATION_EXTENSION_HOST_DID_STOP_FIRST_LOAD,
               type);
     base::MessageLoopForUI::current()->QuitWhenIdle();
   }
 
+  std::vector<std::unique_ptr<extensions::TestExtensionDir>>
+      test_extension_dirs_;
   base::FilePath test_extensions_dir_;
 };
 
 class DevToolsExperimentalExtensionTest : public DevToolsExtensionTest {
  public:
   void SetUpCommandLine(base::CommandLine* command_line) override {
     command_line->AppendSwitch(
         extensions::switches::kEnableExperimentalExtensionApis);
   }
 };
@@ skipping 359 matching lines @@
           kDebuggerTestPage);
 }
 
 // Tests that chrome.devtools extension is correctly exposed.
 IN_PROC_BROWSER_TEST_F(DevToolsExtensionTest,
                        TestDevToolsExtensionAPI) {
   LoadExtension("devtools_extension");
   RunTest("waitForTestResultsInConsole", std::string());
 }
 
-// Tests a chrome.devtools extension panel that embeds an http:// iframe.
-IN_PROC_BROWSER_TEST_F(DevToolsExtensionTest, DevToolsExtensionWithHttpIframe) {
-  ASSERT_TRUE(embedded_test_server()->Start());
-
-  // Our extension must load an 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;
-  dir->WriteManifest(extensions::DictionaryBuilder()
-                         .Set("name", "Devtools Panel w/ HTTP Iframe")
-                         .Set("version", "1")
-                         .Set("manifest_version", 2)
-                         .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.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");
-
-  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>");
-
-  // Install the extension.
-  const Extension* extension = LoadExtensionFromPath(dir->UnpackedPath());
+// 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.  This is tested because this is one of the
+// extension pages with devtools access
+// (https://developer.chrome.com/extensions/devtools).  Also tests that data

[alexmos, 2017/03/20 22:52:50]

nit: insert "iframes with " before data.

[davidsac (gone - try alexmos), 2017/03/23 01:22:34]

Done.

+// URLs and about blank URLs are rendered in the devtools process, unless an

[alexmos, 2017/03/20 22:52:51]

nit: add ":" in about:blank

[davidsac (gone - try alexmos), 2017/03/23 01:22:35]

Done.

+// OOPIF is navigated to about:blank, in which case it does not end up back in

[alexmos, 2017/03/20 22:52:50]

nit: "an OOPIF is navigated" -> "a web OOPIF navigates itself"
(OOPIF does not imply being a web iframe; also, we don't want ambiguity about
who's navigating the frame and inheriting the initiator's origin, which this
test isn't concerned with)

[davidsac (gone - try alexmos), 2017/03/23 01:22:34]

Done.

+// the devtools process.  Also tests that when a web IFrame is navigated back to
+// a devtools extension page, it gets put back in the devtools process.
+// http://crbug.com/570483
+IN_PROC_BROWSER_TEST_F(DevToolsExtensionTest,
+                       HttpIframeInDevToolsExtensionPanel) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+
+  // Install the dynamically-generated extension.
+  const Extension* extension =
+      LoadExtensionForTest("Devtools Extension", "panel_devtools_page.html",
+                           "/renavigation_test_page.html");
   ASSERT_TRUE(extension);
 
-  // Open a devtools window.
   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
+  // Now that we know the panel is loaded, switch to it.
+  SwitchToExtensionPanel(window_, extension, "iframe_panel");
+  content::WaitForLoadStop(main_web_contents());
+
+  std::vector<content::RenderFrameHost*> rfhs =

[alexmos, 2017/03/20 22:52:50]

Given the complexity of this test, it might help with readability if you include
a short comment that outlines the structure of the whole page?  It could include
something like this: 
- DevTools
  - devtools_page from DevTools extension
  - Panel (DevTools extension)
    - iframe (DevTools extension)
      - about:blank
      - data:
      - web URL

[davidsac (gone - try alexmos), 2017/03/23 01:22:36]

Done.

+      main_web_contents()->GetAllFrames();
+  EXPECT_EQ(7U, rfhs.size());

[alexmos, 2017/03/20 22:52:50]

Sanity check: it seems for this test, we'll also install a sidebar pane that
points to renavigation_test_page.html, so it will also have the three iframes. 
Are we relying on the fact that the sidebar pane isn't shown until you switch to
the elements panel?

[davidsac (gone - try alexmos), 2017/03/23 01:22:36]

Correct.  They're there, but they don't load.  Should I add a comment about it? 
I feel like it's mentioned in a couple other places already, but redundancy
might not hurt.

+
+  content::RenderFrameHost* main_devtools_rfh =

[alexmos, 2017/03/20 22:52:50]

nit: given that it's used so often, you could just put a using
content::RenderFrameHost on top (this file already does this for RenderViewHost
and a bunch of others, so it'll fit right in), and then drop "content::" from
RenderFrameHost* everywhere.

[davidsac (gone - try alexmos), 2017/03/23 01:22:36]

Done.

+      main_web_contents()->GetMainFrame();
+  content::RenderFrameHost* devtools_extension_devtools_rfh =

[alexmos, 2017/03/20 22:52:50]

This is for the "devtools_page" from the manifest, so perhaps the name should
have devtools_page?

[davidsac (gone - try alexmos), 2017/03/23 01:22:34]

Done.

+      ChildFrameAt(main_devtools_rfh, 0);
+  content::RenderFrameHost* devtools_panel_extension_rfh =

[davidsac (gone - try alexmos), 2017/03/23 01:22:36]

renamed to devtool_extension_panel_rfh

+      ChildFrameAt(main_devtools_rfh, 1);
+  content::RenderFrameHost* test_frame_rfh =

[alexmos, 2017/03/20 22:52:50]

I'd name this panel_frame_rfh

[davidsac (gone - try alexmos), 2017/03/23 01:22:35]

Done.

+      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 =

[alexmos, 2017/03/20 22:52:51]

Why not just web_frame_rfh?

[davidsac (gone - try alexmos), 2017/03/23 01:22:36]

Done.

+      ChildFrameAt(test_frame_rfh, 2);
+
+  GURL web_url = embedded_test_server()->GetURL("a.com", "/title3.html");
+  GURL about_blank_url = GURL(url::kAboutBlankURL);
+  GURL data_url = GURL("data:text/html,foo");
+
+  EXPECT_TRUE(main_devtools_rfh->GetLastCommittedURL().SchemeIs(
+      content::kChromeDevToolsScheme));
+  EXPECT_EQ(extension->GetResourceURL("/panel_devtools_page.html"),
+            devtools_extension_devtools_rfh->GetLastCommittedURL());
+  EXPECT_EQ(extension->GetResourceURL("/panel.html"),
+            devtools_panel_extension_rfh->GetLastCommittedURL());
+  EXPECT_EQ(extension->GetResourceURL("/renavigation_test_page.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(web_url, about_blank_http_iframe_rfh->GetLastCommittedURL());
+
+  EXPECT_TRUE(main_devtools_rfh->GetSiteInstance()->GetSiteURL().SchemeIs(

[alexmos, 2017/03/20 22:52:50]

nit: you could declare 
  SiteInstance* devtools_instance = main_devtools_rfh->GetSiteInstance();
and then use that everywhere below for a bit of extra clarify and to save a
bunch of space.

[davidsac (gone - try alexmos), 2017/03/23 01:22:34]

Done.

+      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(
+      web_url.host(),
+      about_blank_http_iframe_rfh->GetSiteInstance()->GetSiteURL().host());

[alexmos, 2017/03/20 22:52:50]

I'd add an explicit EXPECT_NE(devtools_instance,
about_blank_http_iframe_rfh->GetSiteInstance()) 
(and similarly in all other tests)

[alexmos, 2017/03/20 22:52:51]

Too bad that DepictFrameTree is internal to content/.  It would've been so nice
to use it here instead of all the GetSiteInstance checks.  Perhaps we should
consider exposing it to tests outside content/ (certainly not for  this CL).

[davidsac (gone - try alexmos), 2017/03/23 01:22:34]

Acknowledged.

[davidsac (gone - try alexmos), 2017/03/23 01:22:35]

Done.

+
+  // Check that if an OOPIF navigates itself to about:blank, it stays an OOPIF.

[alexmos, 2017/03/20 22:52:50]

nit: Check that if the web iframe navigates itself to about:blank, it stays in
the web SiteInstance.

[davidsac (gone - try alexmos), 2017/03/23 01:22:34]

Done.

+
+  std::string about_blank_javascript =
+      "location.href='" + about_blank_url.spec() + "';";
+
+  content::TestNavigationManager web_about_blank_manager(main_web_contents(),
+                                                         about_blank_url);
+
+  ASSERT_TRUE(content::ExecuteScript(about_blank_http_iframe_rfh,
+                                     about_blank_javascript));
+
+  web_about_blank_manager.WaitForNavigationFinished();
+
+  EXPECT_EQ(about_blank_url,
+            about_blank_http_iframe_rfh->GetLastCommittedURL());

[alexmos, 2017/03/20 22:52:51]

Just a note that if we were to screw up the SiteInstance determination logic and
end up swapping processes, we'd have a new RFH and this one would be pending
deletion or already gone, so you might crash here.  I guess it's ok to fail like
this (crash if the RFH is gone, and if it isn't, the last committed URL will
catch the error).

[davidsac (gone - try alexmos), 2017/03/23 01:22:36]

Good thing to consider!  That is a good point though, I should be more conscious
that if code changes and breaks, that my tests actually CATCH the breaking
change and fail when necessary.  I'm glad we dodged the bullet on it this time,
though.

+  EXPECT_EQ(
+      web_url.host(),
+      about_blank_http_iframe_rfh->GetSiteInstance()->GetSiteURL().host());
+
+  // Check that when a web IFrame is navigated back to a devtools extension
+  // page, it gets put back in the devtools process.
+
+  GURL extension_simple_url =
+      extension->GetResourceURL("/simple_test_page.html");
+  std::string renavigation_javascript =
+      "location.href='" + extension_simple_url.spec() + "';";
+
+  content::TestNavigationManager renavigation_manager(main_web_contents(),
+                                                      extension_simple_url);
+
+  ASSERT_TRUE(content::ExecuteScript(about_blank_http_iframe_rfh,
+                                     renavigation_javascript));
+
+  renavigation_manager.WaitForNavigationFinished();
+
+  // The old RFH is no longer valid after the renavigation, so we must get the
+  // new one.
+  content::RenderFrameHost* extension_simple_frame_rfh =
+      ChildFrameAt(test_frame_rfh, 2);
+
+  EXPECT_EQ(extension_simple_url,
+            extension_simple_frame_rfh->GetLastCommittedURL());
+  EXPECT_EQ(main_devtools_rfh->GetSiteInstance(),
+            extension_simple_frame_rfh->GetSiteInstance());
+}
+
+// Tests that http Iframes within the sidebar pane page for the devtools
+// extension that is visible in the elements panel are rendered in their own
+// processes and not in the devtools process or the extension's process.  This
+// is tested because this is one of the extension pages with devtools access
+// (https://developer.chrome.com/extensions/devtools).  http://crbug.com/570483
+IN_PROC_BROWSER_TEST_F(DevToolsExtensionTest,
+                       HttpIframeInDevToolsExtensionSideBarPane) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+
+  GURL web_url = embedded_test_server()->GetURL("a.com", "/title3.html");
+
+  // Install the dynamically-generated extension.
+  const Extension* extension = LoadExtensionForTest(
+      "Devtools Extension", "panel_devtools_page.html", web_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.

[alexmos, 2017/03/20 22:52:51]

nit: "it" should be "the sidebar pane in the elements panel", and "panel" should
be "extension", right?  I.e., the previous function waits for the wrong panel,
and the only reason to still call it is to wait for the devtools extension
itself to load.

[davidsac (gone - try alexmos), 2017/03/23 01:22:35]

Ok, I tried to fix this the best that I could.  The purpose of
"RunTestFunction(window_, "waitForTestResultsInConsole");" is to make sure that
the javascript from the devtools page finishes running and that the placeholders
for the panel or sidebarpane exist, so that they can be successfully switched
to.  In this patch set, they were always loaded together and gave the same
signal when they were finished loading.  Because I thought that this was a
potential race condition, that has been fixed.  They are now loaded separately
always, and I have adjusted the comments accordingly.  

[alexmos, 2017/03/23 17:15:07]

Yes, I agree with your race condition concerns, and separating them out into two
devtools_pages seems much better.  Thanks for addressing this!

+  content::TestNavigationManager web_manager(main_web_contents(), web_url);
+  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.
+  SwitchToPanel(window_, "iframe_pane");
+  web_manager.WaitForNavigationFinished();
+
+  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("/panel_devtools_page.html"),
+            devtools_extension_devtools_rfh->GetLastCommittedURL());
+  EXPECT_EQ(extension->GetResourceURL("/panel.html"),
+            devtools_sidebar_pane_extension_rfh->GetLastCommittedURL());
+  EXPECT_EQ(web_url, http_iframe_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_sidebar_pane_extension_rfh->GetSiteInstance());
+  EXPECT_EQ(web_url.host(),
+            http_iframe_rfh->GetSiteInstance()->GetSiteURL().host());
+}
+
+// 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.  This
+// is tested because this is one of the extension pages with devtools access
+// (https://developer.chrome.com/extensions/devtools).  http://crbug.com/570483
+IN_PROC_BROWSER_TEST_F(DevToolsExtensionTest,
+                       HttpIframeInDevToolsExtensionDevtools) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+
+  // Install the dynamically-generated extension.
+  const Extension* extension =
+      LoadExtensionForTest("Devtools Extension", "web_devtools_page.html",
+                           "" /* panel_iframe_src */);
+  ASSERT_TRUE(extension);
+
+  // Wait for a 'DONE' message sent from popup_iframe.html, indicating that it
   // loaded successfully.
   content::DOMMessageQueue message_queue;
-  SwitchToExtensionPanel(window_, extension, "iframe_panel");
   std::string message;
+  OpenDevToolsWindow(kDebuggerTestPage, false);
+
   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);
+
+  GURL web_url = embedded_test_server()->GetURL("a.com", "/popup_iframe.html");
+
+  EXPECT_TRUE(main_devtools_rfh->GetLastCommittedURL().SchemeIs(
+      content::kChromeDevToolsScheme));
+  EXPECT_EQ(extension->GetResourceURL("/web_devtools_page.html"),
+            devtools_extension_devtools_rfh->GetLastCommittedURL());
+  EXPECT_EQ(web_url, http_iframe_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(web_url.host(),
+            http_iframe_rfh->GetSiteInstance()->GetSiteURL().host());
+}
+
+// Tests that iframes to a non-devtools extension embedded in a devtools
+// extension will be isolated from devtools and the devtools extension.
+// http://crbug.com/570483
+IN_PROC_BROWSER_TEST_F(DevToolsExtensionTest,
+                       NonDevToolsExtensionInDevToolsExtension) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+
+  // Install the dynamically-generated non-devtools extension.
+  const Extension* non_devtools_extension =
+      LoadExtensionForTest("Non-DevTools Extension", "" /* devtools_page */,
+                           "" /* panel_iframe_src */);
+  ASSERT_TRUE(non_devtools_extension);
+
+  GURL non_dt_extension_test_url =
+      non_devtools_extension->GetResourceURL("/simple_test_page.html");
+
+  // Install the dynamically-generated devtools extension.
+  const Extension* devtools_extension =
+      LoadExtensionForTest("Devtools Extension", "panel_devtools_page.html",
+                           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.
+  content::TestNavigationManager non_devtools_manager(
+      main_web_contents(), non_dt_extension_test_url);
+  SwitchToExtensionPanel(window_, devtools_extension, "iframe_panel");
+  non_devtools_manager.WaitForNavigationFinished();
+
+  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("/panel_devtools_page.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());
+
+  // popup_test_page.html's frame should be in extension b's

[alexmos, 2017/03/20 22:52:51]

Wrong URL?  popup_test_page.html isn't used anywhere

[davidsac (gone - try alexmos), 2017/03/23 01:22:35]

Done.

+  // process, not in devtools or extension a's process.

[alexmos, 2017/03/20 22:52:51]

nit: "extension a" -> |devtools_extension|,
"extension b" -> |non_devtools_extension|

[davidsac (gone - try alexmos), 2017/03/23 01:22:36]

Done.

+  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());
+}
+
+// Tests that if a devtools extension's devtools panel page has a subframe to a
+// page for another devtools extension, the subframe is rendered in the devtools
+// process as well.  http://crbug.com/570483
+IN_PROC_BROWSER_TEST_F(DevToolsExtensionTest,
+                       DevToolsExtensionInDevToolsExtension) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+
+  // Install the dynamically-generated extension.
+  const Extension* devtools_b_extension =
+      LoadExtensionForTest("Devtools Extension B", "simple_devtools_page.html",
+                           "" /* panel_iframe_src */);
+  ASSERT_TRUE(devtools_b_extension);
+
+  GURL extension_b_test_url =
+      devtools_b_extension->GetResourceURL("/simple_test_page.html");
+
+  // Install the dynamically-generated extension.

[alexmos, 2017/03/20 22:52:50]

I'd add a comment that this extension's panel iframe will point to an extension
b URL, just to help quickly grasp how this test works.

[davidsac (gone - try alexmos), 2017/03/23 01:22:35]

Done.

+  const Extension* devtools_a_extension =
+      LoadExtensionForTest("Devtools Extension A", "panel_devtools_page.html",
+                           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.
+  content::TestNavigationManager extension_b_manager(main_web_contents(),
+                                                     extension_b_test_url);
+  SwitchToExtensionPanel(window_, devtools_a_extension, "iframe_panel");
+  extension_b_manager.WaitForNavigationFinished();
+
+  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 =
+      content::FrameMatchingPredicate(
+          main_web_contents(), base::Bind(&content::FrameHasSourceUrl,
+                                          devtools_a_extension->GetResourceURL(
+                                              "/panel_devtools_page.html")));
+  EXPECT_TRUE(devtools_extension_a_devtools_rfh);
+  content::RenderFrameHost* devtools_extension_b_devtools_rfh =
+      content::FrameMatchingPredicate(
+          main_web_contents(), base::Bind(&content::FrameHasSourceUrl,
+                                          devtools_b_extension->GetResourceURL(
+                                              "/simple_devtools_page.html")));
+  EXPECT_TRUE(devtools_extension_b_devtools_rfh);
+
+  content::RenderFrameHost* devtools_extension_a_panel_rfh =
+      ChildFrameAt(main_devtools_rfh, 2);
+  content::RenderFrameHost* devtools_extension_b_test_rfh =

[alexmos, 2017/03/20 22:52:51]

nit: I'd replace "test" with "frame", as "test" is too ambiguous.

[davidsac (gone - try alexmos), 2017/03/23 01:22:35]

Done.

+      ChildFrameAt(devtools_extension_a_panel_rfh, 0);
+
+  EXPECT_TRUE(main_devtools_rfh->GetLastCommittedURL().SchemeIs(
+      content::kChromeDevToolsScheme));
+  EXPECT_EQ(devtools_a_extension->GetResourceURL("/panel_devtools_page.html"),
+            devtools_extension_a_devtools_rfh->GetLastCommittedURL());
+  EXPECT_EQ(devtools_b_extension->GetResourceURL("/simple_devtools_page.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.  http://crbug.com/570483
+IN_PROC_BROWSER_TEST_F(DevToolsExtensionTest, DevToolsExtensionInItself) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+
+  // Install the dynamically-generated extension.
+  const Extension* extension =
+      LoadExtensionForTest("Devtools Extension", "panel_devtools_page.html",
+                           "/simple_test_page.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.
+  GURL extension_test_url = extension->GetResourceURL("/simple_test_page.html");
+  content::TestNavigationManager test_page_manager(main_web_contents(),
+                                                   extension_test_url);
+  SwitchToExtensionPanel(window_, extension, "iframe_panel");
+  test_page_manager.WaitForNavigationFinished();
+
+  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 =

[alexmos, 2017/03/20 22:52:51]

nit: test -> panel_frame or something similar

[davidsac (gone - try alexmos), 2017/03/23 01:22:36]

Done.

+      ChildFrameAt(devtools_extension_panel_rfh, 0);
+
+  // all frames should be in the devtools process, including test.html's frame

[alexmos, 2017/03/20 22:52:50]

Did you mean panel instead of test.html?

[davidsac (gone - try alexmos), 2017/03/23 01:22:35]

I actually meant |simple_test_page.html|.  Does that sound ok?

[alexmos, 2017/03/23 17:15:07]

simple_test_page.html doesn't have any frames, so I don't think you want to say
"simple_test_page.html's frame".  Probably just simple_test_page.html instead?

[davidsac (gone - try alexmos), 2017/03/24 23:16:14]

Done.

+  EXPECT_TRUE(main_devtools_rfh->GetLastCommittedURL().SchemeIs(
+      content::kChromeDevToolsScheme));
+  EXPECT_EQ(extension->GetResourceURL("/panel_devtools_page.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());
+}
+
+// Tests that a devtools (not a devtools extension) Iframe can be injected into
+// devtools.  http://crbug.com/570483
+IN_PROC_BROWSER_TEST_F(DevToolsSanityTest, DevtoolsInDevTools) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+
+  GURL devtools_url = GURL("chrome-devtools://devtools/bundled/inspector.html");

[alexmos, 2017/03/20 22:52:51]

Can this use kChromeUIDevToolsURL?

[davidsac (gone - try alexmos), 2017/03/23 01:22:35]

Done.

+
+  OpenDevToolsWindow(kDebuggerTestPage, false);
+
+  std::string javascript =
+      "var devtoolsFrame = document.createElement('iframe');"
+      "document.body.appendChild(devtoolsFrame);"
+      "devtoolsFrame.setAttribute('src', '" +

[alexmos, 2017/03/20 22:52:51]

nit: can shorten to just devtoolsFrame.src = ...

[davidsac (gone - try alexmos), 2017/03/23 01:22:35]

Done.

+      devtools_url.spec() + "');";
+
+  content::RenderFrameHost* main_devtools_rfh =
+      main_web_contents()->GetMainFrame();
+
+  content::TestNavigationManager manager(main_web_contents(), devtools_url);
+

[alexmos, 2017/03/20 22:52:50]

nit: I'd group the ExecuteScript in one block with manager declaration and
manager.Wait...(), and put a blank line after that instead of here, so it's
clear that this is all for this one navigation.

[davidsac (gone - try alexmos), 2017/03/23 01:22:35]

Done.  Good call, I did that in other tests.  I don't know why I spaced out on
doing it for this one.

+  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());
+
+  std::string message;
+
+  EXPECT_TRUE(ExecuteScriptAndExtractString(
+      devtools_iframe_rfh, "domAutomationController.send(document.origin)",
+      &message));
+  EXPECT_EQ("chrome-devtools://devtools", message);

[alexmos, 2017/03/20 22:52:51]

If you end up using kChromeUIDevToolsURL above, you could get the origin from
that instead of hardcoding it here.

[davidsac (gone - try alexmos), 2017/03/23 01:22:36]

Done.  I had to add a slash, though.

 }
 
 // Some web features, when used from an extension, are subject to browser-side
 // security policy enforcement. Make sure they work properly from inside a
 // devtools extension.
 IN_PROC_BROWSER_TEST_F(DevToolsExtensionTest,
                        DevToolsExtensionSecurityPolicyGrants) {
   ASSERT_TRUE(embedded_test_server()->Start());
 
   std::unique_ptr<extensions::TestExtensionDir> dir(
@@ skipping 589 matching lines @@
   RunTestFunction(window, "testWindowInitializedOnNavigateBack");
 
   DevToolsWindowTesting::CloseDevToolsWindowSync(window);
   content::WebUIControllerFactory::UnregisterFactoryForTesting(&test_factory);
 }
 
 // Tests scripts panel showing.
 IN_PROC_BROWSER_TEST_F(DevToolsSanityTest, TestDevToolsSharedWorker) {
   RunTest("testDevToolsSharedWorker", url::kAboutBlankURL);
 }