DefaultProvider: lock only cache writes, not file writes.

In DefaultProvider, we're currently locking the entire operation of changing a default content setting:
1. Changing the setting in the cache
2. Changing the individual preference
3. Possibly changing the dictionary preference

In fact, only the cache access is multi-threaded and should be locked. Locking the preference access can lead to a situation where the same thread re-acquires the non-reentrant lock, such as when a preference is written by a non-incognito provider, what causes a preference read from the incognito provider, on the same (UI) thread.

BUG=476826

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

Cr-Commit-Position: refs/heads/master@{#325028}

1 files changed, 33 insertions, 10 deletions

diff --git a/components/content_settings/core/browser/content_settings_default_provider.cc b/components/content_settings/core/browser/content_settings_default_provider.cc
index 4d3e08b..0af5197 100644
--- a/components/content_settings/core/browser/content_settings_default_provider.cc
+++ b/components/content_settings/core/browser/content_settings_default_provider.cc
@@ -249,9 +249,14 @@ bool DefaultProvider::SetWebsiteSetting(
   scoped_ptr<base::Value> value(in_value);
   {
     base::AutoReset<bool> auto_reset(&updating_preferences_, true);
-    base::AutoLock lock(lock_);
-
-    ChangeSetting(content_type, value.get());
+    // Lock the memory map access, so that values are not read by
+    // |GetRuleIterator| at the same time as they are written here. Do not lock
+    // the preference access though; preference updates send out notifications
+    // whose callbacks may try to reacquire the lock on the same thread.
+    {
+      base::AutoLock lock(lock_);
+      ChangeSetting(content_type, value.get());
+    }
     WriteIndividualPref(content_type, value.get());
 
     // If the changed setting is syncable, write it to the old dictionary
@@ -374,19 +379,31 @@ void DefaultProvider::OnPreferenceChanged(const std::string& name) {
     // If the dictionary preference gets synced from an old version
     // of Chrome, we should update all individual preferences that
     // are marked as syncable.
-    base::AutoLock lock(lock_);
     base::AutoReset<bool> auto_reset(&updating_preferences_, true);
 
     scoped_ptr<ValueMap> dictionary = ReadDictionaryPref();
 
+    // Lock the memory map access, so that values are not read by
+    // |GetRuleIterator| at the same time as they are written here. Do not lock
+    // the preference access though; preference updates send out notifications
+    // whose callbacks may try to reacquire the lock on the same thread.
+    {
+      base::AutoLock lock(lock_);
+      for (const auto& it : *dictionary) {
+        if (!IsContentSettingsTypeSyncable(it.first))
+          continue;
+
+        DCHECK(default_settings_.find(it.first) != default_settings_.end());
+        ChangeSetting(it.first, it.second.get());
+        to_notify.push_back(it.first);
+      }
+    }
+
+    // When the lock is released, write the new settings to preferences.
     for (const auto& it : *dictionary) {
       if (!IsContentSettingsTypeSyncable(it.first))
         continue;
-
-      DCHECK(default_settings_.find(it.first) != default_settings_.end());
-      ChangeSetting(it.first, it.second.get());
       WriteIndividualPref(it.first, it.second.get());
-      to_notify.push_back(it.first);
     }
   } else {
     // Find out which content setting the preference corresponds to.
@@ -407,10 +424,16 @@ void DefaultProvider::OnPreferenceChanged(const std::string& name) {
     // A new individual preference is changed. If it is syncable, we should
     // change its entry in the dictionary preference as well, so that it
     // can be synced to older versions of Chrome.
-    base::AutoLock lock(lock_);
     base::AutoReset<bool> auto_reset(&updating_preferences_, true);
 
-    ChangeSetting(content_type, ReadIndividualPref(content_type).get());
+    // Lock the memory map access, so that values are not read by
+    // |GetRuleIterator| at the same time as they are written here. Do not lock
+    // the preference access though; preference updates send out notifications
+    // whose callbacks may try to reacquire the lock on the same thread.
+    {
+      base::AutoLock lock(lock_);
+      ChangeSetting(content_type, ReadIndividualPref(content_type).get());
+    }
     if (IsContentSettingsTypeSyncable(content_type))
       WriteDictionaryPref(content_type, default_settings_[content_type].get());
     to_notify.push_back(content_type);