Resubmit: Block content scripts 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 822977d9d67b6d9cf3eee8cd6ca83e212cb03f75..36a0b663204c6e7492703e4a72e60d5dca647c97 100644
--- a/chrome/browser/extensions/active_script_controller.cc
+++ b/chrome/browser/extensions/active_script_controller.cc
@@ -47,8 +47,11 @@ ActiveScriptController::PendingRequest::~PendingRequest() {
 ActiveScriptController::ActiveScriptController(
     content::WebContents* web_contents)
     : content::WebContentsObserver(web_contents),
-      enabled_(FeatureSwitch::scripts_require_action()->IsEnabled()) {
+      enabled_(FeatureSwitch::scripts_require_action()->IsEnabled()),
+      extension_registry_observer_(this) {
   CHECK(web_contents);
+  extension_registry_observer_.Add(
+      ExtensionRegistry::Get(web_contents->GetBrowserContext()));
 }
 
 ActiveScriptController::~ActiveScriptController() {
@@ -110,7 +113,7 @@ void ActiveScriptController::OnActiveTabPermissionGranted(
 }
 
 void ActiveScriptController::OnAdInjectionDetected(
-    const std::set<std::string> ad_injectors) {
+    const std::set<std::string>& ad_injectors) {
   // We're only interested in data if there are ad injectors detected.
   if (ad_injectors.empty())
     return;
@@ -213,9 +216,10 @@ void ActiveScriptController::RunPendingForExtension(
   LocationBarController::NotifyChange(web_contents());
 }
 
-void ActiveScriptController::OnNotifyExtensionScriptExecution(
+void ActiveScriptController::OnRequestContentScriptPermission(
     const std::string& extension_id,
-    int page_id) {
+    int page_id,
+    int request_id) {
   if (!Extension::IdIsValid(extension_id)) {
     NOTREACHED() << "'" << extension_id << "' is not a valid id.";
     return;
@@ -229,23 +233,60 @@ void ActiveScriptController::OnNotifyExtensionScriptExecution(
   if (!extension)
     return;
 
-  // Right now, we allow all content scripts to execute, but notify the
-  // controller of them.
-  // TODO(rdevlin.cronin): Fix this in a future CL.
-  if (RequiresUserConsentForScriptInjection(extension))
-    RequestScriptInjection(extension, page_id, base::Bind(&base::DoNothing));
+  // If the request id is -1, that signals that the content script has already
+  // ran (because this feature is not enabled). Add the extension to the list of
+  // permitted extensions (for metrics), and return immediately.
+  if (request_id == -1) {
+    DCHECK(!enabled_);
+    permitted_extensions_.insert(extension->id());
+    return;
+  }
+
+  if (RequiresUserConsentForScriptInjection(extension)) {
+    // This base::Unretained() is safe, because the callback is only invoked by
+    // this object.
+    RequestScriptInjection(
+        extension,
+        page_id,
+        base::Bind(&ActiveScriptController::GrantContentScriptPermission,
+                   base::Unretained(this),
+                   request_id));
+  } else {
+    GrantContentScriptPermission(request_id);
+  }
+}
+
+void ActiveScriptController::GrantContentScriptPermission(int request_id) {
+  content::RenderViewHost* render_view_host =
+      web_contents()->GetRenderViewHost();
+  if (render_view_host) {
+    render_view_host->Send(new ExtensionMsg_GrantContentScriptPermission(
+                               render_view_host->GetRoutingID(),
+                               request_id));
+  }
 }
 
 bool ActiveScriptController::OnMessageReceived(const IPC::Message& message) {
   bool handled = true;
   IPC_BEGIN_MESSAGE_MAP(ActiveScriptController, message)
-    IPC_MESSAGE_HANDLER(ExtensionHostMsg_NotifyExtensionScriptExecution,
-                        OnNotifyExtensionScriptExecution)
+    IPC_MESSAGE_HANDLER(ExtensionHostMsg_RequestContentScriptPermission,
+                        OnRequestContentScriptPermission)
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
   return handled;
 }
 
+void ActiveScriptController::OnExtensionUnloaded(
+      content::BrowserContext* browser_context,
+      const Extension* extension,
+      UnloadedExtensionInfo::Reason reason) {
+  PendingRequestMap::iterator iter = pending_requests_.find(extension->id());
+  if (iter != pending_requests_.end()) {
+    pending_requests_.erase(iter);

[not at google - send to devlin, 2014/06/02 21:28:17]

seems like you could delete the entry from pending_request_,
permitted_extensions_, and active_script_actions_.

given that the page action controller doesn't appear to have any extension
unload logic I presume that the NotifyChange-type stuff is actually handled in
the view. indeed:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

I guess what I'm trying to say is that it should really be
LocationBarController's job to handle this sort of stuff, given it already does
the fetching of the actions in the first place.

in fact it's arguably more correct, so that both the page action and active
script controller don't cause unnecessary paints.

so how about adding OnExtensionUnloaded to ActionProvider's interface. 

[Devlin, 2014/06/02 23:07:52]

Done.

+    LocationBarController::NotifyChange(web_contents());
+  }
+}
+
 void ActiveScriptController::LogUMA() const {
   UMA_HISTOGRAM_COUNTS_100(
       "Extensions.ActiveScriptController.ShownActiveScriptsOnPage",