Bug 1585410 - Fix race in ClientID.jsm r=janerik
☠☠ backed out by 8c1ac1675109 ☠ ☠
authorChris H-C <chutten@mozilla.com>
Tue, 12 Nov 2019 22:46:25 +0000
changeset 501935 474430e7ea37c836c40d3fb9f93a3566f7e36af1
parent 501934 20eae859997faaff797e2c925531a4075d077175
child 501936 bbe85df3365a23b6400c9f0b453f316d950b64c4
push id114172
push userdluca@mozilla.com
push dateTue, 19 Nov 2019 11:31:10 +0000
treeherdermozilla-inbound@b5c5ba07d3db [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjanerik
bugs1585410
milestone72.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 1585410 - Fix race in ClientID.jsm r=janerik If we opt out of Telemetry and then back into it, we might end up with out-of-order writes and deletes to the clientid state file. This would result in us sending pings with c0ffee canary client ids. So let's wait for pending save tasks before we process our removal task. Also, while I'm here, let's add some trace logging to client id operations. Differential Revision: https://phabricator.services.mozilla.com/D51709
toolkit/components/telemetry/app/ClientID.jsm
--- a/toolkit/components/telemetry/app/ClientID.jsm
+++ b/toolkit/components/telemetry/app/ClientID.jsm
@@ -154,51 +154,55 @@ var ClientIDImpl = {
     return this._loadClientIdTask;
   },
 
   /**
    * Load the Client ID from the DataReporting Service state file.
    * If no Client ID is found, we generate a new one.
    */
   async _doLoadClientID() {
+    this._log.trace(`_doLoadClientID`);
     // If there's a removal in progress, let's wait for it
     await this._removeClientIdTask;
 
     // Try to load the client id from the DRS state file.
     try {
       let state = await CommonUtils.readJSON(gStateFilePath);
       if (AppConstants.platform == "android" && state && "wasCanary" in state) {
         this._wasCanary = state.wasCanary;
       }
       if (state && this.updateClientID(state.clientID)) {
+        this._log.trace(`_doLoadClientID: Client id loaded from state.`);
         return this._clientID;
       }
     } catch (e) {
       // fall through to next option
     }
 
     // We dont have an id from the DRS state file yet, generate a new ID.
     this.updateClientID(CommonUtils.generateUUID());
     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.
     await this._saveClientIdTask;
 
+    this._log.trace("_doLoadClientID: New client id loaded and persisted.");
     return this._clientID;
   },
 
   /**
    * Save the client ID to the client ID file.
    *
    * @return {Promise} A promise resolved when the client ID is saved to disk.
    */
   async _saveClientID() {
+    this._log.trace(`_saveClientID`);
     let obj = { clientID: this._clientID };
     // We detected a canary client ID when resetting, storing this as a flag
     if (AppConstants.platform == "android" && this._wasCanary) {
       obj.wasCanary = true;
     }
     await OS.File.makeDir(gDatareportingPath);
     await CommonUtils.writeJSON(obj, gStateFilePath);
     this._saveClientIdTask = null;
@@ -286,38 +290,45 @@ var ClientIDImpl = {
   async _reset() {
     await this._loadClientIdTask;
     await this._saveClientIdTask;
     this._clientID = null;
     this._clientIDHash = null;
   },
 
   async setClientID(id) {
+    this._log.trace("setClientID");
     if (!this.updateClientID(id)) {
       throw new Error("Invalid client ID: " + id);
     }
 
     this._saveClientIdTask = this._saveClientID();
     await this._saveClientIdTask;
     return this._clientID;
   },
 
   async _doRemoveClientID() {
+    this._log.trace("_doRemoveClientID");
+
     // Reset stored id.
     this._clientID = null;
     this._clientIDHash = null;
 
     // Clear the client id from the preference cache.
     Services.prefs.clearUserPref(PREF_CACHED_CLIENTID);
 
+    // If there is a save in progress, wait for it to complete.
+    await this._saveClientIdTask;
+
     // Remove the client id from disk
     await OS.File.remove(gStateFilePath, { ignoreAbsent: true });
   },
 
   async resetClientID() {
+    this._log.trace("resetClientID");
     let oldClientId = this._clientID;
 
     // Wait for the removal.
     // Asynchronous calls to getClientID will also be blocked on this.
     this._removeClientIdTask = this._doRemoveClientID();
     let clear = () => (this._removeClientIdTask = null);
     this._removeClientIdTask.then(clear, clear);