commit 4644daa4db79efa613d714a674634d2a31e07e65
parent 566ab8382ecdfa664704eabb8afbde00ea8e6166
Author: Greg Stoll <gstoll@mozilla.com>
Date: Fri, 14 Nov 2025 17:03:22 +0000
Bug 849620 - handle deleteBranch() calls with trailing dots and double dots consistently r=beth
Differential Revision: https://phabricator.services.mozilla.com/D270322
Diffstat:
2 files changed, 164 insertions(+), 32 deletions(-)
diff --git a/modules/libpref/Preferences.cpp b/modules/libpref/Preferences.cpp
@@ -2860,10 +2860,10 @@ nsPrefBranch::DeleteBranch(const char* aStartingAt) {
// Collect the list of prefs to remove
AutoTArray<const char*, 32> prefNames;
for (auto& pref : PrefsIter(HashTable(), gSharedMap)) {
- // The first disjunct matches branches: e.g. a branch name "foo.bar."
- // matches a name "foo.bar.baz" (but it won't match "foo.barrel.baz").
- // The second disjunct matches leaf nodes: e.g. a branch name "foo.bar."
- // matches a name "foo.bar" (by ignoring the trailing '.').
+ // Match preferences that start with branchName or equal branchNameNoDot.
+ // For inputs ending with "..", this matches the node without the trailing
+ // dot and all children with the double dot prefix.
+ // For other inputs, this matches both the node itself and all children.
if (StringBeginsWith(pref->NameString(), branchName) ||
pref->NameString() == branchNameNoDot) {
prefNames.AppendElement(pref->Name());
diff --git a/modules/libpref/test/unit/test_libPrefs.js b/modules/libpref/test/unit/test_libPrefs.js
@@ -570,32 +570,33 @@ add_task(function test_deleteBranch_observers() {
);
// Verify the preferences were actually deleted
- Assert.throws(
- () => ps.getBoolPref("DeleteTest.branch1.bool"),
- /NS_ERROR_UNEXPECTED/,
- "Deleted boolean pref should throw when accessed"
+ assertPrefNotExists(
+ "DeleteTest.branch1.bool",
+ "Deleted boolean pref should throw when accessed",
+ "getBoolPref"
);
- Assert.throws(
- () => ps.getIntPref("DeleteTest.branch1.int"),
- /NS_ERROR_UNEXPECTED/,
+ assertPrefNotExists(
+ "DeleteTest.branch1.int",
"Deleted integer pref should throw when accessed"
);
- Assert.throws(
- () => ps.getCharPref("DeleteTest.branch1.char"),
- /NS_ERROR_UNEXPECTED/,
- "Deleted char pref should throw when accessed"
+ assertPrefNotExists(
+ "DeleteTest.branch1.char",
+ "Deleted char pref should throw when accessed",
+ "getCharPref"
);
// Verify other preferences were not affected
- Assert.equal(
- ps.getBoolPref("DeleteTest.branch2.bool"),
+ assertPrefExists(
+ "DeleteTest.branch2.bool",
false,
- "Unrelated preferences should not be affected"
+ "Unrelated preferences should not be affected",
+ "getBoolPref"
);
- Assert.equal(
- ps.getCharPref("DeleteTest.other"),
+ assertPrefExists(
+ "DeleteTest.other",
"other",
- "Unrelated preferences should not be affected"
+ "Unrelated preferences should not be affected",
+ "getCharPref"
);
// Clean up observers
@@ -684,20 +685,19 @@ add_task(function test_deleteBranch_user_and_default_values() {
);
// Verify all preferences are actually deleted
- Assert.throws(
- () => ps.getBoolPref("MixedTest.pref1"),
- /NS_ERROR_UNEXPECTED/,
- "Pref with default value should be completely deleted"
+ assertPrefNotExists(
+ "MixedTest.pref1",
+ "Pref with default value should be completely deleted",
+ "getBoolPref"
);
- Assert.throws(
- () => ps.getIntPref("MixedTest.pref2"),
- /NS_ERROR_UNEXPECTED/,
+ assertPrefNotExists(
+ "MixedTest.pref2",
"Pref with default value should be completely deleted"
);
- Assert.throws(
- () => ps.getCharPref("MixedTest.pref3"),
- /NS_ERROR_UNEXPECTED/,
- "User-only pref should be deleted"
+ assertPrefNotExists(
+ "MixedTest.pref3",
+ "User-only pref should be deleted",
+ "getCharPref"
);
ps.removeObserver("MixedTest.", observer);
@@ -745,6 +745,138 @@ add_task(function test_deleteBranch_weak_observers() {
});
/**
+ * Helper function to assert that a preference exists with the expected value.
+ * @param {string} prefName - The preference name
+ * @param {*} expectedValue - The expected value
+ * @param {string} message - The assertion message
+ * @param {Function} [getter=getIntPref] - The preference getter function (e.g., getIntPref, getBoolPref, getCharPref)
+ */
+function assertPrefExists(
+ prefName,
+ expectedValue,
+ message,
+ getter = "getIntPref"
+) {
+ Assert.equal(Services.prefs[getter](prefName), expectedValue, message);
+}
+
+/**
+ * Helper function to assert that a preference does not exist.
+ * @param {string} prefName - The preference name
+ * @param {string} message - The assertion message
+ * @param {Function} [getter=getIntPref] - The preference getter function (e.g., getIntPref, getBoolPref, getCharPref)
+ */
+function assertPrefNotExists(prefName, message, getter = "getIntPref") {
+ Assert.throws(
+ () => Services.prefs[getter](prefName),
+ /NS_ERROR_UNEXPECTED/,
+ message
+ );
+}
+
+/**
+ * Tests specific edge cases for deleteBranch behavior with consecutive dots
+ */
+add_task(function test_deleteBranch_edge_cases() {
+ const ps = Services.prefs;
+
+ // Test case 1: deleteBranch("foo") deletes all preferences
+ ps.setIntPref("EdgeTest.foo", 1);
+ ps.setIntPref("EdgeTest.foo.", 2);
+ ps.setIntPref("EdgeTest.foo.bar", 3);
+ ps.setIntPref("EdgeTest.foo..", 4);
+ ps.setIntPref("EdgeTest.foo..baz", 5);
+
+ ps.deleteBranch("EdgeTest.foo");
+
+ assertPrefNotExists(
+ "EdgeTest.foo",
+ "EdgeTest.foo should be deleted by deleteBranch('EdgeTest.foo')"
+ );
+ assertPrefNotExists(
+ "EdgeTest.foo.",
+ "EdgeTest.foo. should be deleted by deleteBranch('EdgeTest.foo')"
+ );
+ assertPrefNotExists(
+ "EdgeTest.foo.bar",
+ "EdgeTest.foo.bar should be deleted by deleteBranch('EdgeTest.foo')"
+ );
+ assertPrefNotExists(
+ "EdgeTest.foo..",
+ "EdgeTest.foo.. should be deleted by deleteBranch('EdgeTest.foo')"
+ );
+ assertPrefNotExists(
+ "EdgeTest.foo..baz",
+ "EdgeTest.foo..baz should be deleted by deleteBranch('EdgeTest.foo')"
+ );
+
+ // Test case 2: deleteBranch("foo.") also deletes all preferences
+ ps.setIntPref("EdgeTest.foo", 1);
+ ps.setIntPref("EdgeTest.foo.", 2);
+ ps.setIntPref("EdgeTest.foo.bar", 3);
+ ps.setIntPref("EdgeTest.foo..", 4);
+ ps.setIntPref("EdgeTest.foo..baz", 5);
+
+ ps.deleteBranch("EdgeTest.foo.");
+
+ assertPrefNotExists(
+ "EdgeTest.foo",
+ "EdgeTest.foo should be deleted by deleteBranch('EdgeTest.foo.')"
+ );
+ assertPrefNotExists(
+ "EdgeTest.foo.",
+ "EdgeTest.foo. should be deleted by deleteBranch('EdgeTest.foo.')"
+ );
+ assertPrefNotExists(
+ "EdgeTest.foo.bar",
+ "EdgeTest.foo.bar should be deleted by deleteBranch('EdgeTest.foo.')"
+ );
+ assertPrefNotExists(
+ "EdgeTest.foo..",
+ "EdgeTest.foo.. should be deleted by deleteBranch('EdgeTest.foo.')"
+ );
+ assertPrefNotExists(
+ "EdgeTest.foo..baz",
+ "EdgeTest.foo..baz should be deleted by deleteBranch('EdgeTest.foo.')"
+ );
+
+ // Test case 3: deleteBranch("foo..") deletes only "foo.", "foo..", and "foo..baz"
+ ps.setIntPref("EdgeTest.foo", 1);
+ ps.setIntPref("EdgeTest.foo.", 2);
+ ps.setIntPref("EdgeTest.foo.bar", 3);
+ ps.setIntPref("EdgeTest.foo..", 4);
+ ps.setIntPref("EdgeTest.foo..baz", 5);
+
+ ps.deleteBranch("EdgeTest.foo..");
+
+ assertPrefExists(
+ "EdgeTest.foo",
+ 1,
+ "EdgeTest.foo should not be deleted by deleteBranch('EdgeTest.foo..')"
+ );
+ assertPrefNotExists(
+ "EdgeTest.foo.",
+ "EdgeTest.foo. should be deleted by deleteBranch('EdgeTest.foo..')"
+ );
+ assertPrefExists(
+ "EdgeTest.foo.bar",
+ 3,
+ "EdgeTest.foo.bar should not be deleted by deleteBranch('EdgeTest.foo..')"
+ );
+ assertPrefNotExists(
+ "EdgeTest.foo..",
+ "EdgeTest.foo.. should be deleted by deleteBranch('EdgeTest.foo..')"
+ );
+ assertPrefNotExists(
+ "EdgeTest.foo..baz",
+ "EdgeTest.foo..baz should be deleted by deleteBranch('EdgeTest.foo..')"
+ );
+
+ // Clean up
+ ps.deleteBranch("EdgeTest");
+});
+
+/**
* Tests the reported bug where creating a preference value, clearing it,
* then creating it again as a different type raises an exception.
*/
