Make RulesRegistry::process_changed_rules_requested_ a map (relanding)

The original change (corresponds to patch set 1 here) was in https://codereview.chromium.org/79433002. Description follows:
> process_changed_rules_requested_ stores the state of progress
> on writing a set of rules to the rules store. For each extension
> there are separate write tasks. Currently, process_changed_rules_requested_
> is share among all such writes, which is wrong. This CL makes
> process_changed_rules_requested_ a map from extension IDs to the state flags.

However, that change got reverted in https://codereview.chromium.org/90993005, because it leaked stuff.

The problem is that objects scheduled for deletion on FILE thread were scheduled after that thread's message loop was spun. See http://crbug.com/323916#c4 and http://crbug.com/323916#c5 for a detailed analysis.

This CL aims at 2 things:
1) re-land the change from https://codereview.chromium.org/79433002
2) fix the leaks (fix should be in since patch set 2)

BUG=320645,323916

Review URL: https://codereview.chromium.org/91843005

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@237774 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 71 insertions, 46 deletions

diff --git a/chrome/browser/extensions/api/declarative/rules_registry.cc b/chrome/browser/extensions/api/declarative/rules_registry.cc
index e01b6f0..c9db12e 100644
--- a/chrome/browser/extensions/api/declarative/rules_registry.cc
+++ b/chrome/browser/extensions/api/declarative/rules_registry.cc
@@ -4,10 +4,13 @@
 
 #include "chrome/browser/extensions/api/declarative/rules_registry.h"
 
+#include <utility>
+
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/message_loop/message_loop.h"
 #include "base/metrics/histogram.h"
+#include "base/stl_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/time/time.h"
 #include "base/values.h"
@@ -81,8 +84,6 @@ RulesRegistry::RulesRegistry(
       event_name_(event_name),
       webview_key_(webview_key),
       weak_ptr_factory_(profile ? this : NULL),
-      process_changed_rules_requested_(profile ? NOT_SCHEDULED_FOR_PROCESSING
-                                               : NEVER_PROCESS),
       last_generated_rule_identifier_id_(0) {
   if (cache_delegate) {
     cache_delegate_ = cache_delegate->GetWeakPtr();
@@ -271,7 +272,8 @@ void RulesRegistry::MarkReady(base::Time storage_init_time) {
 void RulesRegistry::ProcessChangedRules(const std::string& extension_id) {
   DCHECK(content::BrowserThread::CurrentlyOn(owner_thread()));
 
-  process_changed_rules_requested_ = NOT_SCHEDULED_FOR_PROCESSING;
+  DCHECK(ContainsKey(process_changed_rules_requested_, extension_id));
+  process_changed_rules_requested_[extension_id] = NOT_SCHEDULED_FOR_PROCESSING;
 
   std::vector<linked_ptr<Rule> > new_rules;
   GetAllRules(extension_id, &new_rules);
@@ -285,10 +287,17 @@ void RulesRegistry::ProcessChangedRules(const std::string& extension_id) {
 }
 
 void RulesRegistry::MaybeProcessChangedRules(const std::string& extension_id) {
-  if (process_changed_rules_requested_ != NOT_SCHEDULED_FOR_PROCESSING)
+  // Read and initialize |process_changed_rules_requested_[extension_id]| if
+  // necessary. (Note that the insertion below will not overwrite
+  // |process_changed_rules_requested_[extension_id]| if that already exists.
+  std::pair<ProcessStateMap::iterator, bool> insertion =
+      process_changed_rules_requested_.insert(std::make_pair(
+          extension_id,
+          profile_ ? NOT_SCHEDULED_FOR_PROCESSING : NEVER_PROCESS));
+  if (insertion.first->second != NOT_SCHEDULED_FOR_PROCESSING)
     return;
 
-  process_changed_rules_requested_ = SCHEDULED_FOR_PROCESSING;
+  process_changed_rules_requested_[extension_id] = SCHEDULED_FOR_PROCESSING;
   ready_.Post(FROM_HERE,
               base::Bind(&RulesRegistry::ProcessChangedRules,
                          weak_ptr_factory_.GetWeakPtr(),
diff --git a/chrome/browser/extensions/api/declarative/rules_registry.h b/chrome/browser/extensions/api/declarative/rules_registry.h
index 59000b2..7871c50 100644
--- a/chrome/browser/extensions/api/declarative/rules_registry.h
+++ b/chrome/browser/extensions/api/declarative/rules_registry.h
@@ -195,6 +195,7 @@ class RulesRegistry : public base::RefCountedThreadSafe<RulesRegistry> {
     // to schedule one.
     NOT_SCHEDULED_FOR_PROCESSING
   };
+  typedef std::map<ExtensionId, ProcessChangedRulesState> ProcessStateMap;
 
   base::WeakPtr<RulesRegistry> GetWeakPtr() {
     DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
@@ -204,7 +205,8 @@ class RulesRegistry : public base::RefCountedThreadSafe<RulesRegistry> {
   // Common processing after extension's rules have changed.
   void ProcessChangedRules(const std::string& extension_id);
 
-  // Calls ProcessChangedRules if |process_changed_rules_requested_| ==
+  // Calls ProcessChangedRules if
+  // |process_changed_rules_requested_(extension_id)| ==
   // NOT_SCHEDULED_FOR_PROCESSING.
   void MaybeProcessChangedRules(const std::string& extension_id);
 
@@ -246,7 +248,7 @@ class RulesRegistry : public base::RefCountedThreadSafe<RulesRegistry> {
   // instance.
   base::WeakPtr<RulesCacheDelegate> cache_delegate_;
 
-  ProcessChangedRulesState process_changed_rules_requested_;
+  ProcessStateMap process_changed_rules_requested_;
 
   // Returns whether any existing rule is registered with identifier |rule_id|
   // for extension |extension_id|.
diff --git a/chrome/browser/extensions/api/declarative/rules_registry_with_cache_unittest.cc b/chrome/browser/extensions/api/declarative/rules_registry_with_cache_unittest.cc
index f0c392c..d7763e3 100644
--- a/chrome/browser/extensions/api/declarative/rules_registry_with_cache_unittest.cc
+++ b/chrome/browser/extensions/api/declarative/rules_registry_with_cache_unittest.cc
@@ -9,7 +9,7 @@
 // RulesRegistryWithCache.
 
 #include "base/command_line.h"
-#include "base/message_loop/message_loop.h"
+#include "base/run_loop.h"
 #include "chrome/browser/extensions/api/declarative/rules_cache_delegate.h"
 #include "chrome/browser/extensions/api/declarative/test_rules_registry.h"
 #include "chrome/browser/extensions/extension_prefs.h"
@@ -18,7 +18,7 @@
 #include "chrome/browser/value_store/testing_value_store.h"
 #include "chrome/common/extensions/extension_test_util.h"
 #include "chrome/test/base/testing_profile.h"
-#include "content/public/test/test_browser_thread.h"
+#include "content/public/test/test_browser_thread_bundle.h"
 #include "extensions/common/extension.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -43,20 +43,15 @@ namespace extensions {
 class RulesRegistryWithCacheTest : public testing::Test {
  public:
   RulesRegistryWithCacheTest()
-      : ui_thread_(content::BrowserThread::UI, &message_loop_),
-        file_thread_(content::BrowserThread::FILE, &message_loop_),
-        registry_(new TestRulesRegistry(content::BrowserThread::UI,
-                                        "" /*event_name*/,
+      : cache_delegate_(/*log_storage_init_delay=*/false ),
+        registry_(new TestRulesRegistry(&profile_,
+                                        /*event_name=*/"",
+                                        content::BrowserThread::UI,
+                                        &cache_delegate_,
                                         RulesRegistry::WebViewKey(0, 0))) {}
 
   virtual ~RulesRegistryWithCacheTest() {}
 
-  virtual void TearDown() OVERRIDE {
-    // Make sure that deletion traits of all registries are executed.
-    registry_ = NULL;
-    message_loop_.RunUntilIdle();
-  }
-
   std::string AddRule(const std::string& extension_id,
                       const std::string& rule_id,
                       TestRulesRegistry* registry) {
@@ -90,9 +85,11 @@ class RulesRegistryWithCacheTest : public testing::Test {
   }
 
  protected:
-  base::MessageLoop message_loop_;
-  content::TestBrowserThread ui_thread_;
-  content::TestBrowserThread file_thread_;
+  content::TestBrowserThreadBundle thread_bundle_;
+  TestingProfile profile_;
+  RulesCacheDelegate cache_delegate_;
+  // |registry_| needs to be defined after |thread_bundle_| to ensure that it
+  // is released before the final spinning of threads.
   scoped_refptr<TestRulesRegistry> registry_;
 #if defined OS_CHROMEOS
   chromeos::ScopedTestDeviceSettingsService test_device_settings_service_;
@@ -215,12 +212,11 @@ TEST_F(RulesRegistryWithCacheTest, OnExtensionUninstalled) {
 }
 
 TEST_F(RulesRegistryWithCacheTest, DeclarativeRulesStored) {
-  TestingProfile profile;
   // TestingProfile::Init makes sure that the factory method for a corresponding
   // extension system creates a TestExtensionSystem.
   extensions::TestExtensionSystem* system =
       static_cast<extensions::TestExtensionSystem*>(
-          extensions::ExtensionSystem::Get(&profile));
+          extensions::ExtensionSystem::Get(&profile_));
   ExtensionPrefs* extension_prefs = system->CreateExtensionPrefs(
       CommandLine::ForCurrentProcess(), base::FilePath());
   system->CreateExtensionService(
@@ -229,13 +225,12 @@ TEST_F(RulesRegistryWithCacheTest, DeclarativeRulesStored) {
   TestingValueStore* store = system->value_store();
 
   const std::string event_name("testEvent");
-  const RulesRegistry::WebViewKey key(0, 0);
   const std::string rules_stored_key(
       RulesCacheDelegate::GetRulesStoredKey(
-          event_name, profile.IsOffTheRecord()));
+          event_name, profile_.IsOffTheRecord()));
   scoped_ptr<RulesCacheDelegate> cache_delegate(new RulesCacheDelegate(false));
   scoped_refptr<RulesRegistry> registry(new TestRulesRegistry(
-      &profile, event_name, content::BrowserThread::UI,
+      &profile_, event_name, content::BrowserThread::UI,
       cache_delegate.get(),
       RulesRegistry::WebViewKey(0, 0)));
 
@@ -258,14 +253,14 @@ TEST_F(RulesRegistryWithCacheTest, DeclarativeRulesStored) {
   value->AppendBoolean(true);
   cache_delegate->WriteToStorage(kExtensionId, value.PassAs<base::Value>());
   EXPECT_TRUE(cache_delegate->GetDeclarativeRulesStored(kExtensionId));
-  message_loop_.RunUntilIdle();
+  base::RunLoop().RunUntilIdle();
   EXPECT_EQ(write_count + 1, store->write_count());
   write_count = store->write_count();
 
   value.reset(new base::ListValue);
   cache_delegate->WriteToStorage(kExtensionId, value.PassAs<base::Value>());
   EXPECT_FALSE(cache_delegate->GetDeclarativeRulesStored(kExtensionId));
-  message_loop_.RunUntilIdle();
+  base::RunLoop().RunUntilIdle();
   // No rules currently, but previously there were, so we expect a write.
   EXPECT_EQ(write_count + 1, store->write_count());
   write_count = store->write_count();
@@ -273,7 +268,7 @@ TEST_F(RulesRegistryWithCacheTest, DeclarativeRulesStored) {
   value.reset(new base::ListValue);
   cache_delegate->WriteToStorage(kExtensionId, value.PassAs<base::Value>());
   EXPECT_FALSE(cache_delegate->GetDeclarativeRulesStored(kExtensionId));
-  message_loop_.RunUntilIdle();
+  base::RunLoop().RunUntilIdle();
   EXPECT_EQ(write_count, store->write_count());
 
   // 3. Test reading behavior.
@@ -281,45 +276,43 @@ TEST_F(RulesRegistryWithCacheTest, DeclarativeRulesStored) {
 
   cache_delegate->SetDeclarativeRulesStored(kExtensionId, false);
   cache_delegate->ReadFromStorage(kExtensionId);
-  message_loop_.RunUntilIdle();
+  base::RunLoop().RunUntilIdle();
   EXPECT_EQ(read_count, store->read_count());
   read_count = store->read_count();
 
   cache_delegate->SetDeclarativeRulesStored(kExtensionId, true);
   cache_delegate->ReadFromStorage(kExtensionId);
-  message_loop_.RunUntilIdle();
+  base::RunLoop().RunUntilIdle();
   EXPECT_EQ(read_count + 1, store->read_count());
 }
 
 // Test that each registry has its own "are some rules stored" flag.
 TEST_F(RulesRegistryWithCacheTest, RulesStoredFlagMultipleRegistries) {
-  TestingProfile profile;
   // TestingProfile::Init makes sure that the factory method for a corresponding
   // extension system creates a TestExtensionSystem.
   extensions::TestExtensionSystem* system =
       static_cast<extensions::TestExtensionSystem*>(
-          extensions::ExtensionSystem::Get(&profile));
+          extensions::ExtensionSystem::Get(&profile_));
   ExtensionPrefs* extension_prefs = system->CreateExtensionPrefs(
       CommandLine::ForCurrentProcess(), base::FilePath());
 
   const std::string event_name1("testEvent1");
   const std::string event_name2("testEvent2");
-  const RulesRegistry::WebViewKey key(0, 0);
   const std::string rules_stored_key1(
       RulesCacheDelegate::GetRulesStoredKey(
-          event_name1, profile.IsOffTheRecord()));
+          event_name1, profile_.IsOffTheRecord()));
   const std::string rules_stored_key2(
       RulesCacheDelegate::GetRulesStoredKey(
-          event_name2, profile.IsOffTheRecord()));
+          event_name2, profile_.IsOffTheRecord()));
   scoped_ptr<RulesCacheDelegate> cache_delegate1(new RulesCacheDelegate(false));
   scoped_refptr<RulesRegistry> registry1(new TestRulesRegistry(
-      &profile, event_name1, content::BrowserThread::UI,
+      &profile_, event_name1, content::BrowserThread::UI,
       cache_delegate1.get(),
       RulesRegistry::WebViewKey(0, 0)));
 
   scoped_ptr<RulesCacheDelegate> cache_delegate2(new RulesCacheDelegate(false));
   scoped_refptr<RulesRegistry> registry2(new TestRulesRegistry(
-      &profile, event_name2, content::BrowserThread::UI,
+      &profile_, event_name2, content::BrowserThread::UI,
       cache_delegate2.get(),
       RulesRegistry::WebViewKey(0, 0)));
 
@@ -337,11 +330,9 @@ TEST_F(RulesRegistryWithCacheTest, RulesStoredFlagMultipleRegistries) {
 TEST_F(RulesRegistryWithCacheTest, RulesPreservedAcrossRestart) {
   // This test makes sure that rules are restored from the rule store
   // on registry (in particular, browser) restart.
-
-  TestingProfile profile;
   extensions::TestExtensionSystem* system =
       static_cast<extensions::TestExtensionSystem*>(
-          extensions::ExtensionSystem::Get(&profile));
+          extensions::ExtensionSystem::Get(&profile_));
   ExtensionService* extension_service = system->CreateExtensionService(
       CommandLine::ForCurrentProcess(), base::FilePath(), false);
 
@@ -361,27 +352,50 @@ TEST_F(RulesRegistryWithCacheTest, RulesPreservedAcrossRestart) {
   // 2. First run, adding a rule for the extension.
   scoped_ptr<RulesCacheDelegate> cache_delegate(new RulesCacheDelegate(false));
   scoped_refptr<TestRulesRegistry> registry(new TestRulesRegistry(
-      &profile,
+      &profile_,
       "testEvent",
       content::BrowserThread::UI,
       cache_delegate.get(),
       RulesRegistry::WebViewKey(0, 0)));
 
   AddRule(kExtensionId, kRuleId, registry.get());
-  message_loop_.RunUntilIdle();  // Posted tasks store the added rule.
+  base::RunLoop().RunUntilIdle();  // Posted tasks store the added rule.
   EXPECT_EQ(1, GetNumberOfRules(kExtensionId, registry.get()));
 
   // 3. Restart the TestRulesRegistry and see the rule still there.
   cache_delegate.reset(new RulesCacheDelegate(false));
   registry = new TestRulesRegistry(
-      &profile,
+      &profile_,
       "testEvent",
       content::BrowserThread::UI,
       cache_delegate.get(),
       RulesRegistry::WebViewKey(0, 0));
 
-  message_loop_.RunUntilIdle();  // Posted tasks retrieve the stored rule.
+  base::RunLoop().RunUntilIdle();  // Posted tasks retrieve the stored rule.
   EXPECT_EQ(1, GetNumberOfRules(kExtensionId, registry.get()));
 }
 
+TEST_F(RulesRegistryWithCacheTest, ConcurrentStoringOfRules) {
+  // When an extension updates its rules, the new set of rules is stored to disk
+  // with some delay. While it is acceptable for a quick series of updates for a
+  // single extension to only write the last one, we should never forget to
+  // write a rules update for extension A, just because it is immediately
+  // followed by a rules update for extension B.
+  extensions::TestExtensionSystem* system =
+      static_cast<extensions::TestExtensionSystem*>(
+          extensions::ExtensionSystem::Get(&profile_));
+  system->CreateExtensionPrefs(CommandLine::ForCurrentProcess(),
+                               base::FilePath());
+  system->CreateExtensionService(
+      CommandLine::ForCurrentProcess(), base::FilePath(), false);
+  TestingValueStore* store = system->value_store();
+
+  int write_count = store->write_count();
+  EXPECT_EQ("", AddRule(kExtensionId, kRuleId));
+  EXPECT_EQ("", AddRule(kExtension2Id, kRule2Id));
+  system->SetReady();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(write_count + 2, store->write_count());
+}
+
 }  //  namespace extensions