From 517cd96d4c63013bfc6e878504146aec06c282de Mon Sep 17 00:00:00 2001 From: "Carsten \"Tomcat\" Book" Date: Thu, 16 Oct 2014 15:03:13 +0200 Subject: [PATCH] Backed out changeset 735f5c30d397 (bug 1064333) for XPC Bustage on a CLOSED TREE --HG-- extra : amend_source : 8b12237f285b03acfe6e18cccc9f8480c6d385c2 --- services/common/utils.js | 5 +- .../datareporting/DataReportingService.js | 116 ------------------ services/datareporting/policy.jsm | 3 +- .../tests/xpcshell/test_client_id.js | 86 ------------- .../datareporting/tests/xpcshell/xpcshell.ini | 1 - services/healthreport/healthreporter.jsm | 47 ++++--- .../healthreport/modules-testing/utils.jsm | 6 +- .../tests/xpcshell/test_healthreporter.js | 47 ++++--- .../tests/xpcshell/test_provider_appinfo.js | 15 +-- 9 files changed, 70 insertions(+), 256 deletions(-) delete mode 100644 services/datareporting/tests/xpcshell/test_client_id.js diff --git a/services/common/utils.js b/services/common/utils.js index 8ad3c90d1d21..6d7b30b04e8a 100644 --- a/services/common/utils.js +++ b/services/common/utils.js @@ -398,8 +398,9 @@ this.CommonUtils = { * @return a promise, as produced by OS.File.writeAtomic. */ writeJSON: function(contents, path) { - let data = JSON.stringify(contents); - return OS.File.writeAtomic(path, data, {encoding: "utf-8", tmpPath: path + ".tmp"}); + let encoder = new TextEncoder(); + let array = encoder.encode(JSON.stringify(contents)); + return OS.File.writeAtomic(path, array, {tmpPath: path + ".tmp"}); }, diff --git a/services/datareporting/DataReportingService.js b/services/datareporting/DataReportingService.js index e7a1aed38a11..457c4a3bacb2 100644 --- a/services/datareporting/DataReportingService.js +++ b/services/datareporting/DataReportingService.js @@ -9,9 +9,6 @@ const {classes: Cc, interfaces: Ci, utils: Cu} = Components; Cu.import("resource://gre/modules/Preferences.jsm"); Cu.import("resource://gre/modules/XPCOMUtils.jsm"); Cu.import("resource://services-common/utils.js"); -Cu.import("resource://gre/modules/Promise.jsm"); -Cu.import("resource://gre/modules/Task.jsm"); -Cu.import("resource://gre/modules/osfile.jsm"); const ROOT_BRANCH = "datareporting."; @@ -64,13 +61,6 @@ this.DataReportingService = function () { this._os = Cc["@mozilla.org/observer-service;1"] .getService(Ci.nsIObserverService); - - this._clientID = null; - this._loadClientIdTask = null; - this._saveClientIdTask = null; - - this._stateDir = null; - this._stateFilePath = null; } DataReportingService.prototype = Object.freeze({ @@ -136,9 +126,6 @@ DataReportingService.prototype = Object.freeze({ let policyPrefs = new Preferences(POLICY_BRANCH); this.policy = new DataReportingPolicy(policyPrefs, this._prefs, this); - this._stateDir = OS.Path.join(OS.Constants.Path.profileDir, "datareporting"); - this._stateFilePath = OS.Path.join(this._stateDir, "state.json"); - this._os.addObserver(this, "sessionstore-windows-restored", true); } catch (ex) { Cu.reportError("Exception when initializing data reporting service: " + @@ -297,109 +284,6 @@ DataReportingService.prototype = Object.freeze({ this._prefs.set("service.firstRun", true); }.bind(this)); }, - - _loadClientID: Task.async(function* () { - if (this._loadClientIdTask) { - return this._loadClientIdTask; - } - - // Previously we had the stable client ID managed in FHR. - // As we want to start correlating FHR and telemetry data (and moving towards - // unifying the two), we moved the ID management to the datareporting - // service. Consequently, we try to import the FHR ID first, so we can keep - // using it. - - // Try to load the client id from the DRS state file first. - try { - let state = yield CommonUtils.readJSON(this._stateFilePath); - if (state && 'clientID' in state && typeof(state.clientID) == 'string') { - this._clientID = state.clientID; - this._loadClientIdTask = null; - return this._clientID; - } - } catch (e) { - // fall through to next option - } - - // If we dont have DRS state yet, try to import from the FHR state. - try { - let fhrStatePath = OS.Path.join(OS.Constants.Path.profileDir, "healthreport", "state.json"); - let state = yield CommonUtils.readJSON(fhrStatePath); - if (state && 'clientID' in state && typeof(state.clientID) == 'string') { - this._clientID = state.clientID; - this._loadClientIdTask = null; - this._saveClientID(); - return this._clientID; - } - } catch (e) { - // fall through to next option - } - - // We dont have an id from FHR yet, generate a new ID. - this._clientID = CommonUtils.generateUUID(); - this._loadClientIdTask = null; - this._saveClientIdTask = this._saveClientID(); - - // Wait on persisting the id. Otherwise failure to save the ID would result in - // the client creating and subsequently sending multiple IDs to the server. - // This would appear as multiple clients submitting similar data, which would - // result in orphaning. - yield this._saveClientIdTask; - - return this._clientID; - }), - - _saveClientID: Task.async(function* () { - let obj = { clientID: this._clientID }; - yield OS.File.makeDir(this._stateDir); - yield CommonUtils.writeJSON(obj, this._stateFilePath); - this._saveClientIdTask = null; - }), - - /** - * This returns a promise resolving to the the stable client ID we use for - * data reporting (FHR & Telemetry). Previously exising FHR client IDs are - * migrated to this. - * - * @return Promise The stable client ID. - */ - getClientID: function() { - if (this._loadClientIdTask) { - return this._loadClientIdTask; - } - - if (!this._clientID) { - this._loadClientIdTask = this._loadClientID(); - return this._loadClientIdTask; - } - - return Promise.resolve(this._clientID); - }, - - /** - * Reset the stable client id. - * - * @return Promise The new client ID. - */ - resetClientID: Task.async(function* () { - yield this._loadClientIdTask; - yield this._saveClientIdTask; - - this._clientID = CommonUtils.generateUUID(); - this._saveClientIdTask = this._saveClientID(); - yield this._saveClientIdTask; - - return this._clientID; - }), - - /* - * Simulate a restart of the service. This is for testing only. - */ - _reset: Task.async(function* () { - yield this._loadClientIdTask; - yield this._saveClientIdTask; - this._clientID = null; - }), }); this.NSGetFactory = XPCOMUtils.generateNSGetFactory([DataReportingService]); diff --git a/services/datareporting/policy.jsm b/services/datareporting/policy.jsm index b95d12f9c0f6..f68c7885f0a3 100644 --- a/services/datareporting/policy.jsm +++ b/services/datareporting/policy.jsm @@ -817,8 +817,7 @@ this.DataReportingPolicy.prototype = Object.freeze({ this._log.info("Requesting data submission. Will expire at " + requestExpiresDate); try { - let promise = this._listener[handler](this._inProgressSubmissionRequest); - chained = chained.then(() => promise, null); + this._listener[handler](this._inProgressSubmissionRequest); } catch (ex) { this._log.warn("Exception when calling " + handler + ": " + CommonUtils.exceptionStr(ex)); diff --git a/services/datareporting/tests/xpcshell/test_client_id.js b/services/datareporting/tests/xpcshell/test_client_id.js deleted file mode 100644 index d72b66b2c4b2..000000000000 --- a/services/datareporting/tests/xpcshell/test_client_id.js +++ /dev/null @@ -1,86 +0,0 @@ -/* Any copyright is dedicated to the Public Domain. - * http://creativecommons.org/publicdomain/zero/1.0/ */ - -"use strict"; - -const {classes: Cc, interfaces: Ci, utils: Cu} = Components; - -Cu.import("resource://gre/modules/Task.jsm"); -Cu.import("resource://services-common/utils.js"); -Cu.import("resource://gre/modules/osfile.jsm"); -Cu.import("resource://gre/modules/XPCOMUtils.jsm"); - -XPCOMUtils.defineLazyGetter(this, "gDatareportingService", - () => Cc["@mozilla.org/datareporting/service;1"] - .getService(Ci.nsISupports) - .wrappedJSObject); - -function run_test() { - do_get_profile(); - - // Send the needed startup notifications to the datareporting service - // to ensure that it has been initialized. - gDatareportingService.observe(null, "app-startup", null); - gDatareportingService.observe(null, "profile-after-change", null); - - run_next_test(); -} - -add_task(function* () { - const drsPath = gDatareportingService._stateFilePath; - const fhrDir = OS.Path.join(OS.Constants.Path.profileDir, "healthreport"); - const fhrPath = OS.Path.join(fhrDir, "state.json"); - const uuidRegex = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i; - - yield OS.File.makeDir(fhrDir); - - // Check that we are importing the FHR client ID. - let clientID = CommonUtils.generateUUID(); - yield CommonUtils.writeJSON({clientID: clientID}, fhrPath); - Assert.equal(clientID, yield gDatareportingService.getClientID()); - - // We should persist the ID in DRS now and not pick up a differing ID from FHR. - yield gDatareportingService._reset(); - yield CommonUtils.writeJSON({clientID: CommonUtils.generateUUID()}, fhrPath); - Assert.equal(clientID, yield gDatareportingService.getClientID()); - - // We should be guarded against broken FHR data. - yield gDatareportingService._reset(); - yield OS.File.remove(drsPath); - yield CommonUtils.writeJSON({clientID: -1}, fhrPath); - clientID = yield gDatareportingService.getClientID(); - Assert.equal(typeof(clientID), 'string'); - Assert.ok(uuidRegex.test(clientID)); - - // We should be guarded against invalid FHR json. - yield gDatareportingService._reset(); - yield OS.File.remove(drsPath); - yield OS.File.writeAtomic(fhrPath, "abcd", {encoding: "utf-8", tmpPath: fhrPath + ".tmp"}); - clientID = yield gDatareportingService.getClientID(); - Assert.equal(typeof(clientID), 'string'); - Assert.ok(uuidRegex.test(clientID)); - - // We should be guarded against broken DRS data too and fall back to loading - // the FHR ID. - yield gDatareportingService._reset(); - clientID = CommonUtils.generateUUID(); - yield CommonUtils.writeJSON({clientID: clientID}, fhrPath); - yield CommonUtils.writeJSON({clientID: -1}, drsPath); - Assert.equal(clientID, yield gDatareportingService.getClientID()); - - // We should be guarded against invalid DRS json too. - yield gDatareportingService._reset(); - yield OS.File.remove(fhrPath); - yield OS.File.writeAtomic(drsPath, "abcd", {encoding: "utf-8", tmpPath: drsPath + ".tmp"}); - clientID = yield gDatareportingService.getClientID(); - Assert.equal(typeof(clientID), 'string'); - Assert.ok(uuidRegex.test(clientID)); - - // If both the FHR and DSR data are broken, we should end up with a new client ID. - yield gDatareportingService._reset(); - yield CommonUtils.writeJSON({clientID: -1}, fhrPath); - yield CommonUtils.writeJSON({clientID: -1}, drsPath); - clientID = yield gDatareportingService.getClientID(); - Assert.equal(typeof(clientID), 'string'); - Assert.ok(uuidRegex.test(clientID)); -}); diff --git a/services/datareporting/tests/xpcshell/xpcshell.ini b/services/datareporting/tests/xpcshell/xpcshell.ini index fbd88acf2b53..934227e192db 100644 --- a/services/datareporting/tests/xpcshell/xpcshell.ini +++ b/services/datareporting/tests/xpcshell/xpcshell.ini @@ -5,4 +5,3 @@ skip-if = toolkit == 'android' || toolkit == 'gonk' [test_policy.js] [test_session_recorder.js] -[test_client_id.js] diff --git a/services/healthreport/healthreporter.jsm b/services/healthreport/healthreporter.jsm index 2c79bfaaf0c3..ef0a4c287436 100644 --- a/services/healthreport/healthreporter.jsm +++ b/services/healthreport/healthreporter.jsm @@ -130,18 +130,13 @@ HealthReporterState.prototype = Object.freeze({ return Task.spawn(function* init() { yield OS.File.makeDir(this._stateDir); - let drs = Cc["@mozilla.org/datareporting/service;1"] - .getService(Ci.nsISupports) - .wrappedJSObject; - let drsClientID = yield drs.getClientID(); - let resetObjectState = function () { this._s = { // The payload version. This is bumped whenever there is a // backwards-incompatible change. v: 1, // The persistent client identifier. - clientID: drsClientID, + clientID: CommonUtils.generateUUID(), // Denotes the mechanism used to generate the client identifier. // 1: Random UUID. clientIDVersion: 1, @@ -181,7 +176,22 @@ HealthReporterState.prototype = Object.freeze({ // comes along and fixes us. } - this._s.clientID = drsClientID; + let regen = false; + if (!this._s.clientID) { + this._log.warn("No client ID stored. Generating random ID."); + regen = true; + } + + if (typeof(this._s.clientID) != "string") { + this._log.warn("Client ID is not a string. Regenerating."); + regen = true; + } + + if (regen) { + this._s.clientID = CommonUtils.generateUUID(); + this._s.clientIDVersion = 1; + yield this.save(); + } // Always look for preferences. This ensures that downgrades followed // by reupgrades don't result in excessive data loss. @@ -245,18 +255,21 @@ HealthReporterState.prototype = Object.freeze({ /** * Reset the client ID to something else. - * Returns a promise that is resolved when completed. + * + * This fails if remote IDs are stored because changing the client ID + * while there is remote data will create orphaned records. */ - resetClientID: Task.async(function* () { - let drs = Cc["@mozilla.org/datareporting/service;1"] - .getService(Ci.nsISupports) - .wrappedJSObject; - yield drs.resetClientID(); - this._s.clientID = yield drs.getClientID(); - this._log.info("Reset client id to " + this._s.clientID + "."); + resetClientID: function () { + if (this.remoteIDs.length) { + throw new Error("Cannot reset client ID while remote IDs are stored."); + } - yield this.save(); - }), + this._log.warn("Resetting client ID."); + this._s.clientID = CommonUtils.generateUUID(); + this._s.clientIDVersion = 1; + + return this.save(); + }, _migratePrefs: function () { let prefs = this._reporter._prefs; diff --git a/services/healthreport/modules-testing/utils.jsm b/services/healthreport/modules-testing/utils.jsm index 2568e7712a3b..a08ce954c63c 100644 --- a/services/healthreport/modules-testing/utils.jsm +++ b/services/healthreport/modules-testing/utils.jsm @@ -193,14 +193,12 @@ this.getHealthReporter = function (name, uri=DUMMY_URI, inspected=false) { let policyPrefs = new Preferences(branch + "policy."); let listener = new MockPolicyListener(); listener.onRequestDataUpload = function (request) { - let promise = reporter.requestDataUpload(request); + reporter.requestDataUpload(request); MockPolicyListener.prototype.onRequestDataUpload.call(this, request); - return promise; } listener.onRequestRemoteDelete = function (request) { - let promise = reporter.deleteRemoteData(request); + reporter.deleteRemoteData(request); MockPolicyListener.prototype.onRequestRemoteDelete.call(this, request); - return promise; } let policy = new DataReportingPolicy(policyPrefs, prefs, listener); let type = inspected ? InspectedHealthReporter : HealthReporter; diff --git a/services/healthreport/tests/xpcshell/test_healthreporter.js b/services/healthreport/tests/xpcshell/test_healthreporter.js index 9bb2b3ed2276..1eecc8c4ad4e 100644 --- a/services/healthreport/tests/xpcshell/test_healthreporter.js +++ b/services/healthreport/tests/xpcshell/test_healthreporter.js @@ -21,12 +21,6 @@ Cu.import("resource://testing-common/services/common/bagheeraserver.js"); Cu.import("resource://testing-common/services/metrics/mocks.jsm"); Cu.import("resource://testing-common/services/healthreport/utils.jsm"); Cu.import("resource://testing-common/AppData.jsm"); -Cu.import("resource://gre/modules/XPCOMUtils.jsm"); - -XPCOMUtils.defineLazyGetter(this, "gDatareportingService", - () => Cc["@mozilla.org/datareporting/service;1"] - .getService(Ci.nsISupports) - .wrappedJSObject); const DUMMY_URI = "http://localhost:62013/"; @@ -114,13 +108,6 @@ function ensureUserNotified (reporter) { } function run_test() { - do_get_profile(); - - // Send the needed startup notifications to the datareporting service - // to ensure that it has been initialized. - gDatareportingService.observe(null, "app-startup", null); - gDatareportingService.observe(null, "profile-after-change", null); - run_next_test(); } @@ -758,7 +745,7 @@ add_task(function test_request_remote_data_deletion() { do_check_false(reporter.haveRemoteData()); do_check_false(server.hasDocument(reporter.serverNamespace, id)); - // Client ID should be updated. + // Client ID should be updated. do_check_neq(reporter._state.clientID, null); do_check_neq(reporter._state.clientID, clientID); do_check_eq(reporter._state.clientIDVersion, 1); @@ -1184,6 +1171,38 @@ add_task(function test_state_downgrade_upgrade() { } }); +// Missing client ID in state should be created on state load. +add_task(function* test_state_create_client_id() { + let reporter = getHealthReporter("state_create_client_id"); + + yield CommonUtils.writeJSON({ + v: 1, + remoteIDs: ["id1", "id2"], + lastPingTime: Date.now(), + removeOutdatedLastPayload: true, + }, reporter._state._filename); + + try { + yield reporter.init(); + + do_check_eq(reporter.lastSubmitID, "id1"); + do_check_neq(reporter._state.clientID, null); + do_check_eq(reporter._state.clientID.length, 36); + do_check_eq(reporter._state.clientIDVersion, 1); + + let clientID = reporter._state.clientID; + + // The client ID should be persisted as soon as it is created. + reporter._shutdown(); + + reporter = getHealthReporter("state_create_client_id"); + yield reporter.init(); + do_check_eq(reporter._state.clientID, clientID); + } finally { + reporter._shutdown(); + } +}); + // Invalid stored client ID is reset automatically. add_task(function* test_empty_client_id() { let reporter = getHealthReporter("state_empty_client_id"); diff --git a/services/healthreport/tests/xpcshell/test_provider_appinfo.js b/services/healthreport/tests/xpcshell/test_provider_appinfo.js index 6f5115110478..97b93ef96a54 100644 --- a/services/healthreport/tests/xpcshell/test_provider_appinfo.js +++ b/services/healthreport/tests/xpcshell/test_provider_appinfo.js @@ -3,29 +3,16 @@ "use strict"; -const {interfaces: Ci, results: Cr, utils: Cu, classes: Cc} = Components; +const {interfaces: Ci, results: Cr, utils: Cu} = Components; Cu.import("resource://gre/modules/Metrics.jsm"); Cu.import("resource://gre/modules/Services.jsm"); Cu.import("resource://gre/modules/XPCOMUtils.jsm"); Cu.import("resource://gre/modules/services/healthreport/providers.jsm"); Cu.import("resource://testing-common/services/healthreport/utils.jsm"); -Cu.import("resource://gre/modules/XPCOMUtils.jsm"); - -XPCOMUtils.defineLazyGetter(this, "gDatareportingService", - () => Cc["@mozilla.org/datareporting/service;1"] - .getService(Ci.nsISupports) - .wrappedJSObject); function run_test() { - do_get_profile(); - - // Send the needed startup notifications to the datareporting service - // to ensure that it has been initialized. - gDatareportingService.observe(null, "app-startup", null); - gDatareportingService.observe(null, "profile-after-change", null); - run_next_test(); }