Bug 1354482 - Turn on sending the "shutdown" ping using the pingsender from the second Firefox session. r?gfritzsche,r?chutten draft
authorAlessio Placitelli <alessio.placitelli@gmail.com>
Mon, 10 Apr 2017 12:51:45 +0200
changeset 562237 d8a1fb3932eeef7e7bc77c48bd55dec60118e0cd
parent 561911 819a666afddc804b6099ee1b3cff3a0fdf35ec15
child 624215 d0b28ee5a8986e109faf80c23b7b9cc90a202925
push id54000
push useralessio.placitelli@gmail.com
push dateThu, 13 Apr 2017 18:31:22 +0000
reviewersgfritzsche, chutten
bugs1354482
milestone55.0a1
Bug 1354482 - Turn on sending the "shutdown" ping using the pingsender from the second Firefox session. r?gfritzsche,r?chutten MozReview-Commit-ID: IEYfziF86mF
toolkit/components/telemetry/TelemetryReportingPolicy.jsm
toolkit/components/telemetry/TelemetrySend.jsm
toolkit/components/telemetry/TelemetrySession.jsm
toolkit/components/telemetry/docs/concepts/submission.rst
toolkit/components/telemetry/docs/data/main-ping.rst
toolkit/components/telemetry/docs/internals/preferences.rst
toolkit/components/telemetry/tests/unit/head.js
toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
--- a/toolkit/components/telemetry/TelemetryReportingPolicy.jsm
+++ b/toolkit/components/telemetry/TelemetryReportingPolicy.jsm
@@ -141,16 +141,23 @@ this.TelemetryReportingPolicy = {
    *
    * @return {Boolean} True if we are allowed to upload data, false otherwise.
    */
   canUpload() {
     return TelemetryReportingPolicyImpl.canUpload();
   },
 
   /**
+   * Check if this is the first time the browser ran.
+   */
+  isFirstRun() {
+    return TelemetryReportingPolicyImpl.isFirstRun();
+  },
+
+  /**
    * Test only method, restarts the policy.
    */
   reset() {
     return TelemetryReportingPolicyImpl.reset();
   },
 
   /**
    * Test only method, used to check if user is notified of the policy in tests.
@@ -160,24 +167,34 @@ this.TelemetryReportingPolicy = {
   },
 
   /**
    * Test only method, used to simulate the infobar being shown in xpcshell tests.
    */
   testInfobarShown() {
     return TelemetryReportingPolicyImpl._userNotified();
   },
+
+  /**
+   * Test only method, used to trigger an update of the "first run" state.
+   */
+  testUpdateFirstRun() {
+    return TelemetryReportingPolicyImpl.observe(null, "sessionstore-windows-restored", null);
+  },
 };
 
 var TelemetryReportingPolicyImpl = {
   _logger: null,
   // Keep track of the notification status if user wasn't notified already.
   _notificationInProgress: false,
   // The timer used to show the datachoices notification at startup.
   _startupNotificationTimerId: null,
+  // Keep track of the first session state, as the related preference
+  // is flipped right after the browser starts.
+  _isFirstRun: true,
 
   get _log() {
     if (!this._logger) {
       this._logger = Log.repository.getLoggerWithMessagePrefix(LOGGER_NAME, LOGGER_PREFIX);
     }
 
     return this._logger;
   },
@@ -347,16 +364,20 @@ var TelemetryReportingPolicyImpl = {
     }
 
     // Submission is enabled. We enable upload if user is notified or we need to bypass
     // the policy.
     const bypassNotification = Preferences.get(PREF_BYPASS_NOTIFICATION, false);
     return this.isUserNotifiedOfCurrentPolicy || bypassNotification;
   },
 
+  isFirstRun() {
+    return this._isFirstRun;
+  },
+
   /**
    * Migrate the data policy preferences, if needed.
    */
   _migratePreferences() {
     // Current prefs are mostly the same than the old ones, except for some deprecated ones.
     for (let pref of DEPRECATED_FHR_PREFS) {
       Preferences.reset(pref);
     }
@@ -466,31 +487,31 @@ var TelemetryReportingPolicyImpl = {
     return true;
   },
 
   observe(aSubject, aTopic, aData) {
     if (aTopic != "sessionstore-windows-restored") {
       return;
     }
 
-    const isFirstRun = Preferences.get(PREF_FIRST_RUN, true);
-    if (isFirstRun) {
+    this._isFirstRun = Preferences.get(PREF_FIRST_RUN, true);
+    if (this._isFirstRun) {
       // We're performing the first run, flip firstRun preference for subsequent runs.
       Preferences.set(PREF_FIRST_RUN, false);
 
       try {
         if (this._openFirstRunPage()) {
           return;
         }
       } catch (e) {
         this._log.error("Failed to open privacy policy tab: " + e);
       }
     }
 
     // Show the info bar.
     const delay =
-      isFirstRun ? NOTIFICATION_DELAY_FIRST_RUN_MSEC : NOTIFICATION_DELAY_NEXT_RUNS_MSEC;
+      this._isFirstRun ? NOTIFICATION_DELAY_FIRST_RUN_MSEC : NOTIFICATION_DELAY_NEXT_RUNS_MSEC;
 
     this._startupNotificationTimerId = Policy.setShowInfobarTimeout(
         // Calling |canUpload| eventually shows the infobar, if needed.
         () => this._showInfobar(), delay);
   },
 };
--- a/toolkit/components/telemetry/TelemetrySend.jsm
+++ b/toolkit/components/telemetry/TelemetrySend.jsm
@@ -12,16 +12,17 @@
 "use strict";
 
 this.EXPORTED_SYMBOLS = [
   "TelemetrySend",
 ];
 
 const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
 
+Cu.import("resource://gre/modules/AppConstants.jsm", this);
 Cu.import("resource://gre/modules/XPCOMUtils.jsm", this);
 Cu.import("resource://gre/modules/Task.jsm", this);
 Cu.import("resource://gre/modules/ClientID.jsm");
 Cu.import("resource://gre/modules/Log.jsm", this);
 Cu.import("resource://gre/modules/Preferences.jsm");
 Cu.import("resource://gre/modules/PromiseUtils.jsm");
 Cu.import("resource://gre/modules/ServiceRequest.jsm", this);
 Cu.import("resource://gre/modules/Services.jsm", this);
@@ -793,18 +794,21 @@ var TelemetrySendImpl = {
     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()) {
+    // notified of our policy. We don't support the pingsender on Android,
+    // so ignore this option on that platform (see bug 1335917).
+    if (options.usePingSender &&
+        TelemetryReportingPolicy.canUpload() &&
+        AppConstants.platform != "android") {
       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) {
--- a/toolkit/components/telemetry/TelemetrySession.jsm
+++ b/toolkit/components/telemetry/TelemetrySession.jsm
@@ -148,16 +148,18 @@ XPCOMUtils.defineLazyModuleGetter(this, 
 XPCOMUtils.defineLazyModuleGetter(this, "ThirdPartyCookieProbe",
                                   "resource://gre/modules/ThirdPartyCookieProbe.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "UITelemetry",
                                   "resource://gre/modules/UITelemetry.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "GCTelemetry",
                                   "resource://gre/modules/GCTelemetry.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "TelemetryEnvironment",
                                   "resource://gre/modules/TelemetryEnvironment.jsm");
+XPCOMUtils.defineLazyModuleGetter(this, "TelemetryReportingPolicy",
+                                  "resource://gre/modules/TelemetryReportingPolicy.jsm");
 
 function generateUUID() {
   let str = Cc["@mozilla.org/uuid-generator;1"].getService(Ci.nsIUUIDGenerator).generateUUID().toString();
   // strip {}
   return str.substring(1, str.length - 1);
 }
 
 function getMsSinceProcessStart() {
@@ -1787,20 +1789,25 @@ var Impl = {
     // We don't wait for "shutdown" pings to be written to disk before gathering the
     // "saved-session" payload. Instead we append the promises to this list and wait
     // on both to be saved after kicking off their collection.
     let p = [];
 
     if (IS_UNIFIED_TELEMETRY) {
       let shutdownPayload = this.getSessionPayload(REASON_SHUTDOWN, false);
 
+      // Only send the shutdown ping using the pingsender from the second
+      // browsing session on, to mitigate issues with "bot" profiles (see bug 1354482).
+      let sendWithPingsender = Preferences.get(PREF_SHUTDOWN_PINGSENDER, true) &&
+                               !TelemetryReportingPolicy.isFirstRun();
+
       let options = {
         addClientId: true,
         addEnvironment: true,
-        usePingSender: Preferences.get(PREF_SHUTDOWN_PINGSENDER, false),
+        usePingSender: sendWithPingsender,
       };
       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/concepts/submission.rst
+++ b/toolkit/components/telemetry/docs/concepts/submission.rst
@@ -4,31 +4,39 @@ Submission
 
 *Note:* The server-side behaviour is documented in the `HTTP Edge Server specification <https://wiki.mozilla.org/CloudServices/DataPipeline/HTTPEdgeServerSpecification>`_.
 
 Pings are submitted via a common API on ``TelemetryController``.
 If a ping fails to successfully submit to the server immediately (e.g. because
 of missing internet connection), Telemetry will store it on disk and retry to
 send it until the maximum ping age is exceeded (14 days).
 
-*Note:* the :doc:`main pings <../data/main-ping>` are kept locally even after successful submission to enable the HealthReport and SelfSupport features. They will be deleted after their retention period of 180 days.
+.. note::
+
+  The :doc:`main pings <../data/main-ping>` are kept locally even after successful submission to enable the HealthReport and SelfSupport features. They will be deleted after their retention period of 180 days.
 
 Submission logic
 ================
 
 Sending of pending pings starts as soon as the delayed startup is finished. They are sent in batches, newest-first, with up
 to 10 persisted pings per batch plus all unpersisted pings.
 The send logic then waits for each batch to complete.
 
 If it succeeds we trigger the next send of a ping batch. This is delayed as needed to only trigger one batch send per minute.
 
 If ping sending encounters an error that means retrying later, a backoff timeout behavior is
 triggered, exponentially increasing the timeout for the next try from 1 minute up to a limit of 120 minutes.
 Any new ping submissions and "idle-daily" events reset this behavior as a safety mechanism and trigger immediate ping sending.
 
+Pingsender
+==========
+Some pings (e.g. :doc:`crash pings <../data/crash-ping>` and :doc:`main pings <../data/main-ping>` with reason `shutdown`) are submitted using the :doc:`../internals/pingsender`.
+
+The pingsender tries to send each ping once and, if it fails, no additional attempt is performed: ``TelemetrySend`` will take care of retrying using the previously described submission logic.
+
 Status codes
 ============
 
 The telemetry server team is working towards `the common services status codes <https://wiki.mozilla.org/CloudServices/DataPipeline/HTTPEdgeServerSpecification#Server_Responses>`_, but for now the following logic is sufficient for Telemetry:
 
 * `2XX` - success, don't resubmit
 * `4XX` - there was some problem with the request - the client should not try to resubmit as it would just receive the same response
 * `5XX` - there was a server-side error, the client should try to resubmit later
--- a/toolkit/components/telemetry/docs/data/main-ping.rst
+++ b/toolkit/components/telemetry/docs/data/main-ping.rst
@@ -7,25 +7,27 @@
 
 This is the "main" Telemetry ping type, whose payload contains most of the measurements that are used to track the performance and health of Firefox in the wild.
 It includes the histograms and other performance and diagnostic data.
 
 This ping is triggered by different scenarios, which is documented by the ``reason`` field:
 
 * ``aborted-session`` - this ping is regularly saved to disk (every 5 minutes), overwriting itself, and deleted at shutdown. If a previous aborted session ping is found at startup, it gets sent to the server. The first aborted-session ping is generated as soon as Telemetry starts
 * ``environment-change`` - the :doc:`environment` changed, so the session measurements got reset and a new subsession starts
-* ``shutdown`` - triggered when the browser session ends
+* ``shutdown`` - triggered when the browser session ends. For the first browsing session, this ping is saved to disk and sent on the next browser restart. From the second browsing session on, this ping is sent immediately on shutdown using the :doc:`../internals/pingsender`
 * ``daily`` - a session split triggered in 24h hour intervals at local midnight. If an ``environment-change`` ping is generated by the time it should be sent, the daily ping is rescheduled for the next midnight
 * ``saved-session`` - the *"classic"* Telemetry payload with measurements covering the whole browser session (only submitted for a transition period)
 
 Most reasons lead to a session split, initiating a new *subsession*. We reset important measurements for those subsessions.
 
 After a new subsession split, the ``internal-telemetry-after-subsession-split`` topic is notified to all the observers. *This is an internal topic and is only meant for internal Telemetry usage.*
 
-*Note:* ``saved-session`` is sent with a different ping type (``saved-session``, not ``main``), but otherwise has the same format as discussed here.
+.. note::
+
+  ``saved-session`` is sent with a different ping type (``saved-session``, not ``main``), but otherwise has the same format as discussed here.
 
 Structure:
 
 .. code-block:: js
 
     {
       version: 4,
 
--- a/toolkit/components/telemetry/docs/internals/preferences.rst
+++ b/toolkit/components/telemetry/docs/internals/preferences.rst
@@ -47,17 +47,17 @@ Preferences
   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>`.
+  Allow the ``shutdown`` ping to be sent when the browser shuts down, from the second browsing session on, 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.
 
--- a/toolkit/components/telemetry/tests/unit/head.js
+++ b/toolkit/components/telemetry/tests/unit/head.js
@@ -303,16 +303,21 @@ if (runningInParent) {
   // Set logging preferences for all the tests.
   Services.prefs.setCharPref("toolkit.telemetry.log.level", "Trace");
   // Telemetry archiving should be on.
   Services.prefs.setBoolPref("toolkit.telemetry.archive.enabled", true);
   // Telemetry xpcshell tests cannot show the infobar.
   Services.prefs.setBoolPref("datareporting.policy.dataSubmissionPolicyBypassNotification", true);
   // FHR uploads should be enabled.
   Services.prefs.setBoolPref("datareporting.healthreport.uploadEnabled", true);
+  // Many tests expect the shutdown ping to not be sent on shutdown and will fail
+  // if receive an unexpected ping. Let's globally disable the shutdown ping sender:
+  // the relevant tests will enable this pref when needed.
+  Services.prefs.setBoolPref("toolkit.telemetry.shutdownPingSender.enabled", false);
+
 
   fakePingSendTimer((callback, timeout) => {
     Services.tm.mainThread.dispatch(() => callback(), Ci.nsIThread.DISPATCH_NORMAL);
   },
   () => {});
 
   // This gets imported via fakeNow();
   /* global TelemetrySend */
--- a/toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
+++ b/toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
@@ -14,16 +14,17 @@ Cu.import("resource://gre/modules/Servic
 Cu.import("resource://gre/modules/LightweightThemeManager.jsm", this);
 Cu.import("resource://gre/modules/XPCOMUtils.jsm", this);
 Cu.import("resource://gre/modules/TelemetryController.jsm", this);
 Cu.import("resource://gre/modules/TelemetrySession.jsm", this);
 Cu.import("resource://gre/modules/TelemetryStorage.jsm", this);
 Cu.import("resource://gre/modules/TelemetryEnvironment.jsm", this);
 Cu.import("resource://gre/modules/TelemetrySend.jsm", this);
 Cu.import("resource://gre/modules/TelemetryUtils.jsm", this);
+Cu.import("resource://gre/modules/TelemetryReportingPolicy.jsm", this);
 Cu.import("resource://gre/modules/Task.jsm", this);
 Cu.import("resource://gre/modules/Promise.jsm", this);
 Cu.import("resource://gre/modules/Preferences.jsm");
 Cu.import("resource://gre/modules/osfile.jsm", this);
 
 const PING_FORMAT_VERSION = 4;
 const PING_TYPE_MAIN = "main";
 const PING_TYPE_SAVED_SESSION = "saved-session";
@@ -60,16 +61,17 @@ 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 PREF_POLICY_FIRSTRUN = "toolkit.telemetry.reportingpolicy.firstRun";
 
 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);
 });
@@ -1372,16 +1374,19 @@ add_task(function* test_sendShutdownPing
     // 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);
+  Preferences.set(PREF_POLICY_FIRSTRUN, false);
+  // Make sure the reporting policy picks up the updated pref.
+  TelemetryReportingPolicy.testUpdateFirstRun();
   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.
@@ -1401,22 +1406,36 @@ add_task(function* test_sendShutdownPing
 
   // 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();
 
+  // Make sure we have no pending pings between the runs.
+  yield TelemetryStorage.testClearPendingPings();
 
-  // 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);
+
+  // With both upload enabled and the policy shown, make sure we don't
+  // send the shutdown ping using the pingsender on the first
+  // subsession.
+  Preferences.set(PREF_POLICY_FIRSTRUN, true);
+  // Make sure the reporting policy picks up the updated pref.
+  TelemetryReportingPolicy.testUpdateFirstRun();
+
+  yield TelemetryController.testReset();
+  yield TelemetryController.testShutdown();
+
+  // Reset the pref and restart Telemetry.
+  Preferences.set(PREF_SHUTDOWN_PINGSENDER, false);
+  Preferences.reset(PREF_POLICY_FIRSTRUN);
   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);