diff options
author | msramek <msramek@chromium.org> | 2015-04-14 05:38:26 -0700 |
---|---|---|
committer | Commit bot <commit-bot@chromium.org> | 2015-04-14 12:38:49 +0000 |
commit | 847392d8a13b553c4a5dddc1c62de4d861f4c003 (patch) | |
tree | 3df5dab093f469fc69b20de1fbc5008702b4ca8d /components/content_settings | |
parent | b0e32c07e2088388782ca96791e48cf4ec554dfc (diff) | |
download | chromium_src-847392d8a13b553c4a5dddc1c62de4d861f4c003.zip chromium_src-847392d8a13b553c4a5dddc1c62de4d861f4c003.tar.gz chromium_src-847392d8a13b553c4a5dddc1c62de4d861f4c003.tar.bz2 |
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}
Diffstat (limited to 'components/content_settings')
-rw-r--r-- | components/content_settings/core/browser/content_settings_default_provider.cc | 43 |
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); |