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..452d9c7ff10e80e0ea9028b08744cfd9ce26fa33 100644
--- a/extensions/browser/extension_web_contents_observer.cc
+++ b/extensions/browser/extension_web_contents_observer.cc
@@ -4,6 +4,7 @@
 
 #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/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"
@@ -50,9 +51,17 @@ 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()) ||
+           GetExtensionIdFromFrame(render_frame_host).empty());
+    return;
+  }
   // Notify the render frame of the view type.
   render_frame_host->Send(new ExtensionMsg_NotifyRenderViewType(
-      render_frame_host->GetRoutingID(), GetViewType(web_contents())));
+      render_frame_host->GetRoutingID(), view_type));
 
   const Extension* frame_extension = GetExtensionFromFrame(render_frame_host);
   if (frame_extension) {
@@ -110,6 +119,13 @@ void ExtensionWebContentsObserver::RenderFrameDeleted(
       ->UnregisterRenderFrameHost(render_frame_host);
 }
 
+void ExtensionWebContentsObserver::RenderFrameHostChanged(

[ncarter (slow), 2015/11/02 17:36:20]

If you are listening for changes to GetLastCommittedURL, should you be listening
to DidCommitProvisionalLoadForFrame instead? RenderFrameHostChanged indicates a
cross-process navigation that switches the RenderFrameHost, but it won't trigger
on in-process navigations. 

You can exercise this case in a test by having an iframe that navigates from an
http:// url to a chrome-extension:// url. Without --isolate-extensions or
--site-per-process, iframe navigations will never be cross-process, since making
them swap processes requires out-of-process iframes. Top level navigations to
chrome-extension URLs, however, ought always swap to the extension process.

Also, if an iframe navigates from an extension URL to a non-extension URL, do we
need to do any kind of deregistration (e.g. the opposite of
RegisterMojoServices)?

[robwu, 2015/11/02 22:50:41]

This class is only interested in whether a RFH is an extension frame, not
whether it navigated from one extension page to another. I've updated the CL
with a comment to explain why I need this notification as well.

My use case involves detection of unloaded frames, but I'll ask on
site-isolation-dev to learn more about the available options. 
A non-extension frame cannot be hosted in the extension process, so we're fine
in that respect.

[ncarter (slow), 2015/11/03 23:32:02]

I'm one of the site-isolation-dev folks, fyi.

Are you trying to track all extension frames here, or all extension frames in
extension processes? If the former, then RenderFrameHostChanged will not
suffice.

For an example of a case you'll miss, look at
SiteDetailsBrowserTest.IsolateExtensions, line 527; it navigates an iframe from
an http url to to an extension URL.

If you run that test without --isolate-extensions, then that navigation will
only result in DidCommitProvisionalLoadForFrame() with the arguments
(render_process_id = 3, frame_routing_id = 3, url =
chrome-extension://acfkebijajgppkblbddahakojdedghnf/blank_iframe.html). There
will be no RenderFrameHostChanged there, and this extension frame will never be
located. This is the "unblessed extension" case, where we have an extension
frame in a non-extension process. Note that in this case, GetSiteURL() for the
extension frame's SiteInstance will be something like "http://a.com/".

If you run that same test with --isolate-extensions, you will indeed see
RenderFrameHostChanged((rph=3, frame_route_id=3) -> (rph=4, frame_route_id=7))
followed by a DidCommitProvisionalLoadForFrame for the new frame, but see below.

[robwu, 2015/11/04 00:34:02]

Extension frames in extension processes only. Extension frames in unprivileged
processes have almost not access to extension APIs.
OOP extension frames embedded by unprivileged processes get more API access, so
they should show up. This change is discussed at https://crbug.com/550151

+    content::RenderFrameHost* old_host,
+    content::RenderFrameHost* new_host) {

[ncarter (slow), 2015/11/03 23:32:02]

Which test is it, that needs this logic?

I patched your change in, deleted the body of this function, and ran
"browser_tests --gtest_filter=*InspectEmbedded*:*MessagingNo*." Both those tests
seemed to pass, so I'm assuming it must be something else you were testing.

If you have manual steps, let's try to get them in browsertest form -- this case
seems pretty important.

[robwu, 2015/11/04 00:34:02]

I have a manual test:

1. Build Chrome with both patches applied (this and the one mentioned in the CL
description).
2. Load the extension that I've just mailed to you.
3. Visit http://example.com
4. Paste chrome-extension://llmabahepndgmjgaopcpabgfomdocobb/manifest.json.htm
in the omnibox to visit it (=process swap from example.com to extension).
5. Visit http://example.com/?cs
6. Click on the "sendMessage" button in the tab from step 5.

Expected (WITH RenderFrameHostChanged):
- The first tab (step 4) receives the message and prints a success message.

Actual:
- The message cannot be delivered (https://imgur.com/wr3cZO2) because the RFH of
the first tab is not registered; Its RenderFrameCreated notification is called
with a LastCommittedURL of the previous page, so frame_extension stays nullptr
and the RFH is not registered with the ProcessManager
(https://cs.chromium.org/GetExtensionFromFrame%5C%28render_frame_host)).

If there is no test coverage yet, I'll add a browser_test.

[robwu, 2015/11/04 23:52:23]

I have now added a browsertest that shows why RenderFrameHostChanged is useful
(ProcessManagerBrowserTest.NoBackgroundPage).

+  if (new_host->IsRenderFrameLive())
+    InitializeRenderFrame(new_host);

[ncarter (slow), 2015/11/03 23:32:02]

I put the following line at the top of
WebContentsObserverSanityChecker::RenderFrameHostChanged:

  CHECK(new_host->GetLastCommittedURL().is_empty());

And ran content_browsertests. The CHECK never fired, suggesting that
new_frame->GetLastCommittedURL() consistently returns an empty URL at this point
in time. That's likely a content bug, fwiw.

However, it also means that if this RenderFrameHostChanged override really does
anything useful, it's relying on the fact that GetExtensionIdFromFrame falls
back to the SiteURL if LastCommittedURL is empty.

If you care about LastCommittedURL then you ought to listen for
DidCommitProvisionalLoadForFrame, which is the notification that happens when
that value changes.

[robwu, 2015/11/04 00:34:02]

Yes, that's exactly what I'm relying on. This seems a bad design (from my part)
though, especially without tests... The correct value would be a
chrome-extension:-URL, so what I'm relying on seems not that bad after all, in
either case it plays out well.
I'm only interested in whether the frame has a chrome-extension:-URL.
(My own use case, extension messaging actually desires a notification per page,
so DidNavigateAnyFrame is what I really use, but there are other users of the
API, and I don't know whether the multiple InitializeRenderFrame calls is going
to be received well). That's why I'm planning to adding a new ProcessManager
notification
(https://groups.google.com/a/chromium.org/d/msg/site-isolation-dev/eecchXukLGA...).

+}
+
 bool ExtensionWebContentsObserver::OnMessageReceived(
     const IPC::Message& message,
     content::RenderFrameHost* render_frame_host) {