Bug 917883 - Use AsyncShutdown instead of spinning the event loop in healthreporter.jsm. r=gps

services/healthreport/healthreporter.jsm

@@ -30,6 +30,8 @@ Cu.import("resource://gre/modules/XPCOMUtils.jsm");
 
 XPCOMUtils.defineLazyModuleGetter(this, "UpdateChannel",
                                   "resource://gre/modules/UpdateChannel.jsm");
+XPCOMUtils.defineLazyModuleGetter(this, "AsyncShutdown",
+                                  "resource://gre/modules/AsyncShutdown.jsm");
 
 // Oldest year to allow in date preferences. This module was implemented in
 // 2012 and no dates older than that should be encountered.
@@ -48,7 +50,6 @@ const TELEMETRY_JSON_PAYLOAD_SERIALIZE = "HEALTHREPORT_JSON_PAYLOAD_SERIALIZE_MS
 const TELEMETRY_PAYLOAD_SIZE_UNCOMPRESSED = "HEALTHREPORT_PAYLOAD_UNCOMPRESSED_BYTES";
 const TELEMETRY_PAYLOAD_SIZE_COMPRESSED = "HEALTHREPORT_PAYLOAD_COMPRESSED_BYTES";
 const TELEMETRY_UPLOAD = "HEALTHREPORT_UPLOAD_MS";
-const TELEMETRY_SHUTDOWN_DELAY = "HEALTHREPORT_SHUTDOWN_DELAY_MS";
 const TELEMETRY_COLLECT_CONSTANT = "HEALTHREPORT_COLLECT_CONSTANT_DATA_MS";
 const TELEMETRY_COLLECT_DAILY = "HEALTHREPORT_COLLECT_DAILY_MS";
 const TELEMETRY_SHUTDOWN = "HEALTHREPORT_SHUTDOWN_MS";
@@ -282,7 +283,8 @@ function AbstractHealthReporter(branch, policy, sessionRecorder) {
   this._shutdownRequested = false;
   this._shutdownInitiated = false;
   this._shutdownComplete = false;
-  this._shutdownCompleteCallback = null;
+  this._deferredShutdown = Promise.defer();
+  this._promiseShutdown = this._deferredShutdown.promise;
 
   this._errors = [];
 
@@ -369,7 +371,10 @@ AbstractHealthReporter.prototype = Object.freeze({
       // As soon as we have could storage, we need to register cleanup or
       // else bad things happen on shutdown.
       Services.obs.addObserver(this, "quit-application", false);
-      Services.obs.addObserver(this, "profile-before-change", false);
+      // The database needs to be shut down by the end of shutdown
+      // phase profileBeforeChange.
+      AsyncShutdown.profileBeforeChange.
+        addBlocker("FHR: Flushing storage shutdown", this._promiseShutdown);
 
       this._storageInProgress = true;
       TelemetryStopwatch.start(this._dbOpenHistogram, this);
@@ -495,11 +500,6 @@ AbstractHealthReporter.prototype = Object.freeze({
         this._initiateShutdown();
         break;
 
-      case "profile-before-change":
-        Services.obs.removeObserver(this, "profile-before-change");
-        this._waitForShutdown();
-        break;
-
       case "idle-daily":
         this._performDailyMaintenance();
         break;
@@ -550,80 +550,51 @@ AbstractHealthReporter.prototype = Object.freeze({
       Services.obs.removeObserver(this, "idle-daily");
     } catch (ex) { }
 
-    if (this._providerManager) {
-      let onShutdown = this._onProviderManagerShutdown.bind(this);
-      Task.spawn(this._shutdownProviderManager.bind(this))
-          .then(onShutdown, onShutdown);
-      return;
-    }
-
-    this._log.warn("Don't have provider manager. Proceeding to storage shutdown.");
-    this._shutdownStorage();
-  },
-
-  _shutdownProviderManager: function () {
-    this._log.info("Shutting down provider manager.");
-    for (let provider of this._providerManager.providers) {
+    Task.spawn(function() {
       try {
-        yield provider.shutdown();
-      } catch (ex) {
-        this._log.warn("Error when shutting down provider: " +
-                       CommonUtils.exceptionStr(ex));
+        if (this._providerManager) {
+          this._log.info("Shutting down provider manager.");
+          for (let provider of this._providerManager.providers) {
+            try {
+              yield provider.shutdown();
+            } catch (ex) {
+              this._log.warn("Error when shutting down provider: " +
+                             CommonUtils.exceptionStr(ex));
+            }
+          }
+          this._log.info("Provider manager shut down.");
+          this._providerManager = null;
+          this._onProviderManagerShutdown();
+        }
+        if (this._storage) {
+          this._log.info("Shutting down storage.");
+          try {
+            yield this._storage.close();
+            this._onStorageClose();
+          } catch (error) {
+            this._log.warn("Error when closing storage: " +
+                           CommonUtils.exceptionStr(error));
+          }
+          this._storage = null;
+        }
+
+        this._log.warn("Shutdown complete.");
+        this._shutdownComplete = true;
+      } finally {
+        this._deferredShutdown.resolve();
+        TelemetryStopwatch.finish(TELEMETRY_SHUTDOWN, this);
       }
-    }
+    }.bind(this));
   },
 
-  _onProviderManagerShutdown: function () {
-    this._log.info("Provider manager shut down.");
-    this._providerManager = null;
-    this._shutdownStorage();
+  _onStorageClose: function() {
+    // Do nothing.
+    // This method provides a hook point for the test suite.
   },
 
-  _shutdownStorage: function () {
-    if (!this._storage) {
-      this._onShutdownComplete();
-    }
-
-    this._log.info("Shutting down storage.");
-    let onClose = this._onStorageClose.bind(this);
-    this._storage.close().then(onClose, onClose);
-  },
-
-  _onStorageClose: function (error) {
-    this._log.info("Storage has been closed.");
-
-    if (error) {
-      this._log.warn("Error when closing storage: " +
-                     CommonUtils.exceptionStr(error));
-    }
-
-    this._storage = null;
-    this._onShutdownComplete();
-  },
-
-  _onShutdownComplete: function () {
-    this._log.warn("Shutdown complete.");
-    this._shutdownComplete = true;
-    TelemetryStopwatch.finish(TELEMETRY_SHUTDOWN, this);
-
-    if (this._shutdownCompleteCallback) {
-      this._shutdownCompleteCallback();
-    }
-  },
-
-  _waitForShutdown: function () {
-    if (this._shutdownComplete) {
-      return;
-    }
-
-    TelemetryStopwatch.start(TELEMETRY_SHUTDOWN_DELAY, this);
-    try {
-      this._shutdownCompleteCallback = Async.makeSpinningCallback();
-      this._shutdownCompleteCallback.wait();
-      this._shutdownCompleteCallback = null;
-    } finally {
-      TelemetryStopwatch.finish(TELEMETRY_SHUTDOWN_DELAY, this);
-    }
+  _onProviderManagerShutdown: function() {
+    // Do nothing.
+    // This method provides a hook point for the test suite.
   },
 
   /**
@@ -633,7 +604,7 @@ AbstractHealthReporter.prototype = Object.freeze({
    */
   _shutdown: function () {
     this._initiateShutdown();
-    this._waitForShutdown();
+    return this._promiseShutdown;
   },
 
   /**

services/healthreport/tests/xpcshell/test_healthreporter.js

@@ -120,7 +120,7 @@ add_task(function test_constructor() {
       failed = false;
     }
   } finally {
-    reporter._shutdown();
+    yield reporter._shutdown();
   }
 });
 
@@ -130,7 +130,7 @@ add_task(function test_shutdown_normal() {
   // We can't send "quit-application" notification because the xpcshell runner
   // will shut down!
   reporter._initiateShutdown();
-  reporter._waitForShutdown();
+  yield reporter._promiseShutdown;
 });
 
 add_task(function test_shutdown_storage_in_progress() {
@@ -143,7 +143,7 @@ add_task(function test_shutdown_storage_in_progress() {
 
   reporter.init();
 
-  reporter._waitForShutdown();
+  yield reporter._promiseShutdown;
   do_check_eq(reporter.providerManagerShutdownCount, 0);
   do_check_eq(reporter.storageCloseCount, 1);
 });
@@ -162,7 +162,7 @@ add_task(function test_shutdown_provider_manager_in_progress() {
   reporter.init();
 
   // This will hang if shutdown logic is busted.
-  reporter._waitForShutdown();
+  yield reporter._promiseShutdown;
   do_check_eq(reporter.providerManagerShutdownCount, 1);
   do_check_eq(reporter.storageCloseCount, 1);
 });
@@ -180,7 +180,7 @@ add_task(function test_shutdown_when_provider_manager_errors() {
   reporter.init();
 
   // This will hang if shutdown logic is busted.
-  reporter._waitForShutdown();
+  yield reporter._promiseShutdown;
   do_check_eq(reporter.providerManagerShutdownCount, 1);
   do_check_eq(reporter.storageCloseCount, 1);
 });
@@ -217,7 +217,7 @@ add_task(function test_pull_only_providers() {
     let values = yield reporter._storage.getMeasurementValues(mID);
     do_check_true(values.singular.size > 0);
   } finally {
-    reporter._shutdown();
+    yield reporter._shutdown();
   }
 });
 
@@ -248,7 +248,7 @@ add_task(function test_collect_daily() {
     yield reporter.collectMeasurements();
     do_check_eq(provider.collectDailyCount, 3);
   } finally {
-    reporter._shutdown();
+    yield reporter._shutdown();
   }
 });
 
@@ -272,7 +272,7 @@ add_task(function test_remove_old_lastpayload() {
       do_check_false(yield OS.File.exists(path));
     }
     yield reporter._state.save();
-    reporter._shutdown();
+    yield reporter._shutdown();
 
     let o = yield CommonUtils.readJSON(reporter._state._filename);
     do_check_true(o.removedOutdatedLastpayload);
@@ -284,7 +284,7 @@ add_task(function test_remove_old_lastpayload() {
       do_check_true(yield OS.File.exists(path));
     }
   } finally {
-    reporter._shutdown();
+    yield reporter._shutdown();
   }
 });
 
@@ -316,7 +316,7 @@ add_task(function test_json_payload_simple() {
     payload = yield reporter.getJSONPayload(true);
     do_check_eq(typeof payload, "object");
   } finally {
-    reporter._shutdown();
+    yield reporter._shutdown();
   }
 });
 
@@ -335,7 +335,7 @@ add_task(function test_json_payload_dummy_provider() {
     do_check_true(name in o.data.last);
     do_check_eq(o.data.last[name]._v, 1);
   } finally {
-    reporter._shutdown();
+    yield reporter._shutdown();
   }
 });
 
@@ -353,7 +353,7 @@ add_task(function test_collect_and_obtain_json_payload() {
     payload = yield reporter.collectAndObtainJSONPayload(true);
     do_check_eq(typeof payload, "object");
   } finally {
-    reporter._shutdown();
+    yield reporter._shutdown();
   }
 });
 
@@ -409,7 +409,7 @@ add_task(function test_constant_only_providers_in_json_payload() {
 
     yield deferred.promise;
   } finally {
-    reporter._shutdown();
+    yield reporter._shutdown();
   }
 });
 
@@ -444,7 +444,7 @@ add_task(function test_json_payload_multiple_days() {
     let today = reporter._formatDate(now);
     do_check_true(today in o.data.days);
   } finally {
-    reporter._shutdown();
+    yield reporter._shutdown();
   }
 });
 
@@ -521,7 +521,7 @@ add_task(function test_json_payload_newer_version_overwrites() {
     do_check_eq(o.data.days[day]["MultiMeasurementProvider.DummyMeasurement"]._v, highestVersion);
 
   } finally {
-    reporter._shutdown();
+    yield reporter._shutdown();
   }
 });
 
@@ -546,7 +546,7 @@ add_task(function test_idle_daily() {
     values = yield m.getValues();
     do_check_eq(values.days.size, 180);
   } finally {
-    reporter._shutdown();
+    yield reporter._shutdown();
   }
 });
 
@@ -569,7 +569,7 @@ add_task(function test_data_submission_transport_failure() {
     do_check_eq(data.uploadTransportFailure, 1);
     do_check_eq(Object.keys(data).length, 3);
   } finally {
-    reporter._shutdown();
+    yield reporter._shutdown();
   }
 });
 
@@ -597,7 +597,7 @@ add_task(function test_data_submission_server_failure() {
     do_check_eq(Object.keys(data).length, 3);
   } finally {
     yield shutdownServer(server);
-    reporter._shutdown();
+    yield reporter._shutdown();
   }
 });
 
@@ -638,14 +638,14 @@ add_task(function test_data_submission_success() {
     let d = reporter.lastPingDate;
     let id = reporter.lastSubmitID;
 
-    reporter._shutdown();
+    yield reporter._shutdown();
 
     // Ensure reloading state works.
     reporter = yield getReporter("data_submission_success");
     do_check_eq(reporter.lastSubmitID, id);
     do_check_eq(reporter.lastPingDate.getTime(), d.getTime());
 
-    reporter._shutdown();
+    yield reporter._shutdown();
   } finally {
     yield shutdownServer(server);
   }
@@ -687,7 +687,7 @@ add_task(function test_recurring_daily_pings() {
     do_check_eq(data.uploadSuccess, 2);
     do_check_eq(Object.keys(data).length, 4);
   } finally {
-    reporter._shutdown();
+    yield reporter._shutdown();
     yield shutdownServer(server);
   }
 });
@@ -714,7 +714,7 @@ add_task(function test_request_remote_data_deletion() {
     do_check_false(reporter.haveRemoteData());
     do_check_false(server.hasDocument(reporter.serverNamespace, id));
   } finally {
-    reporter._shutdown();
+    yield reporter._shutdown();
     yield shutdownServer(server);
   }
 });
@@ -736,7 +736,7 @@ add_task(function test_policy_accept_reject() {
     do_check_false(policy.dataSubmissionPolicyAccepted);
     do_check_false(reporter.willUploadData);
   } finally {
-    reporter._shutdown();
+    yield reporter._shutdown();
     yield shutdownServer(server);
   }
 });
@@ -759,7 +759,7 @@ add_task(function test_error_message_scrubbing() {
     reporter._recordError("Foo " + uri.spec);
     do_check_eq(reporter._errors[0], "Foo <AppDataURI>");
   } finally {
-    reporter._shutdown();
+    yield reporter._shutdown();
   }
 });
 
@@ -785,7 +785,7 @@ add_task(function test_basic_appinfo() {
     do_check_eq(payload["version"], 2);
     verify(payload["geckoAppInfo"]);
   } finally {
-    reporter._shutdown();
+    yield reporter._shutdown();
   }
 });
 
@@ -806,13 +806,13 @@ add_task(function test_collect_when_upload_disabled() {
     // We would ideally ensure the timer fires and does the right thing.
     // However, testing the update timer manager is quite involved.
   } finally {
-    reporter._shutdown();
+    yield reporter._shutdown();
   }
 });
 
 add_task(function test_failure_if_not_initialized() {
   let reporter = yield getReporter("failure_if_not_initialized");
-  reporter._shutdown();
+  yield reporter._shutdown();
 
   let error = false;
   try {
@@ -886,7 +886,7 @@ add_task(function test_upload_on_init_failure() {
   do_check_eq(doc.errors.length, 1);
   do_check_true(doc.errors[0].contains("Fake error during provider manager initialization"));
 
-  reporter._shutdown();
+  yield reporter._shutdown();
   yield shutdownServer(server);
 });
 
@@ -912,7 +912,7 @@ add_task(function test_state_prefs_conversion_simple() {
     do_check_false(prefs.isSet("lastSubmitID"));
     do_check_false(prefs.isSet("lastPingTime"));
   } finally {
-    reporter._shutdown();
+    yield reporter._shutdown();
   }
 });
 
@@ -934,7 +934,7 @@ add_task(function test_state_no_json_object() {
     do_check_eq(o.lastPingTime, 0);
     do_check_eq(o.remoteIDs.length, 0);
   } finally {
-    reporter._shutdown();
+    yield reporter._shutdown();
   }
 });
 
@@ -955,7 +955,7 @@ add_task(function test_state_future_version() {
     do_check_eq(o.lastPingTime, 2412);
     do_check_eq(o.remoteIDs.length, 1);
   } finally {
-    reporter._shutdown();
+    yield reporter._shutdown();
   }
 });
 
@@ -974,7 +974,7 @@ add_task(function test_state_invalid_json() {
     do_check_eq(reporter.lastPingDate.getTime(), 0);
     do_check_null(reporter.lastSubmitID);
   } finally {
-    reporter._shutdown();
+    yield reporter._shutdown();
   }
 });
 
@@ -1015,7 +1015,7 @@ add_task(function test_state_multiple_remote_ids() {
     do_check_eq(o.lastPingTime, reporter.lastPingDate.getTime());
   } finally {
     yield shutdownServer(server);
-    reporter._shutdown();
+    yield reporter._shutdown();
   }
 });
 
@@ -1047,6 +1047,6 @@ add_task(function test_state_downgrade_upgrade() {
     do_check_eq(o.remoteIDs.length, 3);
     do_check_eq(o.lastPingTime, now.getTime() + 1000);
   } finally {
-    reporter._shutdown();
+    yield reporter._shutdown();
   }
 });

services/healthreport/tests/xpcshell/test_provider_appinfo.js

@@ -269,6 +269,6 @@ add_task(function test_healthreporter_integration () {
       do_check_true("org.mozilla.appInfo.versions" in measurements);
     }
   } finally {
-    reporter._shutdown();
+    yield reporter._shutdown();
   }
 });

toolkit/components/telemetry/Histograms.json

@@ -3486,13 +3486,6 @@
     "extended_statistics_ok": true,
     "description": "Time (ms) spent to initialize Firefox Health Report service."
   },
-  "HEALTHREPORT_SHUTDOWN_DELAY_MS": {
-    "kind": "exponential",
-    "high": "20000",
-    "n_buckets": 15,
-    "extended_statistics_ok": true,
-    "description": "Time (ms) that Firefox Health Report delays application shutdown by."
-  },
   "HEALTHREPORT_GENERATE_JSON_PAYLOAD_MS": {
     "kind": "exponential",
     "high": "30000",