Bug 855024 - Better management of pull-only providers; r=rnewman

services/healthreport/tests/xpcshell/test_healthreporter.js

@@ -198,6 +198,8 @@ add_task(function test_pull_only_providers() {
     do_check_null(reporter.getProvider("DummyConstantProvider"));
 
     yield reporter._providerManager.ensurePullOnlyProvidersRegistered();
+    do_check_eq(reporter._providerManager._providers.size, 2);
+    do_check_true(reporter._storage.hasProvider("DummyConstantProvider"));
     yield reporter.collectMeasurements();
     yield reporter._providerManager.ensurePullOnlyProvidersUnregistered();
 

services/metrics/providermanager.jsm

@@ -34,7 +34,9 @@ this.ProviderManager = function (storage) {
   this._providerInitializing = false;
 
   this._pullOnlyProviders = {};
-  this._pullOnlyProvidersRegistered = false;
+  this._pullOnlyProvidersRegisterCount = 0;
+  this._pullOnlyProvidersState = this.PULL_ONLY_NOT_REGISTERED;
+  this._pullOnlyProvidersCurrentPromise = null;
 
   // Callback to allow customization of providers after they are constructed
   // but before they call out into their initialization code.
@@ -42,6 +44,11 @@ this.ProviderManager = function (storage) {
 }
 
 this.ProviderManager.prototype = Object.freeze({
+  PULL_ONLY_NOT_REGISTERED: "none",
+  PULL_ONLY_REGISTERING: "registering",
+  PULL_ONLY_UNREGISTERING: "unregistering",
+  PULL_ONLY_REGISTERED: "registered",
+
   get providers() {
     let providers = [];
     for (let [name, entry] of this._providers) {
@@ -225,41 +232,134 @@ this.ProviderManager.prototype = Object.freeze({
    * Ensure that pull-only providers are registered.
    */
   ensurePullOnlyProvidersRegistered: function () {
-    if (this._pullOnlyProvidersRegistered) {
+    let state = this._pullOnlyProvidersState;
+
+    this._pullOnlyProvidersRegisterCount++;
+
+    if (state == this.PULL_ONLY_REGISTERED) {
+      this._log.debug("Requested pull-only provider registration and " +
+                      "providers are already registered.");
       return CommonUtils.laterTickResolvingPromise();
     }
 
-    let onFinished = function () {
-      this._pullOnlyProvidersRegistered = true;
+    // If we're in the process of registering, chain off that request.
+    if (state == this.PULL_ONLY_REGISTERING) {
+      this._log.debug("Requested pull-only provider registration and " +
+                      "registration is already in progress.");
+      return this._pullOnlyProvidersCurrentPromise;
+    }
 
-      return CommonUtils.laterTickResolvingPromise();
-    }.bind(this);
+    this._log.debug("Pull-only provider registration requested.");
+
+    // A side-effect of setting this is that an active unregistration will
+    // effectively short circuit and finish as soon as the in-flight
+    // unregistration (if any) finishes.
+    this._pullOnlyProvidersState = this.PULL_ONLY_REGISTERING;
+
+    let inFlightPromise = this._pullOnlyProvidersCurrentPromise;
+
+    this._pullOnlyProvidersCurrentPromise =
+      Task.spawn(function registerPullProviders() {
+
+      if (inFlightPromise) {
+        this._log.debug("Waiting for in-flight pull-only provider activity " +
+                        "to finish before registering.");
+        try {
+          yield inFlightPromise;
+        } catch (ex) {
+          this._log.warn("Error when waiting for existing pull-only promise: " +
+                         CommonUtils.exceptionStr(ex));
+        }
+      }
 
-    return Task.spawn(function registerPullProviders() {
       for each (let providerType in this._pullOnlyProviders) {
+        // Short-circuit if we're no longer registering.
+        if (this._pullOnlyProvidersState != this.PULL_ONLY_REGISTERING) {
+          this._log.debug("Aborting pull-only provider registration.");
+          break;
+        }
+
         try {
           let provider = this._initProviderFromType(providerType);
+
+          // This is a no-op if the provider is already registered. So, the
+          // only overhead is constructing an instance. This should be cheap
+          // and isn't worth optimizing.
           yield this.registerProvider(provider);
         } catch (ex) {
           this._recordError("Error registering pull-only provider", ex);
         }
       }
-    }.bind(this)).then(onFinished, onFinished);
+
+      // It's possible we changed state while registering. Only mark as
+      // registered if we didn't change state.
+      if (this._pullOnlyProvidersState == this.PULL_ONLY_REGISTERING) {
+        this._pullOnlyProvidersState = this.PULL_ONLY_REGISTERED;
+        this._pullOnlyProvidersCurrentPromise = null;
+      }
+    }.bind(this));
+    return this._pullOnlyProvidersCurrentPromise;
   },
 
   ensurePullOnlyProvidersUnregistered: function () {
-    if (!this._pullOnlyProvidersRegistered) {
+    let state = this._pullOnlyProvidersState;
+
+    // If we're not registered, this is a no-op.
+    if (state == this.PULL_ONLY_NOT_REGISTERED) {
+      this._log.debug("Requested pull-only provider unregistration but none " +
+                      "are registered.");
       return CommonUtils.laterTickResolvingPromise();
     }
 
-    let onFinished = function () {
-      this._pullOnlyProvidersRegistered = false;
+    // If we're currently unregistering, recycle the promise from last time.
+    if (state == this.PULL_ONLY_UNREGISTERING) {
+      this._log.debug("Requested pull-only provider unregistration and " +
+                 "unregistration is in progress.");
+      this._pullOnlyProvidersRegisterCount =
+        Math.max(0, this._pullOnlyProvidersRegisterCount - 1);
 
+      return this._pullOnlyProvidersCurrentPromise;
+    }
+
+    // We ignore this request while multiple entities have requested
+    // registration because we don't want a request from an "inner,"
+    // short-lived request to overwrite the desire of the "parent,"
+    // longer-lived request.
+    if (this._pullOnlyProvidersRegisterCount > 1) {
+      this._log.debug("Requested pull-only provider unregistration while " +
+                      "other callers still want them registered. Ignoring.");
+      this._pullOnlyProvidersRegisterCount--;
       return CommonUtils.laterTickResolvingPromise();
-    }.bind(this);
+    }
+
+    // We are either fully registered or registering with a single consumer.
+    // In both cases we are authoritative and can commence unregistration.
+
+    this._log.debug("Pull-only providers being unregistered.");
+    this._pullOnlyProvidersRegisterCount =
+      Math.max(0, this._pullOnlyProvidersRegisterCount - 1);
+    this._pullOnlyProvidersState = this.PULL_ONLY_UNREGISTERING;
+    let inFlightPromise = this._pullOnlyProvidersCurrentPromise;
+
+    this._pullOnlyProvidersCurrentPromise =
+      Task.spawn(function unregisterPullProviders() {
+
+      if (inFlightPromise) {
+        this._log.debug("Waiting for in-flight pull-only provider activity " +
+                        "to complete before unregistering.");
+        try {
+          yield inFlightPromise;
+        } catch (ex) {
+          this._log.warn("Error when waiting for existing pull-only promise: " +
+                         CommonUtils.exceptionStr(ex));
+        }
+      }
 
-    return Task.spawn(function unregisterPullProviders() {
       for (let provider of this.providers) {
+        if (this._pullOnlyProvidersState != this.PULL_ONLY_UNREGISTERING) {
+          return;
+        }
+
         if (!provider.pullOnly) {
           continue;
         }
@@ -276,7 +376,13 @@ this.ProviderManager.prototype = Object.freeze({
           this.unregisterProvider(provider.name);
         }
       }
-    }.bind(this)).then(onFinished, onFinished);
+
+      if (this._pullOnlyProvidersState == this.PULL_ONLY_UNREGISTERING) {
+        this._pullOnlyProvidersState = this.PULL_ONLY_NOT_REGISTERED;
+        this._pullOnlyProvidersCurrentPromise = null;
+      }
+    }.bind(this));
+    return this._pullOnlyProvidersCurrentPromise;
   },
 
   _popAndInitProvider: function () {

services/metrics/tests/xpcshell/test_metrics_provider_manager.js

@@ -8,7 +8,18 @@ const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
 Cu.import("resource://gre/modules/Metrics.jsm");
 Cu.import("resource://testing-common/services/metrics/mocks.jsm");
 
+const PULL_ONLY_TESTING_CATEGORY = "testing-only-pull-only-providers";
+
 function run_test() {
+  let cm = Cc["@mozilla.org/categorymanager;1"]
+             .getService(Ci.nsICategoryManager);
+  cm.addCategoryEntry(PULL_ONLY_TESTING_CATEGORY, "DummyProvider",
+                      "resource://testing-common/services/metrics/mocks.jsm",
+                      false, true);
+  cm.addCategoryEntry(PULL_ONLY_TESTING_CATEGORY, "DummyConstantProvider",
+                      "resource://testing-common/services/metrics/mocks.jsm",
+                      false, true);
+
   run_next_test();
 };
 
@@ -175,3 +186,85 @@ add_task(function test_collect_daily() {
   yield storage.close();
 });
 
+add_task(function test_pull_only_not_initialized() {
+  let storage = yield Metrics.Storage("pull_only_not_initialized");
+  let manager = new Metrics.ProviderManager(storage);
+  yield manager.registerProvidersFromCategoryManager(PULL_ONLY_TESTING_CATEGORY);
+  do_check_eq(manager.providers.length, 1);
+  do_check_eq(manager.providers[0].name, "DummyProvider");
+  yield storage.close();
+});
+
+add_task(function test_pull_only_registration() {
+  let storage = yield Metrics.Storage("pull_only_registration");
+  let manager = new Metrics.ProviderManager(storage);
+  yield manager.registerProvidersFromCategoryManager(PULL_ONLY_TESTING_CATEGORY);
+  do_check_eq(manager.providers.length, 1);
+
+  // Simple registration and unregistration.
+  yield manager.ensurePullOnlyProvidersRegistered();
+  do_check_eq(manager.providers.length, 2);
+  do_check_neq(manager.getProvider("DummyConstantProvider"), null);
+  yield manager.ensurePullOnlyProvidersUnregistered();
+  do_check_eq(manager.providers.length, 1);
+  do_check_null(manager.getProvider("DummyConstantProvider"));
+
+  // Multiple calls to register work.
+  yield manager.ensurePullOnlyProvidersRegistered();
+  do_check_eq(manager.providers.length, 2);
+  yield manager.ensurePullOnlyProvidersRegistered();
+  do_check_eq(manager.providers.length, 2);
+
+  // Unregister with 2 requests for registration should not unregister.
+  yield manager.ensurePullOnlyProvidersUnregistered();
+  do_check_eq(manager.providers.length, 2);
+
+  // But the 2nd one will.
+  yield manager.ensurePullOnlyProvidersUnregistered();
+  do_check_eq(manager.providers.length, 1);
+
+  yield storage.close();
+});
+
+add_task(function test_pull_only_register_while_registering() {
+  let storage = yield Metrics.Storage("pull_only_register_will_registering");
+  let manager = new Metrics.ProviderManager(storage);
+  yield manager.registerProvidersFromCategoryManager(PULL_ONLY_TESTING_CATEGORY);
+
+  manager.ensurePullOnlyProvidersRegistered();
+  manager.ensurePullOnlyProvidersRegistered();
+  yield manager.ensurePullOnlyProvidersRegistered();
+  do_check_eq(manager.providers.length, 2);
+
+  manager.ensurePullOnlyProvidersUnregistered();
+  manager.ensurePullOnlyProvidersUnregistered();
+  yield manager.ensurePullOnlyProvidersUnregistered();
+  do_check_eq(manager.providers.length, 1);
+
+  yield storage.close();
+});
+
+add_task(function test_pull_only_unregister_while_registering() {
+  let storage = yield Metrics.Storage("pull_only_unregister_while_registering");
+  let manager = new Metrics.ProviderManager(storage);
+  yield manager.registerProvidersFromCategoryManager(PULL_ONLY_TESTING_CATEGORY);
+
+  manager.ensurePullOnlyProvidersRegistered();
+  yield manager.ensurePullOnlyProvidersUnregistered();
+  do_check_eq(manager.providers.length, 1);
+
+  yield storage.close();
+});
+
+add_task(function test_pull_only_register_while_unregistering() {
+  let storage = yield Metrics.Storage("pull_only_register_while_unregistering");
+  let manager = new Metrics.ProviderManager(storage);
+  yield manager.registerProvidersFromCategoryManager(PULL_ONLY_TESTING_CATEGORY);
+
+  yield manager.ensurePullOnlyProvidersRegistered();
+  manager.ensurePullOnlyProvidersUnregistered();
+  yield manager.ensurePullOnlyProvidersRegistered();
+  do_check_eq(manager.providers.length, 2);
+
+  yield storage.close();
+});