Bug 1145188: Shifting TelemetrySession init control to TelemetryController (core). r=Dexter

toolkit/components/telemetry/TelemetryController.jsm

@@ -135,29 +135,39 @@ this.TelemetryController = Object.freeze({
     PREF_LOG_DUMP: PREF_LOG_DUMP,
     PREF_SERVER: PREF_SERVER,
   }),
+
   /**
    * Used only for testing purposes.
    */
-  initLogging: function() {
+  testInitLogging: function() {
     configureLogging();
   },
+
   /**
    * Used only for testing purposes.
    */
-  reset: function() {
+  testReset: function() {
     return Impl.reset();
   },
+
   /**
    * Used only for testing purposes.
    */
-  setup: function() {
+  testSetup: function() {
     return Impl.setupTelemetry(true);
   },
 
   /**
    * Used only for testing purposes.
    */
-  setupContent: function() {
+  testShutdown: function() {
+    return Impl.shutdown();
+  },
+
+  /**
+   * Used only for testing purposes.
+   */
+  testSetupContent: function() {
     return Impl.setupContentTelemetry(true);
   },
 
@@ -298,13 +308,6 @@ this.TelemetryController = Object.freeze({
     return Impl.clientID;
   },
 
-  /**
-   * The AsyncShutdown.Barrier to synchronize with TelemetryController shutdown.
-   */
-  get shutdown() {
-    return Impl._shutdownBarrier.client;
-  },
-
   /**
    * The session recorder instance managed by Telemetry.
    * @return {Object} The active SessionRecorder instance or null if not available.
@@ -621,6 +624,9 @@ var Impl = {
    *   2) _delayedInitTask was scheduled, but didn't run yet.
    *   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;
@@ -654,6 +660,10 @@ var Impl = {
 
     this._attachObservers();
 
+    // Perform a lightweight, early initialization for the component, just registering
+    // a few observers and initializing the session.
+    TelemetrySession.earlyInit(this._testMode);
+
     // For very short session durations, we may never load the client
     // id from disk.
     // We try to cache it in prefs to avoid this, even though this may
@@ -675,8 +685,11 @@ var Impl = {
         // Load the ClientID.
         this._clientID = yield ClientID.getClientID();
 
-        // Purge the pings archive by removing outdated pings. We don't wait for this
+        // Perform TelemetrySession delayed init.
-        // task to complete, but TelemetryStorage blocks on it during shutdown.
+        yield TelemetrySession.delayedInit();
+        // Purge the pings archive by removing outdated pings. We don't wait for
+        // this task to complete, but TelemetryStorage blocks on it during
+        // shutdown.
         TelemetryStorage.runCleanPingArchiveTask();
 
         // Now that FHR/healthreporter is gone, make sure to remove FHR's DB from
@@ -713,6 +726,7 @@ var Impl = {
       this._log.trace("setupContentTelemetry - Content process recording disabled.");
       return;
     }
+    TelemetrySession.setupContent(testing);
   },
 
   // Do proper shutdown waiting and cleanup.
@@ -733,6 +747,8 @@ var Impl = {
       // Stop any ping sending.
       yield TelemetrySend.shutdown();
 
+      yield TelemetrySession.shutdown();
+
       // First wait for clients processing shutdown.
       yield this._shutdownBarrier.wait();
 
@@ -893,12 +909,22 @@ var Impl = {
     this._clientID = null;
     this._detachObservers();
 
+    yield TelemetrySession.testReset();
+
+    this._connectionsBarrier = new AsyncShutdown.Barrier(
+      "TelemetryController: Waiting for pending ping activity"
+    );
+    this._shutdownBarrier = new AsyncShutdown.Barrier(
+      "TelemetryController: Waiting for clients."
+    );
+
     // We need to kick of the controller setup first for tests that check the
     // cached client id.
     let controllerSetup = this.setupTelemetry(true);
 
     yield TelemetrySend.reset();
     yield TelemetryStorage.reset();
+    yield TelemetryEnvironment.testReset();
 
     yield controllerSetup;
   }),

toolkit/components/telemetry/TelemetryEnvironment.jsm

@@ -88,9 +88,21 @@ this.TelemetryEnvironment = {
   RECORD_PREF_VALUE: 2, // We only record user-set prefs.
 
   // Testing method
-  _watchPreferences: function(prefMap) {
+  testWatchPreferences: function(prefMap) {
     return getGlobal()._watchPreferences(prefMap);
   },
+
+  /**
+   * Intended for use in tests only.
+   *
+   * In multiple tests we need a way to shut and re-start telemetry together
+   * with TelemetryEnvironment. This is problematic due to the fact that
+   * TelemetryEnvironment is a singleton. We, therefore, need this helper
+   * method to be able to re-set TelemetryEnvironment.
+   */
+  testReset: function() {
+    return getGlobal().reset();
+  },
 };
 
 const RECORD_PREF_STATE = TelemetryEnvironment.RECORD_PREF_STATE;
@@ -1412,4 +1424,9 @@ EnvironmentCache.prototype = {
       }
     }
   },
+
+  reset: function () {
+    this._shutdown = false;
+    this._delayedInitFinished = false;
+  }
 };

toolkit/components/telemetry/TelemetrySession.jsm

@@ -628,7 +628,7 @@ this.TelemetrySession = Object.freeze({
   /**
    * Used only for testing purposes.
    */
-  reset: function() {
+  testReset: function() {
     Impl._sessionId = null;
     Impl._subsessionId = null;
     Impl._previousSessionId = null;
@@ -637,39 +637,44 @@ this.TelemetrySession = Object.freeze({
     Impl._profileSubsessionCounter = 0;
     Impl._subsessionStartActiveTicks = 0;
     Impl._subsessionStartTimeMonotonic = 0;
-    this.uninstall();
+    this.testUninstall();
-    return this.setup();
   },
   /**
-   * Used only for testing purposes.
+   * Triggers shutdown of the module.
-   * @param {Boolean} [aForceSavePending=true] If true, always saves the ping whether Telemetry
-   *        can send pings or not, which is used for testing.
    */
-  shutdown: function(aForceSavePending = true) {
+  shutdown: function() {
-    return Impl.shutdownChromeProcess(aForceSavePending);
+    return Impl.shutdownChromeProcess();
+  },
+  /**
+   * Sets up components used in the content process.
+   */
+  setupContent: function(testing = false) {
+    return Impl.setupContentProcess(testing);
   },
   /**
    * Used only for testing purposes.
    */
-  setup: function() {
+  testUninstall: function() {
-    return Impl.setupChromeProcess(true);
-  },
-  /**
-   * Used only for testing purposes.
-   */
-  setupContent: function() {
-    return Impl.setupContentProcess(true);
-  },
-  /**
-   * Used only for testing purposes.
-   */
-  uninstall: function() {
     try {
       Impl.uninstall();
     } catch (ex) {
       // Ignore errors
     }
   },
+  /**
+   * Lightweight init function, called as soon as Firefox starts.
+   */
+  earlyInit: function(aTesting = false) {
+    return Impl.earlyInit(aTesting);
+  },
+  /**
+   * Does the "heavy" Telemetry initialization later on, so we
+   * don't impact startup performance.
+   * @return {Promise} Resolved when the initialization completes.
+   */
+  delayedInit: function() {
+    return Impl.delayedInit();
+  },
   /**
    * Send a notification.
    */
@@ -732,17 +737,15 @@ var Impl = {
   _subsessionStartActiveTicks: 0,
   // A task performing delayed initialization of the chrome process
   _delayedInitTask: null,
-  // The deferred promise resolved when the initialization task completes.
-  _delayedInitTaskDeferred: null,
   // Need a timeout in case children are tardy in giving back their memory reports.
   _totalMemoryTimeout: undefined,
+  _testing: false,
   // An accumulator of total memory across all processes. Only valid once the final child reports.
   _totalMemory: null,
   // A Set of outstanding USS report ids
   _childrenToHearFrom: null,
   // monotonically-increasing id for USS reports
   _nextTotalMemoryId: 1,
-  _testing: false,
 
 
   get _log() {
@@ -1397,26 +1400,22 @@ var Impl = {
   },
 
   /**
-   * Initializes telemetry within a timer.
+   * Lightweight init function, called as soon as Firefox starts.
    */
-  setupChromeProcess: function setupChromeProcess(testing) {
+  earlyInit: function(testing) {
+    this._log.trace("earlyInit");
+
     this._initStarted = true;
-    this._log.trace("setupChromeProcess");
     this._testing = testing;
 
-    if (this._delayedInitTask) {
-      this._log.error("setupChromeProcess - init task already running");
-      return this._delayedInitTaskDeferred.promise;
-    }
-
     if (this._initialized && !testing) {
-      this._log.error("setupChromeProcess - already initialized");
+      this._log.error("earlyInit - already initialized");
-      return Promise.resolve();
+      return;
     }
 
     if (!Telemetry.canRecordBase && !testing) {
-      this._log.config("setupChromeProcess - Telemetry recording is disabled, skipping Chrome process setup.");
+      this._log.config("earlyInit - Telemetry recording is disabled, skipping Chrome process setup.");
-      return Promise.resolve();
+      return;
     }
 
     // Generate a unique id once per session so the server can cope with duplicate
@@ -1443,10 +1442,6 @@ var Impl = {
       Preferences.set(PREF_PREVIOUS_BUILDID, thisBuildID);
     }
 
-    TelemetryController.shutdown.addBlocker("TelemetrySession: shutting down",
-                                      () => this.shutdownChromeProcess(),
-                                      () => this._getState());
-
     Services.obs.addObserver(this, "sessionstore-windows-restored", false);
     if (AppConstants.platform === "android") {
       Services.obs.addObserver(this, "application-background", false);
@@ -1458,12 +1453,17 @@ var Impl = {
     ppml.addMessageListener(MESSAGE_TELEMETRY_PAYLOAD, this);
     ppml.addMessageListener(MESSAGE_TELEMETRY_THREAD_HANGS, this);
     ppml.addMessageListener(MESSAGE_TELEMETRY_USS, this);
+},
 
-    // Delay full telemetry initialization to give the browser time to
+/**
-    // run various late initializers. Otherwise our gathered memory
+  * Does the "heavy" Telemetry initialization later on, so we
-    // footprint and other numbers would be too optimistic.
+  * don't impact startup performance.
-    this._delayedInitTaskDeferred = Promise.defer();
+  * @return {Promise} Resolved when the initialization completes.
-    this._delayedInitTask = new DeferredTask(function* () {
+  */
+  delayedInit:function() {
+    this._log.trace("delayedInit");
+
+    this._delayedInitTask = Task.spawn(function* () {
       try {
         this._initialized = true;
 
@@ -1484,7 +1484,7 @@ var Impl = {
           // Write the first aborted-session ping as early as possible. Just do that
           // if we are not testing, since calling Telemetry.reset() will make a previous
           // aborted ping a pending ping.
-          if (!testing) {
+          if (!this._testing) {
             yield this._saveAbortedSessionPing();
           }
 
@@ -1497,17 +1497,14 @@ var Impl = {
           TelemetryScheduler.init();
         }
 
-        this._delayedInitTaskDeferred.resolve();
-      } catch (e) {
-        this._delayedInitTaskDeferred.reject(e);
-      } finally {
         this._delayedInitTask = null;
-        this._delayedInitTaskDeferred = null;
+      } catch (e) {
+        this._delayedInitTask = null;
+        throw e;
       }
-    }.bind(this), testing ? TELEMETRY_TEST_DELAY : TELEMETRY_DELAY);
+    }.bind(this));
 
-    this._delayedInitTask.arm();
+    return this._delayedInitTask;
-    return this._delayedInitTaskDeferred.promise;
   },
 
   /**
@@ -1844,12 +1841,6 @@ var Impl = {
     }
 
     switch (aTopic) {
-    case "profile-after-change":
-      // profile-after-change is only registered for chrome processes.
-      return this.setupChromeProcess();
-    case "app-startup":
-      // app-startup is only registered for content processes.
-      return this.setupContentProcess();
     case "content-child-shutdown":
       // content-child-shutdown is only registered for content processes.
       Services.obs.removeObserver(this, "content-child-shutdown");
@@ -1926,11 +1917,9 @@ var Impl = {
 
   /**
    * This tells TelemetrySession to uninitialize and save any pending pings.
-   * @param testing Optional. If true, always saves the ping whether Telemetry
-   *                can send pings or not, which is used for testing.
    */
-  shutdownChromeProcess: function(testing = false) {
+  shutdownChromeProcess: function() {
-    this._log.trace("shutdownChromeProcess - testing: " + testing);
+    this._log.trace("shutdownChromeProcess");
 
     let cleanup = () => {
       if (IS_UNIFIED_TELEMETRY) {
@@ -1956,25 +1945,24 @@ var Impl = {
     };
 
     // We can be in one the following states here:
-    // 1) setupChromeProcess was never called
+    // 1) delayedInit was never called
     // or it was called and
-    //   2) _delayedInitTask was scheduled, but didn't run yet.
+    //   2) _delayedInitTask is running now.
-    //   3) _delayedInitTask is running now.
+    //   3) _delayedInitTask finished running already.
-    //   4) _delayedInitTask finished running already.
 
     // This handles 1).
     if (!this._initStarted) {
       return Promise.resolve();
      }
 
-    // This handles 4).
+    // This handles 3).
     if (!this._delayedInitTask) {
       // We already ran the delayed initialization.
       return cleanup();
      }
 
-    // This handles 2) and 3).
+     // This handles 2).
-    return this._delayedInitTask.finalize().then(cleanup);
+     return this._delayedInitTask.then(cleanup);
    },
 
   /**

toolkit/components/telemetry/TelemetryStartup.js

@@ -11,8 +11,6 @@ Cu.import("resource://gre/modules/XPCOMUtils.jsm", this);
 
 XPCOMUtils.defineLazyModuleGetter(this, "TelemetryController",
                                   "resource://gre/modules/TelemetryController.jsm");
-XPCOMUtils.defineLazyModuleGetter(this, "TelemetrySession",
-                                  "resource://gre/modules/TelemetrySession.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "TelemetryEnvironment",
                                   "resource://gre/modules/TelemetryEnvironment.jsm");
 
@@ -28,7 +26,6 @@ TelemetryStartup.prototype.QueryInterface = XPCOMUtils.generateQI([Components.in
 TelemetryStartup.prototype.observe = function(aSubject, aTopic, aData) {
   if (aTopic == "profile-after-change" || aTopic == "app-startup") {
     TelemetryController.observe(null, aTopic, null);
-    TelemetrySession.observe(null, aTopic, null);
   }
   if (aTopic == "profile-after-change") {
     annotateEnvironment();

toolkit/components/telemetry/TelemetryStorage.jsm

@@ -447,6 +447,13 @@ this.TelemetryStorage = {
   _testGetArchivedPingDataFromFileName: function(aFileName) {
     return TelemetryStorageImpl._getArchivedPingDataFromFileName(aFileName);
   },
+
+  /**
+   * Only used in tests, this helper allows cleaning up the pending ping storage.
+   */
+  testClearPendingPings: function() {
+    return TelemetryStorageImpl.runRemovePendingPingsTask();
+  }
 };
 
 /**