Bug 1272707 - Part 2: Limit the size of preference values sent to child processes. r=bsmedberg, a=sledru
authorAndrew McCreight <continuation@gmail.com>
Tue, 17 May 2016 07:55:49 -0700
changeset 333056 29a31af637201be453147527100645746fbd8c96
parent 333055 5e792d50b7a180c908ec35fb8f8f6248de4719c5
child 333057 d9474bded24085f858b008c51ac199888bc72a21
push id6048
push userkmoir@mozilla.com
push dateMon, 06 Jun 2016 19:02:08 +0000
treeherdermozilla-beta@46d72a56c57d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbsmedberg, sledru
bugs1272707
milestone48.0a2
Bug 1272707 - Part 2: Limit the size of preference values sent to child processes. r=bsmedberg, a=sledru Don't send any preferences that have a string value that is longer than MAX_ADVISABLE_PREF_LENGTH. This is intended to mitigate OOM issues, as I've seen a parent process crash trying to create a 100mb message to send to the child. Such users likely cannot use e10s at all. This has a test for all combinations of setting the default and user values of a preference to large or small string values, or not setting them at all. I manually verified that filtering out preferences reduces the size of the IPC::Message that is sent to the child by printing out the size of the reply message in PContentParent::OnMessageReceived().
modules/libpref/Preferences.cpp
modules/libpref/nsPrefBranch.cpp
modules/libpref/prefapi.cpp
modules/libpref/prefapi_private_data.h
modules/libpref/test/unit_ipc/test_large_pref.js
modules/libpref/test/unit_ipc/xpcshell.ini
--- a/modules/libpref/Preferences.cpp
+++ b/modules/libpref/Preferences.cpp
@@ -706,25 +706,32 @@ Preferences::SetPreference(const PrefSet
 
 void
 Preferences::GetPreference(PrefSetting* aPref)
 {
   PrefHashEntry *entry = pref_HashTableLookup(aPref->name().get());
   if (!entry)
     return;
 
-  pref_GetPrefFromEntry(entry, aPref);
+  if (pref_EntryHasAdvisablySizedValues(entry)) {
+    pref_GetPrefFromEntry(entry, aPref);
+  }
 }
 
 void
 Preferences::GetPreferences(InfallibleTArray<PrefSetting>* aPrefs)
 {
   aPrefs->SetCapacity(gHashTable->Capacity());
   for (auto iter = gHashTable->Iter(); !iter.Done(); iter.Next()) {
     auto entry = static_cast<PrefHashEntry*>(iter.Get());
+
+    if (!pref_EntryHasAdvisablySizedValues(entry)) {
+      continue;
+    }
+
     dom::PrefSetting *pref = aPrefs->AppendElement();
     pref_GetPrefFromEntry(entry, pref);
   }
 }
 
 NS_IMETHODIMP
 Preferences::GetBranch(const char *aPrefRoot, nsIPrefBranch **_retval)
 {
--- a/modules/libpref/nsPrefBranch.cpp
+++ b/modules/libpref/nsPrefBranch.cpp
@@ -375,17 +375,20 @@ nsresult nsPrefBranch::CheckSanityOfStri
   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.",
+  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. This preference will "
+                                        "not be sent to any content processes.",
                                         aLength,
                                         getPrefName(aPrefName)));
   rv = console->LogStringMessage(NS_ConvertUTF8toUTF16(message).get());
   if (NS_FAILED(rv)) {
     return rv;
   }
   return NS_OK;
 }
--- a/modules/libpref/prefapi.cpp
+++ b/modules/libpref/prefapi.cpp
@@ -361,16 +361,41 @@ pref_savePrefs(PLDHashTable* aTable)
                                        NS_LITERAL_CSTRING("\", ") +
                                        prefValue +
                                        NS_LITERAL_CSTRING(");"));
     }
 
     return savedPrefs;
 }
 
+bool
+pref_EntryHasAdvisablySizedValues(PrefHashEntry* aHashEntry)
+{
+    if (aHashEntry->prefFlags.GetPrefType() != PrefType::String) {
+        return true;
+    }
+
+    char* stringVal;
+    if (aHashEntry->prefFlags.HasDefault()) {
+        stringVal = aHashEntry->defaultPref.stringVal;
+        if (strlen(stringVal) > MAX_ADVISABLE_PREF_LENGTH) {
+            return false;
+        }
+    }
+
+    if (aHashEntry->prefFlags.HasUserValue()) {
+        stringVal = aHashEntry->userPref.stringVal;
+        if (strlen(stringVal) > MAX_ADVISABLE_PREF_LENGTH) {
+            return false;
+        }
+    }
+
+    return true;
+}
+
 static void
 GetPrefValueFromEntry(PrefHashEntry *aHashEntry, dom::PrefSetting* aPref,
                       WhichValue aWhich)
 {
     PrefValue* value;
     dom::PrefValue* settingValue;
     if (aWhich == USER_VALUE) {
         value = &aHashEntry->userPref;
--- a/modules/libpref/prefapi_private_data.h
+++ b/modules/libpref/prefapi_private_data.h
@@ -24,15 +24,18 @@ mozilla::UniquePtr<char*[]>
 pref_savePrefs(PLDHashTable* aTable);
 
 nsresult
 pref_SetPref(const mozilla::dom::PrefSetting& aPref);
 
 int pref_CompareStrings(const void *v1, const void *v2, void* unused);
 PrefHashEntry* pref_HashTableLookup(const char *key);
 
+bool
+pref_EntryHasAdvisablySizedValues(PrefHashEntry* aHashEntry);
+
 void pref_GetPrefFromEntry(PrefHashEntry *aHashEntry,
                            mozilla::dom::PrefSetting* aPref);
 
 size_t
 pref_SizeOfPrivateData(mozilla::MallocSizeOf aMallocSizeOf);
 
 #endif
new file mode 100644
--- /dev/null
+++ b/modules/libpref/test/unit_ipc/test_large_pref.js
@@ -0,0 +1,98 @@
+/* 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/. */
+
+// Large preferences should not be set in the child process.
+// Non-string preferences are not tested here, because their behavior
+// should not be affected by this filtering.
+
+var Ci = Components.interfaces;
+var Cc = Components.classes;
+
+function isParentProcess() {
+    let appInfo = Cc["@mozilla.org/xre/app-info;1"];
+    return (!appInfo || appInfo.getService(Ci.nsIXULRuntime).processType == Ci.nsIXULRuntime.PROCESS_TYPE_DEFAULT);
+}
+
+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;
+}
+
+// from prefapi.h
+const MAX_ADVISABLE_PREF_LENGTH = 4 * 1024;
+
+const largeString = makeBuffer(MAX_ADVISABLE_PREF_LENGTH + 1);
+const smallString = makeBuffer(4);
+
+const testValues = [
+  {name: "None", value: undefined},
+  {name: "Small", value: smallString},
+  {name: "Large", value: largeString},
+];
+
+function prefName(def, user) {
+  return "Test.IPC.default" + def.name + "User" + user.name;
+}
+
+function expectedPrefValue(def, user) {
+  if (user.value) {
+    return user.value;
+  }
+  return def.value;
+}
+
+function run_test() {
+  let pb = Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefBranch);
+  let ps = Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefService);
+  let defaultBranch = ps.getDefaultBranch("");
+
+  let isParent = isParentProcess();
+  if (isParent) {
+    // Set all combinations of none, small and large, for default and user prefs.
+    for (let def of testValues) {
+      for (let user of testValues) {
+        let currPref = prefName(def, user);
+        if (def.value) {
+          defaultBranch.setCharPref(currPref, def.value);
+        }
+        if (user.value) {
+          pb.setCharPref(currPref, user.value);
+        }
+      }
+    }
+
+    run_test_in_child("test_large_pref.js");
+  }
+
+  // Check that each preference is set or not set, as appropriate.
+  for (let def of testValues) {
+    for (let user of testValues) {
+      if (!def.value && !user.value) {
+        continue;
+      }
+      let pref_name = prefName(def, user);
+      if (isParent || (def.name != "Large" && user.name != "Large")) {
+        do_check_eq(pb.getCharPref(pref_name), expectedPrefValue(def, user));
+      } else {
+        // This is the child, and either the default or user value is
+        // large, so the preference should not be set.
+        let prefExists;
+        try {
+          pb.getCharPref(pref_name);
+          prefExists = true;
+        } catch(e) {
+          prefExists = false;
+        }
+        ok(!prefExists,
+           "Pref " + pref_name + " should not be set in the child");
+      }
+    }
+  }
+}
--- a/modules/libpref/test/unit_ipc/xpcshell.ini
+++ b/modules/libpref/test/unit_ipc/xpcshell.ini
@@ -1,10 +1,11 @@
 [DEFAULT]
 head = 
 tail = 
 skip-if = toolkit == 'android' || toolkit == 'gonk'
 
 [test_existing_prefs.js]
 [test_initial_prefs.js]
+[test_large_pref.js]
 [test_observed_prefs.js]
 [test_update_prefs.js]
 [test_user_default_prefs.js]