Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
286003004:
Block tabs.executeScript() from executing until user grants permission (Closed)
|
Created:
6 years, 7 months ago by Devlin Modified:
6 years, 7 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionBlock tabs.executeScript() from executing until user grants permission
Prevent extensions with <all_urls> from executing scripts using executeScript()
without user consent if the scripts-require-action switch is on.
Coming up next: Content scripts.
BUG=362353
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=271528
Patch Set 1 : #
Total comments: 42
Patch Set 2 : #
Total comments: 1
Patch Set 3 : #
Total comments: 17
Patch Set 4 : #
Total comments: 11
Patch Set 5 : Nits and add histograms #Patch Set 6 : #
Total comments: 4
Patch Set 7 : #Patch Set 8 : Epic master rebase #Messages
Total messages: 17 (0 generated)
For testing, have a look at extension_messages_apitest.cc... or at least, the externally connectable ones. They test a wide variety of configurations by generating the extensions to some extent. You might want to consider something similar. https://codereview.chromium.org/286003004/diff/20001/chrome/browser/extension... File chrome/browser/extensions/active_script_controller.cc (right): https://codereview.chromium.org/286003004/diff/20001/chrome/browser/extension... chrome/browser/extensions/active_script_controller.cc:68: extension_registry_->enabled_extensions().GetByID(extension_id); storing extension_registry_ seems a little inconvenient, really... https://codereview.chromium.org/286003004/diff/20001/chrome/browser/extension... chrome/browser/extensions/active_script_controller.cc:79: ad_injectors, requesting_extensions_).size(); we should also measure how many were run https://codereview.chromium.org/286003004/diff/20001/chrome/browser/extension... chrome/browser/extensions/active_script_controller.cc:157: new base::Closure(base::Bind(&base::DoNothing)))); fwiw you might be able to use make_scoped_ptr() here. https://codereview.chromium.org/286003004/diff/20001/chrome/browser/extension... chrome/browser/extensions/active_script_controller.cc:168: if our permissions infrastructure is working we also shouldn't be getting requests for the same extension if it's already been enabled. that's something else to use the set of already-enabled extensions you need to track anyway for UMA. https://codereview.chromium.org/286003004/diff/20001/chrome/browser/extension... chrome/browser/extensions/active_script_controller.cc:185: no blank line https://codereview.chromium.org/286003004/diff/20001/chrome/browser/extension... File chrome/browser/extensions/active_script_controller.h (right): https://codereview.chromium.org/286003004/diff/20001/chrome/browser/extension... chrome/browser/extensions/active_script_controller.h:52: // granted. what if permission wasn't granted? https://codereview.chromium.org/286003004/diff/20001/chrome/browser/extension... chrome/browser/extensions/active_script_controller.h:54: // because there can be a lot of bound parameters, and we'd prefer to avoid "a lot of bound parameters" is an implementation of the caller, not of this function. better would be to use base::Passed to these arguments on the caller's side, if there are a lot. I presume that doesn't copy. https://codereview.chromium.org/286003004/diff/20001/chrome/browser/extension... chrome/browser/extensions/active_script_controller.h:58: scoped_ptr<const base::Closure> callback); pet peeve: re-entrant callbacks. Even worse, callbacks that are *sometimes* re-entrant. It can be a source of very subtle bugs. Better is to either: - return a bool from this function meaning "i have injected now and |callback| will never be run", i.e. a return value of false means that |callback| will be run. - always call in a message loop (i.e. post to MessageLoop::current) even if the value is cached the name is a little confusing; you're not getting permission for anything. you're just (possibly asynchronously) determining if a script should be injected, an implementation detail of which may be to ask the user. https://codereview.chromium.org/286003004/diff/20001/chrome/browser/extension... File chrome/browser/extensions/active_script_controller_browsertest.cc (right): https://codereview.chromium.org/286003004/diff/20001/chrome/browser/extension... chrome/browser/extensions/active_script_controller_browsertest.cc:104: // however, since they are all injected at document_start, this shouldn't be document_start will be an issue actually. if you recall, strictly speaking we *can't* actually delay anything other than document_idle. this is a complexity we'll need to deal with eventually. you may need to notify the browser of each script execution stage... start, end, and idle... and take into account each case separately. https://codereview.chromium.org/286003004/diff/20001/chrome/browser/extension... chrome/browser/extensions/active_script_controller_browsertest.cc:109: // Make sure all running tasks are complete. what tasks? the active script controller ones? https://codereview.chromium.org/286003004/diff/20001/chrome/browser/extension... chrome/browser/extensions/active_script_controller_browsertest.cc:130: tab_helper->location_bar_controller()->active_script_controller(); s/tab_helper->location_bar_controller()/location_bar_controller https://codereview.chromium.org/286003004/diff/20001/chrome/browser/extension... chrome/browser/extensions/active_script_controller_browsertest.cc:162: DCHECK(action); in theory the action could have disappeared, like if between now and waiting for the inject listener there was some bug which caused it to disappear. point being, make it a test failure not a DCHECK. https://codereview.chromium.org/286003004/diff/20001/chrome/browser/extension... chrome/browser/extensions/active_script_controller_browsertest.cc:171: has_action = controller->GetActionForExtension(extension_) != NULL; you do this a couple of times. how about a (private) HasAction() function? you have all the state (and you could pass in the Browser on construction so that the state is there right from creation) in general this method could do with functions like "location_bar_controller()". https://codereview.chromium.org/286003004/diff/20001/chrome/browser/extension... chrome/browser/extensions/active_script_controller_browsertest.cc:203: true /* needs consent */, how about making this an enum so you don't need to write all of these comments https://codereview.chromium.org/286003004/diff/20001/chrome/browser/extension... File chrome/browser/extensions/script_executor.cc (right): https://codereview.chromium.org/286003004/diff/20001/chrome/browser/extension... chrome/browser/extensions/script_executor.cc:159: params, yes, it would make sense to Pass() this. https://codereview.chromium.org/286003004/diff/20001/chrome/browser/extension... File chrome/browser/extensions/script_executor.h (right): https://codereview.chromium.org/286003004/diff/20001/chrome/browser/extension... chrome/browser/extensions/script_executor.h:97: // A helper function, called upon request being given to execute the script. s/A helper function, c/C/ https://codereview.chromium.org/286003004/diff/20001/chrome/test/data/extensi... File chrome/test/data/extensions/active_script/inject_scripts_all_hosts/background.js (right): https://codereview.chromium.org/286003004/diff/20001/chrome/test/data/extensi... chrome/test/data/extensions/active_script/inject_scripts_all_hosts/background.js:7: return; you want to know exactly when the tab is complete, right? in that case you can do function(_, _, tab) { if (tab.status == 'complete') ... https://codereview.chromium.org/286003004/diff/20001/chrome/test/data/extensi... chrome/test/data/extensions/active_script/inject_scripts_all_hosts/background.js:8: remove listener here? https://codereview.chromium.org/286003004/diff/20001/chrome/test/data/extensi... chrome/test/data/extensions/active_script/inject_scripts_all_hosts/background.js:11: { code: "chrome.test.sendMessage('inject succeeded');" }); javascript can be so ugly. how about chrome.tabs.executeScript(tabId, { code: ... }); for some reason I didn't ask for that last time. https://codereview.chromium.org/286003004/diff/20001/chrome/test/data/extensi... File chrome/test/data/extensions/active_script/inject_scripts_explicit_hosts/background.js (right): https://codereview.chromium.org/286003004/diff/20001/chrome/test/data/extensi... chrome/test/data/extensions/active_script/inject_scripts_explicit_hosts/background.js:9: chrome.test.sendMessage('inject attempted'); same comments why doesn't this need the loading check?
Re generating the test extensions... certainly could, and I have no preference (I think the files are more readable as an actual file, rather than as a string literal, but I understand the desire to keep them all in the test itself). Do you have a preference one way or the other? https://codereview.chromium.org/286003004/diff/20001/chrome/browser/extension... File chrome/browser/extensions/active_script_controller.cc (right): https://codereview.chromium.org/286003004/diff/20001/chrome/browser/extension... chrome/browser/extensions/active_script_controller.cc:68: extension_registry_->enabled_extensions().GetByID(extension_id); On 2014/05/15 00:12:36, kalman wrote: > storing extension_registry_ seems a little inconvenient, really... Yeah, you're right. I used to use it more, but I guess they fell away in a refactor. https://codereview.chromium.org/286003004/diff/20001/chrome/browser/extension... chrome/browser/extensions/active_script_controller.cc:79: ad_injectors, requesting_extensions_).size(); On 2014/05/15 00:12:36, kalman wrote: > we should also measure how many were run Done. https://codereview.chromium.org/286003004/diff/20001/chrome/browser/extension... chrome/browser/extensions/active_script_controller.cc:157: new base::Closure(base::Bind(&base::DoNothing)))); On 2014/05/15 00:12:36, kalman wrote: > fwiw you might be able to use make_scoped_ptr() here. Nope, compile error. Tried it. But moot anyway. https://codereview.chromium.org/286003004/diff/20001/chrome/browser/extension... chrome/browser/extensions/active_script_controller.cc:168: On 2014/05/15 00:12:36, kalman wrote: > if our permissions infrastructure is working we also shouldn't be getting > requests for the same extension if it's already been enabled. that's something > else to use the set of already-enabled extensions you need to track anyway for > UMA. Done. https://codereview.chromium.org/286003004/diff/20001/chrome/browser/extension... chrome/browser/extensions/active_script_controller.cc:185: On 2014/05/15 00:12:36, kalman wrote: > no blank line Done. https://codereview.chromium.org/286003004/diff/20001/chrome/browser/extension... File chrome/browser/extensions/active_script_controller.h (right): https://codereview.chromium.org/286003004/diff/20001/chrome/browser/extension... chrome/browser/extensions/active_script_controller.h:52: // granted. On 2014/05/15 00:12:36, kalman wrote: > what if permission wasn't granted? Done. https://codereview.chromium.org/286003004/diff/20001/chrome/browser/extension... chrome/browser/extensions/active_script_controller.h:54: // because there can be a lot of bound parameters, and we'd prefer to avoid On 2014/05/15 00:12:36, kalman wrote: > "a lot of bound parameters" is an implementation of the caller, not of this > function. better would be to use base::Passed to these arguments on the caller's > side, if there are a lot. I presume that doesn't copy. Okay. https://codereview.chromium.org/286003004/diff/20001/chrome/browser/extension... chrome/browser/extensions/active_script_controller.h:58: scoped_ptr<const base::Closure> callback); On 2014/05/15 00:12:36, kalman wrote: > pet peeve: re-entrant callbacks. Even worse, callbacks that are *sometimes* > re-entrant. It can be a source of very subtle bugs. > > Better is to either: > - return a bool from this function meaning "i have injected now and |callback| > will never be run", i.e. a return value of false means that |callback| will be > run. > - always call in a message loop (i.e. post to MessageLoop::current) even if the > value is cached > > the name is a little confusing; you're not getting permission for anything. > you're just (possibly asynchronously) determining if a script should be > injected, an implementation detail of which may be to ask the user. Per offline discussion, neither of these are great. If we inject immediately, callback _is_ run, so the first option doesn't make sense. Returning a bool for whether or not it's re-entrant seems like exposing dirty internals. Always running a message loop leads to possibly running the callback with a defunct ASC/web contents. Instead, make a big scary comment about how callbacks cannot be assumed to run, at any time. https://codereview.chromium.org/286003004/diff/20001/chrome/browser/extension... File chrome/browser/extensions/active_script_controller_browsertest.cc (right): https://codereview.chromium.org/286003004/diff/20001/chrome/browser/extension... chrome/browser/extensions/active_script_controller_browsertest.cc:104: // however, since they are all injected at document_start, this shouldn't be On 2014/05/15 00:12:36, kalman wrote: > document_start will be an issue actually. if you recall, strictly speaking we > *can't* actually delay anything other than document_idle. this is a complexity > we'll need to deal with eventually. > > you may need to notify the browser of each script execution stage... start, end, > and idle... and take into account each case separately. I think this one deserves a bit more thought. So, we still have to handle start and end somehow. We can't just let them run (because then all we do is encourage people not to use idle), and we can't just block them and not show the user. So, we have to presumably do _something_ for them. IIRC, our main two options were: - Tell the user to refresh the page (or do it for them) and run the script at the right time. - Inject it delayed anyway, and trust the user will understand (e.g., if I have an ad blocker, I'd expect to see all the ads disappear once I allow the scripts to run, and wouldn't have a problem with the visual jarring). In either of these cases, this test case holds true. The script should *not* run (if it is not permitted to do so). After some action is taken, the script *should* run. The action may change (e.g., instead of a click, it may be a click and refresh), but that is a minor change. The biggest thing is that I'm very hesitant to drastically increase the number of messages sent from the renderer (for this, we'd have to notify at every document point, and even if no scripts run), all for the sake of a single browser test. WDYT? https://codereview.chromium.org/286003004/diff/20001/chrome/browser/extension... chrome/browser/extensions/active_script_controller_browsertest.cc:109: // Make sure all running tasks are complete. On 2014/05/15 00:12:36, kalman wrote: > what tasks? the active script controller ones? Yeah. Just making sure that if anything is running, it finishes. https://codereview.chromium.org/286003004/diff/20001/chrome/browser/extension... chrome/browser/extensions/active_script_controller_browsertest.cc:130: tab_helper->location_bar_controller()->active_script_controller(); On 2014/05/15 00:12:36, kalman wrote: > s/tab_helper->location_bar_controller()/location_bar_controller Whoops, done. https://codereview.chromium.org/286003004/diff/20001/chrome/browser/extension... chrome/browser/extensions/active_script_controller_browsertest.cc:162: DCHECK(action); On 2014/05/15 00:12:36, kalman wrote: > in theory the action could have disappeared, like if between now and waiting for > the inject listener there was some bug which caused it to disappear. > > point being, make it a test failure not a DCHECK. Actually... it can't. After line 134 (when we check if we have an action), we only wait at line 146, and then return right after. If we didn't get an action at line 134, we will return at line 138 (if we were supposed to have an action) or line 147 (if we weren't). Otherwise, the flow is synchronous. Went ahead and made it a bit more clear by only fetching the action once. https://codereview.chromium.org/286003004/diff/20001/chrome/browser/extension... chrome/browser/extensions/active_script_controller_browsertest.cc:171: has_action = controller->GetActionForExtension(extension_) != NULL; On 2014/05/15 00:12:36, kalman wrote: > you do this a couple of times. how about a (private) HasAction() function? you > have all the state (and you could pass in the Browser on construction so that > the state is there right from creation) > > in general this method could do with functions like "location_bar_controller()". Done (though the end result is much longer). https://codereview.chromium.org/286003004/diff/20001/chrome/browser/extension... chrome/browser/extensions/active_script_controller_browsertest.cc:203: true /* needs consent */, On 2014/05/15 00:12:36, kalman wrote: > how about making this an enum so you don't need to write all of these comments The reason was because if statements like if (requires_consent_ != has_action) fail; are much nicer than if ((requires_consent_ == REQUIRES_CONSENT && !has_action) || (requires_consent_ == DOES_NOT_REQUIRE_CONSENT && has_action)) fail; But I don't feel strongly enough about it, so, done. :) https://codereview.chromium.org/286003004/diff/20001/chrome/browser/extension... File chrome/browser/extensions/script_executor.cc (right): https://codereview.chromium.org/286003004/diff/20001/chrome/browser/extension... chrome/browser/extensions/script_executor.cc:159: params, On 2014/05/15 00:12:36, kalman wrote: > yes, it would make sense to Pass() this. Done. https://codereview.chromium.org/286003004/diff/20001/chrome/browser/extension... File chrome/browser/extensions/script_executor.h (right): https://codereview.chromium.org/286003004/diff/20001/chrome/browser/extension... chrome/browser/extensions/script_executor.h:97: // A helper function, called upon request being given to execute the script. On 2014/05/15 00:12:36, kalman wrote: > s/A helper function, c/C/ Done. https://codereview.chromium.org/286003004/diff/20001/chrome/test/data/extensi... File chrome/test/data/extensions/active_script/inject_scripts_all_hosts/background.js (right): https://codereview.chromium.org/286003004/diff/20001/chrome/test/data/extensi... chrome/test/data/extensions/active_script/inject_scripts_all_hosts/background.js:7: return; On 2014/05/15 00:12:36, kalman wrote: > you want to know exactly when the tab is complete, right? in that case you can > do > > function(_, _, tab) { > if (tab.status == 'complete') ... Actually, the only reason I had it was so that we'd only inject it once. It makes a lot more sense to just remove the listener though. Removed whole thing. https://codereview.chromium.org/286003004/diff/20001/chrome/test/data/extensi... chrome/test/data/extensions/active_script/inject_scripts_all_hosts/background.js:8: On 2014/05/15 00:12:36, kalman wrote: > remove listener here? Done. https://codereview.chromium.org/286003004/diff/20001/chrome/test/data/extensi... chrome/test/data/extensions/active_script/inject_scripts_all_hosts/background.js:11: { code: "chrome.test.sendMessage('inject succeeded');" }); On 2014/05/15 00:12:36, kalman wrote: > javascript can be so ugly. how about > > chrome.tabs.executeScript(tabId, { > code: ... > }); > > for some reason I didn't ask for that last time. Sure, done. https://codereview.chromium.org/286003004/diff/20001/chrome/test/data/extensi... File chrome/test/data/extensions/active_script/inject_scripts_explicit_hosts/background.js (right): https://codereview.chromium.org/286003004/diff/20001/chrome/test/data/extensi... chrome/test/data/extensions/active_script/inject_scripts_explicit_hosts/background.js:9: chrome.test.sendMessage('inject attempted'); On 2014/05/15 00:12:36, kalman wrote: > same comments > > why doesn't this need the loading check? Done. https://codereview.chromium.org/286003004/diff/40001/chrome/browser/extension... File chrome/browser/extensions/active_script_controller.cc (right): https://codereview.chromium.org/286003004/diff/40001/chrome/browser/extension... chrome/browser/extensions/active_script_controller.cc:83: UMA_HISTOGRAM_COUNTS_100( What did you want these to look like? (haven't added them to xml yet since they're gonna change)
https://codereview.chromium.org/286003004/diff/20001/chrome/browser/extension... File chrome/browser/extensions/active_script_controller.h (right): https://codereview.chromium.org/286003004/diff/20001/chrome/browser/extension... chrome/browser/extensions/active_script_controller.h:58: scoped_ptr<const base::Closure> callback); On 2014/05/15 17:45:59, D Cronin wrote: > On 2014/05/15 00:12:36, kalman wrote: > > pet peeve: re-entrant callbacks. Even worse, callbacks that are *sometimes* > > re-entrant. It can be a source of very subtle bugs. > > > > Better is to either: > > - return a bool from this function meaning "i have injected now and |callback| > > will never be run", i.e. a return value of false means that |callback| will be > > run. > > - always call in a message loop (i.e. post to MessageLoop::current) even if > the > > value is cached > > > > the name is a little confusing; you're not getting permission for anything. > > you're just (possibly asynchronously) determining if a script should be > > injected, an implementation detail of which may be to ask the user. > > Per offline discussion, neither of these are great. If we inject immediately, > callback _is_ run, so the first option doesn't make sense. Returning a bool for > whether or not it's re-entrant seems like exposing dirty internals. Always > running a message loop leads to possibly running the callback with a defunct > ASC/web contents. > > Instead, make a big scary comment about how callbacks cannot be assumed to run, > at any time. All I want is for the callback to never be run re-entrantly. I'm ok with it never being run. Basically: if a re-entrant result is possible then it should be part of a return value. FWIW Futures solve these problems very neatly.
lunch, then look again. https://codereview.chromium.org/286003004/diff/20001/chrome/browser/extension... File chrome/browser/extensions/active_script_controller_browsertest.cc (right): https://codereview.chromium.org/286003004/diff/20001/chrome/browser/extension... chrome/browser/extensions/active_script_controller_browsertest.cc:104: // however, since they are all injected at document_start, this shouldn't be On 2014/05/15 17:45:59, D Cronin wrote: > On 2014/05/15 00:12:36, kalman wrote: > > document_start will be an issue actually. if you recall, strictly speaking we > > *can't* actually delay anything other than document_idle. this is a complexity > > we'll need to deal with eventually. > > > > you may need to notify the browser of each script execution stage... start, > end, > > and idle... and take into account each case separately. > > I think this one deserves a bit more thought. > > So, we still have to handle start and end somehow. We can't just let them run > (because then all we do is encourage people not to use idle), and we can't just > block them and not show the user. So, we have to presumably do _something_ for > them. IIRC, our main two options were: > - Tell the user to refresh the page (or do it for them) and run the script at > the right time. > - Inject it delayed anyway, and trust the user will understand (e.g., if I have > an ad blocker, I'd expect to see all the ads disappear once I allow the scripts > to run, and wouldn't have a problem with the visual jarring). > > In either of these cases, this test case holds true. The script should *not* > run (if it is not permitted to do so). After some action is taken, the script > *should* run. The action may change (e.g., instead of a click, it may be a > click and refresh), but that is a minor change. > > The biggest thing is that I'm very hesitant to drastically increase the number > of messages sent from the renderer (for this, we'd have to notify at every > document point, and even if no scripts run), all for the sake of a single > browser test. > > WDYT? I agree, it requires thought, and it's complex enough to implement in a follow-up. Here are my thoughts: The default injection stage is idle, and documentation on how to do otherwise is hard to find :) my point being that I expect extensions will only use document_start/end because they really do have to. I can't say for all extensions why they want that, but it doesn't take much imagination to think up cases where just running them delayed won't work. E.g. injecting a style tag which must come first for specificity, or a script tag which must override the Date prototype; and pretending that it worked is a lie. I expect that the simplest behaviour will be to just ignore this fact and indeed do delay everything. Simplest, because it's most appropriate for dogfooding, and I want to get as far as possible here without adding any UI. I don't think these messages will be just for testing when we do fix this.
phew ok. I think we're close. to answer your question about testing: I think that would be a good idea. writing inline manifest files *is* very awkward, JS even more awkward. - the former is solved by the helper which accepts single quotes rather than double quotes (also DictionaryBuilder helps). - the latter I solved in the messaging tests by having a single JS test file general "test infrastructure". https://codereview.chromium.org/286003004/diff/60001/chrome/browser/extension... File chrome/browser/extensions/active_script_controller.cc (right): https://codereview.chromium.org/286003004/diff/60001/chrome/browser/extension... chrome/browser/extensions/active_script_controller.cc:78: ad_injectors, permitted_extensions_).size(); if the feature isn't enabled then *every* extension is going to end up in |permitted_extensions|, even those which weren't even going to be shown (see below), so this is basically going to be the number of ad injectors. I think the bug is basically that you shouldn't automatically be populating permitted_extensions if it isn't enabled. I also think I'm getting confused because permitted extensions vs pending requests are getting a little bit mixed up if the feature is/isn't allowed. I can see why it's that way, but the comment around here should be: "if we're disabled, we permitted every script injection automatically. in this case we interpret the UMA slightly differently... yada yada" in fact I think it's wrong to be using the same UMA keys if it's enabled vs disabled, it's just going to get muddled. is there a sensible way to test this? I've never seen UMA tested which is funny because there are often bugs. https://codereview.chromium.org/286003004/diff/60001/chrome/browser/extension... chrome/browser/extensions/active_script_controller.cc:81: // because otherwise the data will be boring (100% allowed). I would have expected 0% https://codereview.chromium.org/286003004/diff/60001/chrome/browser/extension... chrome/browser/extensions/active_script_controller.cc:145: permitted_extensions_.insert(extension->id()); this code (lines 134...145) has the potential, in crazy circumstances, of subtle bugs creeping in if callers re-entrantly call into this class again (namely InjectScriptIfHasPermission; or either of the 2 variations you will have). in other words, move this state-update to before you run the requests. to do so efficiently, perhaps: permitted_extensions_->insert(extension->id()); PendingExtensionList requests; iter->second.swap(&requests); pending_request_.erase(extension->id()); etc. https://codereview.chromium.org/286003004/diff/60001/chrome/browser/extension... chrome/browser/extensions/active_script_controller.cc:146: also, TODO grant active-tab-esque permission. https://codereview.chromium.org/286003004/diff/60001/chrome/browser/extension... chrome/browser/extensions/active_script_controller.cc:147: // Inform the location bar that the action is now gone. something to consider in future (just occurred to me): when we do start recording if the user wants to record their consent for a domain, we're going to run into issues with the code that de-dups hosts. https://codereview.chromium.org/286003004/diff/60001/chrome/browser/extension... chrome/browser/extensions/active_script_controller.cc:177: // run scripts. how did this get above the RequiresActionForScriptExecution() check? now it means that basically everything ends up in |permitted_extensions_| and the UMA not very useful (see comment up there; it might be a different mixup happening here). https://codereview.chromium.org/286003004/diff/60001/chrome/browser/extension... chrome/browser/extensions/active_script_controller.cc:181: // If the extension does not require permissions, run it immediately. TODO take into account tab-specific permissions https://codereview.chromium.org/286003004/diff/60001/chrome/browser/extension... chrome/browser/extensions/active_script_controller.cc:188: PendingRequestList* list = &pending_requests_[extension->id()]; nit: why not just PendingRequestList& list?
Re tests: Neither of the utils really made anything nicer, but it's not quite as ugly as we might have thought, so that's could. But I'm officially concerned that we are nearing the point of over-engineering on these. :) https://codereview.chromium.org/286003004/diff/60001/chrome/browser/extension... File chrome/browser/extensions/active_script_controller.cc (right): https://codereview.chromium.org/286003004/diff/60001/chrome/browser/extension... chrome/browser/extensions/active_script_controller.cc:78: ad_injectors, permitted_extensions_).size(); On 2014/05/15 22:21:44, kalman wrote: > if the feature isn't enabled then *every* extension is going to end up in > |permitted_extensions|, even those which weren't even going to be shown (see > below), so this is basically going to be the number of ad injectors. > > I think the bug is basically that you shouldn't automatically be populating > permitted_extensions if it isn't enabled. > > I also think I'm getting confused because permitted extensions vs pending > requests are getting a little bit mixed up if the feature is/isn't allowed. I > can see why it's that way, but the comment around here should be: > > "if we're disabled, we permitted every script injection automatically. in this > case we interpret the UMA slightly differently... yada yada" > > in fact I think it's wrong to be using the same UMA keys if it's enabled vs > disabled, it's just going to get muddled. > > is there a sensible way to test this? I've never seen UMA tested which is funny > because there are often bugs. Yep, definitely a bug there with the permitted. It should be better now, because only count them as permitted if we would have shown a badge. I think that the preventable/non-preventable ad injections should be good whether the feature is enabled or disabled. The script will only inject if it's permitted (whether it's done explicitly or implicitly), so we can tell which ones we let through. We could also add in a "PreventedAdInjectors", which is the number that we successfully blocked. https://codereview.chromium.org/286003004/diff/60001/chrome/browser/extension... chrome/browser/extensions/active_script_controller.cc:81: // because otherwise the data will be boring (100% allowed). On 2014/05/15 22:21:44, kalman wrote: > I would have expected 0% Well, we implicitly allow script injection when it's disabled. So 100% allowed, and 0% denied. Is there a metric scheme you'd prefer here? These were mostly just the first ones that came to mind. https://codereview.chromium.org/286003004/diff/60001/chrome/browser/extension... chrome/browser/extensions/active_script_controller.cc:145: permitted_extensions_.insert(extension->id()); On 2014/05/15 22:21:44, kalman wrote: > this code (lines 134...145) has the potential, in crazy circumstances, of subtle > bugs creeping in if callers re-entrantly call into this class again (namely > InjectScriptIfHasPermission; or either of the 2 variations you will have). > > in other words, move this state-update to before you run the requests. > > to do so efficiently, perhaps: > > permitted_extensions_->insert(extension->id()); > > PendingExtensionList requests; > iter->second.swap(&requests); > pending_request_.erase(extension->id()); > > etc. That would indeed be _crazy_ circumstances. But we've seen crazy stuff, so good point. ;) https://codereview.chromium.org/286003004/diff/60001/chrome/browser/extension... chrome/browser/extensions/active_script_controller.cc:146: On 2014/05/15 22:21:44, kalman wrote: > also, TODO grant active-tab-esque permission. Done. https://codereview.chromium.org/286003004/diff/60001/chrome/browser/extension... chrome/browser/extensions/active_script_controller.cc:147: // Inform the location bar that the action is now gone. On 2014/05/15 22:21:44, kalman wrote: > something to consider in future (just occurred to me): when we do start > recording if the user wants to record their consent for a domain, we're going to > run into issues with the code that de-dups hosts. Ooh, that sounds like fun... https://codereview.chromium.org/286003004/diff/60001/chrome/browser/extension... chrome/browser/extensions/active_script_controller.cc:177: // run scripts. On 2014/05/15 22:21:44, kalman wrote: > how did this get above the RequiresActionForScriptExecution() check? now it > means that basically everything ends up in |permitted_extensions_| and the UMA > not very useful (see comment up there; it might be a different mixup happening > here). Whoops, good point. Let's fix that... Done. https://codereview.chromium.org/286003004/diff/60001/chrome/browser/extension... chrome/browser/extensions/active_script_controller.cc:181: // If the extension does not require permissions, run it immediately. On 2014/05/15 22:21:44, kalman wrote: > TODO take into account tab-specific permissions Done. (added comment in .h above permitted_extensions_.) https://codereview.chromium.org/286003004/diff/60001/chrome/browser/extension... chrome/browser/extensions/active_script_controller.cc:188: PendingRequestList* list = &pending_requests_[extension->id()]; On 2014/05/15 22:21:44, kalman wrote: > nit: why not just PendingRequestList& list? Only 'cuz I never see non-const & in chrome. But, since the only style rule against non-const & is for passing, let's start a trend. ;)
lgtm with some more uma comments. https://codereview.chromium.org/286003004/diff/60001/chrome/browser/extension... File chrome/browser/extensions/active_script_controller.cc (right): https://codereview.chromium.org/286003004/diff/60001/chrome/browser/extension... chrome/browser/extensions/active_script_controller.cc:188: PendingRequestList* list = &pending_requests_[extension->id()]; On 2014/05/15 23:52:35, D Cronin wrote: > On 2014/05/15 22:21:44, kalman wrote: > > nit: why not just PendingRequestList& list? > > Only 'cuz I never see non-const & in chrome. But, since the only style rule > against non-const & is for passing, let's start a trend. ;) indeed. https://codereview.chromium.org/286003004/diff/80001/chrome/browser/extension... File chrome/browser/extensions/active_script_controller.cc (right): https://codereview.chromium.org/286003004/diff/80001/chrome/browser/extension... chrome/browser/extensions/active_script_controller.cc:70: if (PermissionsData::RequiresActionForScriptExecution(extension)) { extreme nit: can this be if (!PermissionData::...) return false? flows better with the rest of the function. https://codereview.chromium.org/286003004/diff/80001/chrome/browser/extension... chrome/browser/extensions/active_script_controller.cc:94: "Extensions.ActiveScriptController.DeniedExtensions", I think I made this comment earlier... but this stuff doesn't have anything to do with ad injection, so should go in LogUMA. https://codereview.chromium.org/286003004/diff/80001/chrome/browser/extension... chrome/browser/extensions/active_script_controller.cc:103: ad_injectors.size() - num_preventable_ad_injectors); and this stuff now needs to take into account the IDs in pending_requests_. https://codereview.chromium.org/286003004/diff/80001/chrome/browser/extension... chrome/browser/extensions/active_script_controller.cc:175: maybe note here for future hapless readers that the reason this effectively does nothing (just notifies) is that an upcoming CL will block :) https://codereview.chromium.org/286003004/diff/80001/chrome/browser/extension... File chrome/browser/extensions/active_script_controller_browsertest.cc (right): https://codereview.chromium.org/286003004/diff/80001/chrome/browser/extension... chrome/browser/extensions/active_script_controller_browsertest.cc:83: const Extension* ActiveScriptControllerBrowserTest::GetExtension( CreateExtension https://codereview.chromium.org/286003004/diff/80001/chrome/browser/extension... chrome/browser/extensions/active_script_controller_browsertest.cc:357: CONTENT_SCRIPT), I'm not too worried about over-engineering this (yet). I expect this to grow and that should hopefuly be easier now
https://codereview.chromium.org/286003004/diff/80001/chrome/browser/extension... File chrome/browser/extensions/active_script_controller.cc (right): https://codereview.chromium.org/286003004/diff/80001/chrome/browser/extension... chrome/browser/extensions/active_script_controller.cc:70: if (PermissionsData::RequiresActionForScriptExecution(extension)) { On 2014/05/16 00:08:33, kalman wrote: > extreme nit: can this be if (!PermissionData::...) return false? flows better > with the rest of the function. Done. https://codereview.chromium.org/286003004/diff/80001/chrome/browser/extension... chrome/browser/extensions/active_script_controller.cc:94: "Extensions.ActiveScriptController.DeniedExtensions", On 2014/05/16 00:08:33, kalman wrote: > I think I made this comment earlier... but this stuff doesn't have anything to > do with ad injection, so should go in LogUMA. Done. https://codereview.chromium.org/286003004/diff/80001/chrome/browser/extension... chrome/browser/extensions/active_script_controller.cc:103: ad_injectors.size() - num_preventable_ad_injectors); On 2014/05/16 00:08:33, kalman wrote: > and this stuff now needs to take into account the IDs in pending_requests_. It shouldn't - in fact, that would make it less accurate, I think. We notify this with all the ad injectors, and these metrics determine which of them we COULD block (but didn't) and which we COULD NOT block (and obviously didn't). If something has a pending request, then it should NOT have injected any ads. The only extensions with pending requests are those that we haven't permitted to run. If it injected ads even though it has a pending request, then it managed to even though it wasn't permitted, which makes it unpreventable. Right? (Sorry for the caps.) https://codereview.chromium.org/286003004/diff/80001/chrome/browser/extension... chrome/browser/extensions/active_script_controller.cc:175: On 2014/05/16 00:08:33, kalman wrote: > maybe note here for future hapless readers that the reason this effectively does > nothing (just notifies) is that an upcoming CL will block :) Done. https://codereview.chromium.org/286003004/diff/80001/chrome/browser/extension... File chrome/browser/extensions/active_script_controller_browsertest.cc (right): https://codereview.chromium.org/286003004/diff/80001/chrome/browser/extension... chrome/browser/extensions/active_script_controller_browsertest.cc:83: const Extension* ActiveScriptControllerBrowserTest::GetExtension( On 2014/05/16 00:08:33, kalman wrote: > CreateExtension GetOrCreateExtension(), probably, since we can return an existing extension.
Ben, please take a quick look at the (small) changes to ASC and ScriptExecutor. Ilya, please review histograms.xml.
lgtm https://codereview.chromium.org/286003004/diff/120001/chrome/browser/extensio... File chrome/browser/extensions/active_script_controller.cc (right): https://codereview.chromium.org/286003004/diff/120001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:80: // run scripts. this comment looks misplaced https://codereview.chromium.org/286003004/diff/120001/chrome/browser/extensio... File chrome/browser/extensions/active_script_controller.h (right): https://codereview.chromium.org/286003004/diff/120001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.h:74: PendingRequest(const base::Closure closure, int page_id); const base::Closure&
Histograms LGTM
https://codereview.chromium.org/286003004/diff/120001/chrome/browser/extensio... File chrome/browser/extensions/active_script_controller.cc (right): https://codereview.chromium.org/286003004/diff/120001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.cc:80: // run scripts. On 2014/05/16 17:32:12, kalman wrote: > this comment looks misplaced Done. https://codereview.chromium.org/286003004/diff/120001/chrome/browser/extensio... File chrome/browser/extensions/active_script_controller.h (right): https://codereview.chromium.org/286003004/diff/120001/chrome/browser/extensio... chrome/browser/extensions/active_script_controller.h:74: PendingRequest(const base::Closure closure, int page_id); On 2014/05/16 17:32:12, kalman wrote: > const base::Closure& Done.
The CQ bit was checked by rdevlin.cronin@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/286003004/...
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
Message was sent while issue was closed.
Change committed as 271528 |
