Bug 1140558 - Part 3 - Pass the old environment data to event listeners on environment changes. r=vladan
☠☠ backed out by 43235a6667fd ☠ ☠
authorGeorg Fritzsche <georg.fritzsche@googlemail.com>
Fri, 27 Mar 2015 21:01:19 +0100
changeset 266511 a54f84a3d26537d9ed5ed0472a4abd480b2c8b79
parent 266510 9623fa2b2e16de005b1508ba257e78b53c55270b
child 266512 d3512bb40d24e6d7f3671bb6c1aea13a7b9bd8a7
push id830
push userraliiev@mozilla.com
push dateFri, 19 Jun 2015 19:24:37 +0000
treeherdermozilla-release@932614382a68 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersvladan
bugs1140558
milestone39.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 1140558 - Part 3 - Pass the old environment data to event listeners on environment changes. r=vladan
toolkit/components/telemetry/TelemetryEnvironment.jsm
toolkit/components/telemetry/TelemetryPing.jsm
toolkit/components/telemetry/TelemetrySession.jsm
toolkit/components/telemetry/docs/environment.rst
toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js
toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
--- a/toolkit/components/telemetry/TelemetryEnvironment.jsm
+++ b/toolkit/components/telemetry/TelemetryEnvironment.jsm
@@ -13,16 +13,17 @@ const myScope = this;
 
 Cu.import("resource://gre/modules/AddonManager.jsm");
 Cu.import("resource://gre/modules/XPCOMUtils.jsm", this);
 Cu.import("resource://gre/modules/Task.jsm");
 Cu.import("resource://gre/modules/Log.jsm");
 Cu.import("resource://gre/modules/Preferences.jsm");
 Cu.import("resource://gre/modules/PromiseUtils.jsm");
 Cu.import("resource://gre/modules/Services.jsm");
+Cu.import("resource://gre/modules/ObjectUtils.jsm");
 
 XPCOMUtils.defineLazyModuleGetter(this, "ctypes",
                                   "resource://gre/modules/ctypes.jsm");
 #ifndef MOZ_WIDGET_GONK
 XPCOMUtils.defineLazyModuleGetter(this, "LightweightThemeManager",
                                   "resource://gre/modules/LightweightThemeManager.jsm");
 #endif
 XPCOMUtils.defineLazyModuleGetter(this, "ProfileAge",
@@ -361,85 +362,75 @@ EnvironmentAddonBuilder.prototype = {
   watchForChanges: function() {
     this._loaded = true;
     AddonManager.addAddonListener(this);
     Services.obs.addObserver(this, EXPERIMENTS_CHANGED_TOPIC, false);
   },
 
   // AddonListener
   onEnabled: function() {
-    this._onChange();
+    this._onAddonChange();
   },
   onDisabled: function() {
-    this._onChange();
+    this._onAddonChange();
   },
   onInstalled: function() {
-    this._onChange();
+    this._onAddonChange();
   },
   onUninstalling: function() {
-    this._onChange();
+    this._onAddonChange();
   },
 
-  _onChange: function() {
-    if (this._pendingTask) {
-      this._environment._log.trace("_onChange - task already pending");
-      return;
-    }
-
-    this._environment._log.trace("_onChange");
-    this._pendingTask = this._updateAddons().then(
-      (changed) => {
-        this._pendingTask = null;
-        if (changed) {
-          this._environment._onEnvironmentChange("addons-changed");
-        }
-      },
-      (err) => {
-        this._pendingTask = null;
-        this._environment._log.error("Error collecting addons", err);
-      });
+  _onAddonChange: function() {
+    this._environment._log.trace("_onAddonChange");
+    this._checkForChanges("addons-changed");
   },
 
   // nsIObserver
   observe: function (aSubject, aTopic, aData) {
     this._environment._log.trace("observe - Topic " + aTopic);
+    this._checkForChanges("experiment-changed");
+  },
 
-    if (aTopic == EXPERIMENTS_CHANGED_TOPIC) {
-      if (this._pendingTask) {
-        return;
-      }
-      this._pendingTask = this._updateAddons().then(
-        (changed) => {
-          this._pendingTask = null;
-          if (changed) {
-            this._environment._onEnvironmentChange("experiment-changed");
-          }
-        },
-        (err) => {
-          this._pendingTask = null;
-          this._environment._log.error("observe: Error collecting addons", err);
-        });
+  _checkForChanges: function(changeReason) {
+    if (this._pendingTask) {
+      this._environment._log.trace("_checkForChanges - task already pending, dropping change with reason " + changeReason);
+      return;
     }
+
+    this._pendingTask = this._updateAddons().then(
+      (result) => {
+        this._pendingTask = null;
+        if (result.changed) {
+          this._environment._onEnvironmentChange(changeReason, result.oldEnvironment);
+        }
+      },
+      (err) => {
+        this._pendingTask = null;
+        this._environment._log.error("_checkForChanges: Error collecting addons", err);
+      });
   },
 
   _shutdownBlocker: function() {
     if (this._loaded) {
       AddonManager.removeAddonListener(this);
       Services.obs.removeObserver(this, EXPERIMENTS_CHANGED_TOPIC);
     }
     return this._pendingTask;
   },
 
   /**
    * Collect the addon data for the environment.
    *
    * This should only be called from _pendingTask; otherwise we risk
    * running this during addon manager shutdown.
    *
-   * @returns Promise<bool> whether the environment changed.
+   * @returns Promise<Object> This returns a Promise resolved with a status object with the following members:
+   *   changed - Whether the environment changed.
+   *   oldEnvironment - Only set if a change occured, contains the environment data before the change.
    */
   _updateAddons: Task.async(function* () {
     this._environment._log.trace("_updateAddons");
     let personaId = null;
 #ifndef MOZ_WIDGET_GONK
     let theme = LightweightThemeManager.currentTheme;
     if (theme) {
       personaId = theme.id;
@@ -450,24 +441,27 @@ EnvironmentAddonBuilder.prototype = {
       activeAddons: yield this._getActiveAddons(),
       theme: yield this._getActiveTheme(),
       activePlugins: this._getActivePlugins(),
       activeGMPlugins: yield this._getActiveGMPlugins(),
       activeExperiment: this._getActiveExperiment(),
       persona: personaId,
     };
 
-    if (JSON.stringify(addons) !=
-        JSON.stringify(this._environment._currentEnvironment.addons)) {
+    let result = {
+      changed: !ObjectUtils.deepEqual(addons, this._environment._currentEnvironment.addons),
+    };
+
+    if (result.changed) {
       this._environment._log.trace("_updateAddons: addons differ");
+      result.oldEnvironment = Cu.cloneInto(this._environment._currentEnvironment, myScope);
       this._environment._currentEnvironment.addons = addons;
-      return true;
     }
-    this._environment._log.trace("_updateAddons: no changes found");
-    return false;
+
+    return result;
   }),
 
   /**
    * Get the addon data in object form.
    * @return Promise<object> containing the addon data.
    */
   _getActiveAddons: Task.async(function* () {
     // Request addons, asynchronously.
@@ -690,17 +684,18 @@ EnvironmentCache.prototype = {
     }
     return Promise.resolve(this.currentEnvironment);
   },
 
   /**
    * Register a listener for environment changes.
    * @param name The name of the listener. If a new listener is registered
    *             with the same name, the old listener will be replaced.
-   * @param listener function(reason, environment)
+   * @param listener function(reason, oldEnvironment) - Will receive a reason for
+                     the change and the environment data before the change.
    */
   registerChangeListener: function (name, listener) {
     this._log.trace("registerChangeListener for " + name);
     if (this._shutdown) {
       this._log.warn("registerChangeListener - already shutdown");
       return;
     }
     this._changeListeners.set(name, listener);
@@ -767,18 +762,19 @@ EnvironmentCache.prototype = {
 
     for (let pref in this._watchedPrefs) {
       Preferences.observe(pref, this._onPrefChanged, this);
     }
   },
 
   _onPrefChanged: function() {
     this._log.trace("_onPrefChanged");
+    let oldEnvironment = Cu.cloneInto(this._currentEnvironment, myScope);
     this._updateSettings();
-    this._onEnvironmentChange("pref-changed");
+    this._onEnvironmentChange("pref-changed", oldEnvironment);
   },
 
   /**
    * Do not receive any more change notifications for the preferences.
    */
   _stopWatchingPrefs: function () {
     this._log.trace("_stopWatchingPrefs");
 
@@ -1054,25 +1050,28 @@ EnvironmentCache.prototype = {
       device: this._getDeviceData(),
 #endif
       os: this._getOSData(),
       hdd: this._getHDDData(),
       gfx: this._getGFXData(),
     };
   },
 
-  _onEnvironmentChange: function (what) {
+  _onEnvironmentChange: function (what, oldEnvironment) {
     this._log.trace("_onEnvironmentChange for " + what);
     if (this._shutdown) {
       this._log.trace("_onEnvironmentChange - Already shut down.");
       return;
     }
 
+    // We are already skipping change events in _checkChanges if there is a pending change task running.
+    // Further throttling is coming in bug 1143714.
+
     for (let [name, listener] of this._changeListeners) {
       try {
         this._log.debug("_onEnvironmentChange - calling " + name);
-        listener(what, this.currentEnvironment);
+        listener(what, oldEnvironment);
       } catch (e) {
         this._log.error("_onEnvironmentChange - listener " + name + " caught error", e);
       }
     }
   },
 };
--- a/toolkit/components/telemetry/TelemetryPing.jsm
+++ b/toolkit/components/telemetry/TelemetryPing.jsm
@@ -171,16 +171,17 @@ this.TelemetryPing = Object.freeze({
    * @param {Object} aPayload The actual data payload for the ping.
    * @param {Object} [aOptions] Options object.
    * @param {Number} [aOptions.retentionDays=14] The number of days to keep the ping on disk
    *                 if sending fails.
    * @param {Boolean} [aOptions.addClientId=false] true if the ping should contain the client
    *                  id, false otherwise.
    * @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} A promise that resolves when the ping is sent.
    */
   send: function(aType, aPayload, aOptions = {}) {
     let options = aOptions;
     options.retentionDays = aOptions.retentionDays || DEFAULT_RETENTION_DAYS;
     options.addClientId = aOptions.addClientId || false;
     options.addEnvironment = aOptions.addEnvironment || false;
 
@@ -194,16 +195,17 @@ this.TelemetryPing = Object.freeze({
    * @param {Object} aPayload The actual data payload for the ping.
    * @param {Object} [aOptions] Options object.
    * @param {Number} [aOptions.retentionDays=14] The number of days to keep the ping on disk
    *                 if sending fails.
    * @param {Boolean} [aOptions.addClientId=false] true if the ping should contain the client
    *                  id, false otherwise.
    * @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} A promise that resolves when the pings are saved.
    */
   savePendingPings: function(aType, aPayload, aOptions = {}) {
     let options = aOptions;
     options.retentionDays = aOptions.retentionDays || DEFAULT_RETENTION_DAYS;
     options.addClientId = aOptions.addClientId || false;
     options.addEnvironment = aOptions.addEnvironment || false;
 
@@ -219,16 +221,17 @@ this.TelemetryPing = Object.freeze({
    * @param {Number} [aOptions.retentionDays=14] The number of days to keep the ping on disk
    *                 if sending fails.
    * @param {Boolean} [aOptions.addClientId=false] true if the ping should contain the client
    *                  id, false otherwise.
    * @param {Boolean} [aOptions.addEnvironment=false] true if the ping should contain the
    *                  environment data.
    * @param {Boolean} [aOptions.overwrite=false] true overwrites a ping with the same name,
    *                  if found.
+   * @param {Object}  [aOptions.overrideEnvironment=null] set to override the environment data.
    * @param {String} [aOptions.filePath] The path to save the ping to. Will save to default
    *                 ping location if not provided.
    *
    * @returns {Promise<Integer>} A promise that resolves with the ping id when the ping is
    *                             saved to disk.
    */
   savePing: function(aType, aPayload, aOptions = {}) {
     let options = aOptions;
@@ -315,16 +318,17 @@ let Impl = {
    *
    * @param {String} aType The type of the ping.
    * @param {Object} aPayload The actual data payload for the ping.
    * @param {Object} aOptions Options object.
    * @param {Boolean} aOptions.addClientId true if the ping should contain the client
    *                  id, false otherwise.
    * @param {Boolean} aOptions.addEnvironment true if the ping should contain the
    *                  environment data.
+   * @param {Object}  [aOptions.overrideEnvironment=null] set to override the environment data.
    *
    * @returns Promise<Object> A promise that resolves when the ping is completely assembled.
    */
   assemblePing: function assemblePing(aType, aPayload, aOptions = {}) {
     this._log.trace("assemblePing - Type " + aType + ", Server " + this._server +
                     ", aOptions " + JSON.stringify(aOptions));
 
     // Clone the payload data so we don't race against unexpected changes in subobjects that are
@@ -342,17 +346,17 @@ let Impl = {
       payload: aPayload,
     };
 
     if (aOptions.addClientId) {
       pingData.clientId = this._clientID;
     }
 
     if (aOptions.addEnvironment) {
-      pingData.environment = TelemetryEnvironment.currentEnvironment;
+      pingData.environment = aOptions.overrideEnvironment || TelemetryEnvironment.currentEnvironment;
     }
 
     return pingData;
   },
 
   popPayloads: function popPayloads() {
     this._log.trace("popPayloads");
     function payloadIter() {
@@ -406,16 +410,17 @@ let Impl = {
    * @param {Object} aPayload The actual data payload for the ping.
    * @param {Object} aOptions Options object.
    * @param {Number} aOptions.retentionDays The number of days to keep the ping on disk
    *                 if sending fails.
    * @param {Boolean} aOptions.addClientId true if the ping should contain the client id,
    *                  false otherwise.
    * @param {Boolean} aOptions.addEnvironment true if the ping should contain the
    *                  environment data.
+   * @param {Object}  aOptions.overrideEnvironment set to override the environment data.
    *
    * @returns {Promise} A promise that resolves when the ping is sent.
    */
   send: function send(aType, aPayload, aOptions) {
     this._log.trace("send - Type " + aType + ", Server " + this._server +
                     ", aOptions " + JSON.stringify(aOptions));
 
     let pingData = this.assemblePing(aType, aPayload, aOptions);
@@ -457,16 +462,17 @@ let Impl = {
    * @param {Object} aPayload The actual data payload for the ping.
    * @param {Object} aOptions Options object.
    * @param {Number} aOptions.retentionDays The number of days to keep the ping on disk
    *                 if sending fails.
    * @param {Boolean} aOptions.addClientId true if the ping should contain the client id,
    *                  false otherwise.
    * @param {Boolean} aOptions.addEnvironment true if the ping should contain the
    *                  environment data.
+   * @param {Object}  [aOptions.overrideEnvironment=null] set to override the environment data.
    *
    * @returns {Promise} A promise that resolves when all the pings are saved to disk.
    */
   savePendingPings: function savePendingPings(aType, aPayload, aOptions) {
     this._log.trace("savePendingPings - Type " + aType + ", Server " + this._server +
                     ", aOptions " + JSON.stringify(aOptions));
 
     let pingData = this.assemblePing(aType, aPayload, aOptions);
@@ -483,16 +489,17 @@ let Impl = {
    *                 if sending fails.
    * @param {Boolean} aOptions.addClientId true if the ping should contain the client id,
    *                  false otherwise.
    * @param {Boolean} aOptions.addEnvironment true if the ping should contain the
    *                  environment data.
    * @param {Boolean} aOptions.overwrite true overwrites a ping with the same name, if found.
    * @param {String} [aOptions.filePath] The path to save the ping to. Will save to default
    *                 ping location if not provided.
+   * @param {Object}  [aOptions.overrideEnvironment=null] set to override the environment data.
    *
    * @returns {Promise} A promise that resolves with the ping id when the ping is saved to
    *                    disk.
    */
   savePing: function savePing(aType, aPayload, aOptions) {
     this._log.trace("savePing - Type " + aType + ", Server " + this._server +
                     ", aOptions " + JSON.stringify(aOptions));
 
--- a/toolkit/components/telemetry/TelemetrySession.jsm
+++ b/toolkit/components/telemetry/TelemetrySession.jsm
@@ -1978,27 +1978,28 @@ let Impl = {
     let filePath = OS.Path.join(dataDir, SESSION_STATE_FILE_NAME);
     try {
       yield CommonUtils.writeJSON(sessionData, filePath);
     } catch(e) {
       this._log.error("_saveSessionData - Failed to write session data to " + filePath, e);
     }
   }),
 
-  _onEnvironmentChange: function(reason, data) {
+  _onEnvironmentChange: function(reason, oldEnvironment) {
     this._log.trace("_onEnvironmentChange", reason);
     let payload = this.getSessionPayload(REASON_ENVIRONMENT_CHANGE, true);
 
     let clonedPayload = Cu.cloneInto(payload, myScope);
     TelemetryScheduler.reschedulePings(REASON_ENVIRONMENT_CHANGE, clonedPayload);
 
     let options = {
       retentionDays: RETENTION_DAYS,
       addClientId: true,
       addEnvironment: true,
+      overrideEnvironment: oldEnvironment,
     };
     TelemetryPing.send(getPingType(payload), payload, options);
   },
 
   _isClassicReason: function(reason) {
     const classicReasons = [
       REASON_SAVED_SESSION,
       REASON_IDLE_DAILY,
--- a/toolkit/components/telemetry/docs/environment.rst
+++ b/toolkit/components/telemetry/docs/environment.rst
@@ -4,16 +4,23 @@ Environment
 
 The environment consists of data that is expected to be characteristic for performance and other behavior and not expected to change too often.
 
 Changes to most of these data points are detected (where possible and sensible) and will lead to a session split in the :doc:`main-ping`.
 The environment data may also be submitted by other ping types.
 
 *Note:* This is not submitted with all ping types due to privacy concerns. This and other data is inspected under the `data collection policy <https://wiki.mozilla.org/Firefox/Data_Collection>`_.
 
+Some parts of the environment must be fetched asynchronously at startup. We don't want other Telemetry components to block on waiting for the environment, so some items may be missing from it until the async fetching finished.
+This currently affects the following sections:
+
+- profile
+- addons
+
+
 Structure::
 
     {
       build: {
         applicationId: <string>, // nsIXULAppInfo.ID
         applicationName: <string>, // "Firefox"
         architecture: <string>, // e.g. "x86", build architecture for the active build
         architecturesInBinary: <string>, // e.g. "i386-x86_64", from nsIMacUtils.architecturesInBinary, only present for mac universal builds
@@ -177,13 +184,8 @@ Structure::
         ],
         activeExperiment: { // section is empty if there's no active experiment
             id: <string>, // id
             branch: <string>, // branch name
         },
         persona: <string>, // id of the current persona, null on GONK
       },
     }
-
-Some parts of the environment must be fetched asynchronously at startup. If a session is very short or terminates early, the following items may be missing from the environment:
-
-- profile
-- addons
--- a/toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js
+++ b/toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js
@@ -637,27 +637,30 @@ add_task(function* test_prefWatchPolicie
   // Set the Environment preferences to watch.
   TelemetryEnvironment._watchPreferences(prefsToWatch);
   let deferred = PromiseUtils.defer();
 
   // Check that the pref values are missing or present as expected
   Assert.strictEqual(TelemetryEnvironment.currentEnvironment.settings.userPrefs[PREF_TEST_1], undefined);
   Assert.strictEqual(TelemetryEnvironment.currentEnvironment.settings.userPrefs[PREF_TEST_4], expectedValue);
 
-  TelemetryEnvironment.registerChangeListener("testWatchPrefs", deferred.resolve);
+  TelemetryEnvironment.registerChangeListener("testWatchPrefs",
+    (reason, data) => deferred.resolve(data));
+  let oldEnvironmentData = TelemetryEnvironment.currentEnvironment;
 
   // Trigger a change in the watched preferences.
   Preferences.set(PREF_TEST_1, expectedValue);
   Preferences.set(PREF_TEST_2, false);
-  yield deferred.promise;
+  let eventEnvironmentData = yield deferred.promise;
 
   // Unregister the listener.
   TelemetryEnvironment.unregisterChangeListener("testWatchPrefs");
 
   // Check environment contains the correct data.
+  Assert.deepEqual(oldEnvironmentData, eventEnvironmentData);
   let userPrefs = TelemetryEnvironment.currentEnvironment.settings.userPrefs;
 
   Assert.equal(userPrefs[PREF_TEST_1], expectedValue,
                "Environment contains the correct preference value.");
   Assert.equal(userPrefs[PREF_TEST_2], "<user-set>",
                "Report that the pref was user set but the value is not shown.");
   Assert.ok(!(PREF_TEST_3 in userPrefs),
             "Do not report if preference not user set.");
--- a/toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
+++ b/toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
@@ -1146,32 +1146,32 @@ add_task(function* test_environmentChang
 
   // Trigger and collect environment-change ping.
   Preferences.set(PREF_TEST, 1);
   let request = yield gRequestIterator.next();
   Assert.ok(!!request);
   let ping = decodeRequestPayload(request);
 
   Assert.equal(ping.type, PING_TYPE_MAIN);
-  Assert.equal(ping.environment.settings.userPrefs[PREF_TEST], 1);
+  Assert.equal(ping.environment.settings.userPrefs[PREF_TEST], undefined);
   Assert.equal(ping.payload.info.reason, REASON_ENVIRONMENT_CHANGE);
   let subsessionStartDate = new Date(ping.payload.info.subsessionStartDate);
   Assert.equal(subsessionStartDate.toISOString(), nowDay.toISOString());
 
   Assert.equal(ping.payload.histograms[COUNT_ID].sum, 1);
   Assert.equal(ping.payload.keyedHistograms[KEYED_ID]["a"].sum, 1);
 
   // Trigger and collect another ping. The histograms should be reset.
   Preferences.set(PREF_TEST, 2);
   request = yield gRequestIterator.next();
   Assert.ok(!!request);
   ping = decodeRequestPayload(request);
 
   Assert.equal(ping.type, PING_TYPE_MAIN);
-  Assert.equal(ping.environment.settings.userPrefs[PREF_TEST], 2);
+  Assert.equal(ping.environment.settings.userPrefs[PREF_TEST], 1);
   Assert.equal(ping.payload.info.reason, REASON_ENVIRONMENT_CHANGE);
   subsessionStartDate = new Date(ping.payload.info.subsessionStartDate);
   Assert.equal(subsessionStartDate.toISOString(), nowDay.toISOString());
 
   Assert.equal(ping.payload.histograms[COUNT_ID].sum, 0);
   Assert.deepEqual(ping.payload.keyedHistograms[KEYED_ID], {});
 });
 
@@ -1230,17 +1230,17 @@ add_task(function* test_savedSessionData
 
   const PREF_TEST = "toolkit.telemetry.test.pref1";
   Preferences.reset(PREF_TEST);
   let prefsToWatch = {};
   prefsToWatch[PREF_TEST] = TelemetryEnvironment.RECORD_PREF_VALUE;
 
   // We expect one new subsession when starting TelemetrySession and one after triggering
   // an environment change.
-  const expectedSubsessions = sessionState.profileSubsessionCounter + 3;
+  const expectedSubsessions = sessionState.profileSubsessionCounter + 2;
   const expectedUUID = "009fd1ad-b85e-4817-b3e5-000000003785";
   fakeGenerateUUID(generateUUID, () => expectedUUID);
 
   if (gIsAndroid) {
     // We don't support subsessions yet on Android, so skip the next checks.
     return;
   }