Track all extension frames in ProcessManager, inspect extensionoptions

extensions/browser/process_manager.cc

Index: extensions/browser/process_manager.cc
diff --git a/extensions/browser/process_manager.cc b/extensions/browser/process_manager.cc
index 8b2b4b534b92e13bad3a2a9530c0f928c93eb284..218c5828a21c541657d8ed0aaea89779f368df5a 100644
--- a/extensions/browser/process_manager.cc
+++ b/extensions/browser/process_manager.cc
@@ -324,17 +324,10 @@ const ProcessManager::FrameSet ProcessManager::GetAllFrames() const {
 ProcessManager::FrameSet ProcessManager::GetRenderFrameHostsForExtension(
     const std::string& extension_id) {
   FrameSet result;
-  scoped_refptr<content::SiteInstance> site_instance(GetSiteInstanceForURL(
-      Extension::GetBaseURLFromExtensionId(extension_id)));
-  if (!site_instance.get())
-    return result;
-
-  // Gather up all the frames for that site.
   for (const auto& key_value : all_extension_frames_) {
-    if (key_value.first->GetSiteInstance() == site_instance)

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

I thought we made sure that extensions could only have one site instance...
deferring to content gurus.

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

The code you're thinking of is actually in this file, about a dozen lines up:
ProcessManager::GetSiteInstanceForURL(). The member 'site_instance_' is a dummy
siteinstance (its site is never set), but it's used to seed a browsinginstance
that encompasses all extensions. This makes it so that
ProcessManager::GetSiteInstanceForURL(x) will return only one site instance per
extension.

Empirically, though, you can have additional SiteInstances for the same
extension id, if ProcessManager() is not looped into the creation of the
siteinstance. All it would take is to call SiteInstance::CreateForURL() with the
extension URL as the second parameter.

I just debugged SiteDetailsBrowserTest.IsolateExtensions, and it hits this by
loading a web-accessible extension resource into a browser tab. It's not clear
to me where the second siteinstance would come from in the test presented here,
but it ought to be easy to debug: set a breakpoint on SiteInstanceImpl::SetSite,
and look at |url|.

Whether this is working as intended is a Charlie question, really.

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

I also expected extensions to have only one SiteInstance, but it does not.

Empirically, ProcessManager::GetSiteInstanceForURL returns a different
SiteInstance for every call.
https://chromium.googlesource.com/chromium/src/+/d5072a821b4f9651e1c0a38ffca8...
suggests that it is possible to have multiple SiteInstances "in rare cases".
Well, apparently not so rare for extensions without background page.

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

SiteInstanceImpl::GetRelatedSiteInstance() just calls
BrowsingInstance::GetSiteInstanceForURL(), which looks up previously return
values in a map. So for a given SiteInstance, GetRelatedSiteInstance is going to
be idempotent.

The race indicated by that comment is when you'd get two site-less SiteInstances
(e.g. for an initial about:blank page) in the same BrowsingInstance, and they
both later do SetSite() to the same site. That race doesn't seem possible with
the SiteInstances generated by ProcessManager::GetSiteInstanceForURL(), which
always have their sites assigned immediately.

My guess is that you're indeed seeing two SiteInstances for the same extension,
but they're in different BrowsingInstances.

[robwu, 2015/10/30 23:02:44]

That is correct. The BrowsingInstances are different.
When I navigate to an extension page (same tab or new tab, via omnibox), then
SiteInstance::CreateForURL is used, and a new BrowsingInstance is generated.

Should I keep my new logic as introduced here, or should I somehow ensure that
the extensions share a single BrowsingInstance? In the former case, the
site_instance_ in ProcessManager seems quite useless...

+    if (GetExtensionID(key_value.first) == extension_id)
       result.insert(key_value.first);
   }
-
   return result;
 }