Bug 1381490 - Enable sending the shutdown ping with the pingsender on the first session. r=gfritzsche
authorAlessio Placitelli <alessio.placitelli@gmail.com>
Wed, 26 Jul 2017 05:55:00 +0200
changeset 371307 34617f86887ebb1fa5cba44d6e339c02ffec785f
parent 371306 5955de904e6a751fe9522b6ed0007755fadd667f
child 371308 dac259c934e50ef03405d93eb329c37fd7c32819
push id32241
push usercbook@mozilla.com
push dateThu, 27 Jul 2017 08:57:54 +0000
treeherdermozilla-central@e5693cea1ec9 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgfritzsche
bugs1381490
milestone56.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 1381490 - Enable sending the shutdown ping with the pingsender on the first session. r=gfritzsche This will land behind a preference, initially turned off, with the possibility to easily enable it. MozReview-Commit-ID: InND8Pv35I1
browser/app/profile/firefox.js
testing/profiles/prefs_general.js
toolkit/components/telemetry/TelemetrySession.jsm
toolkit/components/telemetry/TelemetryUtils.jsm
toolkit/components/telemetry/docs/internals/preferences.rst
toolkit/components/telemetry/tests/unit/head.js
toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
--- a/browser/app/profile/firefox.js
+++ b/browser/app/profile/firefox.js
@@ -1509,16 +1509,18 @@ pref("browser.translation.ui.show", fals
 // Allows to define the translation engine. Bing is default, Yandex may optionally switched on.
 pref("browser.translation.engine", "bing");
 
 // Telemetry settings.
 // Determines if Telemetry pings can be archived locally.
 pref("toolkit.telemetry.archive.enabled", true);
 // Enables sending the shutdown ping when Firefox shuts down.
 pref("toolkit.telemetry.shutdownPingSender.enabled", true);
+// Enables sending the shutdown ping using the pingsender from the first session.
+pref("toolkit.telemetry.shutdownPingSender.enabledFirstSession", false);
 // Enables sending the 'new-profile' ping on new profiles.
 pref("toolkit.telemetry.newProfilePing.enabled", true);
 
 // Telemetry experiments settings.
 pref("experiments.enabled", true);
 pref("experiments.manifest.fetchIntervalSeconds", 86400);
 pref("experiments.manifest.uri", "https://telemetry-experiment.cdn.mozilla.net/manifest/v1/firefox/%VERSION%/%CHANNEL%");
 // Whether experiments are supported by the current application profile.
--- a/testing/profiles/prefs_general.js
+++ b/testing/profiles/prefs_general.js
@@ -246,19 +246,25 @@ user_pref("browser.contentHandlers.types
 user_pref("browser.contentHandlers.types.1.uri", "http://test1.example.org/rss?url=%%s")
 user_pref("browser.contentHandlers.types.2.uri", "http://test1.example.org/rss?url=%%s")
 user_pref("browser.contentHandlers.types.3.uri", "http://test1.example.org/rss?url=%%s")
 user_pref("browser.contentHandlers.types.4.uri", "http://test1.example.org/rss?url=%%s")
 user_pref("browser.contentHandlers.types.5.uri", "http://test1.example.org/rss?url=%%s")
 
 // We want to collect telemetry, but we don't want to send in the results.
 user_pref("toolkit.telemetry.server", "https://%(server)s/telemetry-dummy/");
-// Don't new-profile' ping on new profiles during tests, otherwise the testing framework
+// Don't send 'new-profile' ping on new profiles during tests, otherwise the testing framework
 // might wait on the pingsender to finish and slow down tests.
 user_pref("toolkit.telemetry.newProfilePing.enabled", false);
+// Don't send the 'shutdown' ping using the pingsender on the first session using
+// the 'pingsender' process. Valgrind marks the process as leaky (e.g. see bug 1364068
+// for the 'new-profile' ping) but does not provide enough information
+// to suppress the leak. Running locally does not reproduce the issue,
+// so disable this until we rewrite the pingsender in Rust (bug 1339035).
+user_pref("toolkit.telemetry.shutdownPingSender.enabledFirstSession", false);
 
 // A couple of preferences with default values to test that telemetry preference
 // watching is working.
 user_pref("toolkit.telemetry.test.pref1", true);
 user_pref("toolkit.telemetry.test.pref2", false);
 
 // We don't want to hit the real Firefox Accounts server for tests.  We don't
 // actually need a functioning FxA server, so just set it to something that
--- a/toolkit/components/telemetry/TelemetrySession.jsm
+++ b/toolkit/components/telemetry/TelemetrySession.jsm
@@ -1795,20 +1795,21 @@ var Impl = {
     // 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).
-      // Note: sending the "shutdown" ping using the pingsender is currently disabled
-      // due to a crash happening on OSX platforms. See bug 1357745 for context.
+      const sendOnThisSession =
+        Preferences.get(Utils.Preferences.ShutdownPingSenderFirstSession, false) ||
+        !TelemetryReportingPolicy.isFirstRun();
       let sendWithPingsender = Preferences.get(TelemetryUtils.Preferences.ShutdownPingSender, false) &&
-                               !TelemetryReportingPolicy.isFirstRun();
+                               sendOnThisSession;
 
       let options = {
         addClientId: true,
         addEnvironment: true,
         usePingSender: sendWithPingsender,
       };
       p.push(TelemetryController.submitExternalPing(getPingType(shutdownPayload), shutdownPayload, options)
                                 .catch(e => this._log.error("saveShutdownPings - failed to submit shutdown ping", e)));
--- a/toolkit/components/telemetry/TelemetryUtils.jsm
+++ b/toolkit/components/telemetry/TelemetryUtils.jsm
@@ -27,16 +27,17 @@ this.TelemetryUtils = {
   Preferences: Object.freeze({
     // General Preferences
     ArchiveEnabled: "toolkit.telemetry.archive.enabled",
     CachedClientId: "toolkit.telemetry.cachedClientID",
     FirstRun: "toolkit.telemetry.reportingpolicy.firstRun",
     OverrideOfficialCheck: "toolkit.telemetry.send.overrideOfficialCheck",
     Server: "toolkit.telemetry.server",
     ShutdownPingSender: "toolkit.telemetry.shutdownPingSender.enabled",
+    ShutdownPingSenderFirstSession: "toolkit.telemetry.shutdownPingSender.enabledFirstSession",
     TelemetryEnabled: "toolkit.telemetry.enabled",
     Unified: "toolkit.telemetry.unified",
     NewProfilePingEnabled: "toolkit.telemetry.newProfilePing.enabled",
     NewProfilePingDelay: "toolkit.telemetry.newProfilePing.delay",
     PreviousBuildID: "toolkit.telemetry.previousBuildID",
 
     // Log Preferences
     LogLevel: "toolkit.telemetry.log.level",
--- a/toolkit/components/telemetry/docs/internals/preferences.rst
+++ b/toolkit/components/telemetry/docs/internals/preferences.rst
@@ -49,16 +49,20 @@ Preferences
 ``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, from the second browsing session on, instead of the next restart, using the :doc:`ping sender <pingsender>`.
 
+``toolkit.telemetry.shutdownPingSender.enabledFirstSession``
+
+  Allow the ``shutdown`` ping to be sent using the :doc:`ping sender <pingsender>` from the first browsing session.
+
 ``toolkit.telemetry.newProfilePing.enabled``
 
   Enable the :doc:`../data/new-profile` ping on new profiles.
 
 ``toolkit.telemetry.newProfilePing.delay``
 
   Controls the delay after which the :doc:`../data/new-profile` is sent on new profiles.
 
--- a/toolkit/components/telemetry/tests/unit/head.js
+++ b/toolkit/components/telemetry/tests/unit/head.js
@@ -309,16 +309,17 @@ if (runningInParent) {
   // Telemetry xpcshell tests cannot show the infobar.
   Services.prefs.setBoolPref(TelemetryUtils.Preferences.BypassNotification, true);
   // FHR uploads should be enabled.
   Services.prefs.setBoolPref(TelemetryUtils.Preferences.FhrUploadEnabled, true);
   // Many tests expect the shutdown and the new-profile to not be sent on shutdown
   // and will fail if receive an unexpected ping. Let's globally disable these features:
   // the relevant tests will enable these prefs when needed.
   Services.prefs.setBoolPref(TelemetryUtils.Preferences.ShutdownPingSender, false);
+  Services.prefs.setBoolPref(TelemetryUtils.Preferences.ShutdownPingSenderFirstSession, false);
   Services.prefs.setBoolPref("toolkit.telemetry.newProfilePing.enabled", false);
   // Ensure browser experiments are also disabled, to avoid network activity
   // when toggling PREF_ENABLED.
   Services.prefs.setBoolPref("experiments.enabled", false);
 
 
   fakePingSendTimer((callback, timeout) => {
     Services.tm.dispatchToMainThread(() => callback());
--- a/toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
+++ b/toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
@@ -1403,17 +1403,17 @@ add_task(async function test_sendShutdow
   // Make sure the reporting policy picks up the updated pref.
   TelemetryReportingPolicy.testUpdateFirstRun();
   PingServer.clearRequests();
   Telemetry.clearScalars();
 
   // Shutdown telemetry and wait for an incoming ping.
   let nextPing = PingServer.promiseNextPing();
   await TelemetryController.testShutdown();
-  const ping = await nextPing;
+  let ping = await 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);
   Assert.ok(!(OSSHUTDOWN_SCALAR in ping.payload.processes.parent.scalars),
             "The OS shutdown scalar must not be set.");
   // Try again, this time disable ping upload. The PingSender
@@ -1465,18 +1465,40 @@ add_task(async function test_sendShutdow
   // subsession.
   Preferences.set(TelemetryUtils.Preferences.FirstRun, true);
   // Make sure the reporting policy picks up the updated pref.
   TelemetryReportingPolicy.testUpdateFirstRun();
 
   await TelemetryController.testReset();
   await TelemetryController.testShutdown();
 
+  // Clear the state and prepare for the next test.
+  await TelemetryStorage.testClearPendingPings();
+  PingServer.clearRequests();
+  PingServer.resetPingHandler();
+
+  // Check that we're able to send the shutdown ping using the pingsender
+  // from the first session if the related pref is on.
+  Preferences.set(TelemetryUtils.Preferences.ShutdownPingSenderFirstSession, true);
+  Preferences.set(TelemetryUtils.Preferences.FirstRun, true);
+  TelemetryReportingPolicy.testUpdateFirstRun();
+
+  // Restart/shutdown telemetry and wait for an incoming ping.
+  await TelemetryController.testReset();
+  await TelemetryController.testShutdown();
+  ping = await PingServer.promiseNextPing();
+
+  // 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);
+
   // Reset the pref and restart Telemetry.
   Preferences.set(TelemetryUtils.Preferences.ShutdownPingSender, false);
+  Preferences.set(TelemetryUtils.Preferences.ShutdownPingSenderFirstSession, false);
   Preferences.reset(TelemetryUtils.Preferences.FirstRun);
   PingServer.resetPingHandler();
 });
 
 add_task(async function test_savedSessionData() {
   // Create the directory which will contain the data file, if it doesn't already
   // exist.
   await OS.File.makeDir(DATAREPORTING_PATH);