Bug 1301311 - Cleanly reject pings that are submitted too late in shutdown. r=Dexter
authorVedant Sareen <vedant.sareen@students.iiit.ac.in>
Thu, 05 Jan 2017 22:32:01 +0530
changeset 374444 7c6283b785c0341ffb359008a5eaf300520cb8e3
parent 374443 b9c4115cdeacafa82509973d5d56e678076eb715
child 374445 e6257972e6a6490597824d4a0753ec6cd9a005c7
push id6996
push userjlorenzo@mozilla.com
push dateMon, 06 Mar 2017 20:48:21 +0000
treeherdermozilla-beta@d89512dab048 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersDexter
bugs1301311
milestone53.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1301311 - Cleanly reject pings that are submitted too late in shutdown. r=Dexter Added |_shuttingDown| boolean flag, which is initialized to false and is set to true when Firefox shuts down. This allows rejecting pings sent after shutdown by checking the value of this flag. Also added a new test that shuts down Telemetry using |yield TelemetryController.testShutdown()| and then tries sending a ping to it.
toolkit/components/telemetry/TelemetryController.jsm
toolkit/components/telemetry/tests/unit/test_TelemetryController.js
--- a/toolkit/components/telemetry/TelemetryController.jsm
+++ b/toolkit/components/telemetry/TelemetryController.jsm
@@ -315,16 +315,17 @@ this.TelemetryController = Object.freeze
   promiseInitialized() {
     return Impl.promiseInitialized();
   },
 });
 
 var Impl = {
   _initialized: false,
   _initStarted: false, // Whether we started setting up TelemetryController.
+  _shuttingDown: false, // Whether the browser is shutting down.
   _logger: null,
   _prevValues: {},
   // The previous build ID, if this is the first run with a new build.
   // Undefined if this is not the first run, or the previous build ID is unknown.
   _previousBuildID: undefined,
   _clientID: null,
   // A task performing delayed initialization
   _delayedInitTask: null,
@@ -488,16 +489,23 @@ var Impl = {
    * @param {Boolean} [aOptions.addEnvironment=false] true if the ping should contain the
    *                  environment data.
    * @param {Object}  [aOptions.overrideEnvironment=null] set to override the environment data.
    * @returns {Promise} Test-only - a promise that is resolved with the ping id once the ping is stored or sent.
    */
   submitExternalPing: function send(aType, aPayload, aOptions) {
     this._log.trace("submitExternalPing - type: " + aType + ", aOptions: " + JSON.stringify(aOptions));
 
+    // Reject pings sent after shutdown.
+    if (this._shuttingDown) {
+      const errorMessage = "submitExternalPing - Submission is not allowed after shutdown, discarding ping of type: " + aType;
+      this._log.error(errorMessage);
+      return Promise.reject(new Error(errorMessage));
+    }
+
     // Enforce the type string to only contain sane characters.
     const typeUuid = /^[a-z0-9][a-z0-9-]+[a-z0-9]$/i;
     if (!typeUuid.test(aType)) {
       this._log.error("submitExternalPing - invalid ping type: " + aType);
       let histogram = Telemetry.getKeyedHistogramById("TELEMETRY_INVALID_PING_TYPE_SUBMITTED");
       histogram.add(aType, 1);
       return Promise.reject(new Error("Invalid type string submitted."));
     }
@@ -653,16 +661,17 @@ var Impl = {
    *   3) _delayedInitTask is currently running.
    *   4) _delayedInitTask finished running and is nulled out.
    *
    * @return {Promise} Resolved when TelemetryController and TelemetrySession are fully
    *                   initialized. This is only used in tests.
    */
   setupTelemetry: function setupTelemetry(testing) {
     this._initStarted = true;
+    this._shuttingDown = false;
     this._testMode = testing;
 
     this._log.trace("setupTelemetry");
 
     if (this._delayedInitTask) {
       this._log.error("setupTelemetry - init task already running");
       return this._delayedInitTaskDeferred.promise;
     }
@@ -784,31 +793,33 @@ var Impl = {
       yield this._connectionsBarrier.wait();
 
       // Perform final shutdown operations.
       yield TelemetryStorage.shutdown();
     } finally {
       // Reset state.
       this._initialized = false;
       this._initStarted = false;
+      this._shuttingDown = true;
     }
   }),
 
   shutdown() {
     this._log.trace("shutdown");
 
     // We can be in one the following states here:
     // 1) setupTelemetry was never called
     // or it was called and
     //   2) _delayedInitTask was scheduled, but didn't run yet.
     //   3) _delayedInitTask is running now.
     //   4) _delayedInitTask finished running already.
 
     // This handles 1).
     if (!this._initStarted) {
+      this._shuttingDown = true;
       return Promise.resolve();
     }
 
     // This handles 4).
     if (!this._delayedInitTask) {
       // We already ran the delayed initialization.
       return this._cleanupOnShutdown();
     }
--- a/toolkit/components/telemetry/tests/unit/test_TelemetryController.js
+++ b/toolkit/components/telemetry/tests/unit/test_TelemetryController.js
@@ -447,16 +447,19 @@ add_task(function* test_telemetryEnabled
   Assert.equal(Telemetry.canRecordExtended, true,
                "True must enable Telemetry recording.");
 
   // Also check that the false works as well.
   Preferences.set(PREF_ENABLED, false);
   yield TelemetryController.testReset();
   Assert.equal(Telemetry.canRecordExtended, false,
                "False must disable Telemetry recording.");
+
+  // Restore the state of the pref.
+  Preferences.set(PREF_ENABLED, true);
 });
 
 add_task(function* test_telemetryCleanFHRDatabase() {
   const FHR_DBNAME_PREF = "datareporting.healthreport.dbName";
   const CUSTOM_DB_NAME = "unlikely.to.be.used.sqlite";
   const DEFAULT_DB_NAME = "healthreport.sqlite";
 
   // Check that we're able to remove a FHR DB with a custom name.
@@ -497,11 +500,20 @@ add_task(function* test_telemetryCleanFH
 
   // Trigger the cleanup and check that the files were removed.
   yield TelemetryStorage.removeFHRDatabase();
   for (let dbFilePath of DEFAULT_DB_PATHS) {
     Assert.ok(!(yield OS.File.exists(dbFilePath)), "The DB must not be on the disk anymore: " + dbFilePath);
   }
 });
 
+// Testing shutdown and checking that pings sent afterwards are rejected.
+add_task(function* test_pingRejection() {
+  yield TelemetryController.testReset();
+  yield TelemetryController.testShutdown();
+  yield sendPing(false, false)
+    .then(() => Assert.ok(false, "Pings submitted after shutdown must be rejected."),
+          () => Assert.ok(true, "Ping submitted after shutdown correctly rejected."));
+});
+
 add_task(function* stopServer() {
   yield PingServer.stop();
 });