detect preferences errors

BUG=38352
TEST=none

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

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

18 files changed, 153 insertions, 41 deletions

diff --git a/chrome/browser/autocomplete/search_provider.cc b/chrome/browser/autocomplete/search_provider.cc
index ba33df0..9a01fef 100644
--- a/chrome/browser/autocomplete/search_provider.cc
+++ b/chrome/browser/autocomplete/search_provider.cc
@@ -183,7 +183,7 @@ void SearchProvider::OnURLFetchComplete(const URLFetcher* source,
   if (status.is_success() && response_code == 200) {
     JSONStringValueSerializer deserializer(json_data);
     deserializer.set_allow_trailing_comma(true);
-    scoped_ptr<Value> root_val(deserializer.Deserialize(NULL));
+    scoped_ptr<Value> root_val(deserializer.Deserialize(NULL, NULL));
     const std::wstring& input_text =
         is_keyword_results ? keyword_input_text_ : input_.text();
     have_suggest_results_ =
diff --git a/chrome/browser/automation/automation_provider.cc b/chrome/browser/automation/automation_provider.cc
index cc4ea58..67e73f0 100644
--- a/chrome/browser/automation/automation_provider.cc
+++ b/chrome/browser/automation/automation_provider.cc
@@ -1573,7 +1573,7 @@ void AutomationProvider::SendJSONRequest(
   } else {
     base::JSONReader reader;
     std::string error;
-    values.reset(reader.ReadAndReturnError(json_request, true, &error));
+    values.reset(reader.ReadAndReturnError(json_request, true, NULL, &error));
     if (!error.empty()) {
       error_string = error;
     }
@@ -1656,11 +1656,11 @@ class SetProxyConfigTask : public Task {
   virtual void Run() {
     // First, deserialize the JSON string. If this fails, log and bail.
     JSONStringValueSerializer deserializer(proxy_config_);
-    std::string error_message;
-    scoped_ptr<Value> root(deserializer.Deserialize(&error_message));
+    std::string error_msg;
+    scoped_ptr<Value> root(deserializer.Deserialize(NULL, &error_msg));
     if (!root.get() || root->GetType() != Value::TYPE_DICTIONARY) {
       DLOG(WARNING) << "Received bad JSON string for ProxyConfig: "
-                    << error_message;
+                    << error_msg;
       return;
     }
 
diff --git a/chrome/browser/bookmarks/bookmark_storage.cc b/chrome/browser/bookmarks/bookmark_storage.cc
index 0b7d895..fd145b0 100644
--- a/chrome/browser/bookmarks/bookmark_storage.cc
+++ b/chrome/browser/bookmarks/bookmark_storage.cc
@@ -74,7 +74,7 @@ class BookmarkStorage::LoadTask : public Task {
     bool bookmark_file_exists = file_util::PathExists(path_);
     if (bookmark_file_exists) {
       JSONFileValueSerializer serializer(path_);
-      scoped_ptr<Value> root(serializer.Deserialize(NULL));
+      scoped_ptr<Value> root(serializer.Deserialize(NULL, NULL));
 
       if (root.get()) {
         // Building the index can take a while, so we do it on the background
diff --git a/chrome/browser/browser_theme_pack_unittest.cc b/chrome/browser/browser_theme_pack_unittest.cc
index 1a05582..0b7912f 100644
--- a/chrome/browser/browser_theme_pack_unittest.cc
+++ b/chrome/browser/browser_theme_pack_unittest.cc
@@ -406,7 +406,7 @@ TEST_F(BrowserThemePackTest, CanBuildAndReadPack) {
     std::string error;
     JSONFileValueSerializer serializer(manifest_path);
     scoped_ptr<DictionaryValue> valid_value(
-        static_cast<DictionaryValue*>(serializer.Deserialize(&error)));
+        static_cast<DictionaryValue*>(serializer.Deserialize(NULL, &error)));
     EXPECT_EQ("", error);
     ASSERT_TRUE(valid_value.get());
     ASSERT_TRUE(extension.InitFromValue(*valid_value, true, &error));
diff --git a/chrome/browser/cocoa/extensions/extension_install_prompt_controller_unittest.mm b/chrome/browser/cocoa/extensions/extension_install_prompt_controller_unittest.mm
index f1a02d9..aa1e586 100644
--- a/chrome/browser/cocoa/extensions/extension_install_prompt_controller_unittest.mm
+++ b/chrome/browser/cocoa/extensions/extension_install_prompt_controller_unittest.mm
@@ -53,7 +53,7 @@ public:
     std::string error;
     JSONFileValueSerializer serializer(path);
     scoped_ptr<DictionaryValue> value(static_cast<DictionaryValue*>(
-        serializer.Deserialize(&error)));
+        serializer.Deserialize(NULL, &error)));
     if (!value.get()) {
       LOG(ERROR) << error;
       return;
diff --git a/chrome/browser/extensions/extension_prefs.h b/chrome/browser/extensions/extension_prefs.h
index 43d270e..f9a5aa6 100644
--- a/chrome/browser/extensions/extension_prefs.h
+++ b/chrome/browser/extensions/extension_prefs.h
@@ -113,6 +113,9 @@ class ExtensionPrefs {
 
   static void RegisterUserPrefs(PrefService* prefs);
 
+  // The underlying PrefService.
+  PrefService* pref_service() const { return prefs_; }
+
  private:
 
   // Converts absolute paths in the pref to paths relative to the
diff --git a/chrome/browser/extensions/extension_ui_unittest.cc b/chrome/browser/extensions/extension_ui_unittest.cc
index f1bd648..96af1c8 100644
--- a/chrome/browser/extensions/extension_ui_unittest.cc
+++ b/chrome/browser/extensions/extension_ui_unittest.cc
@@ -16,7 +16,7 @@ namespace {
     Value* value;
 
     JSONFileValueSerializer serializer(path);
-    value = serializer.Deserialize(error);
+    value = serializer.Deserialize(NULL, error);
 
     return static_cast<DictionaryValue*>(value);
   }
diff --git a/chrome/browser/extensions/extensions_service.cc b/chrome/browser/extensions/extensions_service.cc
index de78366..756c750 100644
--- a/chrome/browser/extensions/extensions_service.cc
+++ b/chrome/browser/extensions/extensions_service.cc
@@ -421,7 +421,7 @@ void ExtensionsService::LoadComponentExtensions() {
            component_extension_manifests_.begin();
        it != component_extension_manifests_.end(); ++it) {
     JSONStringValueSerializer serializer(it->manifest);
-    scoped_ptr<Value> manifest(serializer.Deserialize(NULL));
+    scoped_ptr<Value> manifest(serializer.Deserialize(NULL, NULL));
     if (!manifest.get()) {
       NOTREACHED() << "Failed to retrieve manifest for extension";
       continue;
@@ -757,6 +757,9 @@ void ExtensionsService::ReloadExtensions() {
 }
 
 void ExtensionsService::GarbageCollectExtensions() {
+  if (extension_prefs_->pref_service()->read_only())
+    return;
+
   InstalledExtensionSet installed(extension_prefs_.get());
   ChromeThread::PostTask(
       ChromeThread::FILE, FROM_HERE,
diff --git a/chrome/browser/extensions/extensions_service_unittest.cc b/chrome/browser/extensions/extensions_service_unittest.cc
index 38932c9..9888e8f 100644
--- a/chrome/browser/extensions/extensions_service_unittest.cc
+++ b/chrome/browser/extensions/extensions_service_unittest.cc
@@ -150,11 +150,9 @@ class MockProviderVisitor : public ExternalExtensionProvider::Visitor {
     // We also parse the file into a dictionary to compare what we get back
     // from the provider.
     JSONStringValueSerializer serializer(json_data);
-    std::string error_msg;
-    Value* json_value = serializer.Deserialize(&error_msg);
+    Value* json_value = serializer.Deserialize(NULL, NULL);
 
-    if (!error_msg.empty() || !json_value ||
-        !json_value->IsType(Value::TYPE_DICTIONARY)) {
+    if (!json_value || !json_value->IsType(Value::TYPE_DICTIONARY)) {
       NOTREACHED() << L"Unable to deserialize json data";
       return -1;
     } else {
diff --git a/chrome/browser/extensions/external_pref_extension_provider.cc b/chrome/browser/extensions/external_pref_extension_provider.cc
index 841f156..c51f4f2 100644
--- a/chrome/browser/extensions/external_pref_extension_provider.cc
+++ b/chrome/browser/extensions/external_pref_extension_provider.cc
@@ -98,19 +98,16 @@ Version* ExternalPrefExtensionProvider::RegisteredVersion(
 void ExternalPrefExtensionProvider::SetPreferences(
     ValueSerializer* serializer) {
   std::string error_msg;
-  Value* extensions = serializer->Deserialize(&error_msg);
+  Value* extensions = serializer->Deserialize(NULL, &error_msg);
   scoped_ptr<DictionaryValue> dictionary(new DictionaryValue());
-  if (!error_msg.empty()) {
+  if (!extensions) {
     LOG(WARNING) << L"Unable to deserialize json data: "
-                 << error_msg.c_str();
+                 << error_msg;
   } else {
-    // This can be null if the json file specified does not exist.
-    if (extensions) {
-      if (!extensions->IsType(Value::TYPE_DICTIONARY)) {
-        NOTREACHED() << L"Invalid json data";
-      } else {
-        dictionary.reset(static_cast<DictionaryValue*>(extensions));
-      }
+    if (!extensions->IsType(Value::TYPE_DICTIONARY)) {
+      NOTREACHED() << L"Invalid json data";
+    } else {
+      dictionary.reset(static_cast<DictionaryValue*>(extensions));
     }
   }
   prefs_.reset(dictionary.release());
diff --git a/chrome/browser/extensions/image_loading_tracker_unittest.cc b/chrome/browser/extensions/image_loading_tracker_unittest.cc
index 65b47e8..83b4347 100644
--- a/chrome/browser/extensions/image_loading_tracker_unittest.cc
+++ b/chrome/browser/extensions/image_loading_tracker_unittest.cc
@@ -56,12 +56,14 @@ class ImageLoadingTrackerTest : public testing::Test,
     }
     test_file = test_file.AppendASCII("extensions")
                          .AppendASCII("image_loading_tracker");
+    int error_code = 0;
     std::string error;
     JSONFileValueSerializer serializer(test_file.AppendASCII("app.json"));
     scoped_ptr<DictionaryValue> valid_value(
-        static_cast<DictionaryValue*>(serializer.Deserialize(&error)));
-    EXPECT_EQ("", error);
-    if (error != "")
+        static_cast<DictionaryValue*>(serializer.Deserialize(&error_code,
+                                                             &error)));
+    EXPECT_EQ(0, error_code) << error;
+    if (error_code != 0)
       return NULL;
 
     EXPECT_TRUE(valid_value.get());
diff --git a/chrome/browser/geolocation/network_location_request.cc b/chrome/browser/geolocation/network_location_request.cc
index 01bfd93..5e38eff 100644
--- a/chrome/browser/geolocation/network_location_request.cc
+++ b/chrome/browser/geolocation/network_location_request.cc
@@ -292,7 +292,7 @@ bool ParseServerResponse(const std::string& response_body,
   // Parse the response, ignoring comments.
   std::string error_msg;
   scoped_ptr<Value> response_value(base::JSONReader::ReadAndReturnError(
-      response_body, false, &error_msg));
+      response_body, false, NULL, &error_msg));
   if (response_value == NULL) {
     LOG(WARNING) << "ParseServerResponse() : JSONReader failed : "
                  << error_msg << ".\n";
diff --git a/chrome/browser/page_state.cc b/chrome/browser/page_state.cc
index 26b6daf..bf431ca 100644
--- a/chrome/browser/page_state.cc
+++ b/chrome/browser/page_state.cc
@@ -43,7 +43,7 @@ void PageState::InitWithBytes(const std::string& bytes) {
   state_.reset(new DictionaryValue);
 
   JSONStringValueSerializer serializer(bytes);
-  scoped_ptr<Value> root(serializer.Deserialize(NULL));
+  scoped_ptr<Value> root(serializer.Deserialize(NULL, NULL));
 
   if (!root.get()) {
     NOTREACHED();
diff --git a/chrome/browser/pref_service.cc b/chrome/browser/pref_service.cc
index d30b544..65f1afc 100644
--- a/chrome/browser/pref_service.cc
+++ b/chrome/browser/pref_service.cc
@@ -8,6 +8,7 @@
 #include <string>
 
 #include "app/l10n_util.h"
+#include "base/histogram.h"
 #include "base/logging.h"
 #include "base/message_loop.h"
 #include "base/stl_util-inl.h"
@@ -15,8 +16,10 @@
 #include "base/sys_string_conversions.h"
 #include "base/utf_string_conversions.h"
 #include "build/build_config.h"
+#include "chrome/browser/chrome_thread.h"
 #include "chrome/common/json_value_serializer.h"
 #include "chrome/common/notification_service.h"
+#include "grit/chromium_strings.h"
 #include "grit/generated_resources.h"
 
 namespace {
@@ -62,12 +65,21 @@ Value* CreateLocaleDefaultValue(Value::ValueType type, int message_id) {
   return Value::CreateNullValue();
 }
 
+// Forwards a notification after a PostMessage so that we can wait for the
+// MessageLoop to run.
+void NotifyReadError(PrefService* pref, int message_id) {
+  Source<PrefService> source(pref);
+  NotificationService::current()->Notify(NotificationType::PROFILE_ERROR,
+                                         source, Details<int>(&message_id));
+}
+
 }  // namespace
 
 PrefService::PrefService(const FilePath& pref_filename)
     : persistent_(new DictionaryValue),
-      writer_(pref_filename) {
-  ReloadPersistentPrefs();
+      writer_(pref_filename),
+      read_only_(false) {
+  InitFromDisk();
 }
 
 PrefService::~PrefService() {
@@ -88,21 +100,82 @@ PrefService::~PrefService() {
                                        pref_observers_.end());
   pref_observers_.clear();
 
-  if (writer_.HasPendingWrite())
+  if (writer_.HasPendingWrite() && !read_only_)
     writer_.DoScheduledWrite();
 }
 
+void PrefService::InitFromDisk() {
+  PrefReadError error = LoadPersistentPrefs();
+  if (error == PREF_READ_ERROR_NONE)
+    return;
+
+  // Failing to load prefs on startup is a bad thing(TM). See bug 38352 for
+  // an example problem that this can cause.
+  // Do some diagnosis and try to avoid losing data.
+  int message_id = 0;
+  if (error <= PREF_READ_ERROR_JSON_TYPE) {
+    // JSON errors indicate file corruption of some sort.
+    // It's possible the user hand-edited the file, so don't clobber it yet.
+    // Give them a chance to recover the file.
+    // TODO(erikkay) maybe we should just move it aside and continue.
+    read_only_ = true;
+    message_id = IDS_PREFERENCES_CORRUPT_ERROR;
+  } if (error == PREF_READ_ERROR_NO_FILE) {
+    // If the file just doesn't exist, maybe this is first run.  In any case
+    // there's no harm in writing out default prefs in this case.
+  } else {
+    // If the file exists but is simply unreadable, put the file into a state
+    // where we don't try to save changes.  Otherwise, we could clobber the
+    // existing prefs.
+    read_only_ = true;
+    message_id = IDS_PREFERENCES_UNREADABLE_ERROR;
+  }
+
+  if (message_id) {
+    ChromeThread::PostTask(ChromeThread::UI, FROM_HERE,
+        NewRunnableFunction(&NotifyReadError, this, message_id));
+  }
+  UMA_HISTOGRAM_ENUMERATION("PrefService.ReadError", error, 20);
+}
+
 bool PrefService::ReloadPersistentPrefs() {
-  DCHECK(CalledOnValidThread());
+  return (LoadPersistentPrefs() == PREF_READ_ERROR_NONE);
+}
 
+PrefService::PrefReadError PrefService::LoadPersistentPrefs() {
+  DCHECK(CalledOnValidThread());
   JSONFileValueSerializer serializer(writer_.path());
-  scoped_ptr<Value> root(serializer.Deserialize(NULL));
-  if (!root.get())
-    return false;
+
+  int error_code = 0;
+  std::string error_msg;
+  scoped_ptr<Value> root(serializer.Deserialize(&error_code, &error_msg));
+  if (!root.get()) {
+    PLOG(ERROR) << "Error reading Preferences: " << error_msg << " " <<
+        writer_.path().value();
+    PrefReadError pref_error;
+    switch (error_code) {
+      case JSONFileValueSerializer::JSON_ACCESS_DENIED:
+        pref_error = PREF_READ_ERROR_ACCESS_DENIED;
+        break;
+      case JSONFileValueSerializer::JSON_CANNOT_READ_FILE:
+        pref_error = PREF_READ_ERROR_FILE_OTHER;
+        break;
+      case JSONFileValueSerializer::JSON_FILE_LOCKED:
+        pref_error = PREF_READ_ERROR_FILE_LOCKED;
+        break;
+      case JSONFileValueSerializer::JSON_NO_SUCH_FILE:
+        pref_error = PREF_READ_ERROR_NO_FILE;
+        break;
+      default:
+        pref_error = PREF_READ_ERROR_JSON_PARSE;
+        break;
+    }
+    return pref_error;
+  }
 
   // Preferences should always have a dictionary root.
   if (!root->IsType(Value::TYPE_DICTIONARY))
-    return false;
+    return PREF_READ_ERROR_JSON_TYPE;
 
   persistent_.reset(static_cast<DictionaryValue*>(root.release()));
   for (PreferenceSet::iterator it = prefs_.begin();
@@ -110,7 +183,7 @@ bool PrefService::ReloadPersistentPrefs() {
     (*it)->root_pref_ = persistent_.get();
   }
 
-  return true;
+  return PREF_READ_ERROR_NONE;
 }
 
 bool PrefService::SavePersistentPrefs() {
@@ -120,12 +193,20 @@ bool PrefService::SavePersistentPrefs() {
   if (!SerializeData(&data))
     return false;
 
+  // Lie about our ability to save.
+  if (read_only_)
+    return true;
+
   writer_.WriteNow(data);
   return true;
 }
 
 void PrefService::ScheduleSavePersistentPrefs() {
   DCHECK(CalledOnValidThread());
+
+  if (read_only_)
+    return;
+
   writer_.ScheduleWrite(this);
 }
 
diff --git a/chrome/browser/pref_service.h b/chrome/browser/pref_service.h
index f1819ad..75d829e 100644
--- a/chrome/browser/pref_service.h
+++ b/chrome/browser/pref_service.h
@@ -186,7 +186,23 @@ class PrefService : public NonThreadSafe,
   // ImportantFileWriter::DataSerializer
   virtual bool SerializeData(std::string* output);
 
+  bool read_only() const { return read_only_; }
+
  private:
+  // Unique integer code for each type of error so we can report them
+  // distinctly in a histogram.
+  // NOTE: Don't change the order here as it will change the server's meaning
+  // of the histogram.
+  enum PrefReadError {
+    PREF_READ_ERROR_NONE = 0,
+    PREF_READ_ERROR_JSON_PARSE,
+    PREF_READ_ERROR_JSON_TYPE,
+    PREF_READ_ERROR_ACCESS_DENIED,
+    PREF_READ_ERROR_FILE_OTHER,
+    PREF_READ_ERROR_FILE_LOCKED,
+    PREF_READ_ERROR_NO_FILE,
+  };
+
   // Add a preference to the PreferenceMap.  If the pref already exists, return
   // false.  This method takes ownership of |pref|.
   void RegisterPreference(Preference* pref);
@@ -203,6 +219,13 @@ class PrefService : public NonThreadSafe,
   void FireObserversIfChanged(const wchar_t* pref_name,
                               const Value* old_value);
 
+  // Load from disk.  Returns a non-zero error code on failure.
+  PrefReadError LoadPersistentPrefs();
+
+  // Load preferences from disk, attempting to diagnose and handle errors.
+  // This should only be called from the constructor.
+  void InitFromDisk();
+
   scoped_ptr<DictionaryValue> persistent_;
 
   // Helper for safe writing pref data.
@@ -220,6 +243,11 @@ class PrefService : public NonThreadSafe,
 
   friend class ScopedPrefUpdate;
 
+  // Whether the service is in a pseudo-read-only mode where changes are not
+  // actually persisted to disk.  This happens in some cases when there are
+  // read errors during startup.
+  bool read_only_;
+
   DISALLOW_COPY_AND_ASSIGN(PrefService);
 };
 
diff --git a/chrome/browser/pref_service_uitest.cc b/chrome/browser/pref_service_uitest.cc
index 79f00d3..87dcb2c 100644
--- a/chrome/browser/pref_service_uitest.cc
+++ b/chrome/browser/pref_service_uitest.cc
@@ -86,7 +86,7 @@ TEST_F(PreferenceServiceTest, PreservedWindowPlacementIsLoaded) {
   ASSERT_TRUE(file_util::PathExists(tmp_pref_file_));
 
   JSONFileValueSerializer deserializer(tmp_pref_file_);
-  scoped_ptr<Value> root(deserializer.Deserialize(NULL));
+  scoped_ptr<Value> root(deserializer.Deserialize(NULL, NULL));
 
   ASSERT_TRUE(root.get());
   ASSERT_TRUE(root->IsType(Value::TYPE_DICTIONARY));
diff --git a/chrome/browser/sync/glue/preference_change_processor.cc b/chrome/browser/sync/glue/preference_change_processor.cc
index 6203c55..6f5ac4d 100644
--- a/chrome/browser/sync/glue/preference_change_processor.cc
+++ b/chrome/browser/sync/glue/preference_change_processor.cc
@@ -161,7 +161,7 @@ Value* PreferenceChangeProcessor::ReadPreference(
   scoped_ptr<Value> value(reader.JsonToValue(preference.value(), false, false));
   if (!value.get()) {
     LOG(ERROR) << "Failed to deserialize preference value: "
-               << reader.error_message();
+               << reader.GetErrorMessage();
     error_handler()->OnUnrecoverableError();
     return NULL;
   }
diff --git a/chrome/browser/sync/glue/preference_model_associator.cc b/chrome/browser/sync/glue/preference_model_associator.cc
index 169da24..5021b4b 100644
--- a/chrome/browser/sync/glue/preference_model_associator.cc
+++ b/chrome/browser/sync/glue/preference_model_associator.cc
@@ -84,7 +84,7 @@ bool PreferenceModelAssociator::AssociateModels() {
       std::wstring pref_name = UTF8ToWide(preference.name());
       if (!value.get()) {
         LOG(ERROR) << "Failed to deserialize preference value: "
-                   << reader.error_message();
+                   << reader.GetErrorMessage();
         error_handler_->OnUnrecoverableError();
         return false;
       }