Fix partially broken pattern keys and delete invalid pattern keys of the...

Fix partially broken pattern keys and delete invalid pattern keys of the obsolete content settings pattern pref.

BUG=90490
TEST=content_settings_pref_provider_unittest.cc

Review URL: http://codereview.chromium.org/7529025

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

chrome/browser/content_settings/content_settings_pref_provider.cc

@@ -4,7 +4,7 @@
 
 #include "chrome/browser/content_settings/content_settings_pref_provider.h"
 
-#include <list>
+#include <map>
 #include <string>
 #include <utility>
 #include <vector>
@@ -12,6 +12,7 @@
 #include "base/auto_reset.h"
 #include "base/command_line.h"
 #include "base/metrics/histogram.h"
+#include "base/scoped_ptr.h"
 #include "chrome/browser/content_settings/content_settings_pattern.h"
 #include "chrome/browser/content_settings/content_settings_utils.h"
 #include "chrome/browser/prefs/pref_service.h"
@@ -30,6 +31,9 @@
 
 namespace {
 
+typedef std::pair<std::string, std::string> StringPair;
+typedef std::map<std::string, std::string> StringMap;
+
 // The preference keys where resource identifiers are stored for
 // ContentSettingsType values that support resource identifiers.
 const char* kResourceTypeNames[] = {
@@ -964,42 +968,80 @@ void PrefProvider::MigrateObsoletePopupsPref() {
 
 void PrefProvider::MigrateObsoleteContentSettingsPatternPref() {
   if (prefs_->HasPrefPath(prefs::kContentSettingsPatterns) && !is_incognito_) {
-    const DictionaryValue* all_settings_dictionary =
+    const DictionaryValue* patterns_dictionary =
       prefs_->GetDictionary(prefs::kContentSettingsPatterns);
 
-    DictionaryPrefUpdate update(prefs_, prefs::kContentSettingsPatternPairs);
-    DictionaryValue* exceptions_dictionary;
-    exceptions_dictionary = update.Get();
-    for (DictionaryValue::key_iterator i(all_settings_dictionary->begin_keys());
-         i != all_settings_dictionary->end_keys();
-         ++i) {
-      const std::string& key(*i);
-      if (key.empty())
-        continue;
+    // A map with an old key, new key mapping. If the new key is empty then the
+    // value for the old key will be removed.
+    StringMap keys_to_change;
 
-      // Validate pattern string and skip it if it is invalid.
-      std::pair<ContentSettingsPattern, ContentSettingsPattern> pattern_pair =
-          ParsePatternString(key);
-      const ContentSettingsPattern& primary_pattern = pattern_pair.first;
-      if (!primary_pattern.IsValid()) {
-        LOG(DFATAL) << "Invalid pattern strings: " << key;
-        continue;
-      }
+    {
+      DictionaryPrefUpdate update(prefs_, prefs::kContentSettingsPatternPairs);
+      DictionaryValue* pattern_pairs_dictionary = update.Get();
+      for (DictionaryValue::key_iterator i(
+               patterns_dictionary->begin_keys());
+           i != patterns_dictionary->end_keys();
+           ++i) {
+        const std::string& key(*i);
+        // Remove broken pattern keys and fix keys with pattern pairs.
+        size_t sep_pos = key.find(",");
+        ContentSettingsPattern pattern =
+            ContentSettingsPattern::FromString(key.substr(0, sep_pos));
+
+        // Save the key if it contains a invalid patterns to remove it later.
+        // Continue and don't try to migrate the broken pattern key.
+        if (!pattern.IsValid()) {
+          keys_to_change[key] = "";
+          continue;
+        }
 
-      // Copy dictionary value.
-      // Get old settings.
-      DictionaryValue* dictionary = NULL;
-      bool found = all_settings_dictionary->GetDictionaryWithoutPathExpansion(
-          key, &dictionary);
-      DCHECK(found);
+        // If the key contains a pattern pair, then remove the secondary
+        // pattern from the key.
+        if (sep_pos != std::string::npos) {
+          // If the dictionary already has a key that equals the primary pattern
+          // of the corrupted pattern pair key, don't fix the key but remove it.
+          if (patterns_dictionary->HasKey(pattern.ToString())) {
+            keys_to_change[key] = "";
+            continue;
+          }
 
-      // Create new dictionary key.
-      std::string new_pattern_str = CreatePatternString(
-          primary_pattern, ContentSettingsPattern::Wildcard());
+          // If there is more than one key with a pattern pair that has the same
+          // valid primary pattern, then the value of the last key processed
+          // will win and  overwrite the value any previous key.
+          keys_to_change[key] = pattern.ToString();
+        }
 
-      // Existing values are overwritten.
-      exceptions_dictionary->SetWithoutPathExpansion(
-          new_pattern_str, dictionary->DeepCopy());
+        // Copy dictionary value.
+        DictionaryValue* dictionary = NULL;
+        bool found = patterns_dictionary->GetDictionaryWithoutPathExpansion(
+            key, &dictionary);
+        DCHECK(found);
+        std::string new_key = CreatePatternString(
+            pattern, ContentSettingsPattern::Wildcard());
+        // Existing values are overwritten.
+        pattern_pairs_dictionary->SetWithoutPathExpansion(
+            new_key, dictionary->DeepCopy());
+      }
+    }
+
+    {
+      DictionaryPrefUpdate update(prefs_, prefs::kContentSettingsPatterns);
+      DictionaryValue* mutable_patterns_dictionary = update.Get();
+      // Fix broken pattern strings.
+      for (StringMap::iterator i(keys_to_change.begin());
+           i != keys_to_change.end();
+           ++i) {
+        const StringPair& pattern_str_pair(*i);
+        Value* dict_ptr = NULL;
+        bool found = mutable_patterns_dictionary->RemoveWithoutPathExpansion(
+            pattern_str_pair.first, &dict_ptr);
+        scoped_ptr<Value> dict(dict_ptr);
+        DCHECK(found);
+        if (!pattern_str_pair.second.empty()) {
+          mutable_patterns_dictionary->SetWithoutPathExpansion(
+              pattern_str_pair.second, dict.release());
+        }
+      }
     }
   }
 }

chrome/browser/content_settings/content_settings_pref_provider_unittest.cc

@@ -489,4 +489,70 @@ TEST_F(PrefProviderTest, SyncObsoletePref) {
   provider.ShutdownOnUIThread();
 }
 
+
+TEST_F(PrefProviderTest, FixOrRemoveMalformedPatternKeysFromObsoletePref) {
+  TestingProfile profile;
+  PrefService* prefs = profile.GetPrefs();
+
+  // Set obsolete preference for content settings pattern.
+  scoped_ptr<DictionaryValue> settings_dictionary(new DictionaryValue());
+  settings_dictionary->SetInteger("cookies", 2);
+  settings_dictionary->SetInteger("images", 2);
+  settings_dictionary->SetInteger("popups", 2);
+  scoped_ptr<DictionaryValue> all_settings_dictionary(new DictionaryValue());
+  // Good pattern key.
+  all_settings_dictionary->SetWithoutPathExpansion(
+      "http://www.example.com", settings_dictionary->DeepCopy());
+  // Bad pattern key that will be ignored since there is already a good pattern
+  // key for the primary patter of the bad pattern key.
+  all_settings_dictionary->SetWithoutPathExpansion(
+      "http://www.example.com,", settings_dictionary->DeepCopy());
+
+  // Bad pattern key that should be removed.
+  all_settings_dictionary->SetWithoutPathExpansion(
+      "http://www.broken.com*", settings_dictionary->DeepCopy());
+
+  all_settings_dictionary->SetWithoutPathExpansion(
+      "http://www.bar.com,", settings_dictionary->DeepCopy());
+
+  // Bad pattern key with a trailing comma that is supposed to be fixed.
+  // A trailing comma means that the secondary pattern string is empty and hence
+  // invalid.
+  all_settings_dictionary->SetWithoutPathExpansion(
+      "http://www.foo.com,", settings_dictionary->DeepCopy());
+  // Bad pattern key with an invalid secondary pattern that should be removed.
+  all_settings_dictionary->SetWithoutPathExpansion(
+      "http://www.foo.com,error*", settings_dictionary->DeepCopy());
+  // Pattern keys with valid pattern pairs.
+  all_settings_dictionary->SetWithoutPathExpansion(
+      "http://www.foo.com,[*.]bar.com", settings_dictionary->DeepCopy());
+  prefs->Set(prefs::kContentSettingsPatterns, *all_settings_dictionary);
+  all_settings_dictionary->SetWithoutPathExpansion(
+      "http://www.example2.com,*", settings_dictionary->DeepCopy());
+  prefs->Set(prefs::kContentSettingsPatterns, *all_settings_dictionary);
+
+  content_settings::PrefProvider provider(prefs, false);
+
+  // Tests that the broken pattern keys got fixed or removed.
+  const DictionaryValue* patterns_dictionary =
+      prefs->GetDictionary(prefs::kContentSettingsPatterns);
+  EXPECT_EQ(4U, patterns_dictionary->size());
+  EXPECT_TRUE(patterns_dictionary->HasKey("http://www.example.com"));
+  EXPECT_TRUE(patterns_dictionary->HasKey("http://www.bar.com"));
+  EXPECT_TRUE(patterns_dictionary->HasKey("http://www.foo.com"));
+  EXPECT_TRUE(patterns_dictionary->HasKey("http://www.example2.com"));
+
+  // Broken pattern keys that should be removed
+  EXPECT_FALSE(patterns_dictionary->HasKey("http://www.bar.com,"));
+  EXPECT_FALSE(patterns_dictionary->HasKey("http://www.foo.com,"));
+  EXPECT_FALSE(patterns_dictionary->HasKey("http://www.foo.com,error*"));
+  EXPECT_FALSE(patterns_dictionary->HasKey(
+      "http://www.foo.com,[*.]bar.com"));
+  EXPECT_FALSE(patterns_dictionary->HasKey("http://www.example2.com,*"));
+
+  EXPECT_FALSE(patterns_dictionary->HasKey("http://www.broken.com*"));
+
+  provider.ShutdownOnUIThread();
+}
+
 }  // namespace content_settings