Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2213603002:
Prevent duplicate content script injection defined in manifest.json (reland) (Closed)
|
Created:
4 years, 4 months ago by catmullings Modified:
4 years, 3 months ago Reviewers:
Devlin CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPrevent duplicate content script injection defined in manifest.json (reland)
Keeps track of scripts injected by extensions installed. Checks if a script is
already injected, and injects a script only if it has not been injected.
BUG=248627, 631247
TEST= Run target browser_tests with
--gtest_filter="ExtensionApiTest.ContentScript*"
Committed: https://crrev.com/90d8dd145246616da02f5a54f1da1f5bfe6680e9
Committed: https://crrev.com/d4faad4f287364c5b0d97576a3a3c30d5e551b06
Cr-Original-Commit-Position: refs/heads/master@{#407016}
Cr-Commit-Position: refs/heads/master@{#417372}
Patch Set 1 : Pulled from codereview.chromium.org/2116613002 #Patch Set 2 : Initial fix to bug 631247 #
Total comments: 12
Patch Set 3 : Addressed Patch 2 code review comments; no test #Patch Set 4 : Test added #
Total comments: 17
Patch Set 5 : Addressed patch 4 code review comments #Patch Set 6 : Rebased with origin master #
Total comments: 7
Patch Set 7 : Addressed patch 6 code review comments #
Total comments: 16
Patch Set 8 : Addressed patch 7 code review comments #
Total comments: 13
Patch Set 9 : Addressed patch 8 code review comments #
Total comments: 7
Patch Set 10 : Addressed patch 9 code review #Patch Set 11 : Rebased with origin #
Total comments: 10
Patch Set 12 : Addressed lgtm nits #Messages
Total messages: 58 (36 generated)
Patchset #2 (id:20001) has been deleted
catmullings@chromium.org changed reviewers: + rdcronin@google.com
Initial fix to bug 631247; no test yet. https://codereview.chromium.org/2213603002/diff/40001/extensions/renderer/use... File extensions/renderer/user_script_injector.cc (right): https://codereview.chromium.org/2213603002/diff/40001/extensions/renderer/use... extensions/renderer/user_script_injector.cc:271: if (!script_) Should this be removed as well and converted into a DCHECK?
catmullings@chromium.org changed reviewers: + rdevlin.cronin@chromium.org - rdcronin@google.com
We should also add an additional test case for the scenario that caused the crash, to ensure we don't regress (adding it to the existing test should probably be fine). https://codereview.chromium.org/2213603002/diff/40001/extensions/renderer/pro... File extensions/renderer/programmatic_script_injector.cc (right): https://codereview.chromium.org/2213603002/diff/40001/extensions/renderer/pro... extensions/renderer/programmatic_script_injector.cc:66: !scripts_run_info->injected_scripts.empty(); This line means that we only inject js if there's already been a js injection, doesn't it? https://codereview.chromium.org/2213603002/diff/40001/extensions/renderer/scr... File extensions/renderer/script_injection.cc (right): https://codereview.chromium.org/2213603002/diff/40001/extensions/renderer/scr... extensions/renderer/script_injection.cc:247: DCHECK(sources.size() != 0); nit: prefer !sources.empty() https://codereview.chromium.org/2213603002/diff/40001/extensions/renderer/scr... File extensions/renderer/script_injector.h (right): https://codereview.chromium.org/2213603002/diff/40001/extensions/renderer/scr... extensions/renderer/script_injector.h:55: ScriptsRunInfo* scripts_run_info) const = 0; Let's take this as an opportunity to slightly reduce the footprint here and just pass in the std::set<GURL> of injected_scripts https://codereview.chromium.org/2213603002/diff/40001/extensions/renderer/use... File extensions/renderer/user_script_injector.cc (right): https://codereview.chromium.org/2213603002/diff/40001/extensions/renderer/use... extensions/renderer/user_script_injector.cc:145: !scripts_run_info->injected_scripts.empty(); Same comment as in programmatic script injector. https://codereview.chromium.org/2213603002/diff/40001/extensions/renderer/use... extensions/renderer/user_script_injector.cc:206: DCHECK(script_ != NULL); DCHECK(script_) is sufficient.
https://codereview.chromium.org/2213603002/diff/40001/extensions/renderer/scr... File extensions/renderer/script_injection.cc (right): https://codereview.chromium.org/2213603002/diff/40001/extensions/renderer/scr... extensions/renderer/script_injection.cc:247: DCHECK(sources.size() != 0); On 2016/08/03 23:34:44, Devlin wrote: > nit: prefer !sources.empty() Done. https://codereview.chromium.org/2213603002/diff/40001/extensions/renderer/scr... File extensions/renderer/script_injector.h (right): https://codereview.chromium.org/2213603002/diff/40001/extensions/renderer/scr... extensions/renderer/script_injector.h:55: ScriptsRunInfo* scripts_run_info) const = 0; On 2016/08/03 23:34:44, Devlin wrote: > Let's take this as an opportunity to slightly reduce the footprint here and just > pass in the std::set<GURL> of injected_scripts Done. https://codereview.chromium.org/2213603002/diff/40001/extensions/renderer/use... File extensions/renderer/user_script_injector.cc (right): https://codereview.chromium.org/2213603002/diff/40001/extensions/renderer/use... extensions/renderer/user_script_injector.cc:145: !scripts_run_info->injected_scripts.empty(); On 2016/08/03 23:34:44, Devlin wrote: > Same comment as in programmatic script injector. Done. https://codereview.chromium.org/2213603002/diff/40001/extensions/renderer/use... extensions/renderer/user_script_injector.cc:206: DCHECK(script_ != NULL); On 2016/08/03 23:34:44, Devlin (OOO through aug08) wrote: > DCHECK(script_) is sufficient. Done. https://codereview.chromium.org/2213603002/diff/40001/extensions/renderer/use... extensions/renderer/user_script_injector.cc:271: if (!script_) On 2016/08/03 23:31:22, catmullings wrote: > Should this be removed as well and converted into a DCHECK? bump
I'll take another look once the test's added. :) https://codereview.chromium.org/2213603002/diff/40001/extensions/renderer/use... File extensions/renderer/user_script_injector.cc (right): https://codereview.chromium.org/2213603002/diff/40001/extensions/renderer/use... extensions/renderer/user_script_injector.cc:271: if (!script_) On 2016/08/09 23:13:41, catmullings wrote: > On 2016/08/03 23:31:22, catmullings wrote: > > Should this be removed as well and converted into a DCHECK? > > bump Yep. :)
Patchset #4 (id:80001) has been deleted
Test added. Passes all ExtensionsApiTest.*
I think you'll also need to rebase this. On 2016/08/22 22:34:19, catmullings wrote: > Test added. > > Passes all ExtensionsApiTest.* I wouldn't worry about running whole test suites locally - it's pretty time consuming. You can probably just leave it to the trybots unless there's only a handful you want to check. https://codereview.chromium.org/2213603002/diff/100001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/content_scripts/duplicate_script_injection/manifest.json (right): https://codereview.chromium.org/2213603002/diff/100001/chrome/test/data/exten... chrome/test/data/extensions/api_test/content_scripts/duplicate_script_injection/manifest.json:24: "matches": ["*://*.maps.google.com/*"], I don't think this would be sufficient to replicate the crash, because *.maps.google.com won't match maps.google.com. https://codereview.chromium.org/2213603002/diff/100001/extensions/renderer/pr... File extensions/renderer/programmatic_script_injector.h (right): https://codereview.chromium.org/2213603002/diff/100001/extensions/renderer/pr... extensions/renderer/programmatic_script_injector.h:37: std::set<GURL> injected_scripts) const override; we shold avoid copying the set of GURLs if we can. https://codereview.chromium.org/2213603002/diff/100001/extensions/renderer/sc... File extensions/renderer/script_injection.cc (right): https://codereview.chromium.org/2213603002/diff/100001/extensions/renderer/sc... extensions/renderer/script_injection.cc:221: // Occurs when script is already injected. I'd be a little more explicit here - maybe something like: // This can legitimately happen if the extension specified a script to // be run in multiple rules, and the script has already run. // See crbug.com/nnn. That way, it's clear that this is an edge case, rather than something that happens normally. https://codereview.chromium.org/2213603002/diff/100001/extensions/renderer/sc... extensions/renderer/script_injection.cc:222: if (!(should_inject_js || should_inject_css)) { nit: I find this a little more readable after we De Morgan-ify it. :) if (!should_inject_js && !should_inject_css) https://codereview.chromium.org/2213603002/diff/100001/extensions/renderer/us... File extensions/renderer/user_script_injector.cc (right): https://codereview.chromium.org/2213603002/diff/100001/extensions/renderer/us... extensions/renderer/user_script_injector.cc:140: const UserScript::FileList& js_scripts = script_->js_scripts(); if script_ is null here, this will crash. https://codereview.chromium.org/2213603002/diff/100001/extensions/renderer/us... extensions/renderer/user_script_injector.cc:144: for (UserScript::FileList::const_iterator iter = js_scripts.begin(); prefer range-based for loops. https://codereview.chromium.org/2213603002/diff/100001/extensions/renderer/us... extensions/renderer/user_script_injector.cc:164: for (UserScript::FileList::const_iterator iter = css_scripts.begin(); nit: We could probably pull this into a helper function to make it more readable.
Patchset #5 (id:120001) has been deleted
Addressed your code review comments. I will rebase this branch in the next patch, and then run all the trybots. https://codereview.chromium.org/2213603002/diff/100001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/content_scripts/duplicate_script_injection/manifest.json (right): https://codereview.chromium.org/2213603002/diff/100001/chrome/test/data/exten... chrome/test/data/extensions/api_test/content_scripts/duplicate_script_injection/manifest.json:24: "matches": ["*://*.maps.google.com/*"], On 2016/08/22 22:46:52, Devlin wrote: > I don't think this would be sufficient to replicate the crash, because > *.maps.google.com won't match http://maps.google.com. It turns out that *.maps.google.com does match maps.google.com; I had tested it it out just to make sure. I can show you the test, if you'd like. Also in theory, it should match, according to the third example pattern on the match pattern documentation (https://developer.chrome.com/extensions/match_patterns). https://codereview.chromium.org/2213603002/diff/100001/extensions/renderer/pr... File extensions/renderer/programmatic_script_injector.h (right): https://codereview.chromium.org/2213603002/diff/100001/extensions/renderer/pr... extensions/renderer/programmatic_script_injector.h:37: std::set<GURL> injected_scripts) const override; On 2016/08/22 22:46:52, Devlin wrote: > we shold avoid copying the set of GURLs if we can. Done. https://codereview.chromium.org/2213603002/diff/100001/extensions/renderer/sc... File extensions/renderer/script_injection.cc (right): https://codereview.chromium.org/2213603002/diff/100001/extensions/renderer/sc... extensions/renderer/script_injection.cc:221: // Occurs when script is already injected. On 2016/08/22 22:46:52, Devlin wrote: > I'd be a little more explicit here - maybe something like: > // This can legitimately happen if the extension specified a script to > // be run in multiple rules, and the script has already run. > // See crbug.com/nnn. > > That way, it's clear that this is an edge case, rather than something that > happens normally. That's clearer. I'll use yours. Which crbug.com/nnn are you referring to? https://codereview.chromium.org/2213603002/diff/100001/extensions/renderer/sc... extensions/renderer/script_injection.cc:222: if (!(should_inject_js || should_inject_css)) { On 2016/08/22 22:46:52, Devlin wrote: > nit: I find this a little more readable after we De Morgan-ify it. :) > if (!should_inject_js && !should_inject_css) Done. https://codereview.chromium.org/2213603002/diff/100001/extensions/renderer/us... File extensions/renderer/user_script_injector.cc (right): https://codereview.chromium.org/2213603002/diff/100001/extensions/renderer/us... extensions/renderer/user_script_injector.cc:140: const UserScript::FileList& js_scripts = script_->js_scripts(); On 2016/08/22 22:46:52, Devlin wrote: > if script_ is null here, this will crash. Done. https://codereview.chromium.org/2213603002/diff/100001/extensions/renderer/us... extensions/renderer/user_script_injector.cc:144: for (UserScript::FileList::const_iterator iter = js_scripts.begin(); On 2016/08/22 22:46:52, Devlin wrote: > prefer range-based for loops. Done. https://codereview.chromium.org/2213603002/diff/100001/extensions/renderer/us... extensions/renderer/user_script_injector.cc:164: for (UserScript::FileList::const_iterator iter = css_scripts.begin(); On 2016/08/22 22:46:52, Devlin wrote: > nit: We could probably pull this into a helper function to make it more > readable. Done.
Patchset #6 (id:160001) has been deleted
The CQ bit was checked by catmullings@chromium.org to run a CQ dry run
Rebased with origin master.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
it also looks like there's a test failing on the bots. https://codereview.chromium.org/2213603002/diff/100001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/content_scripts/duplicate_script_injection/manifest.json (right): https://codereview.chromium.org/2213603002/diff/100001/chrome/test/data/exten... chrome/test/data/extensions/api_test/content_scripts/duplicate_script_injection/manifest.json:24: "matches": ["*://*.maps.google.com/*"], On 2016/08/24 01:13:19, catmullings wrote: > On 2016/08/22 22:46:52, Devlin wrote: > > I don't think this would be sufficient to replicate the crash, because > > *.maps.google.com won't match http://maps.google.com. > > It turns out that *.maps.google.com does match http://maps.google.com; I had tested it > it out just to make sure. I can show you the test, if you'd like. > > Also in theory, it should match, according to the third example pattern on the > match pattern documentation > (https://developer.chrome.com/extensions/match_patterns). Ah, right you are! Thanks for correcting me. https://codereview.chromium.org/2213603002/diff/100001/extensions/renderer/sc... File extensions/renderer/script_injection.cc (right): https://codereview.chromium.org/2213603002/diff/100001/extensions/renderer/sc... extensions/renderer/script_injection.cc:221: // Occurs when script is already injected. On 2016/08/24 01:13:19, catmullings wrote: > On 2016/08/22 22:46:52, Devlin wrote: > > I'd be a little more explicit here - maybe something like: > > // This can legitimately happen if the extension specified a script to > > // be run in multiple rules, and the script has already run. > > // See crbug.com/nnn. > > > > That way, it's clear that this is an edge case, rather than something that > > happens normally. > > That's clearer. I'll use yours. > Which crbug.com/nnn are you referring to? crbug.com/631247, which is the crash that resulted from assuming this couldn't happen. https://codereview.chromium.org/2213603002/diff/180001/extensions/renderer/sc... File extensions/renderer/script_injector.h (right): https://codereview.chromium.org/2213603002/diff/180001/extensions/renderer/sc... extensions/renderer/script_injector.h:72: ScriptsRunInfo* scripts_run_info) const = 0; Since we only use the set of injected scripts, let's pass just that in here. https://codereview.chromium.org/2213603002/diff/180001/extensions/renderer/us... File extensions/renderer/user_script_injector.cc (right): https://codereview.chromium.org/2213603002/diff/180001/extensions/renderer/us... extensions/renderer/user_script_injector.cc:273: // const UserScript::FileList& css_scripts = script_->css_scripts(); Why this change? https://codereview.chromium.org/2213603002/diff/180001/extensions/renderer/us... extensions/renderer/user_script_injector.cc:299: if (ShouldInjectJs(run_location, scripts_run_info->injected_scripts)) { Something we didn't realize last time, but should have, is that this is no longer correct. After we inject, ShouldInjectJs() is guaranteed to return false. Instead, we'll need to have a different way to track if we injected scripts.
The CQ bit was checked by catmullings@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Addressed patch 6 code review comments. https://codereview.chromium.org/2213603002/diff/100001/extensions/renderer/sc... File extensions/renderer/script_injection.cc (right): https://codereview.chromium.org/2213603002/diff/100001/extensions/renderer/sc... extensions/renderer/script_injection.cc:221: // Occurs when script is already injected. On 2016/08/25 16:52:26, Devlin wrote: > On 2016/08/24 01:13:19, catmullings wrote: > > On 2016/08/22 22:46:52, Devlin wrote: > > > I'd be a little more explicit here - maybe something like: > > > // This can legitimately happen if the extension specified a script to > > > // be run in multiple rules, and the script has already run. > > > // See crbug.com/nnn. > > > > > > That way, it's clear that this is an edge case, rather than something that > > > happens normally. > > > > That's clearer. I'll use yours. > > Which crbug.com/nnn are you referring to? > > crbug.com/631247, which is the crash that resulted from assuming this couldn't > happen. Done. https://codereview.chromium.org/2213603002/diff/180001/extensions/renderer/sc... File extensions/renderer/script_injector.h (right): https://codereview.chromium.org/2213603002/diff/180001/extensions/renderer/sc... extensions/renderer/script_injector.h:72: ScriptsRunInfo* scripts_run_info) const = 0; On 2016/08/25 16:52:26, Devlin wrote: > Since we only use the set of injected scripts, let's pass just that in here. Done. https://codereview.chromium.org/2213603002/diff/180001/extensions/renderer/us... File extensions/renderer/user_script_injector.cc (right): https://codereview.chromium.org/2213603002/diff/180001/extensions/renderer/us... extensions/renderer/user_script_injector.cc:273: // const UserScript::FileList& css_scripts = script_->css_scripts(); On 2016/08/25 16:52:26, Devlin wrote: > Why this change? Not meant to be commented. I was debugging. Uncommenting. https://codereview.chromium.org/2213603002/diff/180001/extensions/renderer/us... extensions/renderer/user_script_injector.cc:273: // const UserScript::FileList& css_scripts = script_->css_scripts(); On 2016/08/25 16:52:26, Devlin wrote: > Why this change? Done. https://codereview.chromium.org/2213603002/diff/180001/extensions/renderer/us... extensions/renderer/user_script_injector.cc:299: if (ShouldInjectJs(run_location, scripts_run_info->injected_scripts)) { On 2016/08/25 16:52:26, Devlin wrote: > Something we didn't realize last time, but should have, is that this is no > longer correct. After we inject, ShouldInjectJs() is guaranteed to return > false. Instead, we'll need to have a different way to track if we injected > scripts. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2213603002/diff/200001/extensions/renderer/pr... File extensions/renderer/programmatic_script_injector.h (right): https://codereview.chromium.org/2213603002/diff/200001/extensions/renderer/pr... extensions/renderer/programmatic_script_injector.h:46: std::set<GURL>& injected_scripts) const override; nit: chromium style (mostly) forbids non-const &s. Prefer a std::set<GURL>*. https://codereview.chromium.org/2213603002/diff/200001/extensions/renderer/sc... File extensions/renderer/script_injection.cc (right): https://codereview.chromium.org/2213603002/diff/200001/extensions/renderer/sc... extensions/renderer/script_injection.cc:235: injector_->GetRunInfo(scripts_run_info, run_location_, complete_, why do we pass |complete_| instead of |should_inject_js| here? https://codereview.chromium.org/2213603002/diff/200001/extensions/renderer/sc... File extensions/renderer/script_injection.h (right): https://codereview.chromium.org/2213603002/diff/200001/extensions/renderer/sc... extensions/renderer/script_injection.h:101: void InjectJs(ScriptsRunInfo* scripts_run_info); Here, too, we can just prefer passing the std::set<GURL>* https://codereview.chromium.org/2213603002/diff/200001/extensions/renderer/us... File extensions/renderer/user_script_injector.cc (right): https://codereview.chromium.org/2213603002/diff/200001/extensions/renderer/us... extensions/renderer/user_script_injector.cc:140: bool UserScriptInjector::ShouldInjectScripts( nit: we can put this whole function in an anonymous namespace in this file. https://codereview.chromium.org/2213603002/diff/200001/extensions/renderer/us... extensions/renderer/user_script_injector.cc:144: const GURL& script_url = file->url(); nitty nit: maybe just inline |script_url|: if (injected_scripts.count(file->url()) == 0) https://codereview.chromium.org/2213603002/diff/200001/extensions/renderer/us... extensions/renderer/user_script_injector.cc:225: // sources.reserve(js_scripts.size()); I think this is still left over from debugging. :) https://codereview.chromium.org/2213603002/diff/200001/extensions/renderer/us... extensions/renderer/user_script_injector.cc:299: if (js_injection_completed) { Hmm.... I think this still won't be quite sufficient, because we might not have injected *all* the scripts. Okay, let's take a step back. What if we pass in the ExecutingScriptsMap into Get[Js|Css]Sources(), and then we can use that to determine whether or not to inject the script, and, if so, add it to the map. Then, we can even entirely do-away with GetRunInfo() as a method. Does that make sense? WDYT?
The CQ bit was checked by catmullings@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2213603002/diff/200001/extensions/renderer/pr... File extensions/renderer/programmatic_script_injector.h (right): https://codereview.chromium.org/2213603002/diff/200001/extensions/renderer/pr... extensions/renderer/programmatic_script_injector.h:46: std::set<GURL>& injected_scripts) const override; On 2016/08/29 20:48:14, Devlin wrote: > nit: chromium style (mostly) forbids non-const &s. Prefer a std::set<GURL>*. Done. https://codereview.chromium.org/2213603002/diff/200001/extensions/renderer/sc... File extensions/renderer/script_injection.cc (right): https://codereview.chromium.org/2213603002/diff/200001/extensions/renderer/sc... extensions/renderer/script_injection.cc:235: injector_->GetRunInfo(scripts_run_info, run_location_, complete_, On 2016/08/29 20:48:14, Devlin wrote: > why do we pass |complete_| instead of |should_inject_js| here? No longer applicable with GetRunInfo rolled into Get[Js|Css]Sources. https://codereview.chromium.org/2213603002/diff/200001/extensions/renderer/sc... extensions/renderer/script_injection.cc:235: injector_->GetRunInfo(scripts_run_info, run_location_, complete_, On 2016/08/29 20:48:14, Devlin wrote: > why do we pass |complete_| instead of |should_inject_js| here? Done. https://codereview.chromium.org/2213603002/diff/200001/extensions/renderer/sc... File extensions/renderer/script_injection.h (right): https://codereview.chromium.org/2213603002/diff/200001/extensions/renderer/sc... extensions/renderer/script_injection.h:101: void InjectJs(ScriptsRunInfo* scripts_run_info); On 2016/08/29 20:48:14, Devlin wrote: > Here, too, we can just prefer passing the std::set<GURL>* I passed other scripts_run_info data fields, since we're no longer using std::set<GURL> injected_scripts. Made the same function signature change for InjectCss() as well. I'm not sure if it's overkill to do this for Inject() too? If it is preferred, I'll probably have to crawl up the stack from where Inject() was called and make the change for its parent functions. https://codereview.chromium.org/2213603002/diff/200001/extensions/renderer/sc... extensions/renderer/script_injection.h:101: void InjectJs(ScriptsRunInfo* scripts_run_info); On 2016/08/29 20:48:14, Devlin wrote: > Here, too, we can just prefer passing the std::set<GURL>* Done. https://codereview.chromium.org/2213603002/diff/200001/extensions/renderer/us... File extensions/renderer/user_script_injector.cc (right): https://codereview.chromium.org/2213603002/diff/200001/extensions/renderer/us... extensions/renderer/user_script_injector.cc:140: bool UserScriptInjector::ShouldInjectScripts( On 2016/08/29 20:48:14, Devlin wrote: > nit: we can put this whole function in an anonymous namespace in this file. Done. https://codereview.chromium.org/2213603002/diff/200001/extensions/renderer/us... extensions/renderer/user_script_injector.cc:144: const GURL& script_url = file->url(); On 2016/08/29 20:48:14, Devlin wrote: > nitty nit: maybe just inline |script_url|: > if (injected_scripts.count(file->url()) == 0) Done. https://codereview.chromium.org/2213603002/diff/200001/extensions/renderer/us... extensions/renderer/user_script_injector.cc:225: // sources.reserve(js_scripts.size()); On 2016/08/29 20:48:14, Devlin wrote: > I think this is still left over from debugging. :) Done. https://codereview.chromium.org/2213603002/diff/200001/extensions/renderer/us... extensions/renderer/user_script_injector.cc:299: if (js_injection_completed) { On 2016/08/29 20:48:14, Devlin wrote: > Hmm.... > > I think this still won't be quite sufficient, because we might not have injected > *all* the scripts. Just documenting: The check if(js_injection_completed) is not sufficient because if the script injection is not complete, then the following code below would not be executed. Instead we can roll the functionality of GetRunInfo() into Get[Js|Css]Sources() and remove GetRunInfo(). > > Okay, let's take a step back. > > What if we pass in the ExecutingScriptsMap into Get[Js|Css]Sources(), and then > we can use that to determine whether or not to inject the script, and, if so, > add it to the map. Then, we can even entirely do-away with GetRunInfo() as a > method. > > Does that make sense? WDYT? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2213603002/diff/220001/extensions/renderer/pr... File extensions/renderer/programmatic_script_injector.h (right): https://codereview.chromium.org/2213603002/diff/220001/extensions/renderer/pr... extensions/renderer/programmatic_script_injector.h:38: std::map<std::string, std::set<std::string>>& Remember, chromium style discourages non-const refs (https://codereview.chromium.org/2213603002/diff/200001/extensions/renderer/pr...). Use a const & here. https://codereview.chromium.org/2213603002/diff/220001/extensions/renderer/sc... File extensions/renderer/script_injection.cc (right): https://codereview.chromium.org/2213603002/diff/220001/extensions/renderer/sc... extensions/renderer/script_injection.cc:233: &(scripts_run_info->num_js)); shouldn't this be num_css? https://codereview.chromium.org/2213603002/diff/220001/extensions/renderer/sc... File extensions/renderer/script_injector.h (right): https://codereview.chromium.org/2213603002/diff/220001/extensions/renderer/sc... extensions/renderer/script_injector.h:81: size_t* num_injected_css_scripts) const = 0; "scripts" here is a little misleading (css isn't a script). Let's use the term "stylesheets" https://codereview.chromium.org/2213603002/diff/220001/extensions/renderer/sc... File extensions/renderer/scripts_run_info.h (right): https://codereview.chromium.org/2213603002/diff/220001/extensions/renderer/sc... extensions/renderer/scripts_run_info.h:44: std::set<GURL> injected_scripts; We don't need this anymore. However, thinking about it, we probably *do* want separate entries for injected js and injected css, since we notify the browser about the injected js. Let's add another ExecutingScriptsMap for injected_stylesheets https://codereview.chromium.org/2213603002/diff/220001/extensions/renderer/us... File extensions/renderer/user_script_injector.cc (right): https://codereview.chromium.org/2213603002/diff/220001/extensions/renderer/us... extensions/renderer/user_script_injector.cc:94: if (executing_scripts[host_id.id()].count(file->url().path()) == 0) { We're always checking the same set in the map, so we could just pass in that set (and also save ourselves the repeated lookup). Then we also don't need to pass in the host_id. https://codereview.chromium.org/2213603002/diff/220001/extensions/renderer/us... extensions/renderer/user_script_injector.cc:232: if ((*executing_scripts)[host_id_.id()].count(script_url.path()) != 0) Let's cache the set of injecting scripts so that we don't need to repeat the lookup (also in InjectCss)
Patchset #9 (id:240001) has been deleted
Patchset #9 (id:260001) has been deleted
Patchset #9 (id:280001) has been deleted
Addressed patch 8 code review comments https://codereview.chromium.org/2213603002/diff/220001/extensions/renderer/pr... File extensions/renderer/programmatic_script_injector.h (right): https://codereview.chromium.org/2213603002/diff/220001/extensions/renderer/pr... extensions/renderer/programmatic_script_injector.h:38: std::map<std::string, std::set<std::string>>& On 2016/09/06 17:21:13, Devlin wrote: > Remember, chromium style discourages non-const refs > (https://codereview.chromium.org/2213603002/diff/200001/extensions/renderer/pr...). > Use a const & here. Done. https://codereview.chromium.org/2213603002/diff/220001/extensions/renderer/sc... File extensions/renderer/script_injection.cc (right): https://codereview.chromium.org/2213603002/diff/220001/extensions/renderer/sc... extensions/renderer/script_injection.cc:233: &(scripts_run_info->num_js)); On 2016/09/06 17:21:13, Devlin wrote: > shouldn't this be num_css? Yup! Thanks for the catch. https://codereview.chromium.org/2213603002/diff/220001/extensions/renderer/sc... extensions/renderer/script_injection.cc:233: &(scripts_run_info->num_js)); On 2016/09/06 17:21:13, Devlin wrote: > shouldn't this be num_css? Done. https://codereview.chromium.org/2213603002/diff/220001/extensions/renderer/sc... File extensions/renderer/script_injector.h (right): https://codereview.chromium.org/2213603002/diff/220001/extensions/renderer/sc... extensions/renderer/script_injector.h:81: size_t* num_injected_css_scripts) const = 0; On 2016/09/06 17:21:13, Devlin wrote: > "scripts" here is a little misleading (css isn't a script). Let's use the term > "stylesheets" Done. https://codereview.chromium.org/2213603002/diff/220001/extensions/renderer/sc... File extensions/renderer/scripts_run_info.h (right): https://codereview.chromium.org/2213603002/diff/220001/extensions/renderer/sc... extensions/renderer/scripts_run_info.h:44: std::set<GURL> injected_scripts; On 2016/09/06 17:21:13, Devlin wrote: > We don't need this anymore. However, thinking about it, we probably *do* want > separate entries for injected js and injected css, since we notify the browser > about the injected js. Let's add another ExecutingScriptsMap for > injected_stylesheets Done. https://codereview.chromium.org/2213603002/diff/220001/extensions/renderer/us... File extensions/renderer/user_script_injector.cc (right): https://codereview.chromium.org/2213603002/diff/220001/extensions/renderer/us... extensions/renderer/user_script_injector.cc:94: if (executing_scripts[host_id.id()].count(file->url().path()) == 0) { On 2016/09/06 17:21:13, Devlin wrote: > We're always checking the same set in the map, so we could just pass in that set > (and also save ourselves the repeated lookup). Then we also don't need to pass > in the host_id. Done. https://codereview.chromium.org/2213603002/diff/220001/extensions/renderer/us... extensions/renderer/user_script_injector.cc:232: if ((*executing_scripts)[host_id_.id()].count(script_url.path()) != 0) On 2016/09/06 17:21:13, Devlin wrote: > Let's cache the set of injecting scripts so that we don't need to repeat the > lookup (also in InjectCss) Done.
The CQ bit was checked by catmullings@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Nice! Just a few last nits. https://codereview.chromium.org/2213603002/diff/300001/extensions/renderer/sc... File extensions/renderer/script_injector.h (right): https://codereview.chromium.org/2213603002/diff/300001/extensions/renderer/sc... extensions/renderer/script_injector.h:56: std::set<std::string>* executing_scripts) const = 0; nit: the "Should" methods can take a const& to the set, since they don't modify the object (the GetSources methods should continue to take a *) https://codereview.chromium.org/2213603002/diff/300001/extensions/renderer/us... File extensions/renderer/user_script_injector.cc (right): https://codereview.chromium.org/2213603002/diff/300001/extensions/renderer/us... extensions/renderer/user_script_injector.cc:89: std::set<std::string>* injected_files) { nit: const & for injected files https://codereview.chromium.org/2213603002/diff/300001/extensions/renderer/us... extensions/renderer/user_script_injector.cc:275: const std::string stylesheet_path = file->url().path(); nit: no need to copy here, so const std::string&
Addressed patch 9 code review https://codereview.chromium.org/2213603002/diff/300001/extensions/renderer/sc... File extensions/renderer/script_injector.h (right): https://codereview.chromium.org/2213603002/diff/300001/extensions/renderer/sc... extensions/renderer/script_injector.h:56: std::set<std::string>* executing_scripts) const = 0; On 2016/09/07 18:22:40, Devlin wrote: > nit: the "Should" methods can take a const& to the set, since they don't modify > the object (the GetSources methods should continue to take a *) Done. https://codereview.chromium.org/2213603002/diff/300001/extensions/renderer/us... File extensions/renderer/user_script_injector.cc (right): https://codereview.chromium.org/2213603002/diff/300001/extensions/renderer/us... extensions/renderer/user_script_injector.cc:89: std::set<std::string>* injected_files) { On 2016/09/07 18:22:40, Devlin wrote: > nit: const & for injected files Done. https://codereview.chromium.org/2213603002/diff/300001/extensions/renderer/us... extensions/renderer/user_script_injector.cc:275: const std::string stylesheet_path = file->url().path(); On 2016/09/07 18:22:40, Devlin wrote: > nit: no need to copy here, so const std::string& Yup, didn't mean to drop the &. https://codereview.chromium.org/2213603002/diff/300001/extensions/renderer/us... extensions/renderer/user_script_injector.cc:275: const std::string stylesheet_path = file->url().path(); On 2016/09/07 18:22:40, Devlin wrote: > nit: no need to copy here, so const std::string& Done.
The CQ bit was checked by catmullings@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #12 (id:360001) has been deleted
Patchset #11 (id:340001) has been deleted
Rebased with origin. https://codereview.chromium.org/2213603002/diff/380001/extensions/renderer/us... File extensions/renderer/user_script_injector.cc (right): https://codereview.chromium.org/2213603002/diff/380001/extensions/renderer/us... extensions/renderer/user_script_injector.cc:237: (*num_injected_js_scripts) += 1; This increment was absent from @lazyboy's implementation. Should I keep this? https://codereview.chromium.org/2213603002/diff/380001/extensions/renderer/us... extensions/renderer/user_script_injector.cc:262: (*num_injected_stylesheets) += 1; Similar to my previous comment/question.
The CQ bit was checked by catmullings@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
awesome, lgtm! https://codereview.chromium.org/2213603002/diff/380001/extensions/renderer/pr... File extensions/renderer/programmatic_script_injector.cc (right): https://codereview.chromium.org/2213603002/diff/380001/extensions/renderer/pr... extensions/renderer/programmatic_script_injector.cc:21: #include "extensions/renderer/scripts_run_info.h" not needed https://codereview.chromium.org/2213603002/diff/380001/extensions/renderer/pr... File extensions/renderer/programmatic_script_injector.h (right): https://codereview.chromium.org/2213603002/diff/380001/extensions/renderer/pr... extensions/renderer/programmatic_script_injector.h:22: struct ScriptsRunInfo; nitty nit: no longer needed. https://codereview.chromium.org/2213603002/diff/380001/extensions/renderer/sc... File extensions/renderer/script_injection.cc (right): https://codereview.chromium.org/2213603002/diff/380001/extensions/renderer/sc... extensions/renderer/script_injection.cc:252: run_location_, executing_scripts, num_injected_js_script); nit: num_injected_scripts, not num_injected_script https://codereview.chromium.org/2213603002/diff/380001/extensions/renderer/us... File extensions/renderer/user_script_injector.cc (right): https://codereview.chromium.org/2213603002/diff/380001/extensions/renderer/us... extensions/renderer/user_script_injector.cc:230: if ((*executing_scripts).count(script_url.path()) != 0) nit: prefer -> over (*foo). So here: if (executing_scripts->count(script_url.path()) != 0) https://codereview.chromium.org/2213603002/diff/380001/extensions/renderer/us... extensions/renderer/user_script_injector.cc:237: (*num_injected_js_scripts) += 1; On 2016/09/08 17:10:32, catmullings wrote: > This increment was absent from @lazyboy's implementation. > Should I keep this? lazyboy's didn't touch the run info much, so this was still handled by UserScriptInjector::GetRunInfo() in his implementation. We definitely still need this. https://codereview.chromium.org/2213603002/diff/380001/extensions/renderer/us... extensions/renderer/user_script_injector.cc:238: (*executing_scripts).insert(script_url.path()); ditto re -> (and also at lines 258 and 263) https://codereview.chromium.org/2213603002/diff/380001/extensions/renderer/us... extensions/renderer/user_script_injector.cc:262: (*num_injected_stylesheets) += 1; On 2016/09/08 17:10:32, catmullings wrote: > Similar to my previous comment/question. ditto https://codereview.chromium.org/2213603002/diff/380001/extensions/renderer/us... File extensions/renderer/user_script_injector.h (right): https://codereview.chromium.org/2213603002/diff/380001/extensions/renderer/us... extensions/renderer/user_script_injector.h:24: struct ScriptsRunInfo; not needed
The CQ bit was checked by catmullings@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2213603002/#ps400001 (title: "Addressed lgtm nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #12 (id:400001)
Message was sent while issue was closed.
Description was changed from ========== Prevent duplicate content script injection defined in manifest.json (reland) Keeps track of scripts injected by extensions installed. Checks if a script is already injected, and injects a script only if it has not been injected. BUG=248627, 631247 TEST= Run target browser_tests with --gtest_filter="ExtensionApiTest.ContentScript*" Committed: https://crrev.com/90d8dd145246616da02f5a54f1da1f5bfe6680e9 Cr-Commit-Position: refs/heads/master@{#407016} ========== to ========== Prevent duplicate content script injection defined in manifest.json (reland) Keeps track of scripts injected by extensions installed. Checks if a script is already injected, and injects a script only if it has not been injected. BUG=248627, 631247 TEST= Run target browser_tests with --gtest_filter="ExtensionApiTest.ContentScript*" Committed: https://crrev.com/90d8dd145246616da02f5a54f1da1f5bfe6680e9 Committed: https://crrev.com/d4faad4f287364c5b0d97576a3a3c30d5e551b06 Cr-Original-Commit-Position: refs/heads/master@{#407016} Cr-Commit-Position: refs/heads/master@{#417372} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/d4faad4f287364c5b0d97576a3a3c30d5e551b06 Cr-Commit-Position: refs/heads/master@{#417372} |
