Track all extension frames in ProcessManager, inspect extensionoptions

chrome/browser/extensions/process_manager_browsertest.cc

Index: chrome/browser/extensions/process_manager_browsertest.cc
diff --git a/chrome/browser/extensions/process_manager_browsertest.cc b/chrome/browser/extensions/process_manager_browsertest.cc
index 980318c77b803ca623a644153d87d3799c12e521..99b1f417c870adf173115c3beac6c11e6f2f51c9 100644
--- a/chrome/browser/extensions/process_manager_browsertest.cc
+++ b/chrome/browser/extensions/process_manager_browsertest.cc
@@ -4,15 +4,23 @@
 
 #include "extensions/browser/process_manager.h"
 
+#include "base/strings/stringprintf.h"
 #include "chrome/browser/extensions/browser_action_test_util.h"
 #include "chrome/browser/extensions/extension_browsertest.h"
 #include "chrome/browser/extensions/extension_service.h"
+#include "chrome/browser/extensions/test_extension_dir.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/common/extensions/extension_process_policy.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "content/public/browser/notification_service.h"
+#include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/web_contents.h"
+#include "content/public/test/browser_test_utils.h"
 #include "content/public/test/test_utils.h"
+#include "extensions/common/value_builder.h"
+#include "extensions/test/background_page_watcher.h"
+#include "extensions/test/extension_test_message_listener.h"
 #include "net/dns/mock_host_resolver.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
 
@@ -20,7 +28,77 @@ namespace extensions {
 
 // Exists as a browser test because ExtensionHosts are hard to create without
 // a real browser.
-typedef ExtensionBrowserTest ProcessManagerBrowserTest;
+class ProcessManagerBrowserTest : public ExtensionBrowserTest {
+ public:
+  // Create an extension with web-accessible frames and an optional background
+  // page.
+  const Extension* CreateExtension(const std::string& name,
+                                   bool has_background_process) {
+    scoped_ptr<TestExtensionDir> dir(new TestExtensionDir);
+
+    DictionaryBuilder manifest;
+    manifest.Set("name", name)
+        .Set("version", "1")
+        .Set("manifest_version", 2)
+        // To allow ExecuteScript* to work.
+        .Set("content_security_policy",
+             "script-src 'self' 'unsafe-eval'; object-src 'self'")
+        .Set("web_accessible_resources", ListBuilder().Append("*"));
+
+    if (has_background_process) {
+      manifest.Set("background", DictionaryBuilder().Set("page", "bg.html"));
+      dir->WriteFile(FILE_PATH_LITERAL("bg.html"),
+                     "<iframe id=bgframe src=empty.html></iframe>");
+    }
+
+    dir->WriteFile(FILE_PATH_LITERAL("blank_iframe.html"),
+                   "<iframe id=frame0 src='about:blank'></iframe>");
+
+    dir->WriteFile(FILE_PATH_LITERAL("srcdoc_iframe.html"),
+                   "<iframe id=frame0 srcdoc='Hello world'></iframe>");
+
+    dir->WriteFile(FILE_PATH_LITERAL("two_iframes.html"),
+                   "<iframe id=frame1 src=empty.html></iframe>"
+                   "<iframe id=frame2 src=empty.html></iframe>");
+
+    dir->WriteFile(FILE_PATH_LITERAL("empty.html"), "");
+
+    dir->WriteManifest(manifest.ToJSON());
+
+    const Extension* extension = LoadExtension(dir->unpacked_path());
+    EXPECT_TRUE(extension);
+    temp_dirs_.push_back(dir.release());
+    return extension;
+  }
+
+  void NavigateIframeToURLAndWait(content::WebContents* web_contents,
+                                  const std::string iframe_id,
+                                  const GURL& url) {
+    // This is an improved version of content::NavigateIframeToURL. Unlike the
+    // other method, this does actually wait until the load of all child frames
+    // completes.
+    std::string script = base::StringPrintf(
+      "var frame = document.getElementById('%s');"
+      "frame.onload = frame.onerror = function(event) {"
+      "  frame.onload = frame.onerror = null;"
+      "  domAutomationController.send(event.type === 'load');"
+      "};"
+      "frame.src = '%s';",
+      iframe_id.c_str(), url.spec().c_str());
+    bool is_loaded = false;
+    EXPECT_TRUE(ExecuteScriptAndExtractBool(web_contents, script, &is_loaded));
+    EXPECT_TRUE(is_loaded);

[ncarter (slow), 2015/11/24 05:22:13]

I had a chat with creis, and we'd probably be fine replacing
content::NavigateIframeToURL with this implementation. Feel free to pursue that.

[robwu, 2015/11/25 00:44:16]

I'll do that in a separate CL to make sure that bisecting/debugging gets easier
in case the new content::NavigateIframeToURL implementation introduces
unexpected changes.

+  }
+
+  size_t IfExtensionsIsolated(size_t if_enabled, size_t if_disabled) {
+    return content::AreAllSitesIsolatedForTesting() ||
+        IsIsolateExtensionsEnabled() ? if_enabled : if_disabled;
+  }
+
+ private:
+  ScopedVector<TestExtensionDir> temp_dirs_;

[ncarter (slow), 2015/11/24 05:22:13]

std::vector<scoped_ptr<TestExtensionDir>>

Since we're trying to deprecate ScopedVector as of a week or two ago.

[robwu, 2015/11/25 00:44:17]

Done.

+

[ncarter (slow), 2015/11/24 05:22:13]

nit: no blank line

[robwu, 2015/11/25 00:44:16]

Done.

+};
 
 // Test that basic extension loading creates the appropriate ExtensionHosts
 // and background pages.
@@ -157,6 +235,173 @@ IN_PROC_BROWSER_TEST_F(ProcessManagerBrowserTest, HttpHostMatchingExtensionId) {
   EXPECT_TRUE(pm->GetBackgroundHostForExtension(extension->id()));
 }
 
+IN_PROC_BROWSER_TEST_F(ProcessManagerBrowserTest, NoBackgroundPage) {
+  ASSERT_TRUE(embedded_test_server()->InitializeAndWaitUntilReady());
+
+  ProcessManager* pm = ProcessManager::Get(profile());
+  const Extension* extension =
+      LoadExtension(test_data_dir_.AppendASCII("api_test")
+                    .AppendASCII("messaging")
+                    .AppendASCII("connect_nobackground"));
+  ASSERT_TRUE(extension);
+
+  // The extension has no background page.
+  EXPECT_EQ(0u, pm->GetRenderFrameHostsForExtension(extension->id()).size());
+
+  // Start in a non-extension process, then navigate to an extension process.
+  ui_test_utils::NavigateToURL(browser(),
+                               embedded_test_server()->GetURL("/empty.html"));
+  EXPECT_EQ(0u, pm->GetRenderFrameHostsForExtension(extension->id()).size());
+
+  const GURL extension_url = extension->url().Resolve("manifest.json");
+  ui_test_utils::NavigateToURL(browser(), extension_url);
+  EXPECT_EQ(1u, pm->GetRenderFrameHostsForExtension(extension->id()).size());
+
+  ui_test_utils::NavigateToURL(browser(), GURL("about:blank"));
+  EXPECT_EQ(0u, pm->GetRenderFrameHostsForExtension(extension->id()).size());
+
+  ui_test_utils::NavigateToURLWithDisposition(
+      browser(), extension_url, NEW_FOREGROUND_TAB,
+      ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);
+  EXPECT_EQ(1u, pm->GetRenderFrameHostsForExtension(extension->id()).size());
+}
+
+// Tests whether frames are correctly classified. Non-extension frames should
+// never appear in the list. Top-level extension frames should always appear.
+// Child extension frames should only appear if it is hosted in an extension
+// process (i.e. if the top-level frame is an extension page, or if OOP frames
+// are enabled for extensions).
+IN_PROC_BROWSER_TEST_F(ProcessManagerBrowserTest, FrameClassification) {
+  const Extension* extension1 = CreateExtension("Extension 1", false);
+  const Extension* extension2 = CreateExtension("Extension 2", true);
+  embedded_test_server()->ServeFilesFromDirectory(extension1->path());
+  ASSERT_TRUE(embedded_test_server()->InitializeAndWaitUntilReady());
+
+  const GURL ext1_parent_url(extension1->url().Resolve("two_iframes.html"));
+  const GURL ext1_empty_url(extension1->url().Resolve("empty.html"));
+  const GURL ext2_parent_url(extension2->url().Resolve("two_iframes.html"));
+  const GURL ext2_empty_url(extension2->url().Resolve("empty.html"));
+
+  ProcessManager* pm = ProcessManager::Get(profile());
+  BackgroundPageWatcher(pm, extension2).WaitForOpen();
+
+  EXPECT_EQ(2u, pm->GetAllFrames().size());
+  EXPECT_EQ(0u, pm->GetRenderFrameHostsForExtension(extension1->id()).size());
+  EXPECT_EQ(2u, pm->GetRenderFrameHostsForExtension(extension2->id()).size());
+
+  ExecuteScriptInBackgroundPageNoWait(extension2->id(),
+                                      "setTimeout(window.close, 0)");
+  BackgroundPageWatcher(pm, extension2).WaitForClose();
+  EXPECT_EQ(0u, pm->GetAllFrames().size());
+  EXPECT_EQ(0u, pm->GetRenderFrameHostsForExtension(extension2->id()).size());
+
+  ui_test_utils::NavigateToURL(
+      browser(), embedded_test_server()->GetURL("/two_iframes.html"));
+  EXPECT_EQ(0u, pm->GetAllFrames().size());
+
+  content::WebContents* tab =
+      browser()->tab_strip_model()->GetActiveWebContents();
+
+  // Tests extension frames in non-extension page.
+  NavigateIframeToURLAndWait(tab, "frame1", ext1_empty_url);
+  EXPECT_EQ(IfExtensionsIsolated(1, 0),
+            pm->GetRenderFrameHostsForExtension(extension1->id()).size());
+  EXPECT_EQ(IfExtensionsIsolated(1, 0), pm->GetAllFrames().size());
+
+  NavigateIframeToURLAndWait(tab, "frame2", ext2_empty_url);
+  EXPECT_EQ(IfExtensionsIsolated(1, 0),
+            pm->GetRenderFrameHostsForExtension(extension2->id()).size());
+  EXPECT_EQ(IfExtensionsIsolated(2, 0), pm->GetAllFrames().size());
+
+  // Tests non-extension page in extension frame
+  ui_test_utils::NavigateToURL(browser(), ext1_parent_url);
+  EXPECT_EQ(3u, pm->GetAllFrames().size());
+  EXPECT_EQ(3u, pm->GetRenderFrameHostsForExtension(extension1->id()).size());
+  EXPECT_EQ(0u, pm->GetRenderFrameHostsForExtension(extension2->id()).size());
+
+  NavigateIframeToURLAndWait(tab, "frame1",
+                             embedded_test_server()->GetURL("/empty.html"));
+  EXPECT_EQ(2u, pm->GetRenderFrameHostsForExtension(extension1->id()).size());
+  EXPECT_EQ(2u, pm->GetAllFrames().size());
+
+  NavigateIframeToURLAndWait(tab, "frame1", ext1_empty_url);
+  EXPECT_EQ(3u, pm->GetAllFrames().size());
+  EXPECT_EQ(3u, pm->GetRenderFrameHostsForExtension(extension1->id()).size());
+
+  // Load a frame from another extension.
+  NavigateIframeToURLAndWait(tab, "frame1", ext2_empty_url);
+  EXPECT_EQ(IfExtensionsIsolated(3, 2), pm->GetAllFrames().size());
+  EXPECT_EQ(2u, pm->GetRenderFrameHostsForExtension(extension1->id()).size());
+  EXPECT_EQ(IfExtensionsIsolated(1, 0),
+            pm->GetRenderFrameHostsForExtension(extension2->id()).size());
+
+  // Destroy all existing frames by navigating to another extension.
+  ui_test_utils::NavigateToURL(browser(),
+                               extension2->url().Resolve("empty.html"));
+  EXPECT_EQ(1u, pm->GetAllFrames().size());
+  EXPECT_EQ(0u, pm->GetRenderFrameHostsForExtension(extension1->id()).size());
+  EXPECT_EQ(1u, pm->GetRenderFrameHostsForExtension(extension2->id()).size());
+
+  // Test about:blank and about:srcdoc child frames.
+  ui_test_utils::NavigateToURL(browser(),
+                               extension2->url().Resolve("srcdoc_iframe.html"));
+  EXPECT_EQ(2u, pm->GetAllFrames().size());
+  EXPECT_EQ(2u, pm->GetRenderFrameHostsForExtension(extension2->id()).size());
+
+  ui_test_utils::NavigateToURL(browser(),
+                               extension2->url().Resolve("blank_iframe.html"));
+  EXPECT_EQ(2u, pm->GetAllFrames().size());
+  EXPECT_EQ(2u, pm->GetRenderFrameHostsForExtension(extension2->id()).size());
+
+  // Test nested frames (same extension).
+  ui_test_utils::NavigateToURL(browser(), ext2_parent_url);
+  EXPECT_EQ(3u, pm->GetAllFrames().size());
+  EXPECT_EQ(3u, pm->GetRenderFrameHostsForExtension(extension2->id()).size());
+
+  NavigateIframeToURLAndWait(tab, "frame1", ext2_parent_url);
+  EXPECT_EQ(5u, pm->GetAllFrames().size());
+  EXPECT_EQ(5u, pm->GetRenderFrameHostsForExtension(extension2->id()).size());
+
+  // The extension frame from the other extension should not be classified as an
+  // extension (unless out-of-process frames are enabled).
+  NavigateIframeToURLAndWait(tab, "frame1", ext1_empty_url);
+  EXPECT_EQ(IfExtensionsIsolated(3, 2), pm->GetAllFrames().size());
+  EXPECT_EQ(IfExtensionsIsolated(1, 0),
+            pm->GetRenderFrameHostsForExtension(extension1->id()).size());
+  EXPECT_EQ(2u, pm->GetRenderFrameHostsForExtension(extension2->id()).size());
+
+  NavigateIframeToURLAndWait(tab, "frame2", ext1_parent_url);
+  EXPECT_EQ(IfExtensionsIsolated(5, 1), pm->GetAllFrames().size());
+  EXPECT_EQ(IfExtensionsIsolated(4, 0),
+            pm->GetRenderFrameHostsForExtension(extension1->id()).size());
+  EXPECT_EQ(1u, pm->GetRenderFrameHostsForExtension(extension2->id()).size());
+
+  // Crash tab where the top-level frame is an extension frame.
+  content::CrashTab(tab);
+  EXPECT_EQ(0u, pm->GetAllFrames().size());
+  EXPECT_EQ(0u, pm->GetRenderFrameHostsForExtension(extension1->id()).size());
+  EXPECT_EQ(0u, pm->GetRenderFrameHostsForExtension(extension2->id()).size());
+
+  // Now load an extension page and a non-extension page...
+  ui_test_utils::NavigateToURLWithDisposition(
+      browser(), ext1_empty_url, NEW_BACKGROUND_TAB,
+      ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);
+  ui_test_utils::NavigateToURL(
+      browser(), embedded_test_server()->GetURL("/two_iframes.html"));
+  EXPECT_EQ(1u, pm->GetAllFrames().size());
+
+  // ... load an extension frame in the non-extension process
+  NavigateIframeToURLAndWait(tab, "frame1", ext1_empty_url);
+  EXPECT_EQ(IfExtensionsIsolated(2, 1),
+            pm->GetRenderFrameHostsForExtension(extension1->id()).size());
+
+  // ... and take down the tab. The extension process is not part of the tab,
+  // so it should be kept alive (minus the frames that died).
+  content::CrashTab(tab);
+  EXPECT_EQ(1u, pm->GetAllFrames().size());
+  EXPECT_EQ(1u, pm->GetRenderFrameHostsForExtension(extension1->id()).size());

[ncarter (slow), 2015/11/24 05:22:13]

This test is awesome.

[robwu, 2015/11/25 00:44:16]

Thanks. It was flaky, and I locally ran the test for a few days to find the
culprit. It turns out that ui_utils::NavigateToURL waits for StopLoading, and
the old RenderFrameHost might still be around (i.e. not deleted) at that point.
I've updated this CL with a new method to handle this edge case.

[ncarter (slow), 2015/11/30 22:56:45]

Good find; I like how you handled it. This makes me wish we had one complete set
of helper methods that covered the whole matrix of "Trigger a navigation using
some method" and "wait for some definition of done" possibilities.

TestFrameNavigationObserver is kind of a good start, but even it doesn't handle
the pending-deletion case as you do here.

+}
+
 // Verify correct keepalive count behavior on network request events.
 // Regression test for http://crbug.com/535716.
 IN_PROC_BROWSER_TEST_F(ProcessManagerBrowserTest, KeepaliveOnNetworkRequest) {