Make ActivityLog observe ScriptExecutor via a base::RepeatingCallback

There once was a ScriptExecutionObserver::Delegate, but it disappeared
in some past refactoring. The lifetime around ScriptExecutionObserver
is quite complex, and relies on base::ObserverList being a
SupportsWeakPtr, which we want to stop doing for https://crbug.com/888973.

There is now exactly one ScriptExecutionObserver implementation
(ActivityLog), and the observer list only ever has exactly 1 or zero
observers in it; added and removed in concert with a TabHelper.
WebViewGuest also has an ObserverList<ScriptExecutionObserver>, but
it never added any observers to it.

We can instead make ActivityLog responsible for the lifetime of its
callbacks from ScriptExecutor. It already dispenses WeakPtrs to itself.

Bug: 888973
Change-Id: Ic773bbcbabea70627b48ef8e0e29ff4e16d1e1f5
Reviewed-on: https://chromium-review.googlesource.com/c/1256398
Commit-Queue: Trent Apted <tapted@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#596454}

chrome/browser/extensions/activity_log/activity_log.cc

@@ -783,6 +783,11 @@ void ActivityLog::OnScriptsExecuted(
   }
 }
 
+void ActivityLog::ObserveScripts(ScriptExecutor* executor) {
+  executor->set_observer(base::BindRepeating(&ActivityLog::OnScriptsExecuted,
+                                             weak_factory_.GetWeakPtr()));
+}
+
 // LOOKUP ACTIONS. -------------------------------------------------------------
 
 void ActivityLog::GetFilteredActions(

chrome/browser/extensions/activity_log/activity_log.h

@@ -23,7 +23,7 @@
 #include "content/public/browser/notification_registrar.h"
 #include "extensions/browser/browser_context_keyed_api_factory.h"
 #include "extensions/browser/extension_registry_observer.h"
-#include "extensions/browser/script_execution_observer.h"
+#include "extensions/browser/script_executor.h"
 #include "extensions/common/dom_action_types.h"
 
 class Profile;
@@ -47,7 +47,6 @@ class ExtensionSystem;
 // each profile.
 //
 class ActivityLog : public BrowserContextKeyedAPI,
-                    public ScriptExecutionObserver,
                     public ExtensionRegistryObserver,
                     public content::NotificationObserver {
  public:
@@ -64,6 +63,14 @@ class ActivityLog : public BrowserContextKeyedAPI,
   // the constructor; use GetInstance instead.
   static ActivityLog* GetInstance(content::BrowserContext* context);
 
+  // Invoked when a ContentScript is executed.
+  void OnScriptsExecuted(const content::WebContents* web_contents,
+                         const ExecutingScriptsMap& extension_ids,
+                         const GURL& on_url);
+
+  // Observe tabs.executeScript on the given |executor|.
+  void ObserveScripts(ScriptExecutor* executor);
+
   // Add/remove observer: the activityLogPrivate API only listens when the
   // ActivityLog extension is registered for an event.
   void AddObserver(Observer* observer);
@@ -145,12 +152,6 @@ class ActivityLog : public BrowserContextKeyedAPI,
   // in prefs.
   void UpdateCachedConsumerCount();
 
-  // ScriptExecutionObserver implementation.
-  // Fires when a ContentScript is executed.
-  void OnScriptsExecuted(const content::WebContents* web_contents,
-                         const ExecutingScriptsMap& extension_ids,
-                         const GURL& on_url) override;
-
   // At the moment, ActivityLog will use only one policy for summarization.
   // These methods are used to choose and set the most appropriate policy.
   // Changing policies at runtime is not recommended, and likely only should be

chrome/browser/extensions/activity_log/activity_log_unittest.cc

@@ -255,11 +255,8 @@ TEST_F(ActivityLogTest, LogPrerender) {
   content::WebContents *contents = contentses[0];
   ASSERT_TRUE(prerender_manager->IsWebContentsPrerendering(contents, NULL));
 
-  ScriptExecutionObserver::ExecutingScriptsMap executing_scripts;
-  executing_scripts[extension->id()].insert("script");
-
-  static_cast<ScriptExecutionObserver*>(activity_log)
-      ->OnScriptsExecuted(contents, executing_scripts, url);
+  activity_log->OnScriptsExecuted(contents, {{extension->id(), {"script"}}},
+                                  url);
 
   activity_log->GetFilteredActions(
       extension->id(), Action::ACTION_ANY, "", "", "", 0,

chrome/browser/extensions/tab_helper.cc

@@ -68,9 +68,7 @@ using content::WebContents;
 
 namespace extensions {
 
-TabHelper::~TabHelper() {
-  RemoveScriptExecutionObserver(ActivityLog::GetInstance(profile_));
-}
+TabHelper::~TabHelper() = default;
 
 TabHelper::TabHelper(content::WebContents* web_contents)
     : content::WebContentsObserver(web_contents),
@@ -78,8 +76,7 @@ TabHelper::TabHelper(content::WebContents* web_contents)
       extension_app_(NULL),
       pending_web_app_action_(NONE),
       last_committed_nav_entry_unique_id_(0),
-      script_executor_(
-          new ScriptExecutor(web_contents, &script_execution_observers_)),
+      script_executor_(new ScriptExecutor(web_contents)),
       extension_action_runner_(new ExtensionActionRunner(web_contents)),
       registry_observer_(this),
       image_loader_ptr_factory_(this),
@@ -93,7 +90,7 @@ TabHelper::TabHelper(content::WebContents* web_contents)
   active_tab_permission_granter_.reset(new ActiveTabPermissionGranter(
       web_contents, SessionTabHelper::IdForTab(web_contents).id(), profile_));
 
-  AddScriptExecutionObserver(ActivityLog::GetInstance(profile_));
+  ActivityLog::GetInstance(profile_)->ObserveScripts(script_executor_.get());
 
   InvokeForContentRulesRegistries([this](ContentRulesRegistry* registry) {
     registry->MonitorWebContentsForRuleEvaluation(this->web_contents());
@@ -124,15 +121,6 @@ bool TabHelper::CanCreateBookmarkApp() const {
          IsValidBookmarkAppUrl(web_contents()->GetURL());
 }
 
-void TabHelper::AddScriptExecutionObserver(ScriptExecutionObserver* observer) {
-  script_execution_observers_.AddObserver(observer);
-}
-
-void TabHelper::RemoveScriptExecutionObserver(
-    ScriptExecutionObserver* observer) {
-  script_execution_observers_.RemoveObserver(observer);
-}
-
 void TabHelper::SetExtensionApp(const Extension* extension) {
   DCHECK(!extension || AppLaunchInfo::GetFullLaunchURL(extension).is_valid());
   if (extension_app_ == extension)
@@ -332,10 +320,10 @@ void TabHelper::OnGetAppInstallState(content::RenderFrameHost* host,
 
 void TabHelper::OnContentScriptsExecuting(
     content::RenderFrameHost* host,
-    const ScriptExecutionObserver::ExecutingScriptsMap& executing_scripts_map,
+    const ExecutingScriptsMap& executing_scripts_map,
     const GURL& on_url) {
-  for (auto& observer : script_execution_observers_)
-    observer.OnScriptsExecuted(web_contents(), executing_scripts_map, on_url);
+  ActivityLog::GetInstance(profile_)->OnScriptsExecuted(
+      web_contents(), executing_scripts_map, on_url);
 }
 
 const Extension* TabHelper::GetExtension(const ExtensionId& extension_app_id) {

chrome/browser/extensions/tab_helper.h

@@ -23,7 +23,6 @@
 #include "content/public/browser/web_contents_user_data.h"
 #include "extensions/browser/extension_function_dispatcher.h"
 #include "extensions/browser/extension_registry_observer.h"
-#include "extensions/browser/script_execution_observer.h"
 #include "extensions/browser/script_executor.h"
 #include "extensions/common/extension_id.h"
 #include "extensions/common/stack_frame.h"
@@ -53,10 +52,6 @@ class TabHelper : public content::WebContentsObserver,
   void CreateHostedAppFromWebContents(bool shortcut_app_requested);
   bool CanCreateBookmarkApp() const;
 
-  // ScriptExecutionObserver::Delegate
-  virtual void AddScriptExecutionObserver(ScriptExecutionObserver* observer);
-  virtual void RemoveScriptExecutionObserver(ScriptExecutionObserver* observer);
-
   // Sets the extension denoting this as an app. If |extension| is non-null this
   // tab becomes an app-tab. WebContents does not listen for unload events for
   // the extension. It's up to consumers of WebContents to do that.
@@ -146,10 +141,9 @@ class TabHelper : public content::WebContentsObserver,
                             const GURL& requestor_url,
                             int return_route_id,
                             int callback_id);
-  void OnContentScriptsExecuting(
-      content::RenderFrameHost* host,
-      const ScriptExecutionObserver::ExecutingScriptsMap& extension_ids,
-      const GURL& on_url);
+  void OnContentScriptsExecuting(content::RenderFrameHost* host,
+                                 const ExecutingScriptsMap& extension_ids,
+                                 const GURL& on_url);
 
   // App extensions related methods:
 
@@ -171,11 +165,6 @@ class TabHelper : public content::WebContentsObserver,
 
   Profile* profile_;
 
-  // Our content script observers. Declare at top so that it will outlive all
-  // other members, since they might add themselves as observers.
-  base::ObserverList<ScriptExecutionObserver>::Unchecked
-      script_execution_observers_;
-
   // If non-null this tab is an app tab and this is the extension the tab was
   // created for.
   const Extension* extension_app_;

extensions/browser/BUILD.gn

@@ -316,7 +316,6 @@ jumbo_source_set("browser_sources") {
     "runtime_data.h",
     "sandboxed_unpacker.cc",
     "sandboxed_unpacker.h",
-    "script_execution_observer.h",
     "script_executor.cc",
     "script_executor.h",
     "serial_extension_host_queue.cc",

extensions/browser/guest_view/web_view/web_view_guest.cc

@@ -399,8 +399,7 @@ void WebViewGuest::DidDropLink(const GURL& url) {
 }
 
 void WebViewGuest::DidInitialize(const base::DictionaryValue& create_params) {
-  script_executor_ =
-      std::make_unique<ScriptExecutor>(web_contents(), &script_observers_);
+  script_executor_ = std::make_unique<ScriptExecutor>(web_contents());
 
   ExtensionsAPIClient::Get()->AttachWebContentsHelpers(web_contents());
   web_view_permission_helper_ = std::make_unique<WebViewPermissionHelper>(this);

extensions/browser/guest_view/web_view/web_view_guest.h

@@ -13,7 +13,6 @@
 #include <vector>
 
 #include "base/macros.h"
-#include "base/observer_list.h"
 #include "components/guest_view/browser/guest_view.h"
 #include "content/public/browser/javascript_dialog_manager.h"
 #include "extensions/browser/guest_view/web_view/javascript_dialog_helper.h"
@@ -321,8 +320,6 @@ class WebViewGuest : public guest_view::GuestView<WebViewGuest> {
 
   // Handles find requests and replies for the webview find API.
   WebViewFindHelper find_helper_;
-
-  base::ObserverList<ScriptExecutionObserver>::Unchecked script_observers_;
   std::unique_ptr<ScriptExecutor> script_executor_;
 
   // True if the user agent is overridden.

extensions/browser/script_execution_observer.h

-// 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.
-
-#ifndef EXTENSIONS_BROWSER_SCRIPT_EXECUTION_OBSERVER_H_
-#define EXTENSIONS_BROWSER_SCRIPT_EXECUTION_OBSERVER_H_
-
-#include <map>
-#include <set>
-#include <string>
-
-class GURL;
-
-namespace content {
-class WebContents;
-}
-
-namespace extensions {
-
-// Observer base class for classes that need to be notified when content
-// scripts and/or tabs.executeScript calls run on a page.
-class ScriptExecutionObserver {
- public:
-  // Map of extensions IDs to the executing script paths.
-  typedef std::map<std::string, std::set<std::string> > ExecutingScriptsMap;
-
-  // Called when script(s) have executed on a page.
-  //
-  // |executing_scripts_map| contains all extensions that are executing
-  // scripts, mapped to the paths for those scripts. The paths may be an empty
-  // set if the script has no path associated with it (e.g. in the case of
-  // tabs.executeScript), but there will still be an entry for the extension.
-  virtual void OnScriptsExecuted(
-      const content::WebContents* web_contents,
-      const ExecutingScriptsMap& executing_scripts_map,
-      const GURL& on_url) = 0;
-
- protected:
-  virtual ~ScriptExecutionObserver();
-};
-
-}  // namespace extensions
-
-#endif  // EXTENSIONS_BROWSER_SCRIPT_EXECUTION_OBSERVER_H_

extensions/browser/script_executor.cc

@@ -8,10 +8,8 @@
 #include <string>
 
 #include "base/bind.h"
-#include "base/callback.h"
 #include "base/hash.h"
 #include "base/logging.h"
-#include "base/macros.h"
 #include "base/pickle.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_view_host.h"
@@ -19,7 +17,6 @@
 #include "content/public/browser/web_contents_observer.h"
 #include "extensions/browser/extension_api_frame_id_map.h"
 #include "extensions/browser/extension_registry.h"
-#include "extensions/browser/script_execution_observer.h"
 #include "extensions/browser/url_loader_factory_manager.h"
 #include "extensions/common/extension_messages.h"
 #include "ipc/ipc_message.h"
@@ -54,15 +51,14 @@ const std::string GenerateInjectionKey(const HostID& host_id,
 // corresponding response comes from the renderer, or the renderer is destroyed.
 class Handler : public content::WebContentsObserver {
  public:
-  Handler(
-      base::ObserverList<ScriptExecutionObserver>::Unchecked* script_observers,
-      content::WebContents* web_contents,
-      const ExtensionMsg_ExecuteCode_Params& params,
-      ScriptExecutor::FrameScope scope,
-      int frame_id,
-      const ScriptExecutor::ExecuteScriptCallback& callback)
+  Handler(ScriptsExecutedNotification observer,
+          content::WebContents* web_contents,
+          const ExtensionMsg_ExecuteCode_Params& params,
+          ScriptExecutor::FrameScope scope,
+          int frame_id,
+          const ScriptExecutor::ScriptFinishedCallback& callback)
       : content::WebContentsObserver(web_contents),
-        script_observers_(AsWeakPtr(script_observers)),
+        observer_(std::move(observer)),
         host_id_(params.host_id),
         request_id_(params.request_id),
         include_sub_frames_(scope == ScriptExecutor::INCLUDE_SUB_FRAMES),
@@ -184,12 +180,9 @@ class Handler : public content::WebContentsObserver {
       results_.Clear();
     }
 
-    if (script_observers_.get() && root_frame_error_.empty() &&
+    if (!observer_.is_null() && root_frame_error_.empty() &&
         host_id_.type() == HostID::EXTENSIONS) {
-      ScriptExecutionObserver::ExecutingScriptsMap id_map;
-      id_map[host_id_.id()] = std::set<std::string>();
-      for (auto& observer : *script_observers_)
-        observer.OnScriptsExecuted(web_contents(), id_map, root_frame_url_);
+      observer_.Run(web_contents(), {{host_id_.id(), {}}}, root_frame_url_);
     }
 
     if (!callback_.is_null())
@@ -197,8 +190,7 @@ class Handler : public content::WebContentsObserver {
     delete this;
   }
 
-  base::WeakPtr<base::ObserverList<ScriptExecutionObserver>::Unchecked>
-      script_observers_;
+  ScriptsExecutedNotification observer_;
 
   // The id of the host (the extension or the webui) doing the injection.
   HostID host_id_;
@@ -229,27 +221,19 @@ class Handler : public content::WebContentsObserver {
   GURL root_frame_url_;
 
   // The callback to run after all injections complete.
-  ScriptExecutor::ExecuteScriptCallback callback_;
+  ScriptExecutor::ScriptFinishedCallback callback_;
 
   DISALLOW_COPY_AND_ASSIGN(Handler);
 };
 
 }  // namespace
 
-ScriptExecutionObserver::~ScriptExecutionObserver() {
-}
-
-ScriptExecutor::ScriptExecutor(
-    content::WebContents* web_contents,
-    base::ObserverList<ScriptExecutionObserver>::Unchecked* script_observers)
-    : next_request_id_(0),
-      web_contents_(web_contents),
-      script_observers_(script_observers) {
+ScriptExecutor::ScriptExecutor(content::WebContents* web_contents)
+    : web_contents_(web_contents) {
   CHECK(web_contents_);
 }
 
-ScriptExecutor::~ScriptExecutor() {
-}
+ScriptExecutor::~ScriptExecutor() {}
 
 void ScriptExecutor::ExecuteScript(const HostID& host_id,
                                    ScriptExecutor::ScriptType script_type,
@@ -265,7 +249,7 @@ void ScriptExecutor::ExecuteScript(const HostID& host_id,
                                    bool user_gesture,
                                    base::Optional<CSSOrigin> css_origin,
                                    ScriptExecutor::ResultType result_type,
-                                   const ExecuteScriptCallback& callback) {
+                                   const ScriptFinishedCallback& callback) {
   if (host_id.type() == HostID::EXTENSIONS) {
     // Don't execute if the extension has been unloaded.
     const Extension* extension =
@@ -298,7 +282,7 @@ void ScriptExecutor::ExecuteScript(const HostID& host_id,
     params.injection_key = GenerateInjectionKey(host_id, file_url, code);
 
   // Handler handles IPCs and deletes itself on completion.
-  new Handler(script_observers_, web_contents_, params, frame_scope, frame_id,
+  new Handler(observer_, web_contents_, params, frame_scope, frame_id,
               callback);
 }
 

extensions/browser/script_executor.h

@@ -5,8 +5,11 @@
 #ifndef EXTENSIONS_BROWSER_SCRIPT_EXECUTOR_H_
 #define EXTENSIONS_BROWSER_SCRIPT_EXECUTOR_H_
 
-#include "base/callback_forward.h"
-#include "base/observer_list.h"
+#include <map>
+#include <set>
+#include <string>
+
+#include "base/callback.h"
 #include "base/optional.h"
 #include "extensions/common/constants.h"
 #include "extensions/common/user_script.h"
@@ -23,19 +26,24 @@ class WebContents;
 }
 
 namespace extensions {
-class ScriptExecutionObserver;
+
+// Contains all extensions that are executing scripts, mapped to the paths for
+// those scripts. The paths may be an empty set if the script has no path
+// associated with it (e.g. in the case of tabs.executeScript), but there will
+// still be an entry for the extension.
+using ExecutingScriptsMap = std::map<std::string, std::set<std::string>>;
+
+// Callback that ScriptExecutor uses to notify when content scripts and/or
+// tabs.executeScript calls run on a page.
+using ScriptsExecutedNotification = base::RepeatingCallback<
+    void(const content::WebContents*, const ExecutingScriptsMap&, const GURL&)>;
 
 // Interface for executing extension content scripts (e.g. executeScript) as
 // described by the ExtensionMsg_ExecuteCode_Params IPC, and notifying the
 // caller when responded with ExtensionHostMsg_ExecuteCodeFinished.
 class ScriptExecutor {
  public:
-  ScriptExecutor(
-      content::WebContents* web_contents,
-      // |script_observers| is assumed to be owned by |this|'s owner, and in
-      // such a way that |this| is destroyed first.
-      base::ObserverList<ScriptExecutionObserver>::Unchecked* script_observers);
-
+  explicit ScriptExecutor(content::WebContents* web_contents);
   ~ScriptExecutor();
 
   // The type of script being injected.
@@ -79,7 +87,7 @@ class ScriptExecutor {
   // Success is implied by an empty error.
   typedef base::Callback<
       void(const std::string&, const GURL&, const base::ListValue&)>
-      ExecuteScriptCallback;
+      ScriptFinishedCallback;
 
   // Executes a script. The arguments match ExtensionMsg_ExecuteCode_Params in
   // extension_messages.h (request_id is populated automatically).
@@ -105,15 +113,22 @@ class ScriptExecutor {
                      bool user_gesture,
                      base::Optional<CSSOrigin> css_origin,
                      ResultType result_type,
-                     const ExecuteScriptCallback& callback);
+                     const ScriptFinishedCallback& callback);
+
+  // Set the observer for ScriptsExecutedNotification callbacks.
+  void set_observer(ScriptsExecutedNotification observer) {
+    observer_ = std::move(observer);
+  }
 
  private:
   // The next value to use for request_id in ExtensionMsg_ExecuteCode_Params.
-  int next_request_id_;
+  int next_request_id_ = 0;
 
   content::WebContents* web_contents_;
 
-  base::ObserverList<ScriptExecutionObserver>::Unchecked* script_observers_;
+  ScriptsExecutedNotification observer_;
+
+  DISALLOW_COPY_AND_ASSIGN(ScriptExecutor);
 };
 
 }  // namespace extensions