Remove ActivityLogState

All activity logging is done on the UI thread now, so there's no
need for a thread-safe cache of extension whitelists or ActivityLog
active state.

Bug: 268984
Change-Id: I3c35017cb8413be59a270a96e587f3c401e80b88
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1849419Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Commit-Queue: Evan Stade <estade@chromium.org>
Cr-Commit-Position: refs/heads/master@{#704850}

chrome/browser/extensions/activity_log/activity_log.cc

@@ -13,19 +13,15 @@
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/json/json_string_value_serializer.h"
-#include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/memory/scoped_refptr.h"
 #include "base/one_shot_event.h"
 #include "base/stl_util.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
-#include "base/synchronization/lock.h"
-#include "base/task/post_task.h"
 #include "base/threading/thread_checker.h"
 #include "base/values.h"
 #include "chrome/browser/browser_process.h"
-#include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/extensions/activity_log/activity_action_constants.h"
 #include "chrome/browser/extensions/activity_log/counting_policy.h"
 #include "chrome/browser/extensions/activity_log/fullstream_ui_policy.h"
@@ -42,8 +38,6 @@
 #include "components/sync_preferences/pref_service_syncable.h"
 #include "content/public/browser/browser_task_traits.h"
 #include "content/public/browser/browser_thread.h"
-#include "content/public/browser/notification_service.h"
-#include "content/public/browser/notification_source.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/web_contents.h"
 #include "extensions/browser/api_activity_monitor.h"
@@ -346,59 +340,9 @@ void ExtractUrls(scoped_refptr<Action> action, Profile* profile) {
   }
 }
 
-// A global, thread-safe record of activity log state.
-class ActivityLogState {
- public:
-  ActivityLogState() {}
-  ~ActivityLogState() {}
-
-  void AddActiveContext(content::BrowserContext* context) {
-    DCHECK_CURRENTLY_ON(BrowserThread::UI);
-    base::AutoLock lock(lock_);
-    contexts_.insert(context);
-  }
-
-  void RemoveActiveContext(content::BrowserContext* context) {
-    DCHECK_CURRENTLY_ON(BrowserThread::UI);
-    base::AutoLock lock(lock_);
-    contexts_.erase(context);
-  }
-
-  bool IsActiveContext(content::BrowserContext* context) {
-    base::AutoLock lock(lock_);
-    return contexts_.count(context) > 0;
-  }
-
-  void AddWhitelistedId(const std::string& id) {
-    DCHECK_CURRENTLY_ON(BrowserThread::UI);
-    base::AutoLock lock(lock_);
-    whitelisted_ids_.insert(id);
-  }
-
-  // We don't remove the id entry from g_activity_log_state because it may be
-  // loaded in multiple profiles, and being whitelisted for the ActivityLog
-  // is a global permission.
-
-  bool IsWhitelistedId(const std::string& id) {
-    base::AutoLock lock(lock_);
-    return whitelisted_ids_.count(id) > 0;
-  }
-
- private:
-  std::set<const content::BrowserContext*> contexts_;
-  std::set<std::string> whitelisted_ids_;
-  base::Lock lock_;
-
-  DISALLOW_COPY_AND_ASSIGN(ActivityLogState);
-};
-
-base::LazyInstance<ActivityLogState>::DestructorAtExit g_activity_log_state =
-    LAZY_INSTANCE_INITIALIZER;
-
 // Returns the ActivityLog associated with the given |browser_context| after
 // checking that |browser_context| is valid.
 ActivityLog* SafeGetActivityLog(content::BrowserContext* browser_context) {
-  DCHECK_CURRENTLY_ON(BrowserThread::UI);
   // There's a chance that the |browser_context| was deleted some time during
   // the thread hops.
   // TODO(devlin): We should probably be doing this more extensively throughout
@@ -411,44 +355,26 @@ ActivityLog* SafeGetActivityLog(content::BrowserContext* browser_context) {
 }
 
 // Calls into the ActivityLog to log an api event or function call.
-// Must be called on the UI thread.
-void LogApiActivityOnUI(content::BrowserContext* browser_context,
-                        const std::string& extension_id,
-                        const std::string& activity_name,
-                        std::unique_ptr<base::ListValue> args,
-                        Action::ActionType type) {
-  DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  ActivityLog* activity_log = SafeGetActivityLog(browser_context);
-  if (!activity_log || !activity_log->ShouldLog(extension_id))
-    return;
-  auto action = base::MakeRefCounted<Action>(extension_id, base::Time::Now(),
-                                             type, activity_name);
-  action->set_args(std::move(args));
-  activity_log->LogAction(action);
-}
-
-// Generic thread-safe handler for API calls and events.
 void LogApiActivity(content::BrowserContext* browser_context,
                     const std::string& extension_id,
                     const std::string& activity_name,
                     const base::ListValue& args,
                     Action::ActionType type) {
-  ActivityLogState& state = g_activity_log_state.Get();
-  if (!state.IsActiveContext(browser_context) ||
-      state.IsWhitelistedId(extension_id))
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  if (ActivityLogAPI::IsExtensionWhitelisted(extension_id))
     return;
-  if (!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
-    base::PostTask(
-        FROM_HERE, {BrowserThread::UI},
-        base::BindOnce(&LogApiActivityOnUI, browser_context, extension_id,
-                       activity_name, args.CreateDeepCopy(), type));
+
+  ActivityLog* activity_log = SafeGetActivityLog(browser_context);
+  if (!activity_log || !activity_log->ShouldLog(extension_id))
     return;
-  }
-  LogApiActivityOnUI(browser_context, extension_id, activity_name,
-                     args.CreateDeepCopy(), type);
+
+  auto action = base::MakeRefCounted<Action>(extension_id, base::Time::Now(),
+                                             type, activity_name);
+  action->set_args(args.CreateDeepCopy());
+  activity_log->LogAction(action);
 }
 
-// Handler for API events. Thread-safe.
+// Handler for API events.
 void LogApiEvent(content::BrowserContext* browser_context,
                  const std::string& extension_id,
                  const std::string& event_name,
@@ -457,7 +383,7 @@ void LogApiEvent(content::BrowserContext* browser_context,
                  Action::ACTION_API_EVENT);
 }
 
-// Handler for API function calls. Thread-safe.
+// Handler for API function calls.
 void LogApiFunction(content::BrowserContext* browser_context,
                     const std::string& extension_id,
                     const std::string& event_name,
@@ -466,18 +392,21 @@ void LogApiFunction(content::BrowserContext* browser_context,
                  Action::ACTION_API_CALL);
 }
 
-// Calls into the ActivityLog to log a webRequest usage.
-// Must be called on the UI thread.
-void LogWebRequestActivityOnUI(content::BrowserContext* browser_context,
-                               const std::string& extension_id,
-                               const GURL& url,
-                               bool is_incognito,
-                               const std::string& api_call,
-                               std::unique_ptr<base::DictionaryValue> details) {
+// Handler for webRequest use.
+void LogWebRequestActivity(content::BrowserContext* browser_context,
+                           const std::string& extension_id,
+                           const GURL& url,
+                           bool is_incognito,
+                           const std::string& api_call,
+                           std::unique_ptr<base::DictionaryValue> details) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  if (ActivityLogAPI::IsExtensionWhitelisted(extension_id))
+    return;
+
   ActivityLog* activity_log = SafeGetActivityLog(browser_context);
   if (!activity_log || !activity_log->ShouldLog(extension_id))
     return;
+
   auto action = base::MakeRefCounted<Action>(
       extension_id, base::Time::Now(), Action::ACTION_WEB_REQUEST, api_call);
   action->set_page_url(url);
@@ -487,28 +416,6 @@ void LogWebRequestActivityOnUI(content::BrowserContext* browser_context,
   activity_log->LogAction(action);
 }
 
-// Handler for webRequest use. Thread-safe.
-void LogWebRequestActivity(content::BrowserContext* browser_context,
-                           const std::string& extension_id,
-                           const GURL& url,
-                           bool is_incognito,
-                           const std::string& api_call,
-                           std::unique_ptr<base::DictionaryValue> details) {
-  ActivityLogState& state = g_activity_log_state.Get();
-  if (!state.IsActiveContext(browser_context) ||
-      state.IsWhitelistedId(extension_id))
-    return;
-  if (!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
-    base::PostTask(FROM_HERE, {BrowserThread::UI},
-                   base::BindOnce(&LogWebRequestActivityOnUI, browser_context,
-                                  extension_id, url, is_incognito, api_call,
-                                  std::move(details)));
-    return;
-  }
-  LogWebRequestActivityOnUI(browser_context, extension_id, url, is_incognito,
-                            api_call, std::move(details));
-}
-
 void SetActivityHandlers() {
   // Set up event handlers. We don't have to worry about unsetting these,
   // because we check whether or not the activity log is active for the context
@@ -617,8 +524,6 @@ void ActivityLog::SetDatabasePolicy(
 ActivityLog::~ActivityLog() {
   if (database_policy_)
     database_policy_->Close();
-  if (is_active_)
-    g_activity_log_state.Get().RemoveActiveContext(profile_);
 }
 
 // MAINTAIN STATUS. ------------------------------------------------------------
@@ -660,7 +565,7 @@ void ActivityLog::OnExtensionLoaded(content::BrowserContext* browser_context,
                                     const Extension* extension) {
   if (!ActivityLogAPI::IsExtensionWhitelisted(extension->id()))
     return;
-  g_activity_log_state.Get().AddWhitelistedId(extension->id());
+
   ++active_consumers_;
 
   if (!extension_system_->ready().is_signaled())
@@ -869,10 +774,6 @@ void ActivityLog::CheckActive(bool use_cached) {
   if (should_be_active == is_active_)
     return;
 
-  ActivityLogState& state = g_activity_log_state.Get();
-  content::BrowserContext* off_the_record =
-      profile_->HasOffTheRecordProfile() ? profile_->GetOffTheRecordProfile()
-                                         : nullptr;
   bool has_db = db_enabled_ && database_policy_;
   bool old_is_active = is_active_;
 
@@ -882,21 +783,10 @@ void ActivityLog::CheckActive(bool use_cached) {
       ChooseDatabasePolicy();
     }
 
-    state.AddActiveContext(profile_);
-    if (off_the_record)
-      state.AddActiveContext(off_the_record);
-    registrar_.Add(this, chrome::NOTIFICATION_PROFILE_CREATED,
-                   content::NotificationService::AllSources());
-    registrar_.Add(this, chrome::NOTIFICATION_PROFILE_DESTROYED,
-                   content::NotificationService::AllSources());
     is_active_ = true;
   } else {
     if (has_db && !needs_db)
       db_enabled_ = false;
-    state.RemoveActiveContext(profile_);
-    if (off_the_record)
-      state.RemoveActiveContext(off_the_record);
-    registrar_.RemoveAll();
     is_active_ = false;
   }
 
@@ -912,28 +802,6 @@ void ActivityLog::CheckActive(bool use_cached) {
   }
 }
 
-void ActivityLog::Observe(int type,
-                          const content::NotificationSource& source,
-                          const content::NotificationDetails& details) {
-  DCHECK(is_active_);
-  switch (type) {
-    case chrome::NOTIFICATION_PROFILE_CREATED: {
-      Profile* profile = content::Source<Profile>(source).ptr();
-      if (profile_->IsSameProfile(profile))
-        g_activity_log_state.Get().AddActiveContext(profile);
-      break;
-    }
-    case chrome::NOTIFICATION_PROFILE_DESTROYED: {
-      Profile* profile = content::Source<Profile>(source).ptr();
-      if (profile_->IsSameProfile(profile))
-        g_activity_log_state.Get().RemoveActiveContext(profile);
-      break;
-    }
-    default:
-      NOTREACHED();
-  }
-}
-
 void ActivityLog::OnExtensionSystemReady() {
   if (active_consumers_ != cached_consumer_count_) {
     CheckActive(false);

chrome/browser/extensions/activity_log/activity_log.h

@@ -19,8 +19,6 @@
 #include "base/threading/thread.h"
 #include "chrome/browser/extensions/activity_log/activity_actions.h"
 #include "chrome/browser/extensions/activity_log/activity_log_policy.h"
-#include "content/public/browser/notification_observer.h"
-#include "content/public/browser/notification_registrar.h"
 #include "extensions/browser/browser_context_keyed_api_factory.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/browser/extension_registry_observer.h"
@@ -47,8 +45,7 @@ class ExtensionSystem;
 // each profile.
 //
 class ActivityLog : public BrowserContextKeyedAPI,
-                    public ExtensionRegistryObserver,
-                    public content::NotificationObserver {
+                    public ExtensionRegistryObserver {
  public:
   // Observers can listen for activity events. There is probably only one
   // observer: the activityLogPrivate API.
@@ -174,11 +171,6 @@ class ActivityLog : public BrowserContextKeyedAPI,
   // whether or not a consumer is active. Otherwise, checks active_consumers_.
   void CheckActive(bool use_cached);
 
-  // content::NotificationObserver:
-  void Observe(int type,
-               const content::NotificationSource& source,
-               const content::NotificationDetails& details) override;
-
   // Called once the ExtensionSystem is ready.
   void OnExtensionSystemReady();
 
@@ -240,8 +232,6 @@ class ActivityLog : public BrowserContextKeyedAPI,
   // reasons.
   bool is_active_;
 
-  content::NotificationRegistrar registrar_;
-
   base::WeakPtrFactory<ActivityLog> weak_factory_{this};
 
   FRIEND_TEST_ALL_PREFIXES(ActivityLogApiTest, TriggerEvent);