Fix bad usage of GetMutableDictionary.

Only the regular profile should be allowed to modify the user pref store.

BUG=74466
TEST=Execute new PrefProviderTest.ObserverIncognito

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

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

2 files changed, 102 insertions, 6 deletions

diff --git a/chrome/browser/content_settings/content_settings_pref_provider.cc b/chrome/browser/content_settings/content_settings_pref_provider.cc
index 4737ef4..a284700 100644
--- a/chrome/browser/content_settings/content_settings_pref_provider.cc
+++ b/chrome/browser/content_settings/content_settings_pref_provider.cc
@@ -562,24 +562,36 @@ void PrefProvider::ReadExceptions(bool overwrite) {
   base::AutoLock auto_lock(lock());
 
   PrefService* prefs = profile_->GetPrefs();
-  DictionaryValue* all_settings_dictionary =
-      prefs->GetMutableDictionary(prefs::kContentSettingsPatterns);
+  const DictionaryValue* all_settings_dictionary =
+      prefs->GetDictionary(prefs::kContentSettingsPatterns);
 
   if (overwrite)
     host_content_settings()->clear();
 
   // Careful: The returned value could be NULL if the pref has never been set.
   if (all_settings_dictionary != NULL) {
+    DictionaryValue* mutable_settings;
+    scoped_ptr<DictionaryValue> mutable_settings_scope;
+
+    if (!is_off_the_record()) {
+      mutable_settings =
+          prefs->GetMutableDictionary(prefs::kContentSettingsPatterns);
+    } else {
+      // Create copy as we do not want to persist anything in OTR prefs.
+      mutable_settings = all_settings_dictionary->DeepCopy();
+      mutable_settings_scope.reset(mutable_settings);
+    }
+
     // Convert all Unicode patterns into punycode form, then read.
-    CanonicalizeContentSettingsExceptions(all_settings_dictionary);
+    CanonicalizeContentSettingsExceptions(mutable_settings);
 
-    for (DictionaryValue::key_iterator i(all_settings_dictionary->begin_keys());
-         i != all_settings_dictionary->end_keys(); ++i) {
+    for (DictionaryValue::key_iterator i(mutable_settings->begin_keys());
+         i != mutable_settings->end_keys(); ++i) {
       const std::string& pattern(*i);
       if (!ContentSettingsPattern(pattern).IsValid())
         LOG(WARNING) << "Invalid pattern stored in content settings";
       DictionaryValue* pattern_settings_dictionary = NULL;
-      bool found = all_settings_dictionary->GetDictionaryWithoutPathExpansion(
+      bool found = mutable_settings->GetDictionaryWithoutPathExpansion(
           pattern, &pattern_settings_dictionary);
       DCHECK(found);
 
diff --git a/chrome/browser/content_settings/content_settings_pref_provider_unittest.cc b/chrome/browser/content_settings/content_settings_pref_provider_unittest.cc
index 6d223da..bb37af0 100644
--- a/chrome/browser/content_settings/content_settings_pref_provider_unittest.cc
+++ b/chrome/browser/content_settings/content_settings_pref_provider_unittest.cc
@@ -7,7 +7,11 @@
 #include "base/auto_reset.h"
 #include "base/command_line.h"
 #include "chrome/browser/content_settings/stub_settings_observer.h"
+#include "chrome/browser/prefs/browser_prefs.h"
+#include "chrome/browser/prefs/default_pref_store.h"
+#include "chrome/browser/prefs/overlay_persistent_pref_store.h"
 #include "chrome/browser/prefs/pref_service.h"
+#include "chrome/browser/prefs/testing_pref_store.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/url_constants.h"
@@ -18,6 +22,25 @@
 #include "googleurl/src/gurl.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+namespace {
+class ContentSettingsPrefService : public PrefService {
+ public:
+  ContentSettingsPrefService(PrefStore* managed_platform_prefs,
+                             PrefStore* managed_cloud_prefs,
+                             PrefStore* extension_prefs,
+                             PrefStore* command_line_prefs,
+                             PersistentPrefStore* user_prefs,
+                             PrefStore* recommended_platform_prefs,
+                             PrefStore* recommended_cloud_prefs,
+                             DefaultPrefStore* default_store)
+      : PrefService(
+          managed_platform_prefs, managed_cloud_prefs, extension_prefs,
+          command_line_prefs, user_prefs, recommended_platform_prefs,
+          recommended_cloud_prefs, default_store) {}
+  virtual ~ContentSettingsPrefService() {}
+};
+}
+
 namespace content_settings {
 
 class PrefDefaultProviderTest : public TestingBrowserProcessTest {
@@ -168,6 +191,67 @@ TEST_F(PrefProviderTest, Observer) {
   EXPECT_EQ(1, observer.counter);
 }
 
+// Test for regression in which the PrefProvider modified the user pref store
+// of the OTR unintentionally: http://crbug.com/74466.
+TEST_F(PrefProviderTest, Incognito) {
+  DefaultPrefStore* default_prefs = new DefaultPrefStore();
+  PersistentPrefStore* user_prefs = new TestingPrefStore();
+  OverlayPersistentPrefStore* otr_user_prefs =
+      new OverlayPersistentPrefStore(user_prefs);
+
+  PrefService* regular_prefs = new ContentSettingsPrefService(
+      NULL,  // managed_platform_prefs
+      NULL,  // managed_cloud_prefs
+      NULL,  // extension_prefs
+      NULL,  // command_line_prefs
+      user_prefs,
+      NULL,  // recommended_platform_prefs,
+      NULL,  // recommended_cloud_prefs,
+      default_prefs);
+
+  Profile::RegisterUserPrefs(regular_prefs);
+  browser::RegisterUserPrefs(regular_prefs);
+
+  PrefService* otr_prefs = new ContentSettingsPrefService(
+        NULL,  // managed_platform_prefs
+        NULL,  // managed_cloud_prefs
+        NULL,  // extension_prefs
+        NULL,  // command_line_prefs
+        otr_user_prefs,
+        NULL,  // recommended_platform_prefs,
+        NULL,  // recommended_cloud_prefs,
+        default_prefs);
+
+  TestingProfile profile;
+  TestingProfile* otr_profile = new TestingProfile;
+  profile.SetOffTheRecordProfile(otr_profile);
+  profile.SetPrefService(regular_prefs);
+  otr_profile->set_off_the_record(true);
+  otr_profile->SetPrefService(otr_prefs);
+
+  PrefProvider pref_content_settings_provider(&profile);
+  PrefProvider pref_content_settings_provider_incognito(otr_profile);
+  ContentSettingsPattern pattern("[*.]example.com");
+  pref_content_settings_provider.SetContentSetting(
+      pattern,
+      pattern,
+      CONTENT_SETTINGS_TYPE_IMAGES,
+      "",
+      CONTENT_SETTING_ALLOW);
+
+  GURL host("http://example.com/");
+  // The value should of course be visible in the regular PrefProvider.
+  EXPECT_EQ(CONTENT_SETTING_ALLOW,
+            pref_content_settings_provider.GetContentSetting(
+                host, host, CONTENT_SETTINGS_TYPE_IMAGES, ""));
+  // And also in the OTR version.
+  EXPECT_EQ(CONTENT_SETTING_ALLOW,
+            pref_content_settings_provider_incognito.GetContentSetting(
+                host, host, CONTENT_SETTINGS_TYPE_IMAGES, ""));
+  // But the value should not be overridden in the OTR user prefs accidentally.
+  EXPECT_FALSE(otr_user_prefs->IsSetInOverlay(prefs::kContentSettingsPatterns));
+}
+
 TEST_F(PrefProviderTest, Patterns) {
   TestingProfile testing_profile;
   PrefProvider pref_content_settings_provider(