Bug 1145188: Shifting TelemetrySession init control to TelemetryController (core). r=Dexter
☠☠ backed out by f60f1ab969da ☠ ☠
authorIaroslav Sheptykin <yarik.sheptykin@googlemail.com>
Sun, 17 Apr 2016 11:57:00 +0200
changeset 295100 9e90db2262ac9fda4c5aab02c5ed74c163217ba2
parent 295099 84bc3fe034f2e40438beb879b5ce59abc62f8695
child 295101 e7570b6d28aaf015f5f69cf3a95f6396bf87e451
push id18973
push useralessio.placitelli@gmail.com
push dateThu, 28 Apr 2016 07:49:33 +0000
treeherderfx-team@e7570b6d28aa [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersDexter
bugs1145188
milestone49.0a1
Bug 1145188: Shifting TelemetrySession init control to TelemetryController (core). r=Dexter
toolkit/components/telemetry/TelemetryController.jsm
toolkit/components/telemetry/TelemetryEnvironment.jsm
toolkit/components/telemetry/TelemetrySession.jsm
toolkit/components/telemetry/TelemetryStartup.js
--- a/toolkit/components/telemetry/TelemetryController.jsm
+++ b/toolkit/components/telemetry/TelemetryController.jsm
@@ -130,39 +130,49 @@ var Policy = {
 this.EXPORTED_SYMBOLS = ["TelemetryController"];
 
 this.TelemetryController = Object.freeze({
   Constants: Object.freeze({
     PREF_LOG_LEVEL: PREF_LOG_LEVEL,
     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);
   },
 
   /**
    * Send a notification.
    */
   observe: function (aSubject, aTopic, aData) {
     return Impl.observe(aSubject, aTopic, aData);
@@ -294,23 +304,16 @@ this.TelemetryController = Object.freeze
    *
    * @return The client id as string, or null.
    */
   get clientID() {
     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.
    */
   getSessionRecorder: function() {
     return Impl._sessionRecorder;
   },
 
   /**
@@ -616,16 +619,19 @@ var Impl = {
    * for performance reasons.
    *
    * This delayed initialization means TelemetryController init can be in the following states:
    * 1) setupTelemetry was never called
    * or it was called and
    *   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;
     this._testMode = testing;
 
     this._log.trace("setupTelemetry");
 
     if (this._delayedInitTask) {
@@ -649,16 +655,20 @@ var Impl = {
     // Initialize the session recorder.
     if (!this._sessionRecorder) {
       this._sessionRecorder = new SessionRecorder(PREF_SESSIONS_BRANCH);
       this._sessionRecorder.onStartup();
     }
 
     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
     // lead to some stale client ids.
     this._clientID = ClientID.getCachedClientID();
 
     // Delay full telemetry initialization to give the browser time to
     // run various late initializers. Otherwise our gathered memory
@@ -670,18 +680,21 @@ var Impl = {
         this._initialized = true;
         TelemetryEnvironment.delayedInit();
 
         yield TelemetrySend.setup(this._testMode);
 
         // Load the ClientID.
         this._clientID = yield ClientID.getClientID();
 
-        // Purge the pings archive by removing outdated pings. We don't wait for this
-        // task to complete, but TelemetryStorage blocks on it during shutdown.
+        // Perform TelemetrySession delayed init.
+        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
         // the profile directory. This is a temporary measure that we should drop
         // in the future.
         TelemetryStorage.removeFHRDatabase();
 
         this._delayedInitTaskDeferred.resolve();
@@ -708,16 +721,17 @@ var Impl = {
     this._testMode = testing;
 
     // We call |enableTelemetryRecording| here to make sure that Telemetry.canRecord* flags
     // are in sync between chrome and content processes.
     if (!this.enableTelemetryRecording()) {
       this._log.trace("setupContentTelemetry - Content process recording disabled.");
       return;
     }
+    TelemetrySession.setupContent(testing);
   },
 
   // Do proper shutdown waiting and cleanup.
   _cleanupOnShutdown: Task.async(function*() {
     if (!this._initialized) {
       return;
     }
 
@@ -728,16 +742,18 @@ var Impl = {
     try {
       // Stop the datachoices infobar display.
       TelemetryReportingPolicy.shutdown();
       TelemetryEnvironment.shutdown();
 
       // Stop any ping sending.
       yield TelemetrySend.shutdown();
 
+      yield TelemetrySession.shutdown();
+
       // First wait for clients processing shutdown.
       yield this._shutdownBarrier.wait();
 
       // ... and wait for any outstanding async ping activity.
       yield this._connectionsBarrier.wait();
 
       // Perform final shutdown operations.
       yield TelemetryStorage.shutdown();
@@ -888,18 +904,28 @@ var Impl = {
 
     return ping;
   },
 
   reset: Task.async(function*() {
     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;
   }),
 };
--- a/toolkit/components/telemetry/TelemetryEnvironment.jsm
+++ b/toolkit/components/telemetry/TelemetryEnvironment.jsm
@@ -83,19 +83,31 @@ this.TelemetryEnvironment = {
     return getGlobal().shutdown();
   },
 
   // Policy to use when saving preferences. Exported for using them in tests.
   RECORD_PREF_STATE: 1, // Don't record the preference value
   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;
 const RECORD_PREF_VALUE = TelemetryEnvironment.RECORD_PREF_VALUE;
 const DEFAULT_ENVIRONMENT_PREFS = new Map([
   ["app.feedback.baseURL", {what: RECORD_PREF_VALUE}],
   ["app.support.baseURL", {what: RECORD_PREF_VALUE}],
   ["accessibility.browsewithcaret", {what: RECORD_PREF_VALUE}],
@@ -1405,9 +1417,14 @@ EnvironmentCache.prototype = {
       try {
         this._log.debug("_onEnvironmentChange - calling " + name);
         listener(what, oldEnvironment);
       } catch (e) {
         this._log.error("_onEnvironmentChange - listener " + name + " caught error", e);
       }
     }
   },
+
+  reset: function () {
+    this._shutdown = false;
+    this._delayedInitFinished = false;
+  }
 };
--- a/toolkit/components/telemetry/TelemetrySession.jsm
+++ b/toolkit/components/telemetry/TelemetrySession.jsm
@@ -612,59 +612,64 @@ this.TelemetrySession = Object.freeze({
    * @return The metadata as a JS object
    */
   getMetadata: function(reason) {
     return Impl.getMetadata(reason);
   },
   /**
    * Used only for testing purposes.
    */
-  reset: function() {
+  testReset: function() {
     Impl._sessionId = null;
     Impl._subsessionId = null;
     Impl._previousSessionId = null;
     Impl._previousSubsessionId = null;
     Impl._subsessionCounter = 0;
     Impl._profileSubsessionCounter = 0;
     Impl._subsessionStartActiveTicks = 0;
     Impl._subsessionStartTimeMonotonic = 0;
-    this.uninstall();
-    return this.setup();
+    this.testUninstall();
   },
   /**
-   * Used only for testing purposes.
-   * @param {Boolean} [aForceSavePending=true] If true, always saves the ping whether Telemetry
-   *        can send pings or not, which is used for testing.
+   * Triggers shutdown of the module.
    */
-  shutdown: function(aForceSavePending = true) {
-    return Impl.shutdownChromeProcess(aForceSavePending);
+  shutdown: function() {
+    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() {
-    return Impl.setupChromeProcess(true);
-  },
-  /**
-   * Used only for testing purposes.
-   */
-  setupContent: function() {
-    return Impl.setupContentProcess(true);
-  },
-  /**
-   * Used only for testing purposes.
-   */
-  uninstall: function() {
+  testUninstall: 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.
    */
   observe: function (aSubject, aTopic, aData) {
     return Impl.observe(aSubject, aTopic, aData);
   },
 });
 
 var Impl = {
@@ -716,27 +721,25 @@ var Impl = {
   _subsessionStartDate: null,
   // Start time of the current subsession using a monotonic clock for the subsession
   // length measurements.
   _subsessionStartTimeMonotonic: 0,
   // The active ticks counted when the subsession starts
   _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() {
     if (!this._logger) {
       this._logger = Log.repository.getLoggerWithMessagePrefix(LOGGER_NAME, LOGGER_PREFIX);
     }
     return this._logger;
   },
@@ -1381,36 +1384,32 @@ var Impl = {
       // observer and catch if it fails because the observer was not added.
       Services.obs.removeObserver(this, TOPIC_CYCLE_COLLECTOR_BEGIN);
     } catch (e) {
       this._log.warn("detachObservers - Failed to remove " + TOPIC_CYCLE_COLLECTOR_BEGIN, e);
     }
   },
 
   /**
-   * 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");
-      return Promise.resolve();
+      this._log.error("earlyInit - already initialized");
+      return;
     }
 
     if (!Telemetry.canRecordBase && !testing) {
-      this._log.config("setupChromeProcess - Telemetry recording is disabled, skipping Chrome process setup.");
-      return Promise.resolve();
+      this._log.config("earlyInit - Telemetry recording is disabled, skipping Chrome process setup.");
+      return;
     }
 
     // Generate a unique id once per session so the server can cope with duplicate
     // submissions, orphaning and other oddities. The id is shared across subsessions.
     this._sessionId = Policy.generateSessionUUID();
     this.startNewSubsession();
     // startNewSubsession sets |_subsessionStartDate| to the current date/time. Use
     // the very same value for |_sessionStartDate|.
@@ -1427,37 +1426,38 @@ var Impl = {
     let previousBuildId = Preferences.get(PREF_PREVIOUS_BUILDID, null);
     let thisBuildID = Services.appinfo.appBuildID;
     // If there is no previousBuildId preference, we send null to the server.
     if (previousBuildId != thisBuildID) {
       this._previousBuildId = previousBuildId;
       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);
     }
     Services.obs.addObserver(this, "xul-window-visible", false);
     this._hasWindowRestoredObserver = true;
     this._hasXulWindowVisibleObserver = true;
 
     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
-    // footprint and other numbers would be too optimistic.
-    this._delayedInitTaskDeferred = Promise.defer();
-    this._delayedInitTask = new DeferredTask(function* () {
+/**
+  * Does the "heavy" Telemetry initialization later on, so we
+  * don't impact startup performance.
+  * @return {Promise} Resolved when the initialization completes.
+  */
+  delayedInit:function() {
+    this._log.trace("delayedInit");
+
+    this._delayedInitTask = Task.spawn(function* () {
       try {
         this._initialized = true;
 
         yield this._loadSessionData();
         // Update the session data to keep track of new subsessions created before
         // the initialization.
         yield TelemetryStorage.saveSessionData(this._getSessionDataObject());
 
@@ -1468,40 +1468,37 @@ var Impl = {
 
         if (IS_UNIFIED_TELEMETRY) {
           // Check for a previously written aborted session ping.
           yield TelemetryController.checkAbortedSessionPing();
 
           // 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();
           }
 
           TelemetryEnvironment.registerChangeListener(ENVIRONMENT_CHANGE_LISTENER,
                                  (reason, data) => this._onEnvironmentChange(reason, data));
 
           // Start the scheduler.
           // We skip this if unified telemetry is off, so we don't
           // trigger the new unified ping types.
           TelemetryScheduler.init();
         }
 
-        this._delayedInitTaskDeferred.resolve();
+        this._delayedInitTask = null;
       } catch (e) {
-        this._delayedInitTaskDeferred.reject(e);
-      } finally {
         this._delayedInitTask = null;
-        this._delayedInitTaskDeferred = null;
+        throw e;
       }
-    }.bind(this), testing ? TELEMETRY_TEST_DELAY : TELEMETRY_DELAY);
+    }.bind(this));
 
-    this._delayedInitTask.arm();
-    return this._delayedInitTaskDeferred.promise;
+    return this._delayedInitTask;
   },
 
   /**
    * Initializes telemetry for a content process.
    */
   setupContentProcess: function setupContentProcess(testing) {
     this._log.trace("setupContentProcess");
     this._testing = testing;
@@ -1828,22 +1825,16 @@ var Impl = {
    */
   observe: function (aSubject, aTopic, aData) {
     // Prevent the cycle collector begin topic from cluttering the log.
     if (aTopic != TOPIC_CYCLE_COLLECTOR_BEGIN) {
       this._log.trace("observe - " + aTopic + " notified.");
     }
 
     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");
       this.uninstall();
 
       this.sendContentProcessPing(REASON_SAVED_SESSION);
       break;
     case TOPIC_CYCLE_COLLECTOR_BEGIN:
@@ -1910,21 +1901,19 @@ var Impl = {
       TelemetryController.addPendingPing(getPingType(payload), payload, options);
       break;
     }
     return undefined;
   },
 
   /**
    * 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) {
-    this._log.trace("shutdownChromeProcess - testing: " + testing);
+  shutdownChromeProcess: function() {
+    this._log.trace("shutdownChromeProcess");
 
     let cleanup = () => {
       if (IS_UNIFIED_TELEMETRY) {
         TelemetryEnvironment.unregisterChangeListener(ENVIRONMENT_CHANGE_LISTENER);
         TelemetryScheduler.shutdown();
       }
       this.uninstall();
 
@@ -1940,35 +1929,34 @@ var Impl = {
           yield TelemetryController.removeAbortedSessionPing();
         }
 
         reset();
       }.bind(this));
     };
 
     // 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.
-    //   3) _delayedInitTask is running now.
-    //   4) _delayedInitTask finished running already.
+    //   2) _delayedInitTask is running now.
+    //   3) _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).
-    return this._delayedInitTask.finalize().then(cleanup);
+     // This handles 2).
+     return this._delayedInitTask.then(cleanup);
    },
 
   /**
    * Gather and send a daily ping.
    * @return {Promise} Resolved when the ping is sent.
    */
   _sendDailyPing: function() {
     this._log.trace("_sendDailyPing");
--- a/toolkit/components/telemetry/TelemetryStartup.js
+++ b/toolkit/components/telemetry/TelemetryStartup.js
@@ -6,34 +6,31 @@
 "use strict";
 
 const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
 
 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");
 
 /**
  * TelemetryStartup is needed to forward the "profile-after-change" notification
  * to TelemetryController.jsm.
  */
 function TelemetryStartup() {
 }
 
 TelemetryStartup.prototype.classID = Components.ID("{117b219f-92fe-4bd2-a21b-95a342a9d474}");
 TelemetryStartup.prototype.QueryInterface = XPCOMUtils.generateQI([Components.interfaces.nsIObserver]);
 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();
     TelemetryEnvironment.registerChangeListener("CrashAnnotator", annotateEnvironment);
     TelemetryEnvironment.onInitialized().then(() => annotateEnvironment());
   }
 }