Resubmit: Block content scripts from executing until user grants permission

chrome/browser/extensions/active_script_controller.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/extensions/active_script_controller.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/metrics/histogram.h"
@@ skipping 29 matching lines @@
     : closure(closure),
       page_id(page_id) {
 }
 
 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() {
   LogUMA();
 }
 
 // static
 ActiveScriptController* ActiveScriptController::GetForWebContents(
     content::WebContents* web_contents) {
   if (!web_contents)
@@ skipping 41 matching lines @@
   if (list.size() == 1u)
     LocationBarController::NotifyChange(web_contents());
 }
 
 void ActiveScriptController::OnActiveTabPermissionGranted(
     const Extension* extension) {
   RunPendingForExtension(extension);
 }
 
 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;
 
   size_t num_preventable_ad_injectors =
       base::STLSetIntersection<std::set<std::string> >(
           ad_injectors, permitted_extensions_).size();
 
   UMA_HISTOGRAM_COUNTS_100(
       "Extensions.ActiveScriptController.PreventableAdInjectors",
@@ skipping 82 matching lines @@
        ++request) {
     // Only run if it's on the proper page.
     if (request->page_id == page_id)
       request->closure.Run();
   }
 
   // Inform the location bar that the action is now gone.
   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;
   }
 
   const Extension* extension =
       ExtensionRegistry::Get(web_contents()->GetBrowserContext())
           ->enabled_extensions().GetByID(extension_id);
   // We shouldn't allow extensions which are no longer enabled to run any
   // scripts. Ignore the request.
   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",
       pending_requests_.size());
 
   // We only log the permitted extensions metric if the feature is enabled,
   // because otherwise the data will be boring (100% allowed).
   if (enabled_) {
     UMA_HISTOGRAM_COUNTS_100(
         "Extensions.ActiveScriptController.PermittedExtensions",
         permitted_extensions_.size());
     UMA_HISTOGRAM_COUNTS_100(
         "Extensions.ActiveScriptController.DeniedExtensions",
         pending_requests_.size());
   }
 }
 
 }  // namespace extensions