Prevent duplicate content script injection defined in manifest.json (reland)

extensions/renderer/user_script_injector.cc

Index: extensions/renderer/user_script_injector.cc
diff --git a/extensions/renderer/user_script_injector.cc b/extensions/renderer/user_script_injector.cc
index da0c760e54425c7c329438150ed892501b18c6d5..9b9f95b5d24f82f3697adaeefbf1e042742ce6fe 100644
--- a/extensions/renderer/user_script_injector.cc
+++ b/extensions/renderer/user_script_injector.cc
@@ -137,16 +137,33 @@ bool UserScriptInjector::ExpectsResults() const {
   return false;
 }
 
+bool UserScriptInjector::ShouldInjectScripts(

[Devlin, 2016/08/29 20:48:14]

nit: we can put this whole function in an anonymous namespace in this file.

[catmullings, 2016/09/01 17:43:51]

Done.

+    const UserScript::FileList& scripts,
+    const std::set<GURL>& injected_scripts) const {
+  for (const std::unique_ptr<UserScript::File>& file : scripts) {
+    const GURL& script_url = file->url();

[Devlin, 2016/08/29 20:48:14]

nitty nit: maybe just inline |script_url|:
if (injected_scripts.count(file->url()) == 0)

[catmullings, 2016/09/01 17:43:51]

Done.

+    // Check if the script is already injected.
+    if (injected_scripts.count(script_url) == 0) {
+      return true;
+    }
+  }
+  return false;
+}
+
 bool UserScriptInjector::ShouldInjectJs(
-    UserScript::RunLocation run_location) const {
+    UserScript::RunLocation run_location,
+    const std::set<GURL>& injected_scripts) const {
   return script_ && script_->run_location() == run_location &&
-         !script_->js_scripts().empty();
+         !script_->js_scripts().empty() &&
+         ShouldInjectScripts(script_->js_scripts(), injected_scripts);
 }
 
 bool UserScriptInjector::ShouldInjectCss(
-    UserScript::RunLocation run_location) const {
+    UserScript::RunLocation run_location,
+    const std::set<GURL>& injected_scripts) const {
   return script_ && run_location == UserScript::DOCUMENT_START &&
-         !script_->css_scripts().empty();
+         !script_->css_scripts().empty() &&
+         ShouldInjectScripts(script_->css_scripts(), injected_scripts);
 }
 
 PermissionsData::AccessType UserScriptInjector::CanExecuteOnFrame(
@@ -197,16 +214,21 @@ PermissionsData::AccessType UserScriptInjector::CanExecuteOnFrame(
 }
 
 std::vector<blink::WebScriptSource> UserScriptInjector::GetJsSources(
-    UserScript::RunLocation run_location) const {
+    UserScript::RunLocation run_location,
+    std::set<GURL>& injected_scripts) const {
+  DCHECK(script_);
   std::vector<blink::WebScriptSource> sources;
-  if (!script_)
-    return sources;
 
   DCHECK_EQ(script_->run_location(), run_location);
 
   const UserScript::FileList& js_scripts = script_->js_scripts();
-  sources.reserve(js_scripts.size());
+  // sources.reserve(js_scripts.size());

[Devlin, 2016/08/29 20:48:14]

I think this is still left over from debugging. :)

[catmullings, 2016/09/01 17:43:51]

Done.

   for (const std::unique_ptr<UserScript::File>& file : js_scripts) {
+    const GURL& script_url = file->url();
+    // Check if the script is already injected.
+    if (injected_scripts.count(script_url) != 0)
+      continue;
+
     base::StringPiece script_content = file->GetContent();
     blink::WebString source;
     if (script_->emulate_greasemonkey()) {
@@ -228,6 +250,8 @@ std::vector<blink::WebScriptSource> UserScriptInjector::GetJsSources(
                                           script_content.length());
     }
     sources.push_back(blink::WebScriptSource(source, file->url()));
+
+    injected_scripts.insert(script_url);
   }
 
   // Emulate Greasemonkey API for scripts that were converted to extension
@@ -239,33 +263,40 @@ std::vector<blink::WebScriptSource> UserScriptInjector::GetJsSources(
 }
 
 std::vector<blink::WebString> UserScriptInjector::GetCssSources(
-    UserScript::RunLocation run_location) const {
+    UserScript::RunLocation run_location,
+    std::set<GURL>& injected_scripts) const {
+  DCHECK(script_);
   DCHECK_EQ(UserScript::DOCUMENT_START, run_location);
 
   std::vector<blink::WebString> sources;
-  if (!script_)
-    return sources;
 
   const UserScript::FileList& css_scripts = script_->css_scripts();
   sources.reserve(css_scripts.size());
   for (const std::unique_ptr<UserScript::File>& file : script_->css_scripts()) {
+    const GURL& script_url = file->url();
+    // Check if the stylesheet is already injected.
+    if (injected_scripts.count(script_url) != 0)
+      continue;
+
     // TODO(lazyboy): |css_content| string is copied into blink::WebString for
     // every frame in the current renderer process. Avoid the copy, possibly by
     // only performing the copy once.
     base::StringPiece css_content = file->GetContent();
     sources.push_back(
         blink::WebString::fromUTF8(css_content.data(), css_content.length()));
+
+    injected_scripts.insert(script_url);
   }
   return sources;
 }
 
-void UserScriptInjector::GetRunInfo(
-    ScriptsRunInfo* scripts_run_info,
-    UserScript::RunLocation run_location) const {
-  if (!script_)
-    return;
+void UserScriptInjector::GetRunInfo(ScriptsRunInfo* scripts_run_info,
+                                    UserScript::RunLocation run_location,
+                                    bool js_injection_completed,
+                                    bool should_inject_css) const {
+  DCHECK(script_);
 
-  if (ShouldInjectJs(run_location)) {
+  if (js_injection_completed) {

[Devlin, 2016/08/29 20:48:14]

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?

[catmullings, 2016/09/01 17:43:51]

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

     const UserScript::FileList& js_scripts = script_->js_scripts();
     scripts_run_info->num_js += js_scripts.size();
     for (const std::unique_ptr<UserScript::File>& iter : js_scripts) {
@@ -274,7 +305,7 @@ void UserScriptInjector::GetRunInfo(
     }
   }
 
-  if (ShouldInjectCss(run_location))
+  if (should_inject_css)
     scripts_run_info->num_css += script_->css_scripts().size();
 }