From 053adafc517d87c8d51aa4b435af944ce9623624 Mon Sep 17 00:00:00 2001 From: Gabriele Svelto Date: Mon, 16 Jan 2017 18:17:02 +0100 Subject: [PATCH] Bug 1310703 - Use the ID generated by the crashreporter client when sending a crash ping so that server-side deduplication works correctly; r=Dexter MozReview-Commit-ID: HfyKTru8QgN --- toolkit/components/crashes/CrashManager.jsm | 8 +++++++- .../telemetry/TelemetryController.jsm | 18 +++++++++++++++++- .../components/telemetry/tests/unit/head.js | 9 +++++++++ .../tests/unit/test_TelemetryController.js | 15 ++++++++++++++- .../tests/unit/test_TelemetrySession.js | 6 ------ 5 files changed, 47 insertions(+), 9 deletions(-) diff --git a/toolkit/components/crashes/CrashManager.jsm b/toolkit/components/crashes/CrashManager.jsm index e35448ecfc1c..08231f10bd6b 100644 --- a/toolkit/components/crashes/CrashManager.jsm +++ b/toolkit/components/crashes/CrashManager.jsm @@ -55,7 +55,9 @@ function parseAndRemoveField(obj, field, parseAsJson = true) { if (field in obj) { if (!parseAsJson) { - value = obj[field]; + // We split extra files on LF characters but Windows-generated ones might + // contain trailing CR characters so trim them here. + value = obj[field].trim(); } else { try { value = JSON.parse(obj[field]); @@ -638,6 +640,9 @@ this.CrashManager.prototype = Object.freeze({ let sessionId = parseAndRemoveField(reportMeta, "TelemetrySessionId", /* parseAsJson */ false); let stackTraces = parseAndRemoveField(reportMeta, "StackTraces"); + // If CrashPingUUID is not present then Telemetry will generate a new UUID + let pingId = parseAndRemoveField(reportMeta, "CrashPingUUID", + /* parseAsJson */ false); // Filter the remaining annotations to remove privacy-sensitive ones reportMeta = this._filterAnnotations(reportMeta); @@ -657,6 +662,7 @@ this.CrashManager.prototype = Object.freeze({ addClientId: true, addEnvironment: true, overrideEnvironment: crashEnvironment, + overridePingId: pingId } ); }, diff --git a/toolkit/components/telemetry/TelemetryController.jsm b/toolkit/components/telemetry/TelemetryController.jsm index 54900f7bc1e0..546bd49f5bff 100644 --- a/toolkit/components/telemetry/TelemetryController.jsm +++ b/toolkit/components/telemetry/TelemetryController.jsm @@ -197,6 +197,8 @@ this.TelemetryController = Object.freeze({ * @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 {String} [aOptions.overridePingId=null] set to override the + * generated ping id. * @returns {Promise} Test-only - a promise that resolves with the ping id once the ping is stored or sent. */ submitExternalPing(aType, aPayload, aOptions = {}) { @@ -229,6 +231,8 @@ this.TelemetryController = Object.freeze({ * @param {Boolean} [aOptions.overwrite=false] true overwrites a ping with the same name, * if found. * @param {Object} [aOptions.overrideEnvironment=null] set to override the environment data. + * @param {String} [aOptions.overridePingId=null] set to override the + * generated ping id. * * @returns {Promise} A promise that resolves with the ping id when the ping is saved to * disk. @@ -286,6 +290,8 @@ this.TelemetryController = Object.freeze({ * @param {Boolean} [aOptions.overwrite=false] true overwrites a ping with the same name, * if found. * @param {Object} [aOptions.overrideEnvironment=null] set to override the environment data. + * @param {String} [aOptions.overridePingId=null] set to override the + * generated ping id. * * @returns {Promise} A promise that resolves with the ping id when the ping is saved to * disk. @@ -395,6 +401,8 @@ var Impl = { * @param {Boolean} aOptions.addEnvironment true if the ping should contain the * environment data. * @param {Object} [aOptions.overrideEnvironment=null] set to override the environment data. + * @param {String} [aOptions.overridePingId=null] set to override the + * generated ping id. * * @returns {Object} An object that contains the assembled ping data. */ @@ -409,7 +417,7 @@ var Impl = { // Fill the common ping fields. let pingData = { type: aType, - id: Policy.generatePingId(), + id: aOptions.overridePingId || Policy.generatePingId(), creationDate: (Policy.now()).toISOString(), version: PING_FORMAT_VERSION, application: this._getApplicationSection(), @@ -451,6 +459,8 @@ var Impl = { * @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 {String} [aOptions.overridePingId=null] set to override the + * generated ping id. * @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) { @@ -489,6 +499,8 @@ var Impl = { * @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 {String} [aOptions.overridePingId=null] set to override the + * generated ping id. * @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) { @@ -534,6 +546,8 @@ var Impl = { * environment data. * @param {Boolean} aOptions.overwrite true overwrites a ping with the same name, if found. * @param {Object} [aOptions.overrideEnvironment=null] set to override the environment data. + * @param {String} [aOptions.overridePingId=null] set to override the + * generated ping id. * * @returns {Promise} A promise that resolves with the ping id when the ping is saved to * disk. @@ -570,6 +584,8 @@ var Impl = { * environment data. * @param {Boolean} aOptions.overwrite true overwrites a ping with the same name, if found. * @param {Object} [aOptions.overrideEnvironment=null] set to override the environment data. + * @param {String} [aOptions.overridePingId=null] set to override the + * generated ping id. * * @returns {Promise} A promise that resolves with the ping id when the ping is saved to * disk. diff --git a/toolkit/components/telemetry/tests/unit/head.js b/toolkit/components/telemetry/tests/unit/head.js index fd23b45563c3..41ebe2556890 100644 --- a/toolkit/components/telemetry/tests/unit/head.js +++ b/toolkit/components/telemetry/tests/unit/head.js @@ -296,9 +296,18 @@ function setEmptyPrefWatchlist() { Cu.import("resource://gre/modules/TelemetryEnvironment.jsm").TelemetryEnvironment; return TelemetryEnvironment.onInitialized().then(() => { TelemetryEnvironment.testWatchPreferences(new Map()); + }); } +// Generate a UUID, used for the ping ID +function generateUUID() { + let str = Cc["@mozilla.org/uuid-generator;1"] + .getService(Ci.nsIUUIDGenerator).generateUUID().toString(); + // strip {} + return str.substring(1, str.length - 1); +} + if (runningInParent) { // Set logging preferences for all the tests. Services.prefs.setCharPref("toolkit.telemetry.log.level", "Trace"); diff --git a/toolkit/components/telemetry/tests/unit/test_TelemetryController.js b/toolkit/components/telemetry/tests/unit/test_TelemetryController.js index 4c85627dd3b8..d18d792540bd 100644 --- a/toolkit/components/telemetry/tests/unit/test_TelemetryController.js +++ b/toolkit/components/telemetry/tests/unit/test_TelemetryController.js @@ -35,7 +35,7 @@ const PREF_UNIFIED = PREF_BRANCH + "unified"; var gClientID = null; -function sendPing(aSendClientId, aSendEnvironment) { +function sendPing(aSendClientId, aSendEnvironment, aOverridePingId) { if (PingServer.started) { TelemetrySend.setServer("http://localhost:" + PingServer.port); } else { @@ -45,6 +45,7 @@ function sendPing(aSendClientId, aSendEnvironment) { let options = { addClientId: aSendClientId, addEnvironment: aSendEnvironment, + overridePingId: aOverridePingId || null, }; return TelemetryController.submitExternalPing(TEST_PING_TYPE, {}, options); } @@ -286,6 +287,18 @@ add_task(function* test_pingHasEnvironmentAndClientId() { Assert.equal(ping.clientId, gClientID, "The correct clientId must be reported."); }); +add_task(function* test_pingIdCanBeOverridden() { + // Send a ping with an overridden id + const myPingId = generateUUID(); + yield sendPing(/* aSendClientId */ false, + /* aSendEnvironment */ false, + myPingId); + let ping = yield PingServer.promiseNextPing(); + checkPingFormat(ping, TEST_PING_TYPE, false, false); + + Assert.equal(ping.id, myPingId, "The ping id must be the one we provided."); +}); + add_task(function* test_archivePings() { let now = new Date(2009, 10, 18, 12, 0, 0); fakeNow(now); diff --git a/toolkit/components/telemetry/tests/unit/test_TelemetrySession.js b/toolkit/components/telemetry/tests/unit/test_TelemetrySession.js index 51fb8fe832a2..2f7ce3fe42b8 100644 --- a/toolkit/components/telemetry/tests/unit/test_TelemetrySession.js +++ b/toolkit/components/telemetry/tests/unit/test_TelemetrySession.js @@ -74,12 +74,6 @@ XPCOMUtils.defineLazyGetter(this, "DATAREPORTING_PATH", function() { var gClientID = null; var gMonotonicNow = 0; -function generateUUID() { - let str = Cc["@mozilla.org/uuid-generator;1"].getService(Ci.nsIUUIDGenerator).generateUUID().toString(); - // strip {} - return str.substring(1, str.length - 1); -} - function truncateDateToDays(date) { return new Date(date.getFullYear(), date.getMonth(),