[Extensions] Fix incorrect counting prefs in activity log

chrome/browser/extensions/activity_log/activity_log.cc

 // Copyright (c) 2011 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/activity_log/activity_log.h"
 
 #include <stddef.h>
 #include <set>
 #include <utility>
 #include <vector>
@@ skipping 527 matching lines @@
 ActivityLog* ActivityLog::GetInstance(content::BrowserContext* context) {
   return ActivityLog::GetFactoryInstance()->Get(
       Profile::FromBrowserContext(context));
 }
 
 // Use GetInstance instead of directly creating an ActivityLog.
 ActivityLog::ActivityLog(content::BrowserContext* context)
     : database_policy_(NULL),
       database_policy_type_(ActivityLogPolicy::POLICY_INVALID),
       profile_(Profile::FromBrowserContext(context)),
+      extension_system_(ExtensionSystem::Get(context)),
       db_enabled_(false),
       testing_mode_(false),
       has_threads_(true),
       extension_registry_observer_(this),
-      watchdog_apps_active_(0),
-      is_active_(false) {
+      active_consumers_(0),
+      cached_consumer_count_(0),
+      is_active_(false),
+      weak_factory_(this) {
   SetActivityHandlers();
 
   // This controls whether logging statements are printed & which policy is set.
   testing_mode_ = base::CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kEnableExtensionActivityLogTesting);
 
   // Check if the watchdog extension is previously installed and active.
-  watchdog_apps_active_ =
+  cached_consumer_count_ =
       profile_->GetPrefs()->GetInteger(prefs::kWatchdogExtensionActive);
 
   observers_ = new base::ObserverListThreadSafe<Observer>;
 
   // Check that the right threads exist for logging to the database.
   // If not, we shouldn't try to do things that require them.
   if (!BrowserThread::IsMessageLoopValid(BrowserThread::DB) ||
       !BrowserThread::IsMessageLoopValid(BrowserThread::FILE) ||
       !BrowserThread::IsMessageLoopValid(BrowserThread::IO)) {
     has_threads_ = false;
   }
 
-  db_enabled_ =
-      has_threads_ && (base::CommandLine::ForCurrentProcess()->HasSwitch(
-                           switches::kEnableExtensionActivityLogging) ||
-                       watchdog_apps_active_);
-
   extension_registry_observer_.Add(ExtensionRegistry::Get(profile_));
-  ChooseDatabasePolicy();
-
-  CheckActive();
+  CheckActive(true);  // use cached
+  extension_system_->ready().Post(
+      FROM_HERE,
+      base::Bind(&ActivityLog::OnExtensionSystemReady,
+                 weak_factory_.GetWeakPtr()));
 }
 
 void ActivityLog::SetDatabasePolicy(
     ActivityLogPolicy::PolicyType policy_type) {
   if (database_policy_type_ == policy_type)
     return;
   if (!IsDatabaseEnabled() && !IsWatchdogAppActive())
     return;
 
   // Deleting the old policy takes place asynchronously, on the database
@@ skipping 39 matching lines @@
     SetDatabasePolicy(ActivityLogPolicy::POLICY_COUNTS);
 }
 
 bool ActivityLog::IsDatabaseEnabled() {
   // Make sure we are not enabled when there are no threads.
   DCHECK(has_threads_ || !db_enabled_);
   return db_enabled_;
 }
 
 bool ActivityLog::IsWatchdogAppActive() {
-  return (watchdog_apps_active_ > 0);
+  return active_consumers_ > 0;
+}
+
+void ActivityLog::UpdateCachedConsumerCount() {
+  cached_consumer_count_ = active_consumers_;
+  profile_->GetPrefs()->SetInteger(prefs::kWatchdogExtensionActive,
+                                   cached_consumer_count_);
 }
 
 void ActivityLog::SetWatchdogAppActiveForTesting(bool active) {
-  watchdog_apps_active_ = active ? 1 : 0;
-  CheckActive();
+  active_consumers_ = active ? 1 : 0;
+  CheckActive(false);  // don't use cached
 }
 
 void ActivityLog::OnExtensionLoaded(content::BrowserContext* browser_context,
                                     const Extension* extension) {
   if (!ActivityLogAPI::IsExtensionWhitelisted(extension->id()))
     return;
-  if (has_threads_)
-    db_enabled_ = true;
   g_activity_log_state.Get().AddWhitelistedId(extension->id());
-  watchdog_apps_active_++;
-  profile_->GetPrefs()->SetInteger(prefs::kWatchdogExtensionActive,
-                                   watchdog_apps_active_);
-  if (watchdog_apps_active_ == 1)
-    ChooseDatabasePolicy();
+  ++active_consumers_;
 
-  if (!is_active_)
-    CheckActive();
+  if (!extension_system_->ready().is_signaled())
+    return;
+
+

[asargent_no_longer_on_chrome, 2016/09/27 22:54:07]

nit: collapse double-newline to just one

[Devlin, 2016/09/29 17:59:42]

Whoops, done.

+  CheckActive(false);  // don't use cached
+  UpdateCachedConsumerCount();
 }
 
 void ActivityLog::OnExtensionUnloaded(content::BrowserContext* browser_context,
                                       const Extension* extension,
                                       UnloadedExtensionInfo::Reason reason) {
   if (!ActivityLogAPI::IsExtensionWhitelisted(extension->id()))
     return;
-  watchdog_apps_active_--;
-  profile_->GetPrefs()->SetInteger(prefs::kWatchdogExtensionActive,
-                                   watchdog_apps_active_);
-  if (watchdog_apps_active_ == 0 &&
-      !base::CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kEnableExtensionActivityLogging)) {
-    db_enabled_ = false;
-  }
+  --active_consumers_;
 
-  if (is_active_)
-    CheckActive();
+  if (!extension_system_->ready().is_signaled())
+    return;
+
+  CheckActive(false);  // don't use cached
+  UpdateCachedConsumerCount();
 }
 
 // OnExtensionUnloaded will also be called right before this.
 void ActivityLog::OnExtensionUninstalled(
     content::BrowserContext* browser_context,
     const Extension* extension,
     extensions::UninstallReason reason) {
   if (ActivityLogAPI::IsExtensionWhitelisted(extension->id()) &&
       !base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kEnableExtensionActivityLogging) &&
-      watchdog_apps_active_ == 0) {
+      active_consumers_ == 0) {
     DeleteDatabase();
   } else if (database_policy_) {
     database_policy_->RemoveExtensionData(extension->id());
   }
 }
 
 void ActivityLog::AddObserver(ActivityLog::Observer* observer) {
   observers_->AddObserver(observer);
 }
 
@@ skipping 21 matching lines @@
       base::StartsWith(action->api_name(), kDomXhrPrefix,
                        base::CompareCase::SENSITIVE) &&
       action->other()) {
     base::DictionaryValue* other = action->mutable_other();
     int dom_verb = -1;
     if (other->GetInteger(constants::kActionDomVerb, &dom_verb) &&
         dom_verb == DomActionType::METHOD) {
       other->SetInteger(constants::kActionDomVerb, DomActionType::XHR);
     }
   }
-
   if (IsDatabaseEnabled() && database_policy_)
     database_policy_->ProcessAction(action);
   if (IsWatchdogAppActive())
     observers_->Notify(FROM_HERE, &Observer::OnExtensionActivity, action);
   if (testing_mode_)
     VLOG(1) << action->PrintForDebug();
 }
 
 bool ActivityLog::ShouldLog(const std::string& extension_id) const {
   return is_active_ && !ActivityLogAPI::IsExtensionWhitelisted(extension_id);
@@ skipping 92 matching lines @@
   urls.push_back(url);
   RemoveURLs(urls);
 }
 
 void ActivityLog::DeleteDatabase() {
   if (!database_policy_)
     return;
   database_policy_->DeleteDatabase();
 }
 
-void ActivityLog::CheckActive() {
-  bool has_db = db_enabled_ && database_policy_;
+void ActivityLog::CheckActive(bool use_cached) {
+  bool has_consumer =
+      active_consumers_ || (use_cached && cached_consumer_count_);
+  bool needs_db =
+      has_threads_ && (has_consumer ||
+                       base::CommandLine::ForCurrentProcess()->HasSwitch(
+                           switches::kEnableExtensionActivityLogging));
+  bool should_be_active = needs_db || has_consumer;
+
+  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_;
 
-  bool old_is_active = is_active_;
-  if (has_db || IsWatchdogAppActive()) {
-    if (is_active_)
-      return;  // Already enabled.
+  if (should_be_active) {
+    if (needs_db && !has_db) {
+      db_enabled_ = true;
+      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 (is_active_) {
+  } 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;
   }
 
-  if (old_is_active != is_active_) {
-    for (content::RenderProcessHost::iterator iter(
-             content::RenderProcessHost::AllHostsIterator());
-         !iter.IsAtEnd(); iter.Advance()) {
-      content::RenderProcessHost* host = iter.GetCurrentValue();
-      if (profile_->IsSameProfile(
-              Profile::FromBrowserContext(host->GetBrowserContext()))) {
-        host->Send(new ExtensionMsg_SetActivityLoggingEnabled(is_active_));
-      }
+  DCHECK_NE(is_active_, old_is_active);
+  for (content::RenderProcessHost::iterator iter(
+           content::RenderProcessHost::AllHostsIterator());
+       !iter.IsAtEnd(); iter.Advance()) {
+    content::RenderProcessHost* host = iter.GetCurrentValue();
+    if (profile_->IsSameProfile(
+            Profile::FromBrowserContext(host->GetBrowserContext()))) {
+      host->Send(new ExtensionMsg_SetActivityLoggingEnabled(is_active_));
     }
   }
 }
 
 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);
+    UpdateCachedConsumerCount();
+  }
+}
+
 template <>
 void BrowserContextKeyedAPIFactory<ActivityLog>::DeclareFactoryDependencies() {
   DependsOn(ExtensionsBrowserClient::Get()->GetExtensionSystemFactory());
   DependsOn(ExtensionRegistryFactory::GetInstance());
 }
 
 }  // namespace extensions