Issue 1413853005: Track all extension frames in ProcessManager, inspect extensionoptions

robwu

rob@robwu.nl changed reviewers: + rdevlin.cronin@chromium.org

5 years, 1 month ago (2015-10-29 23:54:15 UTC) #1

Devlin

rdevlin.cronin@chromium.org changed reviewers: + nick@chromium.org

5 years, 1 month ago (2015-10-30 01:21:43 UTC) #3

Devlin

+Nick (content guru). Please look at ExtensionWebContentsObserver and ProcessManager comments. https://codereview.chromium.org/1413853005/diff/20001/chrome/test/data/extensions/api_test/messaging/connect_nobackground/contentscript.js File chrome/test/data/extensions/api_test/messaging/connect_nobackground/contentscript.js (right): https://codereview.chromium.org/1413853005/diff/20001/chrome/test/data/extensions/api_test/messaging/connect_nobackground/contentscript.js#newcode1 ...

5 years, 1 month ago (2015-10-30 01:21:44 UTC) #4

ncarter (slow)

https://codereview.chromium.org/1413853005/diff/20001/extensions/browser/extension_web_contents_observer.cc File extensions/browser/extension_web_contents_observer.cc (right): https://codereview.chromium.org/1413853005/diff/20001/extensions/browser/extension_web_contents_observer.cc#newcode117 extensions/browser/extension_web_contents_observer.cc:117: InitializeRenderFrame(new_host); Does your test really fail if you don't ...

5 years, 1 month ago (2015-10-30 06:22:20 UTC) #5

https://codereview.chromium.org/1413853005/diff/20001/extensions/browser/exte...
File extensions/browser/extension_web_contents_observer.cc (right):

https://codereview.chromium.org/1413853005/diff/20001/extensions/browser/exte...
extensions/browser/extension_web_contents_observer.cc:117:
InitializeRenderFrame(new_host);
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.

https://codereview.chromium.org/1413853005/diff/20001/extensions/browser/proc...
File extensions/browser/process_manager.cc (left):

https://codereview.chromium.org/1413853005/diff/20001/extensions/browser/proc...
extensions/browser/process_manager.cc:334: if
(key_value.first->GetSiteInstance() == site_instance)
On 2015/10/30 01:21:44, Devlin wrote:
> I thought we made sure that extensions could only have one site instance...
> deferring to content gurus.

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

https://codereview.chromium.org/1413853005/diff/20001/chrome/test/data/extensions/api_test/messaging/connect_nobackground/contentscript.js File chrome/test/data/extensions/api_test/messaging/connect_nobackground/contentscript.js (right): https://codereview.chromium.org/1413853005/diff/20001/chrome/test/data/extensions/api_test/messaging/connect_nobackground/contentscript.js#newcode9 chrome/test/data/extensions/api_test/messaging/connect_nobackground/contentscript.js:9: chrome.runtime.onMessage.addListener(function(message, sender, sendResponse) { On 2015/10/30 01:21:43, Devlin wrote: ...

5 years, 1 month ago (2015-10-30 10:15:50 UTC) #6

https://codereview.chromium.org/1413853005/diff/20001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/messaging/connect_nobackground/contentscript.js
(right):

https://codereview.chromium.org/1413853005/diff/20001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/messaging/connect_nobackground/contentscript.js:9:
chrome.runtime.onMessage.addListener(function(message, sender, sendResponse) {
On 2015/10/30 01:21:43, Devlin wrote:
> this seems like a subtle race condition - can we register the listener before
we
> disconnect?

This can't race because the message has to go to another process via IPC. If a
race would occur, then it means that the JS engine was blocked while waiting for
a reply, and that is something that should cause a test to fail.

https://codereview.chromium.org/1413853005/diff/20001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/messaging/connect_nobackground/test.js
(right):

https://codereview.chromium.org/1413853005/diff/20001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/messaging/connect_nobackground/test.js:11:
console.log('port.onMessage was triggered.');
On 2015/10/30 01:21:43, Devlin wrote:
> nit: try to avoid noisy logs in tests.  My general advice is to do something
> like:
> var enableLogging = false;
> function log(message) {
>   if (enableLogging)
>     console.log(message);
> }
> 
> So that for debugging, we can turn it on, but otherwise, we avoid the noise.
> 
> (We should probably add a test API method or something, but haven't yet.)

We have chrome.test.log, which causes messages to be printed in case of failure.
Sounds like what you want, doesn't it?
https://cs.chromium.org/%22TestNatives::Log%22

https://codereview.chromium.org/1413853005/diff/20001/extensions/browser/exte...
File extensions/browser/extension_web_contents_observer.cc (right):

https://codereview.chromium.org/1413853005/diff/20001/extensions/browser/exte...
extensions/browser/extension_web_contents_observer.cc:117:
InitializeRenderFrame(new_host);
On 2015/10/30 06:22:20, ncarter wrote:
> 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.

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).

> 
> ** 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.

https://codereview.chromium.org/1413853005/diff/20001/extensions/browser/proc...
File extensions/browser/process_manager.cc (left):

https://codereview.chromium.org/1413853005/diff/20001/extensions/browser/proc...
extensions/browser/process_manager.cc:334: if
(key_value.first->GetSiteInstance() == site_instance)
On 2015/10/30 06:22:20, ncarter wrote:
> On 2015/10/30 01:21:44, Devlin wrote:
> > I thought we made sure that extensions could only have one site instance...
> > deferring to content gurus.
> 
> 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.

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)

https://codereview.chromium.org/1413853005/diff/20001/extensions/browser/extension_web_contents_observer.cc File extensions/browser/extension_web_contents_observer.cc (right): https://codereview.chromium.org/1413853005/diff/20001/extensions/browser/extension_web_contents_observer.cc#newcode117 extensions/browser/extension_web_contents_observer.cc:117: InitializeRenderFrame(new_host); On 2015/10/30 10:15:49, robwu wrote: > On 2015/10/30 ...

5 years, 1 month ago (2015-10-30 15:53:44 UTC) #7

https://codereview.chromium.org/1413853005/diff/20001/extensions/browser/exte...
File extensions/browser/extension_web_contents_observer.cc (right):

https://codereview.chromium.org/1413853005/diff/20001/extensions/browser/exte...
extensions/browser/extension_web_contents_observer.cc:117:
InitializeRenderFrame(new_host);
On 2015/10/30 10:15:49, robwu wrote:
> On 2015/10/30 06:22:20, ncarter wrote:
> > 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.
> 
> 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).
> 
> > 
> > ** 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.
> 

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

https://codereview.chromium.org/1413853005/diff/20001/extensions/browser/proc...
File extensions/browser/process_manager.cc (left):

https://codereview.chromium.org/1413853005/diff/20001/extensions/browser/proc...
extensions/browser/process_manager.cc:334: if
(key_value.first->GetSiteInstance() == site_instance)
On 2015/10/30 10:15:50, robwu wrote:
> On 2015/10/30 06:22:20, ncarter wrote:
> > On 2015/10/30 01:21:44, Devlin wrote:
> > > I thought we made sure that extensions could only have one site
instance...
> > > deferring to content gurus.
> > 
> > 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.
> 
> 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.

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.

Devlin

https://codereview.chromium.org/1413853005/diff/20001/chrome/test/data/extensions/api_test/messaging/connect_nobackground/contentscript.js File chrome/test/data/extensions/api_test/messaging/connect_nobackground/contentscript.js (right): https://codereview.chromium.org/1413853005/diff/20001/chrome/test/data/extensions/api_test/messaging/connect_nobackground/contentscript.js#newcode9 chrome/test/data/extensions/api_test/messaging/connect_nobackground/contentscript.js:9: chrome.runtime.onMessage.addListener(function(message, sender, sendResponse) { On 2015/10/30 10:15:49, robwu wrote: ...

5 years, 1 month ago (2015-10-30 16:03:51 UTC) #8

https://codereview.chromium.org/1413853005/diff/20001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/messaging/connect_nobackground/contentscript.js
(right):

https://codereview.chromium.org/1413853005/diff/20001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/messaging/connect_nobackground/contentscript.js:9:
chrome.runtime.onMessage.addListener(function(message, sender, sendResponse) {
On 2015/10/30 10:15:49, robwu wrote:
> On 2015/10/30 01:21:43, Devlin wrote:
> > this seems like a subtle race condition - can we register the listener
before
> we
> > disconnect?
> 
> This can't race because the message has to go to another process via IPC. If a
> race would occur, then it means that the JS engine was blocked while waiting
for
> a reply, and that is something that should cause a test to fail.

Generally in testing, though, we don't want to rely on implementation details
like that.  What if one day all the messaging code lived in the renderer, and
could be synchronous?

https://codereview.chromium.org/1413853005/diff/20001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/messaging/connect_nobackground/test.js
(right):

https://codereview.chromium.org/1413853005/diff/20001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/messaging/connect_nobackground/test.js:11:
console.log('port.onMessage was triggered.');
On 2015/10/30 10:15:49, robwu wrote:
> On 2015/10/30 01:21:43, Devlin wrote:
> > nit: try to avoid noisy logs in tests.  My general advice is to do something
> > like:
> > var enableLogging = false;
> > function log(message) {
> >   if (enableLogging)
> >     console.log(message);
> > }
> > 
> > So that for debugging, we can turn it on, but otherwise, we avoid the noise.
> > 
> > (We should probably add a test API method or something, but haven't yet.)
> 
> We have chrome.test.log, which causes messages to be printed in case of
failure.
> Sounds like what you want, doesn't it?
> https://cs.chromium.org/%22TestNatives::Log%22

Huh - thought that always logged.  Apparently not.  Yes, that's pretty much what
we want.

robwu

https://codereview.chromium.org/1413853005/diff/20001/extensions/browser/process_manager.cc File extensions/browser/process_manager.cc (left): https://codereview.chromium.org/1413853005/diff/20001/extensions/browser/process_manager.cc#oldcode334 extensions/browser/process_manager.cc:334: if (key_value.first->GetSiteInstance() == site_instance) On 2015/10/30 15:53:44, ncarter wrote: ...

5 years, 1 month ago (2015-10-30 23:02:44 UTC) #9

https://codereview.chromium.org/1413853005/diff/20001/extensions/browser/proc...
File extensions/browser/process_manager.cc (left):

https://codereview.chromium.org/1413853005/diff/20001/extensions/browser/proc...
extensions/browser/process_manager.cc:334: if
(key_value.first->GetSiteInstance() == site_instance)
On 2015/10/30 15:53:44, ncarter wrote:
> On 2015/10/30 10:15:50, robwu wrote:
> > On 2015/10/30 06:22:20, ncarter wrote:
> > > On 2015/10/30 01:21:44, Devlin wrote:
> > > > I thought we made sure that extensions could only have one site
> instance...
> > > > deferring to content gurus.
> > > 
> > > 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.
> > 
> > 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.
> 
> 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.

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...

robwu

rob@robwu.nl changed reviewers: + creis@chromium.org, fsamuel@chromium.org

5 years, 1 month ago (2015-11-01 20:22:34 UTC) #10

robwu

Fady: Could you take a look at the <extensionoptions> changes? Previously <extensionoptions> was not visible ...

5 years, 1 month ago (2015-11-01 20:22:35 UTC) #11

robwu

Description was changed from ========== Track all extension frames in ProcessManager, inspect extensionoptions ProcessManager::GetRenderFrameHostsForExtension did ...

5 years, 1 month ago (2015-11-01 20:22:47 UTC) #12

Description was changed from

==========
Track all extension frames in ProcessManager, inspect extensionoptions

ProcessManager::GetRenderFrameHostsForExtension did not return all RFHs
for a given extension, because of two errors:

1. An extension can have multiple SiteInstances, so SiteInstances should
   not be compared by pointer.
2. ExtensionWebContentsObserver failed to register RFHs after a process
   swap.

I discovered this when I noticed that tests started to fail unexpectedly
after applying https://codereview.chromium.org/1413543005. That patch
changes the extension messaging API, to route messages to RFHs instead
of processes. This requires extension frames to be tracked correctly...

Extension tabs, frames, etc. are now visible at "Inspect views" in the
extensions view (when developer mode is enabled), thanks to the fact
that all extension frames are now being tracked (excluding extension
frames that are hosted in a view without classification, e.g. developer
tools and most of the GuestViews).

BUG=432875,550022
TEST=ExtensionApiTest.MessagingNoBackground does not fail any more with
https://codereview.chromium.org/1413543005 after applying this patch.
DeveloperPrivateApiTest.InspectEmbeddedOptionsPage passes.
==========

to

==========
Track all extension frames in ProcessManager, inspect extensionoptions

ProcessManager::GetRenderFrameHostsForExtension did not return all RFHs
for a given extension, because of two errors:

1. An extension can have multiple SiteInstances, so SiteInstances should
   not be compared by pointer.
2. ExtensionWebContentsObserver failed to register RFHs after a process
   swap.

I discovered this when I noticed that tests started to fail unexpectedly
after applying https://codereview.chromium.org/1413543005. That patch
changes the extension messaging API, to route messages to RFHs instead
of processes. This requires extension frames to be tracked correctly...

Extension tabs, frames, etc. are now visible at "Inspect views" in the
extensions view (when developer mode is enabled), thanks to the fact
that all extension frames are now being tracked (excluding extension
frames that are hosted in a view without classification, e.g. developer
tools and most of the GuestViews).

BUG=432875,550022
TEST=ExtensionApiTest.MessagingNoBackground does not fail any more with
https://codereview.chromium.org/1413543005 after applying this patch.
DeveloperPrivateApiTest.InspectEmbeddedOptionsPage passes.
==========

Fady Samuel

https://codereview.chromium.org/1413853005/diff/60001/extensions/browser/guest_view/extension_options/extension_options_guest.cc File extensions/browser/guest_view/extension_options/extension_options_guest.cc (right): https://codereview.chromium.org/1413853005/diff/60001/extensions/browser/guest_view/extension_options/extension_options_guest.cc#newcode113 extensions/browser/guest_view/extension_options/extension_options_guest.cc:113: SetViewType(wc, VIEW_TYPE_EXTENSION_OPTIONS); This seems incomplete? What about other GuestView ...

5 years, 1 month ago (2015-11-01 21:53:29 UTC) #13

Fady Samuel

https://codereview.chromium.org/1413853005/diff/60001/extensions/browser/guest_view/extension_options/extension_options_guest.cc File extensions/browser/guest_view/extension_options/extension_options_guest.cc (right): https://codereview.chromium.org/1413853005/diff/60001/extensions/browser/guest_view/extension_options/extension_options_guest.cc#newcode113 extensions/browser/guest_view/extension_options/extension_options_guest.cc:113: SetViewType(wc, VIEW_TYPE_EXTENSION_OPTIONS); On 2015/11/01 21:53:29, Fady Samuel wrote: > ...

5 years, 1 month ago (2015-11-01 22:09:55 UTC) #14

ncarter (slow)

https://codereview.chromium.org/1413853005/diff/60001/extensions/browser/extension_web_contents_observer.cc File extensions/browser/extension_web_contents_observer.cc (right): https://codereview.chromium.org/1413853005/diff/60001/extensions/browser/extension_web_contents_observer.cc#newcode122 extensions/browser/extension_web_contents_observer.cc:122: void ExtensionWebContentsObserver::RenderFrameHostChanged( If you are listening for changes to ...

5 years, 1 month ago (2015-11-02 17:36:20 UTC) #15

robwu

https://codereview.chromium.org/1413853005/diff/60001/extensions/browser/extension_web_contents_observer.cc File extensions/browser/extension_web_contents_observer.cc (right): https://codereview.chromium.org/1413853005/diff/60001/extensions/browser/extension_web_contents_observer.cc#newcode122 extensions/browser/extension_web_contents_observer.cc:122: void ExtensionWebContentsObserver::RenderFrameHostChanged( On 2015/11/02 17:36:20, ncarter wrote: > If ...

5 years, 1 month ago (2015-11-02 22:50:41 UTC) #16

Charlie Reis

On 2015/11/01 20:22:35, robwu wrote: > Charlie: > I changed the logic of ProcessManager::GetAllFrames to ...

5 years, 1 month ago (2015-11-03 06:45:15 UTC) #17

ncarter (slow)

https://codereview.chromium.org/1413853005/diff/60001/extensions/browser/extension_web_contents_observer.cc File extensions/browser/extension_web_contents_observer.cc (right): https://codereview.chromium.org/1413853005/diff/60001/extensions/browser/extension_web_contents_observer.cc#newcode122 extensions/browser/extension_web_contents_observer.cc:122: void ExtensionWebContentsObserver::RenderFrameHostChanged( On 2015/11/02 22:50:41, robwu wrote: > On ...

5 years, 1 month ago (2015-11-03 23:32:02 UTC) #18

https://codereview.chromium.org/1413853005/diff/60001/extensions/browser/exte...
File extensions/browser/extension_web_contents_observer.cc (right):

https://codereview.chromium.org/1413853005/diff/60001/extensions/browser/exte...
extensions/browser/extension_web_contents_observer.cc:122: void
ExtensionWebContentsObserver::RenderFrameHostChanged(
On 2015/11/02 22:50:41, robwu wrote:
> On 2015/11/02 17:36:20, ncarter wrote:
> > 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. 
> 
> 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. 

> > 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)?
> 
> A non-extension frame cannot be hosted in the extension process, so we're fine
> in that respect.

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.

https://codereview.chromium.org/1413853005/diff/60001/extensions/browser/exte...
extensions/browser/extension_web_contents_observer.cc:124:
content::RenderFrameHost* new_host) {
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.

https://codereview.chromium.org/1413853005/diff/60001/extensions/browser/exte...
extensions/browser/extension_web_contents_observer.cc:126:
InitializeRenderFrame(new_host);
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

https://codereview.chromium.org/1413853005/diff/60001/extensions/browser/extension_web_contents_observer.cc File extensions/browser/extension_web_contents_observer.cc (right): https://codereview.chromium.org/1413853005/diff/60001/extensions/browser/extension_web_contents_observer.cc#newcode122 extensions/browser/extension_web_contents_observer.cc:122: void ExtensionWebContentsObserver::RenderFrameHostChanged( On 2015/11/03 23:32:02, ncarter wrote: > On ...

5 years, 1 month ago (2015-11-04 00:34:02 UTC) #19

https://codereview.chromium.org/1413853005/diff/60001/extensions/browser/exte...
File extensions/browser/extension_web_contents_observer.cc (right):

https://codereview.chromium.org/1413853005/diff/60001/extensions/browser/exte...
extensions/browser/extension_web_contents_observer.cc:122: void
ExtensionWebContentsObserver::RenderFrameHostChanged(
On 2015/11/03 23:32:02, ncarter wrote:
> On 2015/11/02 22:50:41, robwu wrote:
> > On 2015/11/02 17:36:20, ncarter wrote:
> > > 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. 
> > 
> > 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. 
> 
> > > 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)?
> > 
> > A non-extension frame cannot be hosted in the extension process, so we're
fine
> > in that respect.
> 
> 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.

Extension frames in extension processes only. Extension frames in unprivileged
processes have almost not access to extension APIs.

> 
> 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.

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

https://codereview.chromium.org/1413853005/diff/60001/extensions/browser/exte...
extensions/browser/extension_web_contents_observer.cc:124:
content::RenderFrameHost* new_host) {
On 2015/11/03 23:32:02, ncarter wrote:
> 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.

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.

https://codereview.chromium.org/1413853005/diff/60001/extensions/browser/exte...
extensions/browser/extension_web_contents_observer.cc:126:
InitializeRenderFrame(new_host);
On 2015/11/03 23:32:02, ncarter wrote:
> 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.

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.

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

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...).

robwu

Description was changed from ========== Track all extension frames in ProcessManager, inspect extensionoptions ProcessManager::GetRenderFrameHostsForExtension did ...

5 years, 1 month ago (2015-11-04 23:22:35 UTC) #20

Description was changed from

==========
Track all extension frames in ProcessManager, inspect extensionoptions

ProcessManager::GetRenderFrameHostsForExtension did not return all RFHs
for a given extension, because of two errors:

1. An extension can have multiple SiteInstances, so SiteInstances should
   not be compared by pointer.
2. ExtensionWebContentsObserver failed to register RFHs after a process
   swap.

I discovered this when I noticed that tests started to fail unexpectedly
after applying https://codereview.chromium.org/1413543005. That patch
changes the extension messaging API, to route messages to RFHs instead
of processes. This requires extension frames to be tracked correctly...

Extension tabs, frames, etc. are now visible at "Inspect views" in the
extensions view (when developer mode is enabled), thanks to the fact
that all extension frames are now being tracked (excluding extension
frames that are hosted in a view without classification, e.g. developer
tools and most of the GuestViews).

BUG=432875,550022
TEST=ExtensionApiTest.MessagingNoBackground does not fail any more with
https://codereview.chromium.org/1413543005 after applying this patch.
DeveloperPrivateApiTest.InspectEmbeddedOptionsPage passes.
==========

to

==========
Track all extension frames in ProcessManager, inspect extensionoptions

ProcessManager::GetRenderFrameHostsForExtension did not return all RFHs
for a given extension, because of two errors:

1. An extension can have multiple SiteInstances, so SiteInstances should
   not be compared by pointer.
2. ExtensionWebContentsObserver failed to register RFHs after a process
   swap.

I discovered this when I noticed that tests started to fail unexpectedly
after applying https://codereview.chromium.org/1413543005. That patch
changes the extension messaging API, to route messages to RFHs instead
of processes. This requires extension frames to be tracked correctly...

Extension tabs, frames, etc. are now visible at "Inspect views" in the
extensions view (when developer mode is enabled), thanks to the fact
that all extension frames are now being tracked (excluding extension
frames that are hosted in a view without classification, e.g. developer
tools and most of the GuestViews).

BUG=432875,550022
TEST=ExtensionApiTest.MessagingNoBackground does not fail any more with
https://codereview.chromium.org/1413543005 after applying this patch.
DeveloperPrivateApiTest.InspectEmbeddedOptionsPage passes.
ProcessManagerBrowserTest.NoBackgroundPage passes.
==========

robwu

I've added more tests, and also a new notification (ProcessManagerObserver::OnExtensionFrameNavigated) so that I can detect ...

5 years, 1 month ago (2015-11-04 23:52:23 UTC) #21

ncarter (slow)

https://codereview.chromium.org/1413853005/diff/120001/chrome/browser/extensions/process_manager_browsertest.cc File chrome/browser/extensions/process_manager_browsertest.cc (right): https://codereview.chromium.org/1413853005/diff/120001/chrome/browser/extensions/process_manager_browsertest.cc#newcode160 chrome/browser/extensions/process_manager_browsertest.cc:160: IN_PROC_BROWSER_TEST_F(ProcessManagerBrowserTest, NoBackgroundPage) { I uploaded a test that works ...

5 years, 1 month ago (2015-11-05 20:03:23 UTC) #22

robwu

Nick, I have significantly changed the frame registration logic + added passing tests (which failed ...

5 years, 1 month ago (2015-11-12 00:33:58 UTC) #23

ncarter (slow)

https://codereview.chromium.org/1413853005/diff/180001/chrome/browser/extensions/process_manager_browsertest.cc File chrome/browser/extensions/process_manager_browsertest.cc (right): https://codereview.chromium.org/1413853005/diff/180001/chrome/browser/extensions/process_manager_browsertest.cc#newcode90 chrome/browser/extensions/process_manager_browsertest.cc:90: EXPECT_TRUE(is_loaded); I had a chat with creis, and we'd ...

5 years, 1 month ago (2015-11-24 05:22:13 UTC) #24

https://codereview.chromium.org/1413853005/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/process_manager_browsertest.cc (right):

https://codereview.chromium.org/1413853005/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/process_manager_browsertest.cc:90:
EXPECT_TRUE(is_loaded);
I had a chat with creis, and we'd probably be fine replacing
content::NavigateIframeToURL with this implementation. Feel free to pursue that.

https://codereview.chromium.org/1413853005/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/process_manager_browsertest.cc:99:
ScopedVector<TestExtensionDir> temp_dirs_;
std::vector<scoped_ptr<TestExtensionDir>>

Since we're trying to deprecate ScopedVector as of a week or two ago.

https://codereview.chromium.org/1413853005/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/process_manager_browsertest.cc:100: 
nit: no blank line

https://codereview.chromium.org/1413853005/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/process_manager_browsertest.cc:402: EXPECT_EQ(1u,
pm->GetRenderFrameHostsForExtension(extension1->id()).size());
This test is awesome.

https://codereview.chromium.org/1413853005/diff/180001/extensions/browser/ext...
File extensions/browser/extension_web_contents_observer.cc (right):

https://codereview.chromium.org/1413853005/diff/180001/extensions/browser/ext...
extensions/browser/extension_web_contents_observer.cc:68:
!IsExtensionURL(render_frame_host->GetLastCommittedURL()));
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".

https://codereview.chromium.org/1413853005/diff/180001/extensions/browser/ext...
extensions/browser/extension_web_contents_observer.cc:78: // if OOP frames is
enabled.
Is this TODO stale? I'm not sure what it means.

https://codereview.chromium.org/1413853005/diff/180001/extensions/browser/ext...
extensions/browser/extension_web_contents_observer.cc:179: }
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.

https://codereview.chromium.org/1413853005/diff/180001/extensions/browser/ext...
extensions/browser/extension_web_contents_observer.cc:239: if
(url.SchemeIs(url::kAboutScheme) && render_frame_host->GetParent())
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

https://codereview.chromium.org/1413853005/diff/180001/chrome/browser/extensions/process_manager_browsertest.cc File chrome/browser/extensions/process_manager_browsertest.cc (right): https://codereview.chromium.org/1413853005/diff/180001/chrome/browser/extensions/process_manager_browsertest.cc#newcode90 chrome/browser/extensions/process_manager_browsertest.cc:90: EXPECT_TRUE(is_loaded); On 2015/11/24 05:22:13, ncarter wrote: > I had ...

5 years ago (2015-11-25 00:44:17 UTC) #25

https://codereview.chromium.org/1413853005/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/process_manager_browsertest.cc (right):

https://codereview.chromium.org/1413853005/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/process_manager_browsertest.cc:90:
EXPECT_TRUE(is_loaded);
On 2015/11/24 05:22:13, ncarter wrote:
> I had a chat with creis, and we'd probably be fine replacing
> content::NavigateIframeToURL with this implementation. Feel free to pursue
that.

I'll do that in a separate CL to make sure that bisecting/debugging gets easier
in case the new content::NavigateIframeToURL implementation introduces
unexpected changes.

https://codereview.chromium.org/1413853005/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/process_manager_browsertest.cc:99:
ScopedVector<TestExtensionDir> temp_dirs_;
On 2015/11/24 05:22:13, ncarter wrote:
> std::vector<scoped_ptr<TestExtensionDir>>
> 
> Since we're trying to deprecate ScopedVector as of a week or two ago.

Done.

https://codereview.chromium.org/1413853005/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/process_manager_browsertest.cc:100: 
On 2015/11/24 05:22:13, ncarter wrote:
> nit: no blank line

Done.

https://codereview.chromium.org/1413853005/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/process_manager_browsertest.cc:402: EXPECT_EQ(1u,
pm->GetRenderFrameHostsForExtension(extension1->id()).size());
On 2015/11/24 05:22:13, ncarter wrote:
> This test is awesome.

Thanks. It was flaky, and I locally ran the test for a few days to find the
culprit. It turns out that ui_utils::NavigateToURL waits for StopLoading, and
the old RenderFrameHost might still be around (i.e. not deleted) at that point.
I've updated this CL with a new method to handle this edge case.

https://codereview.chromium.org/1413853005/diff/180001/extensions/browser/ext...
File extensions/browser/extension_web_contents_observer.cc (right):

https://codereview.chromium.org/1413853005/diff/180001/extensions/browser/ext...
extensions/browser/extension_web_contents_observer.cc:68:
!IsExtensionURL(render_frame_host->GetLastCommittedURL()));
On 2015/11/24 05:22:13, ncarter wrote:
> 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".

Done.

https://codereview.chromium.org/1413853005/diff/180001/extensions/browser/ext...
extensions/browser/extension_web_contents_observer.cc:78: // if OOP frames is
enabled.
On 2015/11/24 05:22:13, ncarter wrote:
> Is this TODO stale? I'm not sure what it means.

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?

https://codereview.chromium.org/1413853005/diff/180001/extensions/browser/ext...
extensions/browser/extension_web_contents_observer.cc:179: }
On 2015/11/24 05:22:13, ncarter wrote:
> 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.

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).

https://codereview.chromium.org/1413853005/diff/180001/extensions/browser/ext...
extensions/browser/extension_web_contents_observer.cc:239: if
(url.SchemeIs(url::kAboutScheme) && render_frame_host->GetParent())
On 2015/11/24 05:22:13, ncarter wrote:
> 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.

Nice! I'll rebase when your patch lands.

ncarter (slow)

lgtm https://codereview.chromium.org/1413853005/diff/180001/chrome/browser/extensions/process_manager_browsertest.cc File chrome/browser/extensions/process_manager_browsertest.cc (right): https://codereview.chromium.org/1413853005/diff/180001/chrome/browser/extensions/process_manager_browsertest.cc#newcode402 chrome/browser/extensions/process_manager_browsertest.cc:402: EXPECT_EQ(1u, pm->GetRenderFrameHostsForExtension(extension1->id()).size()); On 2015/11/25 00:44:16, robwu wrote: > ...

5 years ago (2015-11-30 22:56:46 UTC) #27

lgtm

https://codereview.chromium.org/1413853005/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/process_manager_browsertest.cc (right):

https://codereview.chromium.org/1413853005/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/process_manager_browsertest.cc:402: EXPECT_EQ(1u,
pm->GetRenderFrameHostsForExtension(extension1->id()).size());
On 2015/11/25 00:44:16, robwu wrote:
> On 2015/11/24 05:22:13, ncarter wrote:
> > This test is awesome.
> 
> Thanks. It was flaky, and I locally ran the test for a few days to find the
> culprit. It turns out that ui_utils::NavigateToURL waits for StopLoading, and
> the old RenderFrameHost might still be around (i.e. not deleted) at that
point.
> I've updated this CL with a new method to handle this edge case.

Good find; I like how you handled it. This makes me wish we had one complete set
of helper methods that covered the whole matrix of "Trigger a navigation using
some method" and "wait for some definition of done" possibilities.

TestFrameNavigationObserver is kind of a good start, but even it doesn't handle
the pending-deletion case as you do here.

https://codereview.chromium.org/1413853005/diff/180001/extensions/browser/ext...
File extensions/browser/extension_web_contents_observer.cc (right):

https://codereview.chromium.org/1413853005/diff/180001/extensions/browser/ext...
extensions/browser/extension_web_contents_observer.cc:78: // if OOP frames is
enabled.
On 2015/11/25 00:44:17, robwu wrote:
> On 2015/11/24 05:22:13, ncarter wrote:
> > Is this TODO stale? I'm not sure what it means.
> 
> 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?

--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.

https://codereview.chromium.org/1413853005/diff/180001/extensions/browser/ext...
extensions/browser/extension_web_contents_observer.cc:239: if
(url.SchemeIs(url::kAboutScheme) && render_frame_host->GetParent())
On 2015/11/25 00:44:17, robwu wrote:
> On 2015/11/24 05:22:13, ncarter wrote:
> > 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.
> 
> Nice! I'll rebase when your patch lands.

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.

https://codereview.chromium.org/1413853005/diff/200001/chrome/browser/extensi...
File chrome/browser/extensions/process_manager_browsertest.cc (right):

https://codereview.chromium.org/1413853005/diff/200001/chrome/browser/extensi...
chrome/browser/extensions/process_manager_browsertest.cc:78:
content::WebContents* web_contents_;
You don't need this; WebContentsObserver has a web_contents() accessor.

robwu

Devlin, could you review the CL, and pay attention to the ProcessManager and ExtensionWebContentsObserver? https://codereview.chromium.org/1413853005/diff/180001/extensions/browser/extension_web_contents_observer.cc ...

5 years ago (2015-12-01 00:45:26 UTC) #28

Devlin, could you review the CL, and pay attention to the ProcessManager and
ExtensionWebContentsObserver?

https://codereview.chromium.org/1413853005/diff/180001/extensions/browser/ext...
File extensions/browser/extension_web_contents_observer.cc (right):

https://codereview.chromium.org/1413853005/diff/180001/extensions/browser/ext...
extensions/browser/extension_web_contents_observer.cc:78: // if OOP frames is
enabled.
On 2015/11/30 22:56:45, ncarter wrote:
> On 2015/11/25 00:44:17, robwu wrote:
> > On 2015/11/24 05:22:13, ncarter wrote:
> > > Is this TODO stale? I'm not sure what it means.
> > 
> > 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?
> 
> --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.

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).

https://codereview.chromium.org/1413853005/diff/180001/extensions/browser/ext...
extensions/browser/extension_web_contents_observer.cc:239: if
(url.SchemeIs(url::kAboutScheme) && render_frame_host->GetParent())
On 2015/11/30 22:56:45, ncarter wrote:
> On 2015/11/25 00:44:17, robwu wrote:
> > On 2015/11/24 05:22:13, ncarter wrote:
> > > 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.
> > 
> > Nice! I'll rebase when your patch lands.
> 
> 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.

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?)

https://codereview.chromium.org/1413853005/diff/200001/chrome/browser/extensi...
File chrome/browser/extensions/process_manager_browsertest.cc (right):

https://codereview.chromium.org/1413853005/diff/200001/chrome/browser/extensi...
chrome/browser/extensions/process_manager_browsertest.cc:78:
content::WebContents* web_contents_;
On 2015/11/30 22:56:46, ncarter wrote:
> You don't need this; WebContentsObserver has a web_contents() accessor.

Done.

Devlin

https://codereview.chromium.org/1413853005/diff/200001/chrome/browser/extensions/api/developer_private/developer_private_apitest.cc File chrome/browser/extensions/api/developer_private/developer_private_apitest.cc (right): https://codereview.chromium.org/1413853005/diff/200001/chrome/browser/extensions/api/developer_private/developer_private_apitest.cc#newcode91 chrome/browser/extensions/api/developer_private/developer_private_apitest.cc:91: LoadExtension(dir.AppendASCII("extensions/delayed_install/v1")); nit: typically prefer .AppendASCII rather than platform-specific separators. ...

5 years ago (2015-12-01 00:58:20 UTC) #29

robwu

I've fixed the nits. Devlin: Your previous review is based on an old patch set, ...

5 years ago (2015-12-01 17:19:57 UTC) #30

ncarter (slow)

https://codereview.chromium.org/1413853005/diff/260001/extensions/browser/extension_web_contents_observer.cc File extensions/browser/extension_web_contents_observer.cc (right): https://codereview.chromium.org/1413853005/diff/260001/extensions/browser/extension_web_contents_observer.cc#newcode241 extensions/browser/extension_web_contents_observer.cc:241: url::Origin(site_url)) && I think this works only because extension ...

5 years ago (2015-12-01 17:51:42 UTC) #31

Devlin

https://codereview.chromium.org/1413853005/diff/260001/chrome/browser/extensions/api/developer_private/inspectable_views_finder.cc File chrome/browser/extensions/api/developer_private/inspectable_views_finder.cc (right): https://codereview.chromium.org/1413853005/diff/260001/chrome/browser/extensions/api/developer_private/inspectable_views_finder.cc#newcode156 chrome/browser/extensions/api/developer_private/inspectable_views_finder.cc:156: host_type == VIEW_TYPE_APP_WINDOW) { Why don't we want to ...

5 years ago (2015-12-01 21:40:49 UTC) #32

robwu

https://codereview.chromium.org/1413853005/diff/200001/chrome/browser/extensions/process_manager_browsertest.cc File chrome/browser/extensions/process_manager_browsertest.cc (right): https://codereview.chromium.org/1413853005/diff/200001/chrome/browser/extensions/process_manager_browsertest.cc#newcode169 chrome/browser/extensions/process_manager_browsertest.cc:169: std::vector<scoped_ptr<TestExtensionDir>> temp_dirs_; On 2015/12/01 00:58:20, Devlin wrote: > nit: ...

5 years ago (2015-12-01 23:19:58 UTC) #33

https://codereview.chromium.org/1413853005/diff/200001/chrome/browser/extensi...
File chrome/browser/extensions/process_manager_browsertest.cc (right):

https://codereview.chromium.org/1413853005/diff/200001/chrome/browser/extensi...
chrome/browser/extensions/process_manager_browsertest.cc:169:
std::vector<scoped_ptr<TestExtensionDir>> temp_dirs_;
On 2015/12/01 00:58:20, Devlin wrote:
> nit: DISALLOW_COPY_AND_ASSIGN

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

https://codereview.chromium.org/1413853005/diff/260001/chrome/browser/extensi...
File chrome/browser/extensions/api/developer_private/inspectable_views_finder.cc
(right):

https://codereview.chromium.org/1413853005/diff/260001/chrome/browser/extensi...
chrome/browser/extensions/api/developer_private/inspectable_views_finder.cc:156:
host_type == VIEW_TYPE_APP_WINDOW) {
On 2015/12/01 21:40:49, Devlin wrote:
> Why don't we want to include APP_WINDOW here?  This is for inspectable views,
so
> I think they should show up, right?

App windows are already dealt with by GetAppWindowViewsForExtension, see
https://chromium.googlesource.com/chromium/src/+/36bdeb3fb8984da7875aa970ba9f...

Without the above check, the assert at line 57 in
DeveloperPrivateApiTest.InspectAppWindowView fails because the app window is
reported twice
(https://chromium.googlesource.com/chromium/src/+/e7057530af5dbb422f1de1c72785...).

https://codereview.chromium.org/1413853005/diff/260001/chrome/browser/extensi...
File chrome/browser/extensions/process_manager_browsertest.cc (right):

https://codereview.chromium.org/1413853005/diff/260001/chrome/browser/extensi...
chrome/browser/extensions/process_manager_browsertest.cc:44: // either present
in the tab, or deleted.
On 2015/12/01 21:40:49, Devlin wrote:
> Elaborate on why we consider the case of all frames in the tab.

Done 2x.

https://codereview.chromium.org/1413853005/diff/260001/chrome/browser/extensi...
chrome/browser/extensions/process_manager_browsertest.cc:109: "<iframe
id=bgframe src=empty.html></iframe>");
On 2015/12/01 21:40:49, Devlin wrote:
> nit: id="bgframe" (also below)

Done (using single quotes instead of double because of the string literal).

https://codereview.chromium.org/1413853005/diff/260001/chrome/browser/extensi...
chrome/browser/extensions/process_manager_browsertest.cc:383: // Tests extension
frames in non-extension page.
On 2015/12/01 21:40:49, Devlin wrote:
> Might be worth expanding on this to explain why the result differs in OOPIF
and
> non-OOPIF.

This is already explained before the test. When OOPIF is enabled, extension
frames are hosted in the extension process.

https://codereview.chromium.org/1413853005/diff/260001/extensions/browser/ext...
File extensions/browser/extension_web_contents_observer.cc (right):

https://codereview.chromium.org/1413853005/diff/260001/extensions/browser/ext...
extensions/browser/extension_web_contents_observer.cc:241:
url::Origin(site_url)) &&
On 2015/12/01 17:51:42, ncarter wrote:
> I think this works only because extension urls don't have ports and
subdomains.
> 
> If these could be http URLs, then this origin comparison would fail --
> GetLastCommittedOrigin() could be http://mail.google.com:1234, but if you
> canonicalize that to a site URL, it's http://google.com, stripping the port
and
> subdomain. (This canonicalization happens is because http://mail.google.com
could
> legally modify its origin by assigning document.domain to a superdomain
> ('google.com'), so the canonical site URL has to match for all origins that
> could become same-origin through this kind of modification.
> 
> I think the slightly safer thing to do is to convert the origin back to a site
> URL, and do the equality comparison in SiteURL-space:
> 
> if (origin.unique() ||
>   site_url !=
>   content::SiteInstance::GetSiteForURL(GURL(origin.Serialize()))) {
> 
> }
> 
> This also will map an http://app.com origin back to its effective
chrome-extension URL.
> I am thinking we should probably expose
> content::SiteInstance::GetSiteForOrigin() to make this pattern more obvious --
I
> will take care of that.

Done.

https://codereview.chromium.org/1413853005/diff/260001/extensions/browser/ext...
extensions/browser/extension_web_contents_observer.cc:244:
render_frame_host->GetLastCommittedURL())))
On 2015/12/01 17:51:42, ncarter wrote:
> Does this MatchesURL logic work properly for the about:blank case?

No.

https://codereview.chromium.org/1413853005/diff/260001/extensions/browser/pro...
File extensions/browser/process_manager_observer.h (right):

https://codereview.chromium.org/1413853005/diff/260001/extensions/browser/pro...
extensions/browser/process_manager_observer.h:45: virtual void
OnExtensionFrameNavigated(
On 2015/12/01 21:40:49, Devlin wrote:
> Am I just missing it?  Where is this used?

This will be used by https://codereview.chromium.org/1413543005/
 in chrome/browser/extensions/api/messaging/extension_message_port.cc. I want to
do all port lifetime management in the browser process, to make sure that ports
get cleaned up as soon as possible, regardless of renderer crashes etc.

Devlin

lgtm if Nick's happy. https://codereview.chromium.org/1413853005/diff/260001/chrome/browser/extensions/api/developer_private/inspectable_views_finder.cc File chrome/browser/extensions/api/developer_private/inspectable_views_finder.cc (right): https://codereview.chromium.org/1413853005/diff/260001/chrome/browser/extensions/api/developer_private/inspectable_views_finder.cc#newcode156 chrome/browser/extensions/api/developer_private/inspectable_views_finder.cc:156: host_type == VIEW_TYPE_APP_WINDOW) { On ...

5 years ago (2015-12-01 23:29:58 UTC) #34

robwu

https://codereview.chromium.org/1413853005/diff/260001/chrome/browser/extensions/process_manager_browsertest.cc File chrome/browser/extensions/process_manager_browsertest.cc (right): https://codereview.chromium.org/1413853005/diff/260001/chrome/browser/extensions/process_manager_browsertest.cc#newcode109 chrome/browser/extensions/process_manager_browsertest.cc:109: "<iframe id=bgframe src=empty.html></iframe>"); On 2015/12/01 23:29:58, Devlin wrote: > ...

5 years ago (2015-12-01 23:51:22 UTC) #35

robwu

+tommi for OWNER stamp There is only one extra line in chrome/browser/speech/chrome_speech_recognition_manager_delegate.cc, trivial to review.

5 years ago (2015-12-01 23:52:18 UTC) #37

Devlin

On 2015/12/01 23:51:22, robwu wrote: > Apologies, I forgot to commit it. Try again. Still ...

5 years ago (2015-12-01 23:52:45 UTC) #38

tommi (sloooow) - chröme

lgtm for chrome_speech_recognition_manager_delegate.cc

5 years ago (2015-12-02 08:32:00 UTC) #39

robwu

The patchset sent to the CQ was uploaded after l-g-t-m from fsamuel@chromium.org, nick@chromium.org, rdevlin.cronin@chromium.org, tommi@chromium.org ...

5 years ago (2015-12-02 10:28:22 UTC) #41

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1413853005/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1413853005/320001

5 years ago (2015-12-02 10:29:05 UTC) #42

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

5 years ago (2015-12-02 11:36:10 UTC) #43

commit-bot: I haz the power

Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_rel_ng/builds/136824)

5 years ago (2015-12-02 11:36:12 UTC) #44

robwu

Devlin, I did 2 amall changes: 1) Removed 2 DCHECKs. 2) Changed frame filter logic ...

5 years ago (2015-12-05 00:39:40 UTC) #48

Devlin

Dang. At one point, I tried to make sure that every WebContents had an assigned ...

5 years ago (2015-12-05 00:57:24 UTC) #49

Devlin

The patchset sent to the CQ was uploaded after l-g-t-m from fsamuel@chromium.org, tommi@chromium.org, nick@chromium.org Link ...

5 years ago (2015-12-05 00:57:59 UTC) #51

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1413853005/420001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1413853005/420001

5 years ago (2015-12-05 00:59:27 UTC) #52

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

5 years ago (2015-12-05 01:47:10 UTC) #53

commit-bot: I haz the power

Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL) win_chromium_compile_dbg_ng on ...

5 years ago (2015-12-05 01:47:11 UTC) #54

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1413853005/420001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1413853005/420001

5 years ago (2015-12-05 09:43:11 UTC) #56

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

5 years ago (2015-12-05 11:43:54 UTC) #57

commit-bot: I haz the power

Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL) win_chromium_compile_dbg_ng on ...

5 years ago (2015-12-05 11:43:55 UTC) #58

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1413853005/420001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1413853005/420001

5 years ago (2015-12-05 20:35:07 UTC) #60

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

5 years ago (2015-12-05 22:36:27 UTC) #61

commit-bot: I haz the power

Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_TIMED_OUT, no build URL) win_chromium_compile_dbg_ng on ...

5 years ago (2015-12-05 22:36:28 UTC) #62

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1413853005/420001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1413853005/420001

5 years ago (2015-12-06 10:51:25 UTC) #64

commit-bot: I haz the power

Description was changed from ========== Track all extension frames in ProcessManager, inspect extensionoptions ProcessManager::GetRenderFrameHostsForExtension did ...

5 years ago (2015-12-06 12:40:37 UTC) #66

Message was sent while issue was closed.

Description was changed from

==========
Track all extension frames in ProcessManager, inspect extensionoptions

ProcessManager::GetRenderFrameHostsForExtension did not return all RFHs
for a given extension, because of two errors:

1. An extension can have multiple SiteInstances, so SiteInstances should
   not be compared by pointer.
2. ExtensionWebContentsObserver failed to register RFHs after a process
   swap.

I discovered this when I noticed that tests started to fail unexpectedly
after applying https://codereview.chromium.org/1413543005. That patch
changes the extension messaging API, to route messages to RFHs instead
of processes. This requires extension frames to be tracked correctly...

Extension tabs, frames, etc. are now visible at "Inspect views" in the
extensions view (when developer mode is enabled), thanks to the fact
that all extension frames are now being tracked (excluding extension
frames that are hosted in a view without classification, e.g. developer
tools and most of the GuestViews).

BUG=432875,550022
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
TEST=ExtensionApiTest.MessagingNoBackground does not fail any more with
https://codereview.chromium.org/1413543005 after applying this patch.
DeveloperPrivateApiTest.InspectEmbeddedOptionsPage passes.
ProcessManagerBrowserTest.NoBackgroundPage passes.
ProcessManagerBrowserTest.FrameClassification passes with and without
OOPIF.
==========

to

==========
Track all extension frames in ProcessManager, inspect extensionoptions

ProcessManager::GetRenderFrameHostsForExtension did not return all RFHs
for a given extension, because of two errors:

1. An extension can have multiple SiteInstances, so SiteInstances should
   not be compared by pointer.
2. ExtensionWebContentsObserver failed to register RFHs after a process
   swap.

I discovered this when I noticed that tests started to fail unexpectedly
after applying https://codereview.chromium.org/1413543005. That patch
changes the extension messaging API, to route messages to RFHs instead
of processes. This requires extension frames to be tracked correctly...

Extension tabs, frames, etc. are now visible at "Inspect views" in the
extensions view (when developer mode is enabled), thanks to the fact
that all extension frames are now being tracked (excluding extension
frames that are hosted in a view without classification, e.g. developer
tools and most of the GuestViews).

BUG=432875,550022
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
TEST=ExtensionApiTest.MessagingNoBackground does not fail any more with
https://codereview.chromium.org/1413543005 after applying this patch.
DeveloperPrivateApiTest.InspectEmbeddedOptionsPage passes.
ProcessManagerBrowserTest.NoBackgroundPage passes.
ProcessManagerBrowserTest.FrameClassification passes with and without
OOPIF.

Committed: https://crrev.com/cdcc4b87e40c1b783541444d9484f33e81a177ba
Cr-Commit-Position: refs/heads/master@{#363368}
==========

commit-bot: I haz the power

5 years ago (2015-12-06 12:40:38 UTC) #67

Message was sent while issue was closed.

Patchset 19 (id:??) landed as
https://crrev.com/cdcc4b87e40c1b783541444d9484f33e81a177ba
Cr-Commit-Position: refs/heads/master@{#363368}

Issue 1413853005: Track all extension frames in ProcessManager, inspect extensionoptions (Closed)

Description

Patch Set 1 #

Patch Set 2 : rebase #

Patch Set 3 : fix nits, fix failing tests (related to crbug.com/550022) #

Patch Set 4 : s/msg/message/ #

Patch Set 5 : Explain purpose of RenderFrameHostChanged #

Patch Set 6 : Add ProcessManagerBrowserTest.NoBackgroundPage test #

Patch Set 7 : Add ProcessManagerObserver::OnExtensionFrameNavigated notification #

Patch Set 8 : Only register extension frames + tests #

Patch Set 9 : rebase #

Patch Set 10 : include extension_process_policy.h #

Patch Set 11 : Nits, treat hosted apps as extensions, no test flakiness #

Patch Set 12 : nits + sandbox tests #

Patch Set 13 : rebase #

Patch Set 14 : nits, git cl format, fix error in ExtensionWebContentsObserver::GetExtensionFromFrame, exclude kGue… #

Patch Set 15 : more nits, free of compiler errors #

Patch Set 16 : Add comments that I forgot to commit in patch set 15 #

Patch Set 17 : Remove DCHECK and update comment (assumption was incorrect) #

Patch Set 18 : Update DCHECK to avoid getting hit in DriveFirstRunTest. #

Patch Set 19 : Cleanup + remove DCHECK #

Messages