Bug 1345345: Fix preference observers reporting changes to arbitrary sub-branches. r?rhelmer draft
authorKris Maglione <maglione.k@gmail.com>
Tue, 07 Mar 2017 20:10:42 -0800
changeset 495424 c6d8038bfa840b2944e0cdbaf3dcec0c5a0c36a7
parent 494526 0aac487103cebe31b430b44d21b89720fc3989ef
child 548377 bababc8594dde170e5db5b46fbdcc4f25f66a02a
push id48333
push usermaglione.k@gmail.com
push dateWed, 08 Mar 2017 20:51:31 +0000
reviewersrhelmer
bugs1345345
milestone54.0a1
Bug 1345345: Fix preference observers reporting changes to arbitrary sub-branches. r?rhelmer MozReview-Commit-ID: CrGHoTiw4kz
browser/modules/SelfSupportBackend.jsm
services/sync/modules/service.js
toolkit/components/places/UnifiedComplete.js
toolkit/modules/Preferences.jsm
toolkit/modules/tests/xpcshell/test_Preferences.js
toolkit/mozapps/extensions/internal/GMPProvider.jsm
--- a/browser/modules/SelfSupportBackend.jsm
+++ b/browser/modules/SelfSupportBackend.jsm
@@ -75,17 +75,17 @@ var SelfSupportBackendInternal = {
   /**
    * Initializes the self support backend.
    */
   init() {
     this._configureLogging();
 
     this._log.trace("init");
 
-    Preferences.observe(PREF_BRANCH_LOG, this._configureLogging, this);
+    Services.prefs.addObserver(PREF_BRANCH_LOG, this, false);
 
     // Only allow to use SelfSupport if Unified Telemetry is enabled.
     let reportingEnabled = IS_UNIFIED_TELEMETRY;
     if (!reportingEnabled) {
       this._log.config("init - Disabling SelfSupport because FHR and Unified Telemetry are disabled.");
       return;
     }
 
@@ -106,17 +106,17 @@ var SelfSupportBackendInternal = {
   },
 
   /**
    * Shut down the self support backend, if active.
    */
   uninit() {
     this._log.trace("uninit");
 
-    Preferences.ignore(PREF_BRANCH_LOG, this._configureLogging, this);
+    Services.prefs.removeObserver(PREF_BRANCH_LOG, this);
 
     // Cancel delayed loading, if still active, when shutting down.
     clearTimeout(this._delayedLoadTimerId);
 
     // Dispose of the hidden browser.
     if (this._browser !== null) {
       if (this._browser.contentWindow) {
         this._browser.contentWindow.removeEventListener("DOMWindowClose", this, true);
@@ -143,16 +143,18 @@ var SelfSupportBackendInternal = {
    * since tabs might still be loading. Then, we open the self support.
    */
   observe(aSubject, aTopic, aData) {
     this._log.trace("observe - Topic " + aTopic);
 
     if (aTopic === "sessionstore-windows-restored") {
       Services.obs.removeObserver(this, "sessionstore-windows-restored");
       this._delayedLoadTimerId = setTimeout(this._loadSelfSupport.bind(this), STARTUP_DELAY_MS);
+    } else if (aTopic === "nsPref:changed") {
+      this._configureLogging();
     }
   },
 
   /**
    * Configure the logger based on the preferences.
    */
   _configureLogging() {
     if (!this._log) {
--- a/services/sync/modules/service.js
+++ b/services/sync/modules/service.js
@@ -304,17 +304,17 @@ Sync11Service.prototype = {
     if (!this._checkCrypto()) {
       this.enabled = false;
       this._log.info("Could not load the Weave crypto component. Disabling " +
                       "Weave, since it will not work correctly.");
     }
 
     Svc.Obs.add("weave:service:setup-complete", this);
     Svc.Obs.add("sync:collection_changed", this); // Pulled from FxAccountsCommon
-    Svc.Prefs.observe("engine.", this);
+    Services.prefs.addObserver(PREFS_BRANCH + "engine.", this, false);
 
     this.scheduler = new SyncScheduler(this);
 
     if (!this.enabled) {
       this._log.info("Firefox Sync disabled.");
     }
 
     this._updateCachedURLs();
--- a/toolkit/components/places/UnifiedComplete.js
+++ b/toolkit/components/places/UnifiedComplete.js
@@ -539,17 +539,17 @@ XPCOMUtils.defineLazyGetter(this, "Prefs
       Ci.nsIObserver,
       Ci.nsISupportsWeakReference ])
   };
 
   // Synchronize suggest.* prefs with autocomplete.enabled at initialization
   syncEnabledPref();
 
   loadPrefs();
-  prefs.observe("", store);
+  Services.prefs.addObserver(PREF_BRANCH, store, false);
   Services.prefs.addObserver("keyword.enabled", store, true);
 
   return Object.seal(store);
 });
 
 // Prefill Sites related
 
 function PrefillSite(url, title) {
--- a/toolkit/modules/Preferences.jsm
+++ b/toolkit/modules/Preferences.jsm
@@ -397,21 +397,21 @@ function PrefObserver(prefName, callback
 
 PrefObserver.prototype = {
   QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver, Ci.nsISupportsWeakReference]),
 
   observe(subject, topic, data) {
     // The pref service only observes whole branches, but we only observe
     // individual preferences, so we check here that the pref that changed
     // is the exact one we're observing (and not some sub-pref on the branch).
-    if (data.indexOf(this.prefName) != 0)
+    if (data != this.prefName)
       return;
 
     if (typeof this.callback == "function") {
-      let prefValue = Preferences.get(data);
+      let prefValue = Preferences.get(this.prefName);
 
       if (this.thisObject)
         this.callback.call(this.thisObject, prefValue);
       else
         this.callback(prefValue);
     } else // typeof this.callback == "object" (nsIObserver)
       this.callback.observe(subject, topic, data);
   }
--- a/toolkit/modules/tests/xpcshell/test_Preferences.js
+++ b/toolkit/modules/tests/xpcshell/test_Preferences.js
@@ -205,102 +205,116 @@ add_test(function test_reset_nonexistent
   run_next_test();
 });
 
 add_test(function test_observe_prefs_function() {
   let observed = false;
   let observer = function() { observed = !observed };
 
   Preferences.observe("test_observe_prefs_function", observer);
+  Preferences.set("test_observe_prefs_function.subpref", "something");
+  do_check_false(observed);
   Preferences.set("test_observe_prefs_function", "something");
   do_check_true(observed);
 
   Preferences.ignore("test_observe_prefs_function", observer);
   Preferences.set("test_observe_prefs_function", "something else");
   do_check_true(observed);
 
   // Clean up.
   Preferences.reset("test_observe_prefs_function");
+  Preferences.reset("test_observe_prefs_function.subpref");
 
   run_next_test();
 });
 
 add_test(function test_observe_prefs_object() {
   let observer = {
     observed: false,
     observe() {
       this.observed = !this.observed;
     }
   };
 
   Preferences.observe("test_observe_prefs_object", observer.observe, observer);
+  Preferences.set("test_observe_prefs_object.subpref", "something");
+  do_check_false(observer.observed);
   Preferences.set("test_observe_prefs_object", "something");
   do_check_true(observer.observed);
 
   Preferences.ignore("test_observe_prefs_object", observer.observe, observer);
   Preferences.set("test_observe_prefs_object", "something else");
   do_check_true(observer.observed);
 
   // Clean up.
   Preferences.reset("test_observe_prefs_object");
+  Preferences.reset("test_observe_prefs_object.subpref");
 
   run_next_test();
 });
 
 add_test(function test_observe_prefs_nsIObserver() {
   let observer = {
     observed: false,
     observe(subject, topic, data) {
       this.observed = !this.observed;
       do_check_true(subject instanceof Ci.nsIPrefBranch);
       do_check_eq(topic, "nsPref:changed");
       do_check_eq(data, "test_observe_prefs_nsIObserver");
     }
   };
 
   Preferences.observe("test_observe_prefs_nsIObserver", observer);
+  Preferences.set("test_observe_prefs_nsIObserver.subpref", "something");
   Preferences.set("test_observe_prefs_nsIObserver", "something");
   do_check_true(observer.observed);
 
   Preferences.ignore("test_observe_prefs_nsIObserver", observer);
   Preferences.set("test_observe_prefs_nsIObserver", "something else");
   do_check_true(observer.observed);
 
   // Clean up.
   Preferences.reset("test_observe_prefs_nsIObserver");
+  Preferences.reset("test_observe_prefs_nsIObserver.subpref");
 
   run_next_test();
 });
 
-/*
+// This should not need to be said, but *DO NOT DISABLE THIS TEST*.
+//
+// Existing consumers of the observer API depend on the observer only
+// being triggered for the exact preference they are observing. This is
+// expecially true for consumers of the function callback variant which
+// passes the preference's new value but not its name.
 add_test(function test_observe_exact_pref() {
   let observed = false;
   let observer = function() { observed = !observed };
 
   Preferences.observe("test_observe_exact_pref", observer);
   Preferences.set("test_observe_exact_pref.sub-pref", "something");
   do_check_false(observed);
 
   // Clean up.
   Preferences.ignore("test_observe_exact_pref", observer);
   Preferences.reset("test_observe_exact_pref.sub-pref");
 
   run_next_test();
 });
-*/
 
 add_test(function test_observe_value_of_set_pref() {
   let observer = function(newVal) { do_check_eq(newVal, "something") };
 
   Preferences.observe("test_observe_value_of_set_pref", observer);
+  Preferences.set("test_observe_value_of_set_pref.subpref", "somethingelse");
   Preferences.set("test_observe_value_of_set_pref", "something");
 
   // Clean up.
   Preferences.ignore("test_observe_value_of_set_pref", observer);
   Preferences.reset("test_observe_value_of_set_pref");
+  Preferences.reset("test_observe_value_of_set_pref.subpref");
 
   run_next_test();
 });
 
 add_test(function test_observe_value_of_reset_pref() {
   let observer = function(newVal) { do_check_true(typeof newVal == "undefined") };
 
   Preferences.set("test_observe_value_of_reset_pref", "something");
--- a/toolkit/mozapps/extensions/internal/GMPProvider.jsm
+++ b/toolkit/mozapps/extensions/internal/GMPProvider.jsm
@@ -525,17 +525,17 @@ var GMPProvider = {
 
   startup() {
     configureLogging();
     this._log = Log.repository.getLoggerWithMessagePrefix("Toolkit.GMP",
                                                           "GMPProvider.");
     this.buildPluginList();
     this.ensureProperCDMInstallState();
 
-    Preferences.observe(GMPPrefs.KEY_LOG_BASE, configureLogging);
+    Services.prefs.addObserver(GMPPrefs.KEY_LOG_BASE, configureLogging, false);
 
     for (let plugin of this._plugins.values()) {
       let wrapper = plugin.wrapper;
       let gmpPath = wrapper.gmpPath;
       let isEnabled = wrapper.isActive;
       this._log.trace("startup - enabled=" + isEnabled + ", gmpPath=" +
                       gmpPath);
 
@@ -576,17 +576,17 @@ var GMPProvider = {
       gmpService.addPluginDirectory(clearkeyPath);
     } catch (e) {
       this._log.warn("startup - adding clearkey CDM failed", e);
     }
   },
 
   shutdown() {
     this._log.trace("shutdown");
-    Preferences.ignore(GMPPrefs.KEY_LOG_BASE, configureLogging);
+    Services.prefs.removeObserver(GMPPrefs.KEY_LOG_BASE, configureLogging);
 
     let shutdownTask = Task.spawn(function*() {
       this._log.trace("shutdown - shutdownTask");
       let shutdownSucceeded = true;
 
       for (let plugin of this._plugins.values()) {
         try {
           yield plugin.wrapper.shutdown();