Bug 872981 - Print a warning whenever something attempts to store more than 4kb of preferences. r=bsmedberg
☠☠ backed out by 281b0297de65 ☠ ☠
authorDavid Rajchenbach-Teller <dteller@mozilla.com>
Mon, 03 Jun 2013 15:30:57 -0400
changeset 145342 1ca1abed8c9a12d1292a3fcbad1cd415eea69cb3
parent 145341 78c0e2d241ff3ad1f4f62076a80d54f6776584b5
child 145343 0a6f4bdb34bfb5c191d4488dca725b0792b7f67d
push id2697
push userbbajaj@mozilla.com
push dateMon, 05 Aug 2013 18:49:53 +0000
treeherdermozilla-beta@dfec938c7b63 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbsmedberg
bugs872981
milestone24.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 872981 - Print a warning whenever something attempts to store more than 4kb of preferences. r=bsmedberg
modules/libpref/src/nsPrefBranch.cpp
modules/libpref/src/nsPrefBranch.h
modules/libpref/test/unit/head_libPrefs.js
modules/libpref/test/unit/test_warnings.js
modules/libpref/test/unit/xpcshell.ini
--- a/modules/libpref/src/nsPrefBranch.cpp
+++ b/modules/libpref/src/nsPrefBranch.cpp
@@ -24,18 +24,22 @@
 #include "mozilla/Services.h"
 
 #include "prefapi_private_data.h"
 
 #ifdef MOZ_CRASHREPORTER
 #include "nsICrashReporter.h"
 #endif
 
+#include "nsIConsoleService.h"
+
 // 1 MB should be enough for everyone.
 static const uint32_t MAX_PREF_LENGTH = 1 * 1024 * 1024;
+// Actually, 4kb should be enough for everyone.
+static const uint32_t MAX_ADVISABLE_PREF_LENGTH = 4 * 1024;
 
 // Definitions
 struct EnumerateData {
   const char  *parent;
   nsTArray<nsCString> *pref_list;
 };
 
 // Prototypes
@@ -164,16 +168,26 @@ NS_IMETHODIMP nsPrefBranch::GetCharPref(
 {
   NS_ENSURE_ARG(aPrefName);
   const char *pref = getPrefName(aPrefName);
   return PREF_CopyCharPref(pref, _retval, mIsDefault);
 }
 
 NS_IMETHODIMP nsPrefBranch::SetCharPref(const char *aPrefName, const char *aValue)
 {
+  nsresult rv = CheckSanityOfStringLength(aPrefName, aValue);
+  if (NS_FAILED(rv)) {
+    return rv;
+  }
+  return SetCharPrefInternal(aPrefName, aValue);
+}
+
+nsresult nsPrefBranch::SetCharPrefInternal(const char *aPrefName, const char *aValue)
+
+{
   if (GetContentChild()) {
     NS_ERROR("cannot set pref from content process");
     return NS_ERROR_NOT_AVAILABLE;
   }
 
   NS_ENSURE_ARG(aPrefName);
   NS_ENSURE_ARG(aValue);
   const char *pref = getPrefName(aPrefName);
@@ -337,16 +351,47 @@ NS_IMETHODIMP nsPrefBranch::GetComplexVa
     }
     return rv;
   }
 
   NS_WARNING("nsPrefBranch::GetComplexValue - Unsupported interface type");
   return NS_NOINTERFACE;
 }
 
+nsresult nsPrefBranch::CheckSanityOfStringLength(const char* aPrefName, const char* aValue) {
+  return CheckSanityOfStringLength(aPrefName, strlen(aValue));
+}
+
+nsresult nsPrefBranch::CheckSanityOfStringLength(const char* aPrefName, const nsAString& aValue) {
+  return CheckSanityOfStringLength(aPrefName, aValue.Length());
+}
+
+nsresult nsPrefBranch::CheckSanityOfStringLength(const char* aPrefName, const uint32_t aLength) {
+  if (aLength > MAX_PREF_LENGTH) {
+    return NS_ERROR_OUT_OF_MEMORY;
+  }
+  if (aLength <= MAX_ADVISABLE_PREF_LENGTH) {
+    return NS_OK;
+  }
+  nsresult rv;
+  nsCOMPtr<nsIConsoleService> console = do_GetService("@mozilla.org/consoleservice;1", &rv);
+  if (NS_FAILED(rv)) {
+    return rv;
+  }
+  nsAutoCString message(nsPrintfCString("Warning: attempting to write %d bytes to preference %s. This is bad for general performance and memory usage. Such an amount of data should rather be written to an external file.",
+                                        aLength,
+                                        aPrefName));
+  rv = console->LogStringMessage(NS_ConvertUTF8toUTF16(message).get());
+  if (NS_FAILED(rv)) {
+    return rv;
+  }
+  return NS_OK;
+}
+
+
 NS_IMETHODIMP nsPrefBranch::SetComplexValue(const char *aPrefName, const nsIID & aType, nsISupports *aValue)
 {
   if (GetContentChild()) {
     NS_ERROR("cannot set pref from content process");
     return NS_ERROR_NOT_AVAILABLE;
   }
 
   NS_ENSURE_ARG(aPrefName);
@@ -357,26 +402,26 @@ NS_IMETHODIMP nsPrefBranch::SetComplexVa
   if (aType.Equals(NS_GET_IID(nsIFile)) || aType.Equals(NS_GET_IID(nsILocalFile))) {
     nsCOMPtr<nsIFile> file = do_QueryInterface(aValue);
     if (!file)
       return NS_NOINTERFACE;
     nsAutoCString descriptorString;
 
     rv = file->GetPersistentDescriptor(descriptorString);
     if (NS_SUCCEEDED(rv)) {
-      rv = SetCharPref(aPrefName, descriptorString.get());
+      rv = SetCharPrefInternal(aPrefName, descriptorString.get());
     }
     return rv;
   }
 
   if (aType.Equals(NS_GET_IID(nsIRelativeFilePref))) {
     nsCOMPtr<nsIRelativeFilePref> relFilePref = do_QueryInterface(aValue);
     if (!relFilePref)
       return NS_NOINTERFACE;
-    
+
     nsCOMPtr<nsIFile> file;
     relFilePref->GetFile(getter_AddRefs(file));
     if (!file)
       return NS_NOINTERFACE;
     nsAutoCString relativeToKey;
     (void) relFilePref->GetRelativeToKey(relativeToKey);
 
     nsCOMPtr<nsIFile> relativeToFile;
@@ -386,54 +431,58 @@ NS_IMETHODIMP nsPrefBranch::SetComplexVa
     rv = directoryService->Get(relativeToKey.get(), NS_GET_IID(nsIFile), getter_AddRefs(relativeToFile));
     if (NS_FAILED(rv))
       return rv;
 
     nsAutoCString relDescriptor;
     rv = file->GetRelativeDescriptor(relativeToFile, relDescriptor);
     if (NS_FAILED(rv))
       return rv;
-    
+
     nsAutoCString descriptorString;
     descriptorString.Append('[');
     descriptorString.Append(relativeToKey);
     descriptorString.Append(']');
     descriptorString.Append(relDescriptor);
-    return SetCharPref(aPrefName, descriptorString.get());
+    return SetCharPrefInternal(aPrefName, descriptorString.get());
   }
 
   if (aType.Equals(NS_GET_IID(nsISupportsString))) {
     nsCOMPtr<nsISupportsString> theString = do_QueryInterface(aValue);
 
     if (theString) {
       nsString wideString;
 
       rv = theString->GetData(wideString);
       if (NS_SUCCEEDED(rv)) {
-        if (wideString.Length() > MAX_PREF_LENGTH) {
-          return NS_ERROR_OUT_OF_MEMORY;
+        // Check sanity of string length before any lengthy conversion
+        rv = CheckSanityOfStringLength(aPrefName, wideString);
+        if (NS_FAILED(rv)) {
+          return rv;
         }
-        rv = SetCharPref(aPrefName, NS_ConvertUTF16toUTF8(wideString).get());
+        rv = SetCharPrefInternal(aPrefName, NS_ConvertUTF16toUTF8(wideString).get());
       }
     }
     return rv;
   }
 
   if (aType.Equals(NS_GET_IID(nsIPrefLocalizedString))) {
     nsCOMPtr<nsIPrefLocalizedString> theString = do_QueryInterface(aValue);
 
     if (theString) {
       nsXPIDLString wideString;
 
       rv = theString->GetData(getter_Copies(wideString));
       if (NS_SUCCEEDED(rv)) {
-        if (wideString.Length() > MAX_PREF_LENGTH) {
-          return NS_ERROR_OUT_OF_MEMORY;
+        // Check sanity of string length before any lengthy conversion
+        rv = CheckSanityOfStringLength(aPrefName, wideString);
+        if (NS_FAILED(rv)) {
+          return rv;
         }
-        rv = SetCharPref(aPrefName, NS_ConvertUTF16toUTF8(wideString).get());
+        rv = SetCharPrefInternal(aPrefName, NS_ConvertUTF16toUTF8(wideString).get());
       }
     }
     return rv;
   }
 
   NS_WARNING("nsPrefBranch::SetComplexValue - Unsupported interface type");
   return NS_NOINTERFACE;
 }
--- a/modules/libpref/src/nsPrefBranch.h
+++ b/modules/libpref/src/nsPrefBranch.h
@@ -189,16 +189,22 @@ public:
 
   size_t SizeOfIncludingThis(nsMallocSizeOfFun aMallocSizeOf);
 
 protected:
   nsPrefBranch()    /* disallow use of this constructer */
     { }
 
   nsresult   GetDefaultFromPropertiesFile(const char *aPrefName, PRUnichar **return_buf);
+  // As SetCharPref, but without any check on the length of |aValue|
+  nsresult   SetCharPrefInternal(const char *aPrefName, const char *aValue);
+  // Reject strings that are more than 1Mb, warn if strings are more than 16kb
+  nsresult   CheckSanityOfStringLength(const char* aPrefName, const nsAString& aValue);
+  nsresult   CheckSanityOfStringLength(const char* aPrefName, const char* aValue);
+  nsresult   CheckSanityOfStringLength(const char* aPrefName, const uint32_t aLength);
   void RemoveExpiredCallback(PrefCallback *aCallback);
   const char *getPrefName(const char *aPrefName);
   void       freeObserverList(void);
 
   friend PLDHashOperator
     FreeObserverFunc(PrefCallback *aKey,
                      nsAutoPtr<PrefCallback> &aCallback,
                      void *aArgs);
--- a/modules/libpref/test/unit/head_libPrefs.js
+++ b/modules/libpref/test/unit/head_libPrefs.js
@@ -2,16 +2,17 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 const NS_APP_USER_PROFILE_50_DIR = "ProfD";
 
 const Ci = Components.interfaces;
 const Cc = Components.classes;
 const Cr = Components.results;
+const Cu = Components.utils;
 
 function do_check_throws(f, result, stack)
 {
   if (!stack)
     stack = Components.stack.caller;
 
   try {
     f();
new file mode 100644
--- /dev/null
+++ b/modules/libpref/test/unit/test_warnings.js
@@ -0,0 +1,89 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+Cu.import("resource://gre/modules/commonjs/sdk/core/promise.js");
+
+let cs = Cc["@mozilla.org/consoleservice;1"].
+  getService(Ci.nsIConsoleService);
+let ps = Cc["@mozilla.org/preferences-service;1"].
+  getService(Ci.nsIPrefService);
+
+function makeBuffer(length) {
+  let string = "x";
+  while (string.length < length) {
+    string = string + string;
+  }
+  if (string.length > length) {
+    string = string.substring(length - string.length);
+  }
+  return string;
+}
+
+/**
+ * @resolves |true| if execution proceeded without warning,
+ * |false| if there was a warning.
+ */
+function checkWarning(pref, buffer) {
+  let deferred = Promise.defer();
+  let complete = false;
+  let listener = {
+    observe: function(event) {
+      let message = event.message;
+      if (!(message.startsWith("Warning: attempting to write")
+            && message.contains(pref))) {
+        return;
+      }
+      if (complete) {
+        return;
+      }
+      complete = true;
+      do_print("Warning while setting " + pref);
+      cs.unregisterListener(listener);
+      deferred.resolve(true);
+    }
+  };
+  do_timeout(1000, function() {
+    if (complete) {
+      return;
+    }
+    complete = true;
+    do_print("No warning while setting " + pref);
+    cs.unregisterListener(listener);
+    deferred.resolve(false);
+  });
+  cs.registerListener(listener);
+  ps.setCharPref(pref, buffer);
+  return deferred.promise;
+}
+
+function run_test() {
+  run_next_test();
+}
+
+add_task(function() {
+  // Very large change, should fail
+  try {
+    ps.setCharPref("string.fail", makeBuffer(16 * 1024 * 1024));
+    do_print("Writing to string.fail should have failed");
+    do_check_true(false); // We should have failed
+  } catch (ex if ex.result == Cr.NS_ERROR_OUT_OF_MEMORY) {
+    do_print("Writing to string.fail failed for the right reasons");
+    do_check_true(true); // Failed for the right reason
+  } catch (ex) {
+    do_print("Writing to string.fail failed for bad reasons");
+    do_check_true(false); // Failed for the wrong reason
+  }
+
+  // Simple change, shouldn't cause a warning
+  do_print("Checking that a simple change doesn't cause a warning");
+  let buf = makeBuffer(100);
+  let warned = yield checkWarning("string.accept", buf);
+  do_check_false(warned);
+
+  // Large change, should cause a warning
+  do_print("Checking that a large change causes a warning");
+  buf = makeBuffer(32 * 1024);
+  warned = yield checkWarning("string.warn", buf);
+  do_check_true(warned);
+});
--- a/modules/libpref/test/unit/xpcshell.ini
+++ b/modules/libpref/test/unit/xpcshell.ini
@@ -1,10 +1,11 @@
 [DEFAULT]
 head = head_libPrefs.js
 tail = 
 
+[test_warnings.js]
 [test_bug345529.js]
 [test_bug506224.js]
 [test_bug577950.js]
 [test_bug790374.js]
 [test_extprefs.js]
 [test_libPrefs.js]