Allow whitelisted content scripts to be injected in WebViews.

Allow whitelisted content scripts to be injected in WebViews.

The code that tried to inject ChromeVox into WebViews in
chrome_web_view_guest_delegate.cc wasn't working. Instead, this allows
whitelisted content scripts to be injected into WebViews.

BUG=484904
Committed: https://crrev.com/0eee9bb6ceb5f90f051ede8665fd05319328b54d
Cr-Commit-Position: refs/heads/master@{#330193}

[dmazzoni, 2015-05-14 22:24:24]

dmazzoni@chromium.org changed reviewers:
+ fsamuel@chromium.org, kalman@chromium.org

[Fady Samuel, 2015-05-14 22:31:45]

This is really cool! Looks like this actually simplifies the code in Webview! A
few <webview> side cleanups please, and then I'll sign off on it :-)

https://codereview.chromium.org/1140173003/diff/1/chrome/browser/guest_view/w...
File chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.cc
(right):

https://codereview.chromium.org/1140173003/diff/1/chrome/browser/guest_view/w...
chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.cc:86: bool
is_main_frame) {
Remove this method and remove it from WebViewGuestDelegate please?

https://codereview.chromium.org/1140173003/diff/1/chrome/browser/guest_view/w...
chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.cc:89: void
ChromeWebViewGuestDelegate::OnDidInitialize() {
Remove this method and remove it from WebViewGuestDelegate please?

https://codereview.chromium.org/1140173003/diff/1/chrome/browser/guest_view/w...
chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.cc:93:
content::RenderFrameHost* render_frame_host) {
Remove this method and remove it from WebViewGuestDelegate please?

[dmazzoni, 2015-05-14 22:50:46]

Great! Always nice to clean up the code as a side benefit.

https://codereview.chromium.org/1140173003/diff/1/chrome/browser/guest_view/w...
File chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.cc
(right):

https://codereview.chromium.org/1140173003/diff/1/chrome/browser/guest_view/w...
chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.cc:86: bool
is_main_frame) {
On 2015/05/14 22:31:45, Fady Samuel wrote:
> Remove this method and remove it from WebViewGuestDelegate please?

Done.

https://codereview.chromium.org/1140173003/diff/1/chrome/browser/guest_view/w...
chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.cc:89: void
ChromeWebViewGuestDelegate::OnDidInitialize() {
On 2015/05/14 22:31:45, Fady Samuel wrote:
> Remove this method and remove it from WebViewGuestDelegate please?

Done.

https://codereview.chromium.org/1140173003/diff/1/chrome/browser/guest_view/w...
chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.cc:93:
content::RenderFrameHost* render_frame_host) {
On 2015/05/14 22:31:45, Fady Samuel wrote:
> Remove this method and remove it from WebViewGuestDelegate please?

Done.

[not at google - send to devlin, 2015-05-14 23:11:18]

https://codereview.chromium.org/1140173003/diff/20001/extensions/browser/user...
File extensions/browser/user_script_loader.cc (right):

https://codereview.chromium.org/1140173003/diff/20001/extensions/browser/user...
extensions/browser/user_script_loader.cc:383: bool whitelisted_only =
process->IsIsolatedGuest() && host_id().id().empty();
I need to look into this more, but my reaction is, why this permission checking
is done in as part of ExtensionMsg_UpdateUserScripts. That's just notifying the
renderer that new user scripts are available, but they actually already are.
Seems a little fragile.

A more robust way would be to make sure we only share component extensions
content script memory regions with isolated guests, like,

if (process->IsIsolatedGuest() && !extension->is_component_or_whatever()) {
  // don't share the shared memory handle with isolated guests
  // unless this is a component extension.
  return;
}

if there were an extension available to SendUpdate. Which obviously there isn't
- actually this gets the *set* of things to update, so you'd need to filter out
any of the |changed_hosts| which aren't component extensions (nor guest view
scripts) if it's a guest view.

Hopefully that makes sense.

[dmazzoni, 2015-05-14 23:28:37]

https://codereview.chromium.org/1140173003/diff/20001/extensions/browser/user...
File extensions/browser/user_script_loader.cc (right):

https://codereview.chromium.org/1140173003/diff/20001/extensions/browser/user...
extensions/browser/user_script_loader.cc:383: bool whitelisted_only =
process->IsIsolatedGuest() && host_id().id().empty();
On 2015/05/14 23:11:18, kalman wrote:
> I need to look into this more, but my reaction is, why this permission
checking
> is done in as part of ExtensionMsg_UpdateUserScripts. That's just notifying
the
> renderer that new user scripts are available, but they actually already are.
> Seems a little fragile.

Sure.

> A more robust way would be to make sure we only share component extensions
> content script memory regions with isolated guests, like,
> 
> if (process->IsIsolatedGuest() && !extension->is_component_or_whatever()) {
>   // don't share the shared memory handle with isolated guests
>   // unless this is a component extension.
>   return;
> }
> 
> if there were an extension available to SendUpdate. Which obviously there
isn't
> - actually this gets the *set* of things to update, so you'd need to filter
out
> any of the |changed_hosts| which aren't component extensions (nor guest view
> scripts) if it's a guest view.
> 
> Hopefully that makes sense.

Filtering out extensions that aren't allowed to run in that renderer from
changed_hosts makes perfect sense, I can do that.

Is it really possible to filter out just some of the content scripts from the
shared memory now? I thought it was just one big shared memory region now,
so it'd require a lot of refactoring to split it into a separate region per
extension - and it'd probably waste a lot of resources too.

Currently each content script is shared with every process (other than webview
guest
processes), even though many / most content scripts have origin restrictions
so they'll never be used in some renderers.

[not at google - send to devlin, 2015-05-14 23:30:18]

> Filtering out extensions that aren't allowed to run in that renderer from
> changed_hosts makes perfect sense, I can do that.
> 
> Is it really possible to filter out just some of the content scripts from the
> shared memory now? I thought it was just one big shared memory region now,
> so it'd require a lot of refactoring to split it into a separate region per
> extension - and it'd probably waste a lot of resources too.

It was like that for a long time, but somebody did that lot-of-refactoring a
little while ago I believe. Resource-wise I guess it turned out ok, but yes, it
was worrying. Still is worrying.

[Fady Samuel, 2015-05-14 23:32:00]

One more bit of cleanup :-) This generally looks good from a web_view directory
perspective, but I'll wait until you address kalman@'s comment to sign off.

https://codereview.chromium.org/1140173003/diff/20001/extensions/browser/gues...
File extensions/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/1140173003/diff/20001/extensions/browser/gues...
extensions/browser/guest_view/web_view/web_view_guest.cc:810: void
WebViewGuest::DocumentLoadedInFrame(
Delete this method?

[dmazzoni, 2015-05-14 23:47:16]

On Thu, May 14, 2015 at 4:30 PM, <kalman@chromium.org> wrote:

> Filtering out extensions that aren't allowed to run in that renderer from
>> changed_hosts makes perfect sense, I can do that.
>>
>
>  Is it really possible to filter out just some of the content scripts from
>> the
>> shared memory now? I thought it was just one big shared memory region now,
>> so it'd require a lot of refactoring to split it into a separate region
>> per
>> extension - and it'd probably waste a lot of resources too.
>>
>
> It was like that for a long time, but somebody did that lot-of-refactoring
> a
> little while ago I believe. Resource-wise I guess it turned out ok, but
> yes, it
> was worrying. Still is worrying.


I don't see it in the code. UserScriptLoader has a single SharedMemory for
all of its content scripts.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[not at google - send to devlin, 2015-05-14 23:57:40]

On 2015/05/14 23:47:16, dmazzoni wrote:
> On Thu, May 14, 2015 at 4:30 PM, <mailto:kalman@chromium.org> wrote:
> 
> > Filtering out extensions that aren't allowed to run in that renderer from
> >> changed_hosts makes perfect sense, I can do that.
> >>
> >
> >  Is it really possible to filter out just some of the content scripts from
> >> the
> >> shared memory now? I thought it was just one big shared memory region now,
> >> so it'd require a lot of refactoring to split it into a separate region
> >> per
> >> extension - and it'd probably waste a lot of resources too.
> >>
> >
> > It was like that for a long time, but somebody did that lot-of-refactoring
> > a
> > little while ago I believe. Resource-wise I guess it turned out ok, but
> > yes, it
> > was worrying. Still is worrying.
> 
> 
> I don't see it in the code. UserScriptLoader has a single SharedMemory for
> all of its content scripts.

You're right. The refactor did happen if you follow up the
inheritance/construction chain, but it leads to two factory classes, one for
per-extension regions (the "declarative" one) and one shared across all
extensions (the "shared" one). Pity we didn't just bite the bullet and make them
individual by default and save the indirection, but perhaps it would have been
too wasteful.

So, I guess we're back to your original CL. I'll comment inline.

[not at google - send to devlin, 2015-05-15 00:05:36]

lgtm though looks like there is a test failure relating to this. ping if fixing
it is non-trivial.

https://codereview.chromium.org/1140173003/diff/20001/extensions/renderer/use...
File extensions/renderer/user_script_set.cc (right):

https://codereview.chromium.org/1140173003/diff/20001/extensions/renderer/use...
extensions/renderer/user_script_set.cc:146: const
ExtensionsClient::ScriptingWhitelist& whitelist =
PermissionsData::CanExecuteScriptEverywhere is simpler. You could even choose to
put it in a single conditional then...

[not at google - send to devlin, 2015-05-15 04:47:34]

1 last comment for the paranoid.

https://codereview.chromium.org/1140173003/diff/20001/extensions/renderer/use...
File extensions/renderer/user_script_set.cc (right):

https://codereview.chromium.org/1140173003/diff/20001/extensions/renderer/use...
extensions/renderer/user_script_set.cc:197: if (host_id.type() ==
HostID::EXTENSIONS) {
Could you also paranoid-double-check with a CHECK() in here that if this is a
guestview process the Extension must be able to access all pages?

This little block of code should really use a switch, but anyhow, just something
like

case HostID::EXTENSIONS: {
  const Extension* extension = extensions_.GetByID(host_id.id());
  if (!extension)
    return injection_host.Pass();
  // Extra sanity check - if this is a guestview process, we shouldn't
  // be populating |scripts_| for non-component extensions in the first
  // place, but be really sure.
  if (/* is a guestview process */)
    CHECK(PermissionsData::CanExecuteScriptEverywhere(extension));
  injection_host.reset(new ExtensionInjectionHost(extension));
  break;
}

... assuming there is a method that lets me find "is a guestview process". (I
think sending down the bool in the IPC is still the right way to do the check,
though).

[dmazzoni, 2015-05-15 17:33:50]

On 2015/05/15 04:47:34, kalman wrote:
> ... assuming there is a method that lets me find "is a guestview process". (I
> think sending down the bool in the IPC is still the right way to do the check,
> though).

I don't believe there's a way for a process to know if it's a guestview process
currently. I'm not sure if there's just never been a need or if that's a
deliberate design decision. Fady?

I thought of maybe adding a new extensions::ViewType, but it looks like there's
already a TODO to get rid of that.

[dmazzoni, 2015-05-15 17:33:57]

https://codereview.chromium.org/1140173003/diff/20001/extensions/browser/gues...
File extensions/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/1140173003/diff/20001/extensions/browser/gues...
extensions/browser/guest_view/web_view/web_view_guest.cc:810: void
WebViewGuest::DocumentLoadedInFrame(
On 2015/05/14 23:31:59, Fady Samuel wrote:
> Delete this method?

Done.

https://codereview.chromium.org/1140173003/diff/20001/extensions/renderer/use...
File extensions/renderer/user_script_set.cc (right):

https://codereview.chromium.org/1140173003/diff/20001/extensions/renderer/use...
extensions/renderer/user_script_set.cc:146: const
ExtensionsClient::ScriptingWhitelist& whitelist =
On 2015/05/15 00:05:36, kalman wrote:
> PermissionsData::CanExecuteScriptEverywhere is simpler. You could even choose
to
> put it in a single conditional then...

PermissionsData::CanExecuteScriptEverywhere takes an Extension but we just have
a UserScript, so I added a new static function
PermissionsData::IsExtensionIdWhitelisted.

https://codereview.chromium.org/1140173003/diff/40001/chrome/renderer/extensi...
File chrome/renderer/extensions/renderer_permissions_policy_delegate.cc (right):

https://codereview.chromium.org/1140173003/diff/40001/chrome/renderer/extensi...
chrome/renderer/extensions/renderer_permissions_policy_delegate.cc:33: if
(PermissionsData::IsExtensionIdWhitelisted(extension->id()))
Note: should this be PermissionsData::CanExecuteScriptEverywhere() instead? That
would also return true for component extensions, which would be a change in
behavior. Would that be more correct or not?

[not at google - send to devlin, 2015-05-15 17:38:11]

https://codereview.chromium.org/1140173003/diff/20001/extensions/renderer/use...
File extensions/renderer/user_script_set.cc (right):

https://codereview.chromium.org/1140173003/diff/20001/extensions/renderer/use...
extensions/renderer/user_script_set.cc:146: const
ExtensionsClient::ScriptingWhitelist& whitelist =
On 2015/05/15 17:33:57, dmazzoni wrote:
> On 2015/05/15 00:05:36, kalman wrote:
> > PermissionsData::CanExecuteScriptEverywhere is simpler. You could even
choose
> to
> > put it in a single conditional then...
> 
> PermissionsData::CanExecuteScriptEverywhere takes an Extension but we just
have
> a UserScript, so I added a new static function
> PermissionsData::IsExtensionIdWhitelisted.

You should be able to extract an Extension from |extensions_|?

https://codereview.chromium.org/1140173003/diff/40001/chrome/renderer/extensi...
File chrome/renderer/extensions/renderer_permissions_policy_delegate.cc (right):

https://codereview.chromium.org/1140173003/diff/40001/chrome/renderer/extensi...
chrome/renderer/extensions/renderer_permissions_policy_delegate.cc:33: if
(PermissionsData::IsExtensionIdWhitelisted(extension->id()))
On 2015/05/15 17:33:57, dmazzoni wrote:
> Note: should this be PermissionsData::CanExecuteScriptEverywhere() instead?
That
> would also return true for component extensions, which would be a change in
> behavior. Would that be more correct or not?

I think basically everything should use CanExecuteScriptEverywhere.
ExtensionsClient is just a giant global delegate interface, better to keep that
as much as possible an implementation detail of whoever needs it rather than an
interface to rely on (I would hope).

[dmazzoni, 2015-05-15 17:56:28]

Issues addressed, and I fixed the test failure too.

https://codereview.chromium.org/1140173003/diff/20001/extensions/renderer/use...
File extensions/renderer/user_script_set.cc (right):

https://codereview.chromium.org/1140173003/diff/20001/extensions/renderer/use...
extensions/renderer/user_script_set.cc:146: const
ExtensionsClient::ScriptingWhitelist& whitelist =
On 2015/05/15 17:38:11, kalman wrote:
> On 2015/05/15 17:33:57, dmazzoni wrote:
> > On 2015/05/15 00:05:36, kalman wrote:
> > > PermissionsData::CanExecuteScriptEverywhere is simpler. You could even
> choose
> > to
> > > put it in a single conditional then...
> > 
> > PermissionsData::CanExecuteScriptEverywhere takes an Extension but we just
> have
> > a UserScript, so I added a new static function
> > PermissionsData::IsExtensionIdWhitelisted.
> 
> You should be able to extract an Extension from |extensions_|?

Done.

https://codereview.chromium.org/1140173003/diff/40001/chrome/renderer/extensi...
File chrome/renderer/extensions/renderer_permissions_policy_delegate.cc (right):

https://codereview.chromium.org/1140173003/diff/40001/chrome/renderer/extensi...
chrome/renderer/extensions/renderer_permissions_policy_delegate.cc:33: if
(PermissionsData::IsExtensionIdWhitelisted(extension->id()))
On 2015/05/15 17:38:11, kalman wrote:
> On 2015/05/15 17:33:57, dmazzoni wrote:
> > Note: should this be PermissionsData::CanExecuteScriptEverywhere() instead?
> That
> > would also return true for component extensions, which would be a change in
> > behavior. Would that be more correct or not?
> 
> I think basically everything should use CanExecuteScriptEverywhere.
> ExtensionsClient is just a giant global delegate interface, better to keep
that
> as much as possible an implementation detail of whoever needs it rather than
an
> interface to rely on (I would hope).

Done.

[dmazzoni, 2015-05-15 17:57:21]

+nasko for OWNERS review of extensions/common/extension_messages.h

[dmazzoni, 2015-05-15 17:57:54]

dmazzoni@chromium.org changed reviewers:
+ nasko@chromium.org

[dmazzoni, 2015-05-15 17:57:55]

Oops, trying again - +nasko for OWNERS review of
extensions/common/extension_messages.h

[nasko, 2015-05-15 19:44:07]

IPC LGTM

[Fady Samuel, 2015-05-15 20:03:08]

guest_view lgtm!

[dmazzoni, 2015-05-15 20:13:01]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2015-05-15 20:13:02]

The patchset sent to the CQ was uploaded after l-g-t-m from kalman@chromium.org,
nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1140173003/#ps100002
(title: "Don't assume extensions_->GetByID will succeed")

[commit-bot: I haz the power, 2015-05-15 20:14:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1140173003/100002

[commit-bot: I haz the power, 2015-05-15 20:59:18]

Committed patchset #7 (id:100002)

[commit-bot: I haz the power, 2015-05-18 11:27:22]

Patchset 7 (id:??) landed as
https://crrev.com/0eee9bb6ceb5f90f051ede8665fd05319328b54d
Cr-Commit-Position: refs/heads/master@{#330193}