Track all extension frames in ProcessManager, inspect extensionoptions

extensions/browser/extension_web_contents_observer.cc

Index: extensions/browser/extension_web_contents_observer.cc
diff --git a/extensions/browser/extension_web_contents_observer.cc b/extensions/browser/extension_web_contents_observer.cc
index 665fc0809cca1b3ebcfa428ae79d762d9c8c5caa..321c3c30820b23a3c62808e21ae31f914b2d9ce8 100644
--- a/extensions/browser/extension_web_contents_observer.cc
+++ b/extensions/browser/extension_web_contents_observer.cc
@@ -4,7 +4,9 @@
 
 #include "extensions/browser/extension_web_contents_observer.h"
 
+#include "components/guest_view/browser/guest_view_base.h"
 #include "content/public/browser/child_process_security_policy.h"
+#include "content/public/browser/navigation_details.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/render_view_host.h"
@@ -24,6 +26,14 @@
 
 namespace extensions {
 
+namespace {
+
+bool IsExtensionURL(const GURL& url) {
+  return url.SchemeIs(kExtensionScheme) || url.SchemeIs(content::kGuestScheme);
+}
+
+}  // namespace
+
 // static
 ExtensionWebContentsObserver* ExtensionWebContentsObserver::GetForWebContents(
     content::WebContents* web_contents) {
@@ -50,18 +60,34 @@ void ExtensionWebContentsObserver::InitializeRenderFrame(
   DCHECK(render_frame_host);
   DCHECK(render_frame_host->IsRenderFrameLive());
 
+  ViewType view_type = GetViewType(web_contents());
+  if (view_type == VIEW_TYPE_INVALID) {
+    // This is expected to happen only for non-extension pages (e.g. devtools)
+    // and GuestViews.
+    DCHECK(guest_view::GuestViewBase::FromWebContents(web_contents()) ||
+           !IsExtensionURL(render_frame_host->GetLastCommittedURL()));

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

Use render_frame_host->GetSiteInstance()->GetSiteURL() here.

I think you grok why already, but just to be clear: This is called from
RenderFrameCreated, so GetLastCommittedURL() might not be meaningful at that
time. The risk is that, during a cross-process navigation (say, from site1 to
site2), we'll create a pending host for site2 (different from the current host,
which is for site1), and the pending host will be announced via
WCO::RenderFrameCreated. During RenderFrameCreate(pending_host),
pending_host->GetLastCommittedURL() will return "site1".

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

Done.

+    return;
+  }
+
+  // At the initialization of the render frame, the last committed URL is not
+  // reliable, so do not take it into account in determining whether it is an
+  // extension frame.
+  const Extension* frame_extension =
+      GetExtensionFromFrame(render_frame_host, false);
+  // TODO(robwu): Add DCHECK that verifies that |frame_extension| always exists
+  // if OOP frames is enabled.

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

Is this TODO stale? I'm not sure what it means.

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

When OOPIF is enabled, only extension frames should trigger RenderFrameCreated.
So I expect |frame_extension| to be a non-null pointer in those cases.

I still need to figure out how to check this here. --isolate-extensions and
--site-per-process are in the chrome/ layer, not the extensions layer. What do
you suggest?

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

--site-per-process is a content flag, so that should work for testing.

Alex is actually about to move the --isolate-extensions flag definition to
extensions, but the flag won't have any effect there yet, until we hook it up to
the ContentClient used by extensions/ browsertests, which will be a bit more
plumbing.

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

I'll use --site-per-process for now, and hope that the FYI bots offer enough
coverage (assuming that my CQ_INCLUDE_TRYBOTS syntax is correct).

+  if (!frame_extension)
+    return;
+
   // Notify the render frame of the view type.
   render_frame_host->Send(new ExtensionMsg_NotifyRenderViewType(
-      render_frame_host->GetRoutingID(), GetViewType(web_contents())));
-
-  const Extension* frame_extension = GetExtensionFromFrame(render_frame_host);
-  if (frame_extension) {
-    ExtensionsBrowserClient::Get()->RegisterMojoServices(render_frame_host,
-                                                         frame_extension);
-    ProcessManager::Get(browser_context_)
-        ->RegisterRenderFrameHost(web_contents(), render_frame_host,
-                                  frame_extension);
-  }
+      render_frame_host->GetRoutingID(), view_type));
+
+  ExtensionsBrowserClient::Get()->RegisterMojoServices(render_frame_host,
+                                                       frame_extension);
+  ProcessManager::Get(browser_context_)
+      ->RegisterRenderFrameHost(web_contents(), render_frame_host,
+                                frame_extension);
 }
 
 content::WebContents* ExtensionWebContentsObserver::GetAssociatedWebContents()
@@ -110,6 +136,49 @@ void ExtensionWebContentsObserver::RenderFrameDeleted(
       ->UnregisterRenderFrameHost(render_frame_host);
 }
 
+void ExtensionWebContentsObserver::DidCommitProvisionalLoadForFrame(
+    content::RenderFrameHost* render_frame_host,
+    const GURL& url,
+    ui::PageTransition transition_type) {
+  ProcessManager* pm = ProcessManager::Get(browser_context_);
+
+  if (pm->IsRenderFrameHostRegistered(render_frame_host)) {
+    const Extension* frame_extension =
+        GetExtensionFromFrame(render_frame_host, true);
+
+    if (!frame_extension)
+      pm->UnregisterRenderFrameHost(render_frame_host);
+  }
+}
+
+void ExtensionWebContentsObserver::DidNavigateAnyFrame(
+    content::RenderFrameHost* render_frame_host,
+    const content::LoadCommittedDetails& details,
+    const content::FrameNavigateParams& params) {
+  if (details.is_in_page)
+    return;
+
+  const Extension* frame_extension =
+      GetExtensionFromFrame(render_frame_host, true);
+  ProcessManager* pm = ProcessManager::Get(browser_context_);
+
+  if (!frame_extension) {
+    // Should have been unregistered by DidCommitProvisionalLoadForFrame.
+    DCHECK(!pm->IsRenderFrameHostRegistered(render_frame_host));
+    return;
+  }
+
+  if (pm->IsRenderFrameHostRegistered(render_frame_host)) {
+    // Notify ProcessManager, because some clients do not just want to know
+    // whether the frame is in an extension process, but also whether the frame
+    // was navigated.
+    pm->DidNavigateRenderFrameHost(render_frame_host);
+  } else {
+    pm->RegisterRenderFrameHost(web_contents(), render_frame_host,
+                                frame_extension);
+  }

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

After consulting with creis@ and debugging the code a bit, I think it's better
to move all this logic from DidNavigateAnyFrame into
DidCommitProvisionalLoadForFrame. The two methods are called at almost the same
time, but the difference is that content-initiated subframe navigations will
always call DidCommitProvisionalLoadForFrame, but may skip DidNavigateAnyFrame
(the AUTO_SUBFRAME case in navigator_impl.cc).

By moving the code, you will lose the "details.is_in_page" early return, but
since the origin can change during an AUTO_SUBFRAME navigation, it seems best to
avoid DidNavigateAnyFrame.

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

I need DidNavigateAnyFrame to detect whether the frame navigated away. This is
needed in the other CL because that means that I have to clean up objects that
are tied to the lifetime of a web page (MessagePort).

+}
+
 bool ExtensionWebContentsObserver::OnMessageReceived(
     const IPC::Message& message,
     content::RenderFrameHost* render_frame_host) {
@@ -146,24 +215,44 @@ void ExtensionWebContentsObserver::PepperInstanceDeleted() {
 
 std::string ExtensionWebContentsObserver::GetExtensionIdFromFrame(
     content::RenderFrameHost* render_frame_host) const {
-  content::SiteInstance* site_instance = render_frame_host->GetSiteInstance();
-  GURL url = render_frame_host->GetLastCommittedURL();
-  if (!url.is_empty()) {
-    if (site_instance->GetSiteURL().GetOrigin() != url.GetOrigin())
-      return std::string();
-  } else {
-    url = site_instance->GetSiteURL();
-  }
-
-  return url.SchemeIs(kExtensionScheme) ? url.host() : std::string();
+  const Extension* extension = GetExtensionFromFrame(render_frame_host, true);
+  return extension ? extension->id() : std::string();
 }
 
 const Extension* ExtensionWebContentsObserver::GetExtensionFromFrame(
-    content::RenderFrameHost* render_frame_host) const {
-  return ExtensionRegistry::Get(
-             render_frame_host->GetProcess()->GetBrowserContext())
-      ->enabled_extensions()
-      .GetByID(GetExtensionIdFromFrame(render_frame_host));
+    content::RenderFrameHost* render_frame_host,
+    bool verify_url) const {
+  const GURL site_url(render_frame_host->GetSiteInstance()->GetSiteURL());
+  if (!IsExtensionURL(site_url))
+    return nullptr;
+
+  const std::string& extension_id = site_url.host();
+
+  // This verify_url logic can completely be removed once site isolation is
+  // fully enabled.
+  if (verify_url) {
+    GURL url(render_frame_host->GetLastCommittedURL());
+    if (!IsExtensionURL(url)) {
+      // Any about:* frame (in particular about:blank and about:srcdoc) runs in
+      // the security context of the parent page that created it. Treat such
+      // frames as extension pages iff the parent frame is registered.
+      if (url.SchemeIs(url::kAboutScheme) && render_frame_host->GetParent())

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

This heuristic (which I know I suggested!) may not actually be sufficient, it
turns out: there are window.open cases where you can wind up with an about:blank
and an origin different from the parent. At best it's brittle, since it's
effectively simulating the blink/content implementation ... and the
blink/content treatment of non-standard schemes and origins can differ markedly
from the spec.

I've put up https://codereview.chromium.org/1475433002/ which will let you just
grab the effective origin of a frame from the content layer. You can call that
instead of GetLastCommittedURL from this function, and you should be good to go
-- no looking at the parent required.

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

Nice! I'll rebase when your patch lands.

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

It's landed.

When you use GetLastCommittedOrigin(), you'll have to decide whether, when it
yields a unique origin, to fall back to the last committed URL. The cases where
you'd encounter an extension URL with a unique origin, to my knowledge, is via
<iframe sandbox> and also (I would hope) resources sandboxed by the extension
manifest.

My best guess is to treat unique-origin frames with extension URLs as "not
extensions", since the purpose of sandboxing is to make them not-extensiony.

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

Yep, this makes most sense.

(is it really the cases that sandboxed unique-origin frames don't get their own
site? What is the point of site isolation if untrusted content in a sandboxed
frames can compromise the renderer of the original origin?)

+        return GetExtensionFromFrame(render_frame_host->GetParent(), true);
+      return nullptr;
+    }
+    if (url.host() != extension_id) {
+      return nullptr;
+    }
+  }
+
+  const Extension* extension =
+      ExtensionRegistry::Get(
+          render_frame_host->GetProcess()->GetBrowserContext())
+          ->enabled_extensions()
+          .GetByID(extension_id);
+
+  // A hosted app is not an extension. Its SiteInstance only contains web pages.
+  return extension && !extension->is_hosted_app() ? extension : nullptr;
 }
 
 const Extension* ExtensionWebContentsObserver::GetExtension(