Bug 1341349 - Enable sending the crash ping from the crashreporter client only when the overall configuration would allow pings to be sent r?gfritzsche
The crashreporter client will send a crash ping autonomously only when it
finds a valid TelemetryServerUrl annotation. This patch makes this annotation
conditional on all the preferences that regulate sending pings (including
official telemetry flags) and prevents it from sending pings before the user
has been notified of the privacy policy.
This is achieved by adding a new _annotateCrashReport() method to the
TelemetrySend obejct which can be called before we've initialized the rest of
the components. It is invoked manually early in the startup process so that
startup crashes are properly annotated, then it's invoked again when the user
is informed of the privacy policy as well as when one of the relevant
preferences is altered. This ensures that the annotations are stripped if the
user disables uploading pings without having to restart Firefox.
MozReview-Commit-ID: 2DKnoWGT1Bp
--- a/toolkit/components/telemetry/TelemetryController.jsm
+++ b/toolkit/components/telemetry/TelemetryController.jsm
@@ -714,37 +714,40 @@ var Impl = {
}
this._attachObservers();
// Perform a lightweight, early initialization for the component, just registering
// a few observers and initializing the session.
TelemetrySession.earlyInit(this._testMode);
+ // Annotate crash reports so that we get pings for startup crashes
+ TelemetrySend.earlyInit();
+
// For very short session durations, we may never load the client
// id from disk.
// We try to cache it in prefs to avoid this, even though this may
// lead to some stale client ids.
this._clientID = ClientID.getCachedClientID();
// Delay full telemetry initialization to give the browser time to
// run various late initializers. Otherwise our gathered memory
// footprint and other numbers would be too optimistic.
this._delayedInitTaskDeferred = Promise.defer();
this._delayedInitTask = new DeferredTask(function* () {
try {
// TODO: This should probably happen after all the delayed init here.
this._initialized = true;
TelemetryEnvironment.delayedInit();
- yield TelemetrySend.setup(this._testMode);
-
// Load the ClientID.
this._clientID = yield ClientID.getClientID();
+ yield TelemetrySend.setup(this._testMode);
+
// Perform TelemetrySession delayed init.
yield TelemetrySession.delayedInit();
// Purge the pings archive by removing outdated pings. We don't wait for
// this task to complete, but TelemetryStorage blocks on it during
// shutdown.
TelemetryStorage.runCleanPingArchiveTask();
// Now that FHR/healthreporter is gone, make sure to remove FHR's DB from
--- a/toolkit/components/telemetry/TelemetrySend.jsm
+++ b/toolkit/components/telemetry/TelemetrySend.jsm
@@ -14,16 +14,17 @@
this.EXPORTED_SYMBOLS = [
"TelemetrySend",
];
const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
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);
Cu.import("resource://gre/modules/TelemetryUtils.jsm", this);
Cu.import("resource://gre/modules/Timer.jsm", this);
@@ -40,16 +41,17 @@ XPCOMUtils.defineLazyServiceGetter(this,
const Utils = TelemetryUtils;
const LOGGER_NAME = "Toolkit.Telemetry";
const LOGGER_PREFIX = "TelemetrySend::";
const PREF_BRANCH = "toolkit.telemetry.";
const PREF_SERVER = PREF_BRANCH + "server";
const PREF_UNIFIED = PREF_BRANCH + "unified";
+const PREF_ENABLED = PREF_BRANCH + "enabled";
const PREF_FHR_UPLOAD_ENABLED = "datareporting.healthreport.uploadEnabled";
const PREF_OVERRIDE_OFFICIAL_CHECK = PREF_BRANCH + "send.overrideOfficialCheck";
const TOPIC_IDLE_DAILY = "idle-daily";
const TOPIC_QUIT_APPLICATION = "quit-application";
// Whether the FHR/Telemetry unification features are enabled.
// Changing this pref requires a restart.
@@ -172,16 +174,24 @@ this.TelemetrySend = {
return OVERDUE_PING_FILE_AGE;
},
get pendingPingCount() {
return TelemetrySendImpl.pendingPingCount;
},
/**
+ * Partial setup that runs immediately at startup. This currently triggers
+ * the crash report annotations.
+ */
+ earlyInit() {
+ TelemetrySendImpl.earlyInit();
+ },
+
+ /**
* Initializes this module.
*
* @param {Boolean} testing Whether this is run in a test. This changes some behavior
* to enable proper testing.
* @return {Promise} Resolved when setup is finished.
*/
setup(testing = false) {
return TelemetrySendImpl.setup(testing);
@@ -544,24 +554,30 @@ var TelemetrySendImpl = {
// This is true when running in the test infrastructure.
_testMode: false,
// This holds pings that we currently try and haven't persisted yet.
_currentPings: new Map(),
// Count of pending pings that were overdue.
_overduePingCount: 0,
- // Whether sending pings has been overridden. We have it here instead
- // of the top of the file to ease testing.
- _overrideOfficialCheck: false,
-
OBSERVER_TOPICS: [
TOPIC_IDLE_DAILY,
],
+ OBSERVED_PREFERENCES: [
+ PREF_ENABLED,
+ PREF_FHR_UPLOAD_ENABLED,
+ ],
+
+ // Whether sending pings has been overridden.
+ get _overrideOfficialCheck() {
+ return Preferences.get(PREF_OVERRIDE_OFFICIAL_CHECK, false);
+ },
+
get _log() {
if (!this._logger) {
this._logger = Log.repository.getLoggerWithMessagePrefix(LOGGER_NAME, LOGGER_PREFIX);
}
return this._logger;
},
@@ -576,43 +592,82 @@ var TelemetrySendImpl = {
get pendingPingCount() {
return TelemetryStorage.getPendingPingList().length + this._currentPings.size;
},
setTestModeEnabled(testing) {
this._testMode = testing;
},
+ earlyInit() {
+ this._annotateCrashReport();
+ },
+
async setup(testing) {
this._log.trace("setup");
this._testMode = testing;
this._sendingEnabled = true;
- this._overrideOfficialCheck = Preferences.get(PREF_OVERRIDE_OFFICIAL_CHECK, false);
Services.obs.addObserver(this, TOPIC_IDLE_DAILY, false);
this._server = Preferences.get(PREF_SERVER, undefined);
+ // Annotate crash reports so that crash pings are sent correctly and listen
+ // to pref changes to adjust the annotations accordingly.
+ for (let pref of this.OBSERVED_PREFERENCES) {
+ Preferences.observe(pref, this._annotateCrashReport, this);
+ }
+ this._annotateCrashReport();
+
// Check the pending pings on disk now.
try {
await this._checkPendingPings();
} catch (ex) {
this._log.error("setup - _checkPendingPings rejected", ex);
}
// Enforce the pending pings storage quota. It could take a while so don't
// block on it.
TelemetryStorage.runEnforcePendingPingsQuotaTask();
// Start sending pings, but don't block on this.
SendScheduler.triggerSendingPings(true);
},
/**
+ * Triggers the crash report annotations depending on the current
+ * configuration. This communicates to the crash reporter if it can send a
+ * crash ping or not. This method can be called safely before setup() has
+ * been called.
+ */
+ _annotateCrashReport() {
+ try {
+ const cr = Cc["@mozilla.org/toolkit/crash-reporter;1"];
+ if (cr) {
+ const crs = cr.getService(Ci.nsICrashReporter);
+
+ let clientId = ClientID.getCachedClientID();
+ let server = this._server || Preferences.get(PREF_SERVER, undefined);
+
+ if (!this.sendingEnabled() || !TelemetryReportingPolicy.canUpload()) {
+ // If we cannot send pings then clear the crash annotations
+ crs.annotateCrashReport("TelemetryClientId", "");
+ crs.annotateCrashReport("TelemetryServerURL", "");
+ } else {
+ crs.annotateCrashReport("TelemetryClientId", clientId);
+ crs.annotateCrashReport("TelemetryServerURL", server);
+ }
+ }
+ } catch (e) {
+ // Ignore errors when crash reporting is disabled
+ }
+ },
+
+ /**
* Discard old pings from the pending pings and detect overdue ones.
* @return {Boolean} True if we have overdue pings, false otherwise.
*/
async _checkPendingPings() {
// Scan the pending pings - that gives us a list sorted by last modified, descending.
let infos = await TelemetryStorage.loadPendingPingList();
this._log.info("_checkPendingPings - pending ping count: " + infos.length);
if (infos.length == 0) {
@@ -633,16 +688,23 @@ var TelemetrySendImpl = {
Utils.millisecondsToDays(Math.abs(now.getTime() - pingInfo.lastModificationDate));
Telemetry.getHistogramById("TELEMETRY_PENDING_PINGS_AGE").add(ageInDays);
}
},
async shutdown() {
this._shutdown = true;
+ for (let pref of this.OBSERVED_PREFERENCES) {
+ // FIXME: When running tests this causes errors to be printed out if
+ // TelemetrySend.shutdown() is called twice in a row without calling
+ // TelemetrySend.setup() in-between.
+ Preferences.ignore(pref, this._annotateCrashReport, this);
+ }
+
for (let topic of this.OBSERVER_TOPICS) {
try {
Services.obs.removeObserver(this, topic);
} catch (ex) {
this._log.error("shutdown - failed to remove observer for " + topic, ex);
}
}
@@ -663,35 +725,37 @@ var TelemetrySendImpl = {
},
reset() {
this._log.trace("reset");
this._shutdown = false;
this._currentPings = new Map();
this._overduePingCount = 0;
- this._overrideOfficialCheck = Preferences.get(PREF_OVERRIDE_OFFICIAL_CHECK, false);
const histograms = [
"TELEMETRY_SUCCESS",
"TELEMETRY_SEND_SUCCESS",
"TELEMETRY_SEND_FAILURE",
];
histograms.forEach(h => Telemetry.getHistogramById(h).clear());
return SendScheduler.reset();
},
/**
* Notify that we can start submitting data to the servers.
*/
notifyCanUpload() {
- // Let the scheduler trigger sending pings if possible.
+ // Let the scheduler trigger sending pings if possible, also inform the
+ // crash reporter that it can send crash pings if appropriate.
SendScheduler.triggerSendingPings(true);
+ this._annotateCrashReport();
+
return this.promisePendingPingActivity();
},
observe(subject, topic, data) {
switch (topic) {
case TOPIC_IDLE_DAILY:
SendScheduler.triggerSendingPings(true);
break;
--- a/toolkit/components/telemetry/TelemetrySession.jsm
+++ b/toolkit/components/telemetry/TelemetrySession.jsm
@@ -17,17 +17,16 @@ Cu.import("resource://gre/modules/XPCOMU
Cu.import("resource://gre/modules/Promise.jsm", this);
Cu.import("resource://gre/modules/DeferredTask.jsm", this);
Cu.import("resource://gre/modules/Preferences.jsm");
Cu.import("resource://gre/modules/Task.jsm");
Cu.import("resource://gre/modules/Timer.jsm");
Cu.import("resource://gre/modules/TelemetrySend.jsm", this);
Cu.import("resource://gre/modules/TelemetryUtils.jsm", this);
Cu.import("resource://gre/modules/AppConstants.jsm");
-Cu.import("resource://gre/modules/ClientID.jsm");
const Utils = TelemetryUtils;
const myScope = this;
// When modifying the payload in incompatible ways, please bump this version number
const PAYLOAD_VERSION = 4;
const PING_TYPE_MAIN = "main";
@@ -62,17 +61,16 @@ 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_SERVER = PREF_BRANCH + "server";
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";
@@ -195,35 +193,22 @@ function getPingType(aPayload) {
}
return PING_TYPE_MAIN;
}
/**
* Annotate the current session ID with the crash reporter to map potential
* crash pings with the related main ping.
- *
- * @param sessionId {String} The telemetry session ID
- * @param clientId {String} The telemetry client ID
- * @param telemetryServer {String} The URL of the telemetry server
*/
-function annotateCrashReport(sessionId, clientId, telemetryServer) {
+function annotateCrashReport(sessionId) {
try {
const cr = Cc["@mozilla.org/toolkit/crash-reporter;1"];
if (cr) {
- const crs = cr.getService(Ci.nsICrashReporter);
-
- crs.setTelemetrySessionId(sessionId);
-
- // We do not annotate the crash if telemetry is disabled to prevent the
- // crashreporter client from sending the crash ping.
- if (Utils.isTelemetryEnabled) {
- crs.annotateCrashReport("TelemetryClientId", clientId);
- crs.annotateCrashReport("TelemetryServerURL", telemetryServer);
- }
+ cr.getService(Ci.nsICrashReporter).setTelemetrySessionId(sessionId);
}
} catch (e) {
// Ignore errors when crash reporting is disabled
}
}
/**
* Read current process I/O counters.
@@ -1484,20 +1469,17 @@ var Impl = {
// Generate a unique id once per session so the server can cope with duplicate
// submissions, orphaning and other oddities. The id is shared across subsessions.
this._sessionId = Policy.generateSessionUUID();
this.startNewSubsession();
// startNewSubsession sets |_subsessionStartDate| to the current date/time. Use
// the very same value for |_sessionStartDate|.
this._sessionStartDate = this._subsessionStartDate;
- // Annotate crash reports using the cached client ID which can be accessed
- // synchronously. If it is not available yet, we'll update it later.
- annotateCrashReport(this._sessionId, ClientID.getCachedClientID(),
- Preferences.get(PREF_SERVER, undefined));
+ annotateCrashReport(this._sessionId);
// Initialize some probes that are kept in their own modules
this._thirdPartyCookies = new ThirdPartyCookieProbe();
this._thirdPartyCookies.init();
// Record old value and update build ID preference if this is the first
// run with a new build ID.
let previousBuildId = Preferences.get(PREF_PREVIOUS_BUILDID, null);
@@ -1542,20 +1524,16 @@ var Impl = {
this.gatherMemory();
if (Telemetry.canRecordExtended) {
GCTelemetry.init();
}
Telemetry.asyncFetchTelemetryData(function() {});
- // Update the crash annotation with the proper client ID.
- annotateCrashReport(this._sessionId, yield ClientID.getClientID(),
- Preferences.get(PREF_SERVER, undefined));
-
if (IS_UNIFIED_TELEMETRY) {
// Check for a previously written aborted session ping.
yield TelemetryController.checkAbortedSessionPing();
// Write the first aborted-session ping as early as possible. Just do that
// if we are not testing, since calling Telemetry.reset() will make a previous
// aborted ping a pending ping.
if (!this._testing) {
--- a/toolkit/crashreporter/test/unit/test_crashreporter_crash.js
+++ b/toolkit/crashreporter/test/unit/test_crashreporter_crash.js
@@ -27,28 +27,104 @@ function run_test() {
});
}
if (is_win7_or_newer)
do_check_true(CrashTestUtils.dumpHasStream(mdump.path, CrashTestUtils.MD_MEMORY_INFO_LIST_STREAM));
});
// check setting some basic data
do_crash(function() {
+ // Add various annotations
crashReporter.annotateCrashReport("TestKey", "TestValue");
crashReporter.annotateCrashReport("\u2665", "\u{1F4A9}");
crashReporter.appendAppNotesToCrashReport("Junk");
crashReporter.appendAppNotesToCrashReport("MoreJunk");
+
// TelemetrySession setup will trigger the session annotation
let scope = {};
Components.utils.import("resource://gre/modules/TelemetryController.jsm", scope);
scope.TelemetryController.testSetup();
},
function(mdump, extra) {
do_check_eq(extra.TestKey, "TestValue");
do_check_eq(extra["\u2665"], "\u{1F4A9}");
do_check_eq(extra.Notes, "JunkMoreJunk");
const UUID_REGEX = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i;
Assert.ok("TelemetrySessionId" in extra,
"The TelemetrySessionId field is present in the extra file");
Assert.ok(UUID_REGEX.test(extra.TelemetrySessionId),
"The TelemetrySessionId is a UUID");
+ Assert.ok(!("TelemetryClientId" in extra),
+ "The TelemetryClientId field is omitted by default");
+ Assert.ok(!("TelemetryServerURL" in extra),
+ "The TelemetryServerURL field is omitted by default");
});
+
+ do_crash(function() {
+ // Enable the FHR, official policy bypass (since we're in a test) and
+ // specify a telemetry server & client ID.
+ let prefs = Components.classes["@mozilla.org/preferences-service;1"]
+ .getService(Components.interfaces.nsIPrefBranch);
+ prefs.setBoolPref("datareporting.policy.dataSubmissionPolicyBypassNotification", true);
+ prefs.setBoolPref("datareporting.healthreport.uploadEnabled", true);
+ prefs.setCharPref("toolkit.telemetry.server", "http://a.telemetry.server");
+ prefs.setCharPref("toolkit.telemetry.cachedClientID",
+ "f3582dee-22b9-4d73-96d1-79ef5bf2fc24");
+
+ // TelemetrySession setup will trigger the session annotation
+ let scope = {};
+ Components.utils.import("resource://gre/modules/TelemetryController.jsm", scope);
+ Components.utils.import("resource://gre/modules/TelemetrySend.jsm", scope);
+ scope.TelemetrySend.setTestModeEnabled(true);
+ scope.TelemetryController.testSetup();
+ }, function(mdump, extra) {
+ Assert.ok("TelemetryClientId" in extra,
+ "The TelemetryClientId field is present when the FHR is on");
+ Assert.equal(extra.TelemetryClientId,
+ "f3582dee-22b9-4d73-96d1-79ef5bf2fc24",
+ "The TelemetryClientId matches the expected value");
+ Assert.ok("TelemetryServerURL" in extra,
+ "The TelemetryServerURL field is present when the FHR is on");
+ Assert.equal(extra.TelemetryServerURL, "http://a.telemetry.server",
+ "The TelemetryServerURL matches the expected value");
+ });
+
+ do_crash(function() {
+ // Disable the FHR upload, no telemetry annotations should be present.
+ let prefs = Components.classes["@mozilla.org/preferences-service;1"]
+ .getService(Components.interfaces.nsIPrefBranch);
+ prefs.setBoolPref("datareporting.policy.dataSubmissionPolicyBypassNotification", true);
+ prefs.setBoolPref("datareporting.healthreport.uploadEnabled", false);
+
+ // TelemetrySession setup will trigger the session annotation
+ let scope = {};
+ Components.utils.import("resource://gre/modules/TelemetryController.jsm", scope);
+ Components.utils.import("resource://gre/modules/TelemetrySend.jsm", scope);
+ scope.TelemetrySend.setTestModeEnabled(true);
+ scope.TelemetryController.testSetup();
+ }, function(mdump, extra) {
+ Assert.ok(!("TelemetryClientId" in extra),
+ "The TelemetryClientId field is omitted when FHR upload is disabled");
+ Assert.ok(!("TelemetryServerURL" in extra),
+ "The TelemetryServerURL field is omitted when FHR upload is disabled");
+ });
+
+ do_crash(function() {
+ // No telemetry annotations should be present if the user has not been
+ // notified yet
+ let prefs = Components.classes["@mozilla.org/preferences-service;1"]
+ .getService(Components.interfaces.nsIPrefBranch);
+ prefs.setBoolPref("datareporting.policy.dataSubmissionPolicyBypassNotification", false);
+ prefs.setBoolPref("datareporting.healthreport.uploadEnabled", true);
+
+ // TelemetrySession setup will trigger the session annotation
+ let scope = {};
+ Components.utils.import("resource://gre/modules/TelemetryController.jsm", scope);
+ Components.utils.import("resource://gre/modules/TelemetrySend.jsm", scope);
+ scope.TelemetrySend.setTestModeEnabled(true);
+ scope.TelemetryController.testSetup();
+ }, function(mdump, extra) {
+ Assert.ok(!("TelemetryClientId" in extra),
+ "The TelemetryClientId field is omitted when FHR upload is disabled");
+ Assert.ok(!("TelemetryServerURL" in extra),
+ "The TelemetryServerURL field is omitted when FHR upload is disabled");
+ });
}