Bug 1497960: Make ProfileAge return the same instances for the same profile directory and preload times.json to avoid data races. r=janerik

ProfileAge now returns a promise that resolves to an instance that has already
loaded its times.json. This makes multiple attempts to update data in times.json
safer.

Differential Revision: https://phabricator.services.mozilla.com/D8463

--HG--
extra : moz-landing-system : lando

browser/components/migration/FirefoxProfileMigrator.js

@@ -218,11 +218,17 @@ FirefoxProfileMigrator.prototype._getResourcesInternal = function(sourceProfileD
         file.copyTo(currentProfileDir, "");
       }
       // And record the fact a migration (ie, a reset) happened.
-      let timesAccessor = new ProfileAge(currentProfileDir.path);
-      timesAccessor.recordProfileReset().then(
-        () => aCallback(true),
-        () => aCallback(false)
-      );
+      let recordMigration = async () => {
+        try {
+          let profileTimes = await ProfileAge(currentProfileDir.path);
+          await profileTimes.recordProfileReset();
+          aCallback(true);
+        } catch (e) {
+          aCallback(false);
+        }
+      };
+
+      recordMigration();
     },
   };
   let telemetry = {

browser/components/newtab/lib/ASRouterTargeting.jsm

@@ -125,10 +125,10 @@ const TargetingGetters = {
     return new Date();
   },
   get profileAgeCreated() {
-    return new ProfileAge(null, null).created;
+    return ProfileAge().then(times => times.created);
   },
   get profileAgeReset() {
-    return new ProfileAge(null, null).reset;
+    return ProfileAge().then(times => times.reset);
   },
   get usesFirefoxSync() {
     return Services.prefs.prefHasUserValue(FXA_USERNAME_PREF);

browser/components/newtab/lib/ManualMigration.jsm

@@ -39,7 +39,7 @@ this.ManualMigration = class ManualMigration {
   }
 
   async isMigrationMessageExpired() {
-    let profileAge = new ProfileAge();
+    let profileAge = await ProfileAge();
     let profileCreationDate = await profileAge.created;
     let daysSinceProfileCreation = (Date.now() - profileCreationDate) / MS_PER_DAY;
 

browser/components/newtab/lib/SnippetsFeed.jsm

@@ -59,7 +59,7 @@ this.SnippetsFeed = class SnippetsFeed {
   }
 
   async getProfileInfo() {
-    const profileAge = new ProfileAge(null, null);
+    const profileAge = await ProfileAge();
     const createdDate = await profileAge.created;
     const resetDate = await profileAge.reset;
     return {

browser/components/newtab/test/browser/browser_asrouter_targeting.js

@@ -109,7 +109,7 @@ add_task(async function check_localeLanguageCode() {
 });
 
 add_task(async function checkProfileAgeCreated() {
-  let profileAccessor = new ProfileAge();
+  let profileAccessor = await ProfileAge();
   is(await ASRouterTargeting.Environment.profileAgeCreated, await profileAccessor.created,
     "should return correct profile age creation date");
 
@@ -119,7 +119,7 @@ add_task(async function checkProfileAgeCreated() {
 });
 
 add_task(async function checkProfileAgeReset() {
-  let profileAccessor = new ProfileAge();
+  let profileAccessor = await ProfileAge();
   is(await ASRouterTargeting.Environment.profileAgeReset, await profileAccessor.reset,
     "should return correct profile age reset");
 

browser/components/nsBrowserGlue.js

@@ -1152,7 +1152,7 @@ BrowserGlue.prototype = {
 
   async _calculateProfileAgeInDays() {
     let ProfileAge = ChromeUtils.import("resource://gre/modules/ProfileAge.jsm", {}).ProfileAge;
-    let profileAge = new ProfileAge(null, null);
+    let profileAge = await ProfileAge();
 
     let creationDate = await profileAge.created;
     let resetDate = await profileAge.reset;

browser/components/uitour/UITour.jsm

@@ -1550,7 +1550,7 @@ var UITour = {
 
       // Expose Profile creation and last reset dates in weeks.
       const ONE_WEEK = 7 * 24 * 60 * 60 * 1000;
-      let profileAge = new ProfileAge(null, null);
+      let profileAge = await ProfileAge();
       let createdDate = await profileAge.created;
       let resetDate = await profileAge.reset;
       let createdWeeksAgo = Math.floor((Date.now() - createdDate) / ONE_WEEK);

browser/components/uitour/test/browser_UITour.js

@@ -322,12 +322,13 @@ var tests = [
       ok(result.profileResetWeeksAgo === null, "profileResetWeeksAgo should be null.");
 
       // Set profile reset date to 15 days ago.
-      let profileAccessor = new ProfileAge();
-      profileAccessor.recordProfileReset(Date.now() - (15 * 24 * 60 * 60 * 1000));
-      gContentAPI.getConfiguration("appinfo", (result2) => {
-        ok(typeof(result2.profileResetWeeksAgo) === "number", "profileResetWeeksAgo should be number.");
-        is(result2.profileResetWeeksAgo, 2, "profileResetWeeksAgo should be 2.");
-        done();
+      ProfileAge().then(profileAccessor => {
+        profileAccessor.recordProfileReset(Date.now() - (15 * 24 * 60 * 60 * 1000));
+        gContentAPI.getConfiguration("appinfo", (result2) => {
+          ok(typeof(result2.profileResetWeeksAgo) === "number", "profileResetWeeksAgo should be number.");
+          is(result2.profileResetWeeksAgo, 2, "profileResetWeeksAgo should be 2.");
+          done();
+        });
       });
     });
   },

toolkit/components/places/UnifiedComplete.js

@@ -411,8 +411,9 @@ XPCOMUtils.defineLazyGetter(this, "PreloadedSiteStorage", () => Object.seal({
   },
 }));
 
-XPCOMUtils.defineLazyGetter(this, "ProfileAgeCreatedPromise", () => {
-  return (new ProfileAge(null, null)).created;
+XPCOMUtils.defineLazyGetter(this, "ProfileAgeCreatedPromise", async () => {
+  let times = await ProfileAge();
+  return times.created;
 });
 
 // Helper functions

toolkit/components/telemetry/app/TelemetryEnvironment.jsm

@@ -1443,8 +1443,7 @@ EnvironmentCache.prototype = {
    * @returns Promise<> resolved when the I/O is complete.
    */
   async _updateProfile() {
-    const logger = Log.repository.getLoggerWithMessagePrefix(LOGGER_NAME, "ProfileAge - ");
-    let profileAccessor = new ProfileAge(null, logger);
+    let profileAccessor = await ProfileAge();
 
     let creationDate = await profileAccessor.created;
     let resetDate = await profileAccessor.reset;

toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js

@@ -11,6 +11,8 @@ ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm", this);
 ChromeUtils.import("resource://testing-common/httpd.js");
 ChromeUtils.import("resource://testing-common/MockRegistrar.jsm", this);
 ChromeUtils.import("resource://gre/modules/FileUtils.jsm");
+ChromeUtils.import("resource://services-common/utils.js");
+ChromeUtils.import("resource://gre/modules/osfile.jsm");
 
 // AttributionCode is only needed for Firefox
 ChromeUtils.defineModuleGetter(this, "AttributionCode",
@@ -20,9 +22,6 @@ ChromeUtils.defineModuleGetter(this, "AttributionCode",
 ChromeUtils.defineModuleGetter(this, "LightweightThemeManager",
                                "resource://gre/modules/LightweightThemeManager.jsm");
 
-ChromeUtils.defineModuleGetter(this, "ProfileAge",
-                               "resource://gre/modules/ProfileAge.jsm");
-
 ChromeUtils.defineModuleGetter(this, "ExtensionTestUtils",
                                "resource://testing-common/ExtensionXPCShellUtils.jsm");
 
@@ -296,12 +295,10 @@ function spoofGfxAdapter() {
 }
 
 function spoofProfileReset() {
-  let profileAccessor = new ProfileAge();
-
-  return profileAccessor.writeTimes({
+  return CommonUtils.writeJSON({
     created: PROFILE_CREATION_DATE_MS,
     reset: PROFILE_RESET_DATE_MS,
-  });
+  }, OS.Path.join(OS.Constants.Path.profileDir, "times.json"));
 }
 
 function spoofPartnerInfo() {

toolkit/modules/ProfileAge.jsm

@@ -12,6 +12,8 @@ ChromeUtils.import("resource://gre/modules/osfile.jsm");
 ChromeUtils.import("resource://gre/modules/Log.jsm");
 ChromeUtils.import("resource://services-common/utils.js");
 
+const FILE_TIMES = "times.json";
+
 /**
  * Calculate how many days passed between two dates.
  * @param {Object} aStartDate The starting date.
@@ -22,134 +24,28 @@ function getElapsedTimeInDays(aStartDate, aEndDate) {
   return TelemetryUtils.millisecondsToDays(aEndDate - aStartDate);
 }
 
+
 /**
- * Profile access to times.json (eg, creation/reset time).
- * This is separate from the provider to simplify testing and enable extraction
- * to a shared location in the future.
+ * Traverse the contents of the profile directory, finding the oldest file
+ * and returning its creation timestamp.
  */
-var ProfileAge = function(profile, log) {
-  this.profilePath = profile || OS.Constants.Path.profileDir;
-  if (!this.profilePath) {
-    throw new Error("No profile directory.");
+async function getOldestProfileTimestamp(profilePath, log) {
+  let start = Date.now();
+  let oldest = start + 1000;
+  let iterator = new OS.File.DirectoryIterator(profilePath);
+  log.debug("Iterating over profile " + profilePath);
+  if (!iterator) {
+    throw new Error("Unable to fetch oldest profile entry: no profile iterator.");
   }
-  if (!log) {
-    log = Log.repository.getLogger("Toolkit.ProfileAge");
-  }
-  this._log = log;
-};
-this.ProfileAge.prototype = {
-  /**
-   * There are three ways we can get our creation time:
-   *
-   * 1. From our own saved value (to avoid redundant work).
-   * 2. From the on-disk JSON file.
-   * 3. By calculating it from the filesystem.
-   *
-   * If we have to calculate, we write out the file; if we have
-   * to touch the file, we persist in-memory.
-   *
-   * @return a promise that resolves to the profile's creation time.
-   */
-  get created() {
-    function onSuccess(times) {
-      if (times.created) {
-        return times.created;
-      }
-      return onFailure.call(this, null, times);
-    }
 
-    function onFailure(err, times) {
-      return this.computeAndPersistCreated(times)
-                 .then(function onSuccess(created) {
-                         return created;
-                       });
-    }
+  Services.telemetry.scalarAdd("telemetry.profile_directory_scans", 1);
+  let histogram = Services.telemetry.getHistogramById("PROFILE_DIRECTORY_FILE_AGE");
 
-    return this.getTimes()
-               .then(onSuccess.bind(this),
-                     onFailure.bind(this));
-  },
+  try {
+    await iterator.forEach(async (entry) => {
+      try {
+        let info = await OS.File.stat(entry.path);
 
-  /**
-   * Explicitly make `file`, a filename, a full path
-   * relative to our profile path.
-   */
-  getPath(file) {
-    return OS.Path.join(this.profilePath, file);
-  },
-
-  /**
-   * Return a promise which resolves to the JSON contents
-   * of the time file, using the already read value if possible.
-   */
-  getTimes(file = "times.json") {
-    if (this._times) {
-      return Promise.resolve(this._times);
-    }
-    return this.readTimes(file).then(
-      times => {
-        return this._times = times || {};
-      }
-    );
-  },
-
-  /**
-   * Return a promise which resolves to the JSON contents
-   * of the time file in this accessor's profile.
-   */
-  readTimes(file = "times.json") {
-    return CommonUtils.readJSON(this.getPath(file));
-  },
-
-  /**
-   * Return a promise representing the writing of `contents`
-   * to `file` in the specified profile.
-   */
-  writeTimes(contents, file = "times.json") {
-    return CommonUtils.writeJSON(contents, this.getPath(file));
-  },
-
-  /**
-   * Merge existing contents with a 'created' field, writing them
-   * to the specified file. Promise, naturally.
-   */
-  computeAndPersistCreated(existingContents, file = "times.json") {
-    let path = this.getPath(file);
-    function onOldest(oldest) {
-      let contents = existingContents || {};
-      contents.created = oldest;
-      this._times = contents;
-      Services.telemetry.scalarSet("telemetry.profile_directory_scan_date",
-        TelemetryUtils.millisecondsToDays(Date.now()));
-      return this.writeTimes(contents, path)
-                 .then(function onSuccess() {
-                   return oldest;
-                 });
-    }
-
-    return this.getOldestProfileTimestamp()
-               .then(onOldest.bind(this));
-  },
-
-  /**
-   * Traverse the contents of the profile directory, finding the oldest file
-   * and returning its creation timestamp.
-   */
-  getOldestProfileTimestamp() {
-    let self = this;
-    let start = Date.now();
-    let oldest = start + 1000;
-    let iterator = new OS.File.DirectoryIterator(this.profilePath);
-    self._log.debug("Iterating over profile " + this.profilePath);
-    if (!iterator) {
-      throw new Error("Unable to fetch oldest profile entry: no profile iterator.");
-    }
-
-    Services.telemetry.scalarAdd("telemetry.profile_directory_scans", 1);
-    let histogram = Services.telemetry.getHistogramById("PROFILE_DIRECTORY_FILE_AGE");
-
-    function onEntry(entry) {
-      function onStatSuccess(info) {
         // OS.File doesn't seem to be behaving. See Bug 827148.
         // Let's do the best we can. This whole function is defensive.
         let date = info.winBirthDate || info.macBirthDate;
@@ -158,7 +54,7 @@ this.ProfileAge.prototype = {
           // and Windows, where birthTime is defined.
           // That means we're unable to function on Linux, so we use mtime
           // instead.
-          self._log.debug("No birth date. Using mtime.");
+          log.debug("No birth date. Using mtime.");
           date = info.lastModificationDate;
         }
 
@@ -169,36 +65,83 @@ this.ProfileAge.prototype = {
           let age_in_days = Math.max(0, getElapsedTimeInDays(timestamp, start));
           histogram.add(age_in_days);
 
-          self._log.debug("Using date: " + entry.path + " = " + date);
+          log.debug("Using date: " + entry.path + " = " + date);
           if (timestamp < oldest) {
             oldest = timestamp;
           }
         }
-      }
-
-      function onStatFailure(e) {
+      } catch (e) {
         // Never mind.
-        self._log.debug("Stat failure", e);
+        log.debug("Stat failure", e);
       }
+    });
+  } catch (reason) {
+    throw new Error("Unable to fetch oldest profile entry: " + reason);
+  } finally {
+    iterator.close();
+  }
 
-      return OS.File.stat(entry.path)
-                    .then(onStatSuccess, onStatFailure);
+  return oldest;
+}
+
+/**
+ * Profile access to times.json (eg, creation/reset time).
+ * This is separate from the provider to simplify testing and enable extraction
+ * to a shared location in the future.
+ */
+class ProfileAgeImpl {
+  constructor(profile, times) {
+    this.profilePath = profile || OS.Constants.Path.profileDir;
+    this._times = times;
+    this._log = Log.repository.getLogger("Toolkit.ProfileAge");
+  }
+
+  /**
+   * There are two ways we can get our creation time:
+   *
+   * 1. From the on-disk JSON file.
+   * 2. By calculating it from the filesystem.
+   *
+   * If we have to calculate, we write out the file; if we have
+   * to touch the file, we persist in-memory.
+   *
+   * @return a promise that resolves to the profile's creation time.
+   */
+  get created() {
+    // This can be an expensive operation so make sure we only do it once.
+    if (this._created) {
+      return this._created;
     }
 
-    let promise = iterator.forEach(onEntry);
-
-    function onSuccess() {
-      iterator.close();
-      return oldest;
+    if (!this._times.created) {
+      this._created = this.computeAndPersistCreated();
+    } else {
+      this._created = Promise.resolve(this._times.created);
     }
 
-    function onFailure(reason) {
-      iterator.close();
-      throw new Error("Unable to fetch oldest profile entry: " + reason);
-    }
+    return this._created;
+  }
 
-    return promise.then(onSuccess, onFailure);
-  },
+  /**
+   * Return a promise representing the writing the current times to the profile.
+   */
+  writeTimes() {
+    return CommonUtils.writeJSON(this._times, OS.Path.join(this.profilePath, FILE_TIMES));
+  }
+
+  /**
+   * Calculates the created time by scanning the profile directory, sets it in
+   * the current set of times and persists it to the profile. Returns a promise
+   * that resolves when all of that is complete.
+   */
+  async computeAndPersistCreated() {
+    let oldest = await getOldestProfileTimestamp(this.profilePath, this._log);
+    this._times.created = oldest;
+    Services.telemetry.scalarSet("telemetry.profile_directory_scan_date",
+      TelemetryUtils.millisecondsToDays(Date.now()));
+    await this.writeTimes();
+    return oldest;
+  }
 
   /**
    * Record (and persist) when a profile reset happened.  We just store a
@@ -207,21 +150,49 @@ this.ProfileAge.prototype = {
    * be able to make use of that.
    * Returns a promise that is resolved once the file has been written.
    */
-  recordProfileReset(time = Date.now(), file = "times.json") {
-    return this.getTimes(file).then(
-      times => {
-        times.reset = time;
-        return this.writeTimes(times, file);
-      }
-    );
-  },
+  recordProfileReset(time = Date.now()) {
+    this._times.reset = time;
+    return this.writeTimes();
+  }
 
   /* Returns a promise that resolves to the time the profile was reset,
    * or undefined if not recorded.
    */
   get reset() {
-    return this.getTimes().then(
-      times => times.reset
-    );
-  },
-};
+    if ("reset" in this._times) {
+      return Promise.resolve(this._times.reset);
+    }
+    return Promise.resolve(undefined);
+  }
+}
+
+// A Map from profile directory to a promise that resolves to the ProfileAgeImpl.
+const PROFILES = new Map();
+
+async function initProfileAge(profile) {
+  let timesPath = OS.Path.join(profile, FILE_TIMES);
+
+  try {
+    let times = await CommonUtils.readJSON(timesPath);
+    return new ProfileAgeImpl(profile, times || {});
+  } catch (e) {
+    return new ProfileAgeImpl(profile, {});
+  }
+}
+
+/**
+ * Returns a promise that resolves to an instance of ProfileAgeImpl. Will always
+ * return the same instance for every call for the same profile.
+ *
+ * @param {string} profile The path to the profile directory.
+ * @return {Promise<ProfileAgeImpl>} Resolves to the ProfileAgeImpl.
+ */
+function ProfileAge(profile = OS.Constants.Path.profileDir) {
+  if (PROFILES.has(profile)) {
+    return PROFILES.get(profile);
+  }
+
+  let promise = initProfileAge(profile);
+  PROFILES.set(profile, promise);
+  return promise;
+}

toolkit/modules/tests/xpcshell/test_ProfileAge.js

@@ -0,0 +1,63 @@
+ChromeUtils.import("resource://gre/modules/ProfileAge.jsm");
+ChromeUtils.import("resource://gre/modules/osfile.jsm");
+ChromeUtils.import("resource://services-common/utils.js");
+
+const gProfD = do_get_profile();
+
+// Creates a unique profile directory to use for a test.
+function withDummyProfile(task) {
+  return async () => {
+    let profile = OS.Path.join(gProfD.path, "" + Math.floor(Math.random() * 100));
+    await OS.File.makeDir(profile);
+    await task(profile);
+    await OS.File.removeDir(profile);
+  };
+}
+
+add_task(withDummyProfile(async (profile) => {
+  let times = await ProfileAge(profile);
+  Assert.ok((await times.created) > 0, "We can't really say what this will be, just assume if it is a number it's ok.");
+  Assert.equal(await times.reset, undefined, "Reset time is undefined in a new profile");
+}));
+
+add_task(withDummyProfile(async (profile) => {
+  const CREATED_TIME = Date.now() - 2000;
+  const RESET_TIME = Date.now() - 1000;
+
+  CommonUtils.writeJSON({
+    created: CREATED_TIME,
+  }, OS.Path.join(profile, "times.json"));
+
+  let times = await ProfileAge(profile);
+  Assert.equal((await times.created), CREATED_TIME, "Should have seen the right profile time.");
+
+  let times2 = await ProfileAge(profile);
+  Assert.equal(times, times2, "Should have got the same instance.");
+
+  let promise = times.recordProfileReset(RESET_TIME);
+  Assert.equal((await times2.reset), RESET_TIME, "Should have seen the right reset time in the second instance immediately.");
+  await promise;
+
+  let results = await CommonUtils.readJSON(OS.Path.join(profile, "times.json"));
+  Assert.deepEqual(results, {
+    created: CREATED_TIME,
+    reset: RESET_TIME,
+  }, "Should have seen the right results.");
+}));
+
+add_task(withDummyProfile(async (profile) => {
+  const RESET_TIME = Date.now() - 1000;
+  const RESET_TIME2 = Date.now() - 2000;
+
+  // The last call to recordProfileReset should always win.
+  let times = await ProfileAge(profile);
+  await Promise.all([
+    times.recordProfileReset(RESET_TIME),
+    times.recordProfileReset(RESET_TIME2),
+  ]);
+
+  let results = await CommonUtils.readJSON(OS.Path.join(profile, "times.json"));
+  Assert.deepEqual(results, {
+    reset: RESET_TIME2,
+  }, "Should have seen the right results.");
+}));

toolkit/modules/tests/xpcshell/xpcshell.ini

@@ -72,3 +72,4 @@ skip-if = toolkit == 'android'
 [test_Log_stackTrace.js]
 [test_servicerequest_xhr.js]
 [test_EventEmitter.js]
+[test_ProfileAge.js]