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..653aeb111aaf90389c902c8def5bc5a942ce6afc 100644
--- a/extensions/browser/extension_web_contents_observer.cc
+++ b/extensions/browser/extension_web_contents_observer.cc
@@ -110,6 +110,13 @@ void ExtensionWebContentsObserver::RenderFrameDeleted(
       ->UnregisterRenderFrameHost(render_frame_host);
 }
 
+void ExtensionWebContentsObserver::RenderFrameHostChanged(

[Devlin, 2015/10/30 01:21:44]

We used to have this, and it was removed for some reason.  Unfortunately, I
think that reason is documented in since-removed chat logs.  So + a few content
gurus to double check this.

Basically, what we want is an up-to-date set of render frames so that we can
initialize them with extension-y data.

+    content::RenderFrameHost* old_host,
+    content::RenderFrameHost* new_host) {
+  if (new_host->IsRenderFrameLive())
+    InitializeRenderFrame(new_host);

[ncarter (slow), 2015/10/30 06:22:20]

Does your test really fail if you don't add this override? RenderFrameCreated
and RenderFrameDeleted, together with ForEachFrame() when you install the
observer**, ought to cover cover your use case here.

If the RenderFrameHostChanged override is really necessary here, then that's a
content bug, a pretty bad one, and I am very very eager to learn more.

As written, I would expect this code to end up sending
ExtensionMsg_NotifyRenderViewType and calling RegisterMojoServices()
redundantly. As a rule of thumb, if you don't do anything with |old_host|, then
you probably shouldn't override RenderFrameHostChanged.

** There is one theoretical bug I know of, which is that ForEachFrame doesn't
tell you about existing live pending RenderFrameHosts. I've not seen any case
yet where this matters, and I've been on the lookout for one. It's not been a
problem in practice, apparently, because most WebContentsObservers in practice
install themselves right at the time of WebContents creation, before there's
been a chance to have a pending navigation.

*** I know these observer methods are really confusing. I'm going to try to do
something about it this quarter.

[robwu, 2015/10/30 10:15:49]

This override is needed, because the RFH's GetLastCommittedURL() is used by
GetExtensionFromFrame to determine whether the frame should be registered with
the process manager. When RenderFrameCreated is called, the committed URL is
still the one of the previous page. When I add some more logging
(http://pastebin.com/bUeJwYtM), I get the a log (http://pastebin.com/AFrkPGgi)
that shows the issue. If I remove the RenderFrameHostChanged override, then the
frame will never be registered with the ProcessManager.

If your concern is the duplicate IPC, I can add a conditional around 
ExtensionMsg_NotifyRenderViewType, so that it is only sent once (in the
constructor and in RenderFrameCreated).

[ncarter (slow), 2015/10/30 15:53:44]

I see. I'll take another look, then.

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