Block tabs.executeScript() from executing until user grants permission

chrome/browser/extensions/active_script_controller.cc

Index: chrome/browser/extensions/active_script_controller.cc
diff --git a/chrome/browser/extensions/active_script_controller.cc b/chrome/browser/extensions/active_script_controller.cc
index 36209aff27729a13b0aa4b29b2aaf815cfed78b6..72a48ef4df145db24a50981f7fdce15b42cb8467 100644
--- a/chrome/browser/extensions/active_script_controller.cc
+++ b/chrome/browser/extensions/active_script_controller.cc
@@ -4,6 +4,8 @@
 
 #include "chrome/browser/extensions/active_script_controller.h"
 
+#include "base/bind.h"
+#include "base/bind_helpers.h"
 #include "base/metrics/histogram.h"
 #include "base/stl_util.h"
 #include "chrome/browser/extensions/extension_action.h"
@@ -12,6 +14,7 @@
 #include "chrome/common/extensions/api/extension_action/action_info.h"
 #include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/navigation_entry.h"
+#include "content/public/browser/render_view_host.h"
 #include "content/public/browser/web_contents.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/common/extension.h"
@@ -26,7 +29,11 @@ namespace extensions {
 ActiveScriptController::ActiveScriptController(
     content::WebContents* web_contents)
     : content::WebContentsObserver(web_contents),
-      enabled_(FeatureSwitch::scripts_require_action()->IsEnabled()) {
+      enabled_(FeatureSwitch::scripts_require_action()->IsEnabled()),
+      extension_registry_(NULL) {
+  CHECK(web_contents);
+  extension_registry_ =
+      ExtensionRegistry::Get(web_contents->GetBrowserContext());
 }
 
 ActiveScriptController::~ActiveScriptController() {
@@ -48,31 +55,28 @@ ActiveScriptController* ActiveScriptController::GetForWebContents(
   return location_bar_controller->active_script_controller();
 }
 
-void ActiveScriptController::NotifyScriptExecuting(
-    const std::string& extension_id, int page_id) {
+void ActiveScriptController::GetPermissionForInjection(
+    const std::string& extension_id,
+    int page_id,
+    scoped_ptr<const base::Closure> callback) {
   content::NavigationEntry* visible_entry =
       web_contents()->GetController().GetVisibleEntry();
-  if (!visible_entry ||
-      extensions_executing_scripts_.count(extension_id) ||
-      visible_entry->GetPageID() != page_id) {
+  if (!visible_entry || visible_entry->GetPageID() != page_id)
     return;
-  }
 
   const Extension* extension =
-      ExtensionRegistry::Get(web_contents()->GetBrowserContext())
-          ->enabled_extensions().GetByID(extension_id);
-  if (extension &&
-      PermissionsData::RequiresActionForScriptExecution(extension)) {
-    extensions_executing_scripts_.insert(extension_id);
-    LocationBarController::NotifyChange(web_contents());
-  }
+      extension_registry_->enabled_extensions().GetByID(extension_id);

[not at google - send to devlin, 2014/05/15 00:12:36]

storing extension_registry_ seems a little inconvenient, really...

[Devlin, 2014/05/15 17:45:59]

Yeah, you're right.  I used to use it more, but I guess they fell away in a
refactor.

+  if (!extension)
+    return;
+
+  AddOrProcessRequest(extension, callback.Pass());
 }
 
 void ActiveScriptController::OnAdInjectionDetected(
     const std::vector<std::string> ad_injectors) {
   size_t num_preventable_ad_injectors =
       base::STLSetIntersection<std::set<std::string> >(
-          ad_injectors, extensions_executing_scripts_).size();
+          ad_injectors, requesting_extensions_).size();

[not at google - send to devlin, 2014/05/15 00:12:36]

we should also measure how many were run

[Devlin, 2014/05/15 17:45:59]

Done.

 
   UMA_HISTOGRAM_COUNTS_100(
       "Extensions.ActiveScriptController.PreventableAdInjectors",
@@ -84,7 +88,7 @@ void ActiveScriptController::OnAdInjectionDetected(
 
 ExtensionAction* ActiveScriptController::GetActionForExtension(
     const Extension* extension) {
-  if (!enabled_ || extensions_executing_scripts_.count(extension->id()) == 0)
+  if (!enabled_ || pending_requests_.count(extension->id()) == 0)
     return NULL;  // No action for this extension.
 
   ActiveScriptMap::iterator existing =
@@ -112,13 +116,73 @@ ExtensionAction* ActiveScriptController::GetActionForExtension(
 
 LocationBarController::Action ActiveScriptController::OnClicked(
     const Extension* extension) {
-  DCHECK(extensions_executing_scripts_.count(extension->id()) > 0);
+  DCHECK(extension);
+  PendingRequestMap::iterator iter =
+      pending_requests_.find(extension->id());
+  DCHECK(iter != pending_requests_.end());
+
+  // Run all pending injections for the given extension.
+  PendingRequestList& list = iter->second;
+  for (PendingRequestList::iterator request = list.begin();
+       request != list.end();
+       ++request) {
+    (*request)->Run();
+  }
+  pending_requests_.erase(extension->id());
+
+  // Inform the location bar that the action is now gone.
+  LocationBarController::NotifyChange(web_contents());
+
   return LocationBarController::ACTION_NONE;
 }
 
 void ActiveScriptController::OnNavigated() {
   LogUMA();
-  extensions_executing_scripts_.clear();
+  requesting_extensions_.clear();
+  pending_requests_.clear();
+}
+
+void ActiveScriptController::OnNotifyExtensionScriptExecution(
+    const std::string& extension_id,
+    int page_id) {
+  if (!Extension::IdIsValid(extension_id)) {
+    NOTREACHED() << "'" << extension_id << "' is not a valid id.";
+    return;
+  }
+
+  GetPermissionForInjection(
+      extension_id,
+      page_id,
+      scoped_ptr<const base::Closure>(
+          new base::Closure(base::Bind(&base::DoNothing))));

[not at google - send to devlin, 2014/05/15 00:12:36]

fwiw you might be able to use make_scoped_ptr() here.

[Devlin, 2014/05/15 17:45:59]

Nope, compile error.  Tried it.  But moot anyway.

+}
+
+void ActiveScriptController::AddOrProcessRequest(
+    const Extension* extension,
+    scoped_ptr<const base::Closure> request) {
+  // If the extension does not require permissions, run it immediately.
+  if (!PermissionsData::RequiresActionForScriptExecution(extension)) {
+    request->Run();
+    return;
+  }
+

[not at google - send to devlin, 2014/05/15 00:12:36]

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.

[Devlin, 2014/05/15 17:45:59]

Done.

+  // Track the extension as having requested permission.
+  requesting_extensions_.insert(extension->id());
+
+  // If the feature is not enabled, run the script.
+  if (!enabled_) {
+    request->Run();
+    return;
+  }
+
+  PendingRequestList* list = &pending_requests_[extension->id()];
+  list->push_back(make_linked_ptr(request.release()));
+
+  // If this was the first entry, notify the location bar that there's a new
+  // icon.
+  if (list->size() == 1u)
+    LocationBarController::NotifyChange(web_contents());
+

[not at google - send to devlin, 2014/05/15 00:12:36]

no blank line 

[Devlin, 2014/05/15 17:45:59]

Done.

 }
 
 bool ActiveScriptController::OnMessageReceived(const IPC::Message& message) {
@@ -131,20 +195,10 @@ bool ActiveScriptController::OnMessageReceived(const IPC::Message& message) {
   return handled;
 }
 
-void ActiveScriptController::OnNotifyExtensionScriptExecution(
-    const std::string& extension_id,
-    int page_id) {
-  if (!Extension::IdIsValid(extension_id)) {
-    NOTREACHED() << "'" << extension_id << "' is not a valid id.";
-    return;
-  }
-  NotifyScriptExecuting(extension_id, page_id);
-}
-
 void ActiveScriptController::LogUMA() const {
   UMA_HISTOGRAM_COUNTS_100(
       "Extensions.ActiveScriptController.ShownActiveScriptsOnPage",
-      extensions_executing_scripts_.size());
+      pending_requests_.size());
 }
 
 }  // namespace extensions