Resubmit: Block content scripts from executing until user grants permission

Resubmit: Block content scripts from executing until user grants permission

Original CL: https://codereview.chromium.org/288053002/
Original Description:
Prevent extensions with <all_urls> from running content scripts without user
consent if the scripts-require-action switch is on.

-----------------------------------------------

This had a problem in that when user scripts are updated, the old versions
are invalidated (as they rely on StringPieces, which do not actually own
content). Fix is to update all user scripts, even if they didn't actually
change.

Also add in ActiveScriptController removing actions for unloaded extensions.

TBR=jschuh@chromium.org (for extension_messages.h, no change from original patch)
BUG=362353
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=274659

[not at google - send to devlin, 2014-06-02 18:42:55]

https://codereview.chromium.org/313453002/diff/20001/chrome/common/extensions...
File chrome/common/extensions/manifest_handlers/content_scripts_handler.cc
(right):

https://codereview.chromium.org/313453002/diff/20001/chrome/common/extensions...
chrome/common/extensions/manifest_handlers/content_scripts_handler.cc:430:
user_script.set_id(g_next_user_script_id++);
is there a test for ContentScriptsHandler that we could update?

https://codereview.chromium.org/313453002/diff/20001/extensions/renderer/user...
File extensions/renderer/user_script_slave.cc (right):

https://codereview.chromium.org/313453002/diff/20001/extensions/renderer/user...
extensions/renderer/user_script_slave.cc:187: for
(ScopedVector<ScriptInjection>::iterator iter =
O(n^2) makes me nervous. It seems like using a std::map for script_injections
would be just as easy as a vector, apart from needing to use linked_ptrs
everywhere.

chat to me in person if you have questions about this, but FYI, linked_ptrs add
a lot of syntactic noise and I like to typedef std::map<int64,
linked_ptr<ScriptInjection> > as something.

[Devlin, 2014-06-02 21:04:30]

Also added an id-pickling test for user script, and, more importantly, a test
for removing an extension with pending injections.

https://codereview.chromium.org/313453002/diff/20001/chrome/common/extensions...
File chrome/common/extensions/manifest_handlers/content_scripts_handler.cc
(right):

https://codereview.chromium.org/313453002/diff/20001/chrome/common/extensions...
chrome/common/extensions/manifest_handlers/content_scripts_handler.cc:430:
user_script.set_id(g_next_user_script_id++);
On 2014/06/02 18:42:55, kalman wrote:
> is there a test for ContentScriptsHandler that we could update?

Sure.

https://codereview.chromium.org/313453002/diff/20001/extensions/renderer/user...
File extensions/renderer/user_script_slave.cc (right):

https://codereview.chromium.org/313453002/diff/20001/extensions/renderer/user...
extensions/renderer/user_script_slave.cc:187: for
(ScopedVector<ScriptInjection>::iterator iter =
On 2014/06/02 18:42:55, kalman wrote:
> O(n^2) makes me nervous. It seems like using a std::map for script_injections
> would be just as easy as a vector, apart from needing to use linked_ptrs
> everywhere.
> 
> chat to me in person if you have questions about this, but FYI, linked_ptrs
add
> a lot of syntactic noise and I like to typedef std::map<int64,
> linked_ptr<ScriptInjection> > as something.

Per in-person chat, vectors will likely be faster for these lookups (since they
are gonna be pretty small).  Added a note to make that clear.

[not at google - send to devlin, 2014-06-02 21:28:17]

lgtm with a couple of minor changes

https://codereview.chromium.org/313453002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/active_script_controller.cc (right):

https://codereview.chromium.org/313453002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller.cc:285:
pending_requests_.erase(iter);
seems like you could delete the entry from pending_request_,
permitted_extensions_, and active_script_actions_.

given that the page action controller doesn't appear to have any extension
unload logic I presume that the NotifyChange-type stuff is actually handled in
the view. indeed:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

I guess what I'm trying to say is that it should really be
LocationBarController's job to handle this sort of stuff, given it already does
the fetching of the actions in the first place.

in fact it's arguably more correct, so that both the page action and active
script controller don't cause unnecessary paints.

so how about adding OnExtensionUnloaded to ActionProvider's interface.

https://codereview.chromium.org/313453002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/active_script_controller_browsertest.cc (right):

https://codereview.chromium.org/313453002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller_browsertest.cc:425:
inject_success_listener.WaitUntilSatisfied();
it doesn't seem like you're necessarily testing renderer logic here. the
GetActionForExtension(..) is browser-side logic, and all you're doing here is
clicking on the icon? how about, like with the other tests (Verify() etc),
assert that the script did get injected, the other one didn't, and that no icon
is showing anymore?

[Devlin, 2014-06-02 22:19:15]

https://codereview.chromium.org/313453002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/active_script_controller_browsertest.cc (right):

https://codereview.chromium.org/313453002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller_browsertest.cc:425:
inject_success_listener.WaitUntilSatisfied();
On 2014/06/02 21:28:17, kalman wrote:
> it doesn't seem like you're necessarily testing renderer logic here. the
> GetActionForExtension(..) is browser-side logic, and all you're doing here is
> clicking on the icon? how about, like with the other tests (Verify() etc),
> assert that the script did get injected, the other one didn't, and that no
icon
> is showing anymore?

We are testing that the script gets injected by waiting until the listener is
satisfied.  If it's not injected, this will fail.  We can also check that the
icon disappears, but that seems like it's simply testing what was tested above,
a pattern which we frequently discourage because it leads to obscenely long
tests which give no added benefit.  But maybe I'm missing something - is there a
reason the execution of the action would be significantly different?

[not at google - send to devlin, 2014-06-02 22:20:19]

https://codereview.chromium.org/313453002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/active_script_controller_browsertest.cc (right):

https://codereview.chromium.org/313453002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller_browsertest.cc:425:
inject_success_listener.WaitUntilSatisfied();
On 2014/06/02 22:19:16, Devlin wrote:
> On 2014/06/02 21:28:17, kalman wrote:
> > it doesn't seem like you're necessarily testing renderer logic here. the
> > GetActionForExtension(..) is browser-side logic, and all you're doing here
is
> > clicking on the icon? how about, like with the other tests (Verify() etc),
> > assert that the script did get injected, the other one didn't, and that no
> icon
> > is showing anymore?
> 
> We are testing that the script gets injected by waiting until the listener is
> satisfied.  If it's not injected, this will fail.  We can also check that the
> icon disappears, but that seems like it's simply testing what was tested
above,
> a pattern which we frequently discourage because it leads to obscenely long
> tests which give no added benefit.  But maybe I'm missing something - is there
a
> reason the execution of the action would be significantly different?

good point. renderer is tested.

[Devlin, 2014-06-02 23:07:52]

https://codereview.chromium.org/313453002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/active_script_controller.cc (right):

https://codereview.chromium.org/313453002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/active_script_controller.cc:285:
pending_requests_.erase(iter);
On 2014/06/02 21:28:17, kalman wrote:
> seems like you could delete the entry from pending_request_,
> permitted_extensions_, and active_script_actions_.
> 
> given that the page action controller doesn't appear to have any extension
> unload logic I presume that the NotifyChange-type stuff is actually handled in
> the view. indeed:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
> 
> I guess what I'm trying to say is that it should really be
> LocationBarController's job to handle this sort of stuff, given it already
does
> the fetching of the actions in the first place.
> 
> in fact it's arguably more correct, so that both the page action and active
> script controller don't cause unnecessary paints.
> 
> so how about adding OnExtensionUnloaded to ActionProvider's interface. 

Done.

[Devlin, 2014-06-03 19:05:44]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2014-06-03 19:07:42]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/313453002/...

[commit-bot: I haz the power, 2014-06-03 22:41:06]

Change committed as 274659