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..121c88b4c1a915542e6141c0a7510c4b8fc3553d 100644
--- a/chrome/browser/extensions/process_manager_browsertest.cc
+++ b/chrome/browser/extensions/process_manager_browsertest.cc
@@ -4,23 +4,170 @@
 
 #include "extensions/browser/process_manager.h"
 
+#include "base/callback.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/run_loop.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"
 
 namespace extensions {
 
+namespace {
+
+void AddFrameToSet(std::set<content::RenderFrameHost*>* frames,
+                   content::RenderFrameHost* rfh) {
+  if (rfh->IsRenderFrameLive())
+    frames->insert(rfh);
+}
+
+}  // namespace
+
+// Takes a snapshot of all frames upon construction. When Wait is called, a
+// MessageLoop is created and Quit when all previously recorded frames are
+// either present in the tab, or deleted.
+class NavigationCompletedObserver : public content::WebContentsObserver {
+ public:
+  explicit NavigationCompletedObserver(content::WebContents* web_contents)
+    : content::WebContentsObserver(web_contents),
+      web_contents_(web_contents),
+      message_loop_runner_(new content::MessageLoopRunner) {
+    web_contents->ForEachFrame(
+        base::Bind(&AddFrameToSet, base::Unretained(&frames_)));
+  }
+
+  void Wait() {
+    if (!AreAllFramesInTab())
+      message_loop_runner_->Run();
+  }
+
+  void RenderFrameDeleted(content::RenderFrameHost* rfh) override {
+    if (frames_.erase(rfh) != 0 && message_loop_runner_->loop_running() &&
+        AreAllFramesInTab())
+      message_loop_runner_->Quit();
+  }
+
+ private:
+  bool AreAllFramesInTab() {

[Devlin, 2015/12/01 00:58:20]

nit: please document this method.

[robwu, 2015/12/01 17:19:57]

Done.

+    std::set<content::RenderFrameHost*> current_frames;
+    web_contents_->ForEachFrame(
+        base::Bind(&AddFrameToSet, base::Unretained(&current_frames)));
+    for (content::RenderFrameHost* frame : frames_) {
+      if (current_frames.find(frame) == current_frames.end())
+        return false;
+    }
+    return true;
+  }
+
+  content::WebContents* web_contents_;

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

You don't need this; WebContentsObserver has a web_contents() accessor.

[robwu, 2015/12/01 00:45:26]

Done.

+  std::set<content::RenderFrameHost*> frames_;
+  scoped_refptr<content::MessageLoopRunner> message_loop_runner_;

[Devlin, 2015/12/01 00:58:20]

nit: DISALLOW_COPY_AND_ASSIGN

[robwu, 2015/12/01 17:19:57]

Done.

+};
+
 // 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);

[Devlin, 2015/12/01 00:58:20]

nitty nit: prefer explicit construction (i.e., "new TestExtensionDir()")

[robwu, 2015/12/01 17:19:57]

Done.

+
+    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.Pass());
+    return extension;
+  }
+
+  // ui_test_utils::NavigateToURL sometimes returns too early: It returns as
+  // soon as the StopLoading notification has been triggered. This does not
+  // imply that RenderFrameDeleted was called, so the test may continue too
+  // early and fail when ProcessManager::GetAllFrames() returns too many frames
+  // (namely frames that are in the process of being deleted).
+  void NavigateToURL(const GURL& url) {
+    NavigationCompletedObserver observer(
+        browser()->tab_strip_model()->GetActiveWebContents());
+
+    ui_test_utils::NavigateToURL(browser(), url);
+
+    // Wait until the last RenderFrameHosts are deleted. This wait doesn't take
+    // long.
+    observer.Wait();
+  }
+
+  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);
+  }
+
+  size_t IfExtensionsIsolated(size_t if_enabled, size_t if_disabled) {
+    return content::AreAllSitesIsolatedForTesting() ||
+        IsIsolateExtensionsEnabled() ? if_enabled : if_disabled;
+  }
+
+ private:
+  std::vector<scoped_ptr<TestExtensionDir>> temp_dirs_;

[Devlin, 2015/12/01 00:58:20]

nit: DISALLOW_COPY_AND_ASSIGN

[robwu, 2015/12/01 17:19:57]

Done.

[robwu, 2015/12/01 23:19:57]

I added this and compilation failed, so I've reverted the change. The error was:
chrome/browser/extensions/process_manager_browsertest.cc:352:1: error:
constructor for 'extensions::ProcessManagerBrowserTest_FrameClassification_Test'
must explicitly initialize the base class
'extensions::ProcessManagerBrowserTest' which does not have a default
constructor

+};
 
 // Test that basic extension loading creates the appropriate ExtensionHosts
 // and background pages.
@@ -157,6 +304,167 @@ 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")

[Devlin, 2015/12/01 00:58:20]

indentation looks off - was this git cl formatted?

[robwu, 2015/12/01 17:19:57]

No. I've now ran git cl format and the indention increased by four spaces.

+                    .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.
+  NavigateToURL(embedded_test_server()->GetURL("/empty.html"));
+  EXPECT_EQ(0u, pm->GetRenderFrameHostsForExtension(extension->id()).size());
+
+  const GURL extension_url = extension->url().Resolve("manifest.json");
+  NavigateToURL(extension_url);
+  EXPECT_EQ(1u, pm->GetRenderFrameHostsForExtension(extension->id()).size());
+
+  NavigateToURL(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"));

[Devlin, 2015/12/01 00:58:20]

nit: for consts like these, I usually prefer kExt1ParentUrl syntax.

[robwu, 2015/12/01 17:19:57]

Done.

+  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());

[Devlin, 2015/12/01 00:58:20]

For this, it would probably be nice to include comments listing which frames are
expected.

[robwu, 2015/12/01 17:19:57]

Done.

+
+  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());

[Devlin, 2015/12/01 00:58:20]

nit: unless GetAllFrames() is supremely busted, this check isn't necessary.  But
if you want to keep it, I won't object.

[robwu, 2015/12/01 17:19:57]

In the current implementation it does not matter, but that is not necessarily
true in the future. Besides asserting the obviously correct behavior, it also
serves as documentation of what is expected to happen.

+
+  NavigateToURL(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
+  NavigateToURL(ext1_parent_url);

[Devlin, 2015/12/01 00:58:20]

Here, too, comments would be nice.  It's hard to keep the mental map of which
tabs have which frames.

[robwu, 2015/12/01 17:19:57]

Done.

+  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.
+  NavigateToURL(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.
+  NavigateToURL(extension2->url().Resolve("srcdoc_iframe.html"));
+  EXPECT_EQ(2u, pm->GetAllFrames().size());
+  EXPECT_EQ(2u, pm->GetRenderFrameHostsForExtension(extension2->id()).size());
+
+  NavigateToURL(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).
+  NavigateToURL(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);
+  NavigateToURL(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());
+}
+
 // Verify correct keepalive count behavior on network request events.
 // Regression test for http://crbug.com/535716.
 IN_PROC_BROWSER_TEST_F(ProcessManagerBrowserTest, KeepaliveOnNetworkRequest) {