Bug 1380256 - Implement the update ping with reason "success". r?chutten,mhowell,rweiss draft
authorAlessio Placitelli <alessio.placitelli@gmail.com>
Mon, 21 Aug 2017 11:50:59 +0200
changeset 657194 c659fd999170fedb2f6954ba2fe6b86feabb76bb
parent 657025 14eea6bedcf3e2f46ea7c908e1ac9b7d256a42f0
child 729377 6d9fabea6032047027c1e0f6e6ed1f6464ab9bfa
push id77469
push useralessio.placitelli@gmail.com
push dateFri, 01 Sep 2017 06:54:21 +0000
reviewerschutten, mhowell, rweiss
bugs1380256
milestone57.0a1
Bug 1380256 - Implement the update ping with reason "success". r?chutten,mhowell,rweiss This patch enables sending the "update" ping with reason "success" after the browser is restarted when an update is successfully applied. MozReview-Commit-ID: 8LYxhTTrs7l
browser/components/nsBrowserContentHandler.js
toolkit/components/telemetry/UpdatePing.jsm
toolkit/components/telemetry/docs/data/update-ping.rst
toolkit/components/telemetry/tests/browser/browser.ini
toolkit/components/telemetry/tests/browser/browser_UpdatePingSuccess.js
--- a/browser/components/nsBrowserContentHandler.js
+++ b/browser/components/nsBrowserContentHandler.js
@@ -11,16 +11,18 @@ XPCOMUtils.defineLazyModuleGetter(this, 
 XPCOMUtils.defineLazyModuleGetter(this, "PrivateBrowsingUtils",
                                   "resource://gre/modules/PrivateBrowsingUtils.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "RecentWindow",
                                   "resource:///modules/RecentWindow.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "ShellService",
                                   "resource:///modules/ShellService.jsm");
 XPCOMUtils.defineLazyServiceGetter(this, "WindowsUIUtils",
                                    "@mozilla.org/windows-ui-utils;1", "nsIWindowsUIUtils");
+XPCOMUtils.defineLazyModuleGetter(this, "UpdatePing",
+                                  "resource://gre/modules/UpdatePing.jsm");
 
 const nsISupports            = Components.interfaces.nsISupports;
 
 const nsIBrowserDOMWindow    = Components.interfaces.nsIBrowserDOMWindow;
 const nsIBrowserHandler      = Components.interfaces.nsIBrowserHandler;
 const nsIBrowserHistory      = Components.interfaces.nsIBrowserHistory;
 const nsIChannel             = Components.interfaces.nsIChannel;
 const nsICommandLine         = Components.interfaces.nsICommandLine;
@@ -486,16 +488,17 @@ nsBrowserContentHandler.prototype = {
     var willRestoreSession = false;
     try {
       // Read the old value of homepage_override.mstone before
       // needHomepageOverride updates it, so that we can later add it to the
       // URL if we do end up showing an overridePage. This makes it possible
       // to have the overridePage's content vary depending on the version we're
       // upgrading from.
       let old_mstone = Services.prefs.getCharPref("browser.startup.homepage_override.mstone", "unknown");
+      let old_buildId = Services.prefs.getCharPref("browser.startup.homepage_override.buildID", "unknown");
       override = needHomepageOverride(prefb);
       if (override != OVERRIDE_NONE) {
         switch (override) {
           case OVERRIDE_NEW_PROFILE:
             // New profile.
             overridePage = Services.urlFormatter.formatURLPref("startup.homepage_welcome_url");
             additionalPage = Services.urlFormatter.formatURLPref("startup.homepage_welcome_url.additional");
             // Turn on 'later run' pages for new profiles.
@@ -507,21 +510,30 @@ nsBrowserContentHandler.prototype = {
             // into account because that requires waiting for the session file
             // to be read. If a crash occurs after updating, before restarting,
             // we may open the startPage in addition to restoring the session.
             var ss = Components.classes["@mozilla.org/browser/sessionstartup;1"]
                                .getService(Components.interfaces.nsISessionStartup);
             willRestoreSession = ss.isAutomaticRestoreEnabled();
 
             overridePage = Services.urlFormatter.formatURLPref("startup.homepage_override_url");
-            if (prefb.prefHasUserValue("app.update.postupdate"))
+            if (prefb.prefHasUserValue("app.update.postupdate")) {
               overridePage = getPostUpdateOverridePage(overridePage);
+              // Send the update ping to signal that the update was successful.
+              UpdatePing.handleUpdateSuccess(old_mstone, old_buildId);
+            }
 
             overridePage = overridePage.replace("%OLD_VERSION%", old_mstone);
             break;
+          case OVERRIDE_NEW_BUILD_ID:
+            if (prefb.prefHasUserValue("app.update.postupdate")) {
+              // Send the update ping to signal that the update was successful.
+              UpdatePing.handleUpdateSuccess(old_mstone, old_buildId);
+            }
+            break;
         }
       }
     } catch (ex) {}
 
     // formatURLPref might return "about:blank" if getting the pref fails
     if (overridePage == "about:blank")
       overridePage = "";
 
--- a/toolkit/components/telemetry/UpdatePing.jsm
+++ b/toolkit/components/telemetry/UpdatePing.jsm
@@ -36,16 +36,68 @@ this.UpdatePing = {
     if (!this._enabled) {
       return;
     }
 
     Services.obs.addObserver(this, UPDATE_DOWNLOADED_TOPIC);
   },
 
   /**
+   * Get the information about the update we're going to apply/was just applied
+   * from the update manager.
+   *
+   * @return {nsIUpdate} The information about the update, if available, or null.
+   */
+  _getActiveUpdate() {
+    let updateManager =
+      Cc["@mozilla.org/updates/update-manager;1"].getService(Ci.nsIUpdateManager);
+    if (!updateManager || !updateManager.activeUpdate) {
+      return null;
+    }
+
+    return updateManager.activeUpdate;
+  },
+
+  /**
+   * Generate an "update" ping with reason "success" and dispatch it
+   * to the Telemetry system.
+   *
+   * @param {String} aPreviousVersion The browser version we updated from.
+   * @param {String} aPreviousBuildId The browser build id we updated from.
+   */
+  handleUpdateSuccess(aPreviousVersion, aPreviousBuildId) {
+    this._log.trace("handleUpdateSuccess");
+
+    // An update could potentially change the update channel. Moreover,
+    // updates can only be applied if the update's channel matches with the build channel.
+    // There's no way to pass this information from the caller nor the environment as,
+    // in that case, the environment would report the "new" channel. However, the
+    // update manager should still have information about the active update: given the
+    // previous assumptions, we can simply get the channel from the update and assume
+    // it matches with the state previous to the update.
+    let update = this._getActiveUpdate();
+
+    const payload = {
+      reason: "success",
+      previousChannel: update ? update.channel : null,
+      previousVersion: aPreviousVersion,
+      previousBuildId: aPreviousBuildId,
+    };
+
+    const options = {
+      addClientId: true,
+      addEnvironment: true,
+      usePingSender: false,
+    };
+
+    TelemetryController.submitExternalPing(PING_TYPE, payload, options)
+                       .catch(e => this._log.error("handleUpdateSuccess - failed to submit update ping", e));
+  },
+
+  /**
    * Generate an "update" ping with reason "ready" and dispatch it
    * to the Telemetry system.
    *
    * @param {String} aUpdateState The state of the downloaded patch. See
    *        nsIUpdateService.idl for a list of possible values.
    */
   _handleUpdateReady(aUpdateState) {
     const ALLOWED_STATES = [
@@ -53,25 +105,22 @@ this.UpdatePing = {
     ];
     if (!ALLOWED_STATES.includes(aUpdateState)) {
       this._log.trace("Unexpected update state: " + aUpdateState);
       return;
     }
 
     // Get the information about the update we're going to apply from the
     // update manager.
-    let updateManager =
-      Cc["@mozilla.org/updates/update-manager;1"].getService(Ci.nsIUpdateManager);
-    if (!updateManager || !updateManager.activeUpdate) {
+    let update = this._getActiveUpdate();
+    if (!update) {
       this._log.trace("Cannot get the update manager or no update is currently active.");
       return;
     }
 
-    let update = updateManager.activeUpdate;
-
     const payload = {
       reason: "ready",
       targetChannel: update.channel,
       targetVersion: update.appVersion,
       targetBuildId: update.buildID,
     };
 
     const options = {
--- a/toolkit/components/telemetry/docs/data/update-ping.rst
+++ b/toolkit/components/telemetry/docs/data/update-ping.rst
@@ -1,50 +1,66 @@
 
 "update" ping
 ==================
 
-This opt-out ping is sent from Firefox Desktop when a browser update is ready to be applied. There is a
-plan to send this ping after an update is successfully applied and the work will happen in `bug 1380256 <https://bugzilla.mozilla.org/show_bug.cgi?id=1380256>`_.
+This opt-out ping is sent from Firefox Desktop when a browser update is ready to be applied and after it was correctly applied.
 
 Structure:
 
 .. code-block:: js
 
     {
       type: "update",
       ... common ping data
       clientId: <UUID>,
       environment: { ... },
       payload: {
-        reason: <string>, // "ready"
-        targetChannel: <string>, // "nightly"
-        targetVersion: <string>, // "56.01a"
-        targetBuildId: <string>, // "20080811053724"
+        reason: <string>, // "ready", "success"
+        targetChannel: <string>, // "nightly" (only present for reason = "ready")
+        targetVersion: <string>, // "56.01a" (only present for reason = "ready")
+        targetBuildId: <string>, // "20080811053724" (only present for reason = "ready")
+        previousChannel: <string>, // "nightly" or null (only present for reason = "success")
+        previousVersion: <string>, // "55.01a" (only present for reason = "success")
+        previousBuildId: <string>, // "20080810053724" (only present for reason = "success")
       }
     }
 
 payload.reason
 --------------
-This field only supports the value ``ready``, meaning that the ping was generated after an update was downloaded
-and marked as ready to be processed. For *non-staged* updates this happens as soon as the download
-finishes and is verified while for *staged* updates this happens before the staging step is started.
+This field supports the following values:
+
+- ``ready`` meaning that the ping was generated after an update was downloaded and marked as ready to be processed. For *non-staged* updates this happens as soon as the download finishes and is verified while for *staged* updates this happens before the staging step is started.
+- ``ready`` the ping was generated after the browser was restarted and the update correctly applied.
 
 payload.targetChannel
 -----------------------
 The Firefox channel the update was fetched from (only valid for pings with reason "ready").
 
 payload.targetVersion
 -----------------------
 The Firefox version the browser is updating to. Follows the same format a application.version (only valid for pings with reason "ready").
 
 payload.targetBuildId
 -----------------------
 The Firefox build id the browser is updating to. Follows the same format a application.buildId (only valid for pings with reason "ready").
 
+payload.previousChannel
+-----------------------
+The Firefox channel the profile was on before the update was applied (only valid for pings with reason "success").
+This can be ``null``.
+
+payload.previousVersion
+-----------------------
+The Firefox version the browser is updating from. Follows the same format a application.version (only valid for pings with reason "success").
+
+payload.previousBuildId
+-----------------------
+The Firefox build id the browser is updating from. Follows the same format a application.buildId (only valid for pings with reason "success").
+
 Expected behaviours
 -------------------
 The following is a list of conditions and expected behaviours for the ``update`` ping:
 
 - **The ping is generated once every time an update is downloaded, after it was verified:**
 
   - *for users who saw the privacy policy*, the ``update`` ping is sent immediately;
   - *for users who did not see the privacy policy*, the ``update`` ping is saved to disk and after the policy is displayed.
--- a/toolkit/components/telemetry/tests/browser/browser.ini
+++ b/toolkit/components/telemetry/tests/browser/browser.ini
@@ -1,5 +1,6 @@
 # This Source Code Form is subject to the terms of the Mozilla Public
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 [browser_TelemetryGC.js]
+[browser_UpdatePingSuccess.js]
new file mode 100644
--- /dev/null
+++ b/toolkit/components/telemetry/tests/browser/browser_UpdatePingSuccess.js
@@ -0,0 +1,61 @@
+/* Any copyright is dedicated to the Public Domain.
+   http://creativecommons.org/publicdomain/zero/1.0/
+*/
+
+"use strict";
+
+Cu.import("resource://testing-common/TelemetryArchiveTesting.jsm", this);
+
+add_task(async function test_updatePing() {
+  const TEST_VERSION = "37.85";
+  const TEST_BUILDID = "20150711123724";
+
+  // Set the preferences needed for the test: they will be cleared up
+  // after it runs.
+  await SpecialPowers.pushPrefEnv({"set": [
+    ["app.update.postupdate", true],
+    ["browser.startup.homepage_override.mstone", TEST_VERSION],
+    ["browser.startup.homepage_override.buildID", TEST_BUILDID],
+    ["toolkit.telemetry.log.level", "Trace"]
+  ]});
+
+  // Start monitoring the ping archive.
+  let archiveChecker = new TelemetryArchiveTesting.Checker();
+  await archiveChecker.promiseInit();
+
+  // Manually call the BrowserContentHandler: this automatically gets called when
+  // the browser is started and an update was applied successfully in order to
+  // display the "update" info page.
+  Cc["@mozilla.org/browser/clh;1"].getService(Ci.nsIBrowserHandler).defaultArgs;
+
+  // We cannot control when the ping will be generated/archived after we trigger
+  // an update, so let's make sure to have one before moving on with validation.
+  let updatePing;
+  await BrowserTestUtils.waitForCondition(async function() {
+    // Check that the ping made it into the Telemetry archive.
+    // The test data is defined in ../data/sharedUpdateXML.js
+    updatePing = await archiveChecker.promiseFindPing("update", [
+        [["payload", "reason"], "success"],
+        [["payload", "previousBuildId"], TEST_BUILDID],
+        [["payload", "previousVersion"], TEST_VERSION]
+      ]);
+    return !!updatePing;
+  }, "Make sure the ping is generated before trying to validate it.", 500, 100);
+
+  ok(updatePing, "The 'update' ping must be correctly sent.");
+
+  // We have no easy way to simulate a previously applied update from toolkit/telemetry.
+  // Instead of moving this test to mozapps/update as well, just test that the
+  // "previousChannel" field is present and either a string or null.
+  ok("previousChannel" in updatePing.payload,
+     "The payload must contain the 'previousChannel' field");
+  const channelField = updatePing.payload.previousChannel;
+  if (channelField != null) {
+    ok(typeof(channelField) == "string", "'previousChannel' must be a string, if available.");
+  }
+
+  // Also make sure that the ping contains both a client id and an
+  // environment section.
+  ok("clientId" in updatePing, "The update ping must report a client id.");
+  ok("environment" in updatePing, "The update ping must report the environment.");
+});