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

chrome/browser/extensions/active_script_controller.h

Index: chrome/browser/extensions/active_script_controller.h
diff --git a/chrome/browser/extensions/active_script_controller.h b/chrome/browser/extensions/active_script_controller.h
index ec5d929bb788e596c227e26f2a17436e4b817a40..8fede1718c13d7b203db5ac5148bbef4f1482b5e 100644
--- a/chrome/browser/extensions/active_script_controller.h
+++ b/chrome/browser/extensions/active_script_controller.h
@@ -8,9 +8,12 @@
 #include <map>
 #include <set>
 #include <string>
+#include <vector>
 
+#include "base/callback.h"
 #include "base/compiler_specific.h"
 #include "base/memory/linked_ptr.h"
+#include "base/memory/scoped_ptr.h"
 #include "chrome/browser/extensions/location_bar_controller.h"
 #include "content/public/browser/web_contents_observer.h"
 
@@ -26,6 +29,7 @@ class ExtensionAction;
 
 namespace extensions {
 class Extension;
+class ExtensionRegistry;
 
 // The provider for ExtensionActions corresponding to scripts which are actively
 // running or need permission.
@@ -42,10 +46,16 @@ class ActiveScriptController : public LocationBarController::ActionProvider,
   static ActiveScriptController* GetForWebContents(
       content::WebContents* web_contents);
 
-  // Notify the ActiveScriptController that an extension is running a script.
-  // TODO(rdevlin.cronin): Soon, this should be ask the user for permission,
-  // rather than simply notifying them.
-  void NotifyScriptExecuting(const std::string& extension_id, int page_id);
+  // Checks whether permission is needed for the script to run. If no permission
+  // is needed, runs the |callback| immediately. If permission is needed, asks
+  // the user for permission, and will run the script upon permission being
+  // granted.

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

what if permission wasn't granted?

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

Done.

+  // Even though base::Closure can be safely passed, we use a scoped_ptr here
+  // because there can be a lot of bound parameters, and we'd prefer to avoid

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

"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.

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

Okay.

+  // copying.
+  void GetPermissionForInjection(const std::string& extension_id,
+                                 int page_id,
+                                 scoped_ptr<const base::Closure> callback);

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

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.

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

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.

[not at google - send to devlin, 2014/05/15 18:19:56]

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.

 
   // Notifies the ActiveScriptController of detected ad injection.
   void OnAdInjectionDetected(const std::vector<std::string> ad_injectors);
@@ -58,13 +68,26 @@ class ActiveScriptController : public LocationBarController::ActionProvider,
   virtual void OnNavigated() OVERRIDE;
 
  private:
-  // content::WebContentsObserver implementation.
-  virtual bool OnMessageReceived(const IPC::Message& message) OVERRIDE;
+  typedef std::vector<linked_ptr<const base::Closure> > PendingRequestList;
+  typedef std::map<std::string, PendingRequestList> PendingRequestMap;
 
-  // Handle the NotifyExtensionScriptExecution message.
+  // Handles the NotifyExtensionScriptExecution message.
   void OnNotifyExtensionScriptExecution(const std::string& extension_id,
                                         int page_id);
 
+  // Returns true if the ActiveScriptController should process the request. This
+  // can be false if, e.g., the page id does not match, the extension cannot
+  // be found, etc.
+  bool ShouldProcessRequest(const Extension* extension, int page_id);
+
+  // Adds the |request| to the map of pending requests, or processes it
+  // immediately if no permission is needed.
+  void AddOrProcessRequest(const Extension* extension,
+                           scoped_ptr<const base::Closure> request);
+
+  // content::WebContentsObserver implementation.
+  virtual bool OnMessageReceived(const IPC::Message& message) OVERRIDE;
+
   // Log metrics.
   void LogUMA() const;
 
@@ -73,11 +96,16 @@ class ActiveScriptController : public LocationBarController::ActionProvider,
   // always allowing scripts to run and never displaying actions.
   bool enabled_;
 
-  // The extensions that have called ExecuteScript on the current frame.
-  std::set<std::string> extensions_executing_scripts_;
+  // The map of extension_id:pending_request of all pending requests.
+  PendingRequestMap pending_requests_;
+
+  // The extensions which have requested permission at some point in this page.
+  // Used for metrics. Separate from |pending_requests_|, as pending requests
+  // are erased when the request is fulfilled.
+  std::set<std::string> requesting_extensions_;
 
-  // The extensions which have injected ads.
-  std::set<std::string> ad_injectors_;
+  // The associated ExtensionRegistry, for convenience.
+  ExtensionRegistry* extension_registry_;
 
   // Script badges that have been generated for extensions. This is both those
   // with actions already declared that are copied and normalised, and actions