Bug 1336360 - Use the PingSender for sending pings when the browser shuts down. r=chutten draft
authorAlessio Placitelli <alessio.placitelli@gmail.com>
Thu, 23 Feb 2017 14:58:02 +0100
changeset 553686 764894c323beec0c79b9817a39fabb13af96bf99
parent 553535 1c8b7f6c954532057ccacc761e3027760bfa8734
child 622138 0474ee3370ce6b429483c08754a2db0a879af6a8
push id51717
push useralessio.placitelli@gmail.com
push dateThu, 30 Mar 2017 13:00:05 +0000
reviewerschutten
bugs1336360
milestone55.0a1
Bug 1336360 - Use the PingSender for sending pings when the browser shuts down. r=chutten MozReview-Commit-ID: 6bNQs87UQ9m
toolkit/components/telemetry/TelemetryController.jsm
toolkit/components/telemetry/TelemetrySend.jsm
toolkit/components/telemetry/TelemetrySession.jsm
toolkit/components/telemetry/docs/internals/preferences.rst
toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
--- a/toolkit/components/telemetry/TelemetryController.jsm
+++ b/toolkit/components/telemetry/TelemetryController.jsm
@@ -192,21 +192,23 @@ this.TelemetryController = Object.freeze
    * @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=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.
+   * @param {Boolean} [aOptions.usePingSender=false] if true, send the ping using the PingSender.
    * @returns {Promise} Test-only - a promise that resolves with the ping id once the ping is stored or sent.
    */
   submitExternalPing(aType, aPayload, aOptions = {}) {
     aOptions.addClientId = aOptions.addClientId || false;
     aOptions.addEnvironment = aOptions.addEnvironment || false;
+    aOptions.usePingSender = aOptions.usePingSender || false;
 
     return Impl.submitExternalPing(aType, aPayload, aOptions);
   },
 
   /**
    * Get the current session ping data as it would be sent out or stored.
    *
    * @param {bool} aSubsession Whether to get subsession data. Optional, defaults to false.
@@ -446,16 +448,17 @@ var 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=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.
+   * @param {Boolean} [aOptions.usePingSender=false] if true, send the ping using the PingSender.
    * @returns {Promise} Test-only - a promise that is resolved with the ping id once the ping is stored or sent.
    */
   _submitPingLogic: Task.async(function* (aType, aPayload, aOptions) {
     // Make sure to have a clientId if we need one. This cover the case of submitting
     // a ping early during startup, before Telemetry is initialized, if no client id was
     // cached.
     if (!this._clientID && aOptions.addClientId) {
       Telemetry.getHistogramById("TELEMETRY_PING_SUBMISSION_WAITING_CLIENTID").add();
@@ -468,32 +471,33 @@ var Impl = {
     this._log.trace("submitExternalPing - ping assembled, id: " + pingData.id);
 
     // Always persist the pings if we are allowed to. We should not yield on any of the
     // following operations to keep this function synchronous for the majority of the calls.
     let archivePromise = TelemetryArchive.promiseArchivePing(pingData)
       .catch(e => this._log.error("submitExternalPing - Failed to archive ping " + pingData.id, e));
     let p = [ archivePromise ];
 
-    p.push(TelemetrySend.submitPing(pingData));
+    p.push(TelemetrySend.submitPing(pingData, {usePingSender: aOptions.usePingSender}));
 
     return Promise.all(p).then(() => pingData.id);
   }),
 
   /**
    * Submit ping payloads to Telemetry.
    *
    * @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=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.
+   * @param {Boolean} [aOptions.usePingSender=false] if true, send the ping using the PingSender.
    * @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;
--- a/toolkit/components/telemetry/TelemetrySend.jsm
+++ b/toolkit/components/telemetry/TelemetrySend.jsm
@@ -29,16 +29,18 @@ Cu.import("resource://gre/modules/Teleme
 Cu.import("resource://gre/modules/Timer.jsm", this);
 
 XPCOMUtils.defineLazyModuleGetter(this, "AsyncShutdown",
                                   "resource://gre/modules/AsyncShutdown.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "TelemetryStorage",
                                   "resource://gre/modules/TelemetryStorage.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "TelemetryReportingPolicy",
                                   "resource://gre/modules/TelemetryReportingPolicy.jsm");
+XPCOMUtils.defineLazyModuleGetter(this, "OS",
+                                  "resource://gre/modules/osfile.jsm");
 XPCOMUtils.defineLazyServiceGetter(this, "Telemetry",
                                    "@mozilla.org/base/telemetry;1",
                                    "nsITelemetry");
 
 const Utils = TelemetryUtils;
 
 const LOGGER_NAME = "Toolkit.Telemetry";
 const LOGGER_PREFIX = "TelemetrySend::";
@@ -208,20 +210,23 @@ this.TelemetrySend = {
   },
 
   /**
    * Submit a ping for sending. This will:
    * - send the ping right away if possible or
    * - save the ping to disk and send it at the next opportunity
    *
    * @param {Object} ping The ping data to send, must be serializable to JSON.
+   * @param {Object} [aOptions] Options object.
+   * @param {Boolean} [options.usePingSender=false] if true, send the ping using the PingSender.
    * @return {Promise} Test-only - a promise that is resolved when the ping is sent or saved.
    */
-  submitPing(ping) {
-    return TelemetrySendImpl.submitPing(ping);
+  submitPing(ping, options = {}) {
+    options.usePingSender = options.usePingSender || false;
+    return TelemetrySendImpl.submitPing(ping, options);
   },
 
   /**
    * Count of pending pings that were found to be overdue at startup.
    */
   get overduePingsCount() {
     return TelemetrySendImpl.overduePingsCount;
   },
@@ -757,24 +762,56 @@ var TelemetrySendImpl = {
   observe(subject, topic, data) {
     switch (topic) {
     case TOPIC_IDLE_DAILY:
       SendScheduler.triggerSendingPings(true);
       break;
     }
   },
 
-  submitPing(ping) {
-    this._log.trace("submitPing - ping id: " + ping.id);
+  /**
+   * Spawn the PingSender process that sends a ping. This function does
+   * not return an error or throw, it only logs an error.
+   *
+   * Even if the function doesn't fail, it doesn't mean that the ping was
+   * successfully sent, as we have no control over the spawned process. If it,
+   * succeeds, the ping is eventually removed from the disk to prevent duplicated
+   * submissions.
+   *
+   * @param {String} pingId The id of the ping to send.
+   * @param {String} submissionURL The complete Telemetry-compliant URL for the ping.
+   */
+  _sendWithPingSender(pingId, submissionURL) {
+    this._log.trace("_sendWithPingSender - sending " + pingId + " to " + submissionURL);
+    try {
+      const pingPath = OS.Path.join(TelemetryStorage.pingDirectoryPath, pingId);
+      Telemetry.runPingSender(submissionURL, pingPath);
+    } catch (e) {
+      this._log.error("_sendWithPingSender - failed to submit shutdown ping", e);
+    }
+  },
+
+  submitPing(ping, options) {
+    this._log.trace("submitPing - ping id: " + ping.id + ", options: " + JSON.stringify(options));
 
     if (!this.sendingEnabled(ping)) {
       this._log.trace("submitPing - Telemetry is not allowed to send pings.");
       return Promise.resolve();
     }
 
+    // Send the ping using the PingSender, if requested and the user was
+    // notified of our policy.
+    if (options.usePingSender && TelemetryReportingPolicy.canUpload()) {
+      const url = this._buildSubmissionURL(ping);
+      // Serialize the ping to the disk and spawn the PingSender.
+      let savePromise = savePing(ping);
+      savePromise.then(() => this._sendWithPingSender(ping.id, url));
+      return savePromise;
+    }
+
     if (!this.canSendNow) {
       // Sending is disabled or throttled, add this to the persisted pending pings.
       this._log.trace("submitPing - can't send ping now, persisting to disk - " +
                       "canSendNow: " + this.canSendNow);
       return savePing(ping);
     }
 
     // Let the scheduler trigger sending pings if possible.
@@ -924,16 +961,21 @@ var TelemetrySendImpl = {
       if (TelemetryStorage.isDeletionPing(id)) {
         return TelemetryStorage.removeDeletionPing();
       }
       return TelemetryStorage.removePendingPing(id);
     }
     return Promise.resolve();
   },
 
+  _buildSubmissionURL(ping) {
+    const version = isV4PingFormat(ping) ? PING_FORMAT_VERSION : 1;
+    return this._server + this._getSubmissionPath(ping) + "?v=" + version;
+  },
+
   _getSubmissionPath(ping) {
     // The new ping format contains an "application" section, the old one doesn't.
     let pathComponents;
     if (isV4PingFormat(ping)) {
       // We insert the Ping id in the URL to simplify server handling of duplicated
       // pings.
       let app = ping.application;
       pathComponents = [
@@ -967,19 +1009,17 @@ var TelemetrySendImpl = {
       // We can't send the pings to the server, so don't try to.
       this._log.trace("_doPing - Can't send ping " + ping.id);
       return Promise.resolve();
     }
 
     this._log.trace("_doPing - server: " + this._server + ", persisted: " + isPersisted +
                     ", id: " + id);
 
-    const isNewPing = isV4PingFormat(ping);
-    const version = isNewPing ? PING_FORMAT_VERSION : 1;
-    const url = this._server + this._getSubmissionPath(ping) + "?v=" + version;
+    const url = this._buildSubmissionURL(ping);
 
     let request = new ServiceRequest();
     request.mozBackgroundRequest = true;
     request.timeout = PING_SUBMIT_TIMEOUT_MS;
 
     request.open("POST", url, true);
     request.overrideMimeType("text/plain");
     request.setRequestHeader("Content-Type", "application/json; charset=UTF-8");
@@ -1045,17 +1085,17 @@ var TelemetrySendImpl = {
         this._log.error("_doPing - error submitting to " + url + ", status: " + status
                         + ", type: " + event.type);
       }
 
       onRequestFinished(success, event);
     };
 
     // If that's a legacy ping format, just send its payload.
-    let networkPayload = isNewPing ? ping : ping.payload;
+    let networkPayload = isV4PingFormat(ping) ? ping : ping.payload;
     request.setRequestHeader("Content-Encoding", "gzip");
     let converter = Cc["@mozilla.org/intl/scriptableunicodeconverter"]
                     .createInstance(Ci.nsIScriptableUnicodeConverter);
     converter.charset = "UTF-8";
     let startTime = new Date();
     let utf8Payload = converter.ConvertFromUnicode(JSON.stringify(networkPayload));
     utf8Payload += converter.Finish();
     Telemetry.getHistogramById("TELEMETRY_STRINGIFY").add(new Date() - startTime);
--- a/toolkit/components/telemetry/TelemetrySession.jsm
+++ b/toolkit/components/telemetry/TelemetrySession.jsm
@@ -60,16 +60,17 @@ const MIN_SUBSESSION_LENGTH_MS = Prefere
 const LOGGER_NAME = "Toolkit.Telemetry";
 const LOGGER_PREFIX = "TelemetrySession" + (Utils.isContentProcess ? "#content::" : "::");
 
 const PREF_BRANCH = "toolkit.telemetry.";
 const PREF_PREVIOUS_BUILDID = PREF_BRANCH + "previousBuildID";
 const PREF_FHR_UPLOAD_ENABLED = "datareporting.healthreport.uploadEnabled";
 const PREF_ASYNC_PLUGIN_INIT = "dom.ipc.plugins.asyncInit.enabled";
 const PREF_UNIFIED = PREF_BRANCH + "unified";
+const PREF_SHUTDOWN_PINGSENDER = PREF_BRANCH + "shutdownPingSender.enabled";
 
 const MESSAGE_TELEMETRY_PAYLOAD = "Telemetry:Payload";
 const MESSAGE_TELEMETRY_THREAD_HANGS = "Telemetry:ChildThreadHangs";
 const MESSAGE_TELEMETRY_GET_CHILD_THREAD_HANGS = "Telemetry:GetChildThreadHangs";
 const MESSAGE_TELEMETRY_USS = "Telemetry:USS";
 const MESSAGE_TELEMETRY_GET_CHILD_USS = "Telemetry:GetChildUSS";
 
 const DATAREPORTING_DIRECTORY = "datareporting";
@@ -1732,16 +1733,17 @@ var Impl = {
     let p = [];
 
     if (IS_UNIFIED_TELEMETRY) {
       let shutdownPayload = this.getSessionPayload(REASON_SHUTDOWN, false);
 
       let options = {
         addClientId: true,
         addEnvironment: true,
+        usePingSender: Preferences.get(PREF_SHUTDOWN_PINGSENDER, false),
       };
       p.push(TelemetryController.submitExternalPing(getPingType(shutdownPayload), shutdownPayload, options)
                                 .catch(e => this._log.error("saveShutdownPings - failed to submit shutdown ping", e)));
      }
 
     // As a temporary measure, we want to submit saved-session too if extended Telemetry is enabled
     // to keep existing performance analysis working.
     if (Telemetry.canRecordExtended) {
--- a/toolkit/components/telemetry/docs/internals/preferences.rst
+++ b/toolkit/components/telemetry/docs/internals/preferences.rst
@@ -45,16 +45,20 @@ Preferences
 
   This sets the Telemetry logging verbosity per ``Log.jsm``, with ``Trace`` or ``0`` being the most verbose and the default being ``Warn``.
   By default logging goes only the console service.
 
 ``toolkit.telemetry.log.dump``
 
   Sets whether to dump Telemetry log messages to ``stdout`` too.
 
+``toolkit.telemetry.shutdownPingSender.enabled``
+
+  Allow the ``shutdown`` ping to be sent when the browser shuts down, instead of the next restart, using the :doc:`ping sender <pingsender>`.
+
 Data-choices notification
 -------------------------
 
 ``toolkit.telemetry.reportingpolicy.firstRun``
 
   This preference is not present until the first run. After, its value is set to false. This is used to show the infobar with a more aggressive timeout if it wasn't shown yet.
 
 ``datareporting.policy.firstRunURL``
--- a/toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
+++ b/toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
@@ -58,16 +58,18 @@ const NUMBER_OF_THREADS_TO_LAUNCH = 30;
 var gNumberOfThreadsLaunched = 0;
 
 const MS_IN_ONE_HOUR  = 60 * 60 * 1000;
 const MS_IN_ONE_DAY   = 24 * MS_IN_ONE_HOUR;
 
 const PREF_BRANCH = "toolkit.telemetry.";
 const PREF_SERVER = PREF_BRANCH + "server";
 const PREF_FHR_UPLOAD_ENABLED = "datareporting.healthreport.uploadEnabled";
+const PREF_BYPASS_NOTIFICATION = "datareporting.policy.dataSubmissionPolicyBypassNotification";
+const PREF_SHUTDOWN_PINGSENDER = "toolkit.telemetry.shutdownPingSender.enabled";
 
 const DATAREPORTING_DIR = "datareporting";
 const ABORTED_PING_FILE_NAME = "aborted-session-ping";
 const ABORTED_SESSION_UPDATE_INTERVAL_MS = 5 * 60 * 1000;
 
 XPCOMUtils.defineLazyGetter(this, "DATAREPORTING_PATH", function() {
   return OS.Path.join(OS.Constants.Path.profileDir, DATAREPORTING_DIR);
 });
@@ -1366,16 +1368,66 @@ add_task(function* test_savedPingsOnShut
       (ping.type == PING_TYPE_SAVED_SESSION) ? REASON_SAVED_SESSION : REASON_SHUTDOWN;
 
     checkPingFormat(ping, ping.type, true, true);
     Assert.equal(ping.payload.info.reason, expectedReason);
     Assert.equal(ping.clientId, gClientID);
   }
 });
 
+add_task(function* test_sendShutdownPing() {
+  if (gIsAndroid ||
+      (AppConstants.platform == "linux" && OS.Constants.Sys.bits == 32)) {
+    // We don't support the pingsender on Android, yet, see bug 1335917.
+    // We also don't suppor the pingsender testing on Treeherder for
+    // Linux 32 bit (due to missing libraries). So skip it there too.
+    // See bug 1310703 comment 78.
+    return;
+  }
+
+  Preferences.set(PREF_SHUTDOWN_PINGSENDER, true);
+  PingServer.clearRequests();
+
+  // Shutdown telemetry and wait for an incoming ping.
+  let nextPing = PingServer.promiseNextPing();
+  yield TelemetryController.testShutdown();
+  const ping = yield nextPing;
+
+  // Check that we received a shutdown ping.
+  checkPingFormat(ping, ping.type, true, true);
+  Assert.equal(ping.payload.info.reason, REASON_SHUTDOWN);
+  Assert.equal(ping.clientId, gClientID);
+
+  // Try again, this time disable ping upload. The PingSender
+  // should not be sending any ping!
+  PingServer.registerPingHandler(() => Assert.ok(false, "Telemetry must not send pings if not allowed to."));
+  Preferences.set(PREF_FHR_UPLOAD_ENABLED, false);
+  yield TelemetryController.testReset();
+  yield TelemetryController.testShutdown();
+
+  // Make sure we have no pending pings between the runs.
+  yield TelemetryStorage.testClearPendingPings();
+
+  // Enable ping upload and disable the "submission policy".
+  // The shutdown ping must not be sent.
+  Preferences.set(PREF_FHR_UPLOAD_ENABLED, true);
+  Preferences.set(PREF_BYPASS_NOTIFICATION, false);
+  yield TelemetryController.testReset();
+  yield TelemetryController.testShutdown();
+
+
+  // Reset the pref and restart Telemetry.
+  Preferences.reset(PREF_SHUTDOWN_PINGSENDER);
+  // We cannot reset PREF_BYPASS_NOTIFICATION, as we need it to be
+  // |true| in tests.
+  Preferences.set(PREF_BYPASS_NOTIFICATION, true);
+  PingServer.resetPingHandler();
+  yield TelemetryController.testReset();
+});
+
 add_task(function* test_savedSessionData() {
   // Create the directory which will contain the data file, if it doesn't already
   // exist.
   yield OS.File.makeDir(DATAREPORTING_PATH);
   getHistogram("TELEMETRY_SESSIONDATA_FAILED_LOAD").clear();
   getHistogram("TELEMETRY_SESSIONDATA_FAILED_PARSE").clear();
   getHistogram("TELEMETRY_SESSIONDATA_FAILED_VALIDATION").clear();