Bug 1656154 - Allow ensureEcosystemAnonId to use a stale cached profile. r=rfkelly

Also ensures only one `ensureEcosystemAnonId()` is in flight at once,
`getEcosystemAnonId()` no longer calls `ensureEcosystemAnonId()` and a minor
fix or 2.

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

services/fxaccounts/FxAccountsProfile.jsm

@@ -166,11 +166,12 @@ FxAccountsProfile.prototype = {
   // Get the user's profile data, fetching from the network if necessary.
   // Most callers should instead use `getProfile()`; this methods exists to support
   // callers who need to await the underlying network request.
-  async ensureProfile() {
+  async ensureProfile({ staleOk = false } = {}) {
     const profileCache = await this._getProfileCache();
     if (
       !profileCache ||
-      Date.now() > this._cachedAt + this.PROFILE_FRESHNESS_THRESHOLD
+      (Date.now() > this._cachedAt + this.PROFILE_FRESHNESS_THRESHOLD &&
+        !staleOk)
     ) {
       const profile = await this._fetchAndCacheProfile().catch(err => {
         log.error("Background refresh of profile failed", err);

services/fxaccounts/FxAccountsTelemetry.jsm

@@ -40,6 +40,7 @@ class FxAccountsTelemetry {
   constructor(fxai) {
     this._fxai = fxai;
     Services.telemetry.setEventRecordingEnabled("fxa", true);
+    this._promiseEnsureEcosystemAnonId = null;
   }
 
   // Records an event *in the Fxa/Sync ping*.
@@ -82,15 +83,16 @@ class FxAccountsTelemetry {
   async getEcosystemAnonId() {
     try {
       // N.B. `getProfile()` may kick off a silent background update but won't await network requests.
-      const profile = await this._internal.profile.getProfile();
-      if (profile.hasOwnProperty("ecosystemAnonId")) {
+      const profile = await this._fxai.profile.getProfile();
+      if (profile && profile.hasOwnProperty("ecosystemAnonId")) {
         return profile.ecosystemAnonId;
       }
     } catch (err) {
       log.error("Getting ecosystemAnonId from profile failed", err);
     }
-    // Calling `ensureEcosystemAnonId()` so the calling code doesn't have to do this when a call
-    // to `getProfile()` doesn't produce `ecosystemAnonId`.
+    // Calling `ensureEcosystemAnonId()` so the calling code doesn't have to do
+    // this when a call/ to `getProfile()` doesn't produce `ecosystemAnonId`.
+    // (ie, so that the next call to `getEcosystemAnonId() will return it)
     this.ensureEcosystemAnonId().catch(err => {
       log.error("Failed ensuring we have an anon-id in the background ", err);
     });
@@ -102,11 +104,30 @@ class FxAccountsTelemetry {
   // This asynchronous method resolves with the "ecosystemAnonId" value on success, and rejects
   // with an error if no user is signed in or if the value could not be obtained from the
   // FxA server.
-  async ensureEcosystemAnonId(generatePlaceholder = true) {
+  //
+  // If a call to this is already in-flight, the promise from that original
+  // call is returned.
+  async ensureEcosystemAnonId() {
+    if (!this._promiseEnsureEcosystemAnonId) {
+      this._promiseEnsureEcosystemAnonId = this._ensureEcosystemAnonId().finally(
+        () => {
+          this._promiseEnsureEcosystemAnonId = null;
+        }
+      );
+    }
+    return this._promiseEnsureEcosystemAnonId;
+  }
+
+  async _ensureEcosystemAnonId(generatePlaceholder = true) {
     const telemetry = this;
     return this._fxai.withCurrentAccountState(async function(state) {
-      const profile = await telemetry._fxai.profile.ensureProfile();
-      if (profile.hasOwnProperty("ecosystemAnonId")) {
+      // Fetching a fresh profile should never update the ID, and saving a
+      // network request matters for telemetry, so we are fine with a slightly
+      // stale profile.
+      const profile = await telemetry._fxai.profile.ensureProfile({
+        staleOk: true,
+      });
+      if (profile && profile.hasOwnProperty("ecosystemAnonId")) {
         return profile.ecosystemAnonId;
       }
       if (!generatePlaceholder) {
@@ -140,7 +161,7 @@ class FxAccountsTelemetry {
       } catch (err) {
         if (err && err.code && err.code === 412) {
           // Another client raced us to upload the placeholder, fetch it.
-          return telemetry.ensureEcosystemAnonId(false);
+          return telemetry._ensureEcosystemAnonId(false);
         }
         throw err;
       }

services/fxaccounts/tests/xpcshell/test_profile.js

@@ -358,7 +358,7 @@ add_task(async function test_ensureProfile() {
     // profile retrieval when there is no cached profile info
     {
       threshold: 1000,
-      hasRecentCachedProfile: false,
+      expectsCachedProfileReturned: false,
       cachedProfile: null,
       fetchedProfile: {
         uid: ACCOUNT_UID,
@@ -371,7 +371,7 @@ add_task(async function test_ensureProfile() {
       // Note: The threshold for this test case is being set to an arbitrary value that will
       // be greater than Date.now() so the retrieved cached profile will be deemed recent.
       threshold: Date.now() + 5000,
-      hasRecentCachedProfile: true,
+      expectsCachedProfileReturned: true,
       cachedProfile: {
         uid: `${ACCOUNT_UID}2`,
         email: `${ACCOUNT_EMAIL}2`,
@@ -381,7 +381,7 @@ add_task(async function test_ensureProfile() {
     // profile retrieval when the cached profile is old and a new profile is fetched
     {
       threshold: 1000,
-      hasRecentCachedProfile: false,
+      expectsCachedProfileReturned: false,
       cachedProfile: {
         uid: `${ACCOUNT_UID}3`,
         email: `${ACCOUNT_EMAIL}3`,
@@ -397,7 +397,7 @@ add_task(async function test_ensureProfile() {
     // profile retrieval when the cached profile is old and a null profile is fetched
     {
       threshold: 1000,
-      hasRecentCachedProfile: false,
+      expectsCachedProfileReturned: false,
       cachedProfile: {
         uid: `${ACCOUNT_UID}5`,
         email: `${ACCOUNT_EMAIL}5`,
@@ -409,7 +409,7 @@ add_task(async function test_ensureProfile() {
     // profile retrieval when the cached profile is old and fetching a new profile errors
     {
       threshold: 1000,
-      hasRecentCachedProfile: false,
+      expectsCachedProfileReturned: false,
       cachedProfile: {
         uid: `${ACCOUNT_UID}6`,
         email: `${ACCOUNT_EMAIL}6`,
@@ -422,7 +422,7 @@ add_task(async function test_ensureProfile() {
       // Note: The threshold for this test case is being set to an arbitrary value that will
       // be greater than Date.now() so the retrieved cached profile will be deemed recent.
       threshold: Date.now() + 5000,
-      hasRecentCachedProfile: false,
+      expectsCachedProfileReturned: false,
       cachedProfile: null,
       fetchedProfile: {
         uid: `${ACCOUNT_UID}7`,
@@ -431,9 +431,34 @@ add_task(async function test_ensureProfile() {
       },
       fetchAndCacheProfileResolves: true,
     },
+    // profile retrieval when the cached profile is old but staleOk is true.
+    {
+      threshold: 1000,
+      expectsCachedProfileReturned: true,
+      cachedProfile: {
+        uid: `${ACCOUNT_UID}5`,
+        email: `${ACCOUNT_EMAIL}5`,
+        avatar: "myimg5",
+      },
+      fetchAndCacheProfileResolves: false,
+      staleOk: true,
+    },
+    // staleOk but no cached profile
+    {
+      threshold: 1000,
+      expectsCachedProfileReturned: false,
+      cachedProfile: null,
+      fetchedProfile: {
+        uid: ACCOUNT_UID,
+        email: ACCOUNT_EMAIL,
+        avatar: "myimg",
+      },
+      staleOk: true,
+    },
   ];
 
   for (const tc of testCases) {
+    print(`test case: ${JSON.stringify(tc)}`);
     let mockProfile = sinon.mock(profile);
     mockProfile
       .expects("_getProfileCache")
@@ -447,10 +472,13 @@ add_task(async function test_ensureProfile() {
       );
     profile.PROFILE_FRESHNESS_THRESHOLD = tc.threshold;
 
-    if (tc.hasRecentCachedProfile) {
+    let options = {};
+    if (tc.staleOk || false) {
+      options.staleOk = true;
+    }
+    if (tc.expectsCachedProfileReturned) {
       mockProfile.expects("_fetchAndCacheProfile").never();
-
-      let actualProfile = await profile.ensureProfile();
+      let actualProfile = await profile.ensureProfile(options);
       mockProfile.verify();
       Assert.equal(actualProfile, tc.cachedProfile);
     } else if (tc.fetchAndCacheProfileResolves) {
@@ -459,7 +487,7 @@ add_task(async function test_ensureProfile() {
         .once()
         .resolves(tc.fetchedProfile);
 
-      let actualProfile = await profile.ensureProfile();
+      let actualProfile = await profile.ensureProfile(options);
       let expectedProfile = tc.fetchedProfile
         ? tc.fetchedProfile
         : tc.cachedProfile;
@@ -471,7 +499,7 @@ add_task(async function test_ensureProfile() {
         .once()
         .rejects();
 
-      let actualProfile = await profile.ensureProfile();
+      let actualProfile = await profile.ensureProfile(options);
       mockProfile.verify();
       Assert.equal(actualProfile, tc.cachedProfile);
     }

services/fxaccounts/tests/xpcshell/test_telemetry.js

@@ -26,6 +26,7 @@ const { FxAccountsTelemetry } = ChromeUtils.import(
 XPCOMUtils.defineLazyModuleGetters(this, {
   FxAccountsConfig: "resource://gre/modules/FxAccountsConfig.jsm",
   jwcrypto: "resource://services-crypto/jwcrypto.jsm",
+  PromiseUtils: "resource://gre/modules/PromiseUtils.jsm",
 });
 
 _("Misc tests for FxAccounts.telemetry");
@@ -99,7 +100,7 @@ add_task(async function test_getEcosystemAnonId() {
       profileServerUrl: "http://testURL",
     });
     const telemetry = new FxAccountsTelemetry({});
-    telemetry._internal = { profile };
+    telemetry._fxai = { profile };
     const mockProfile = sinon.mock(profile);
     const mockTelemetry = sinon.mock(telemetry);
 
@@ -131,55 +132,6 @@ add_task(async function test_getEcosystemAnonId() {
   }
 });
 
-add_task(async function test_ensureEcosystemAnonId_fromProfile() {
-  const expecteErrorMessage =
-    "Profile data does not contain an 'ecosystemAnonId'";
-  const expectedEcosystemAnonId = "aaaaaaaaaaa";
-
-  const testCases = [
-    {
-      profile: {
-        ecosystemAnonId: expectedEcosystemAnonId,
-      },
-      generatePlaceholder: true,
-    },
-    {
-      profile: {},
-      generatePlaceholder: false,
-    },
-  ];
-
-  for (const tc of testCases) {
-    const profile = new FxAccountsProfile({
-      profileServerUrl: "http://testURL",
-    });
-    const telemetry = new FxAccountsTelemetry({
-      profile,
-      withCurrentAccountState: async cb => {
-        return cb({});
-      },
-    });
-    const mockProfile = sinon.mock(profile);
-
-    mockProfile
-      .expects("ensureProfile")
-      .once()
-      .returns(tc.profile);
-
-    if (tc.profile.ecosystemAnonId) {
-      const actualEcoSystemAnonId = await telemetry.ensureEcosystemAnonId();
-      Assert.equal(actualEcoSystemAnonId, expectedEcosystemAnonId);
-    } else {
-      try {
-        await telemetry.ensureEcosystemAnonId(tc.generatePlaceholder);
-      } catch (e) {
-        Assert.equal(expecteErrorMessage, e.message);
-      }
-    }
-    mockProfile.verify();
-  }
-});
-
 add_task(async function test_ensureEcosystemAnonId_failToGenerateKeys() {
   const expectedErrorMessage =
     "Unable to fetch ecosystem_anon_id_keys from FxA server";
@@ -217,7 +169,7 @@ add_task(async function test_ensureEcosystemAnonId_failToGenerateKeys() {
       .returns(tc.serverConfig);
 
     try {
-      await telemetry.ensureEcosystemAnonId(true);
+      await telemetry.ensureEcosystemAnonId();
     } catch (e) {
       Assert.equal(expectedErrorMessage, e.message);
       mockProfile.verify();
@@ -226,6 +178,75 @@ add_task(async function test_ensureEcosystemAnonId_failToGenerateKeys() {
   }
 });
 
+add_task(async function test_ensureEcosystemAnonId_selfRace() {
+  const expectedEcosystemAnonId = "self-race-id";
+
+  const mockedUpdate = sinon.mock().once();
+
+  const profileClient = new FxAccountsProfileClient({
+    serverURL: "http://testURL",
+  });
+  const profile = new FxAccountsProfile({ profileClient });
+  const telemetry = new FxAccountsTelemetry({
+    profile,
+    withCurrentAccountState: async cb => {
+      return cb({ updateUserAccountData: mockedUpdate });
+    },
+  });
+
+  const mockProfile = sinon.mock(profile);
+  const mockFxAccountsConfig = sinon.mock(FxAccountsConfig);
+  const mockJwcrypto = sinon.mock(jwcrypto);
+  const mockProfileClient = sinon.mock(profileClient);
+
+  mockProfile
+    .expects("ensureProfile")
+    .once()
+    .returns({});
+
+  mockProfileClient
+    .expects("setEcosystemAnonId")
+    .once()
+    .returns(null);
+
+  // We are going to "block" the config document promise and make 2 calls
+  // to ensureEcosystemAnonId() while blocked, just to ensure we don't
+  // actually enter the ensureEcosystemAnonId() impl twice.
+  const deferInConfigDocument = PromiseUtils.defer();
+  const deferConfigDocument = PromiseUtils.defer();
+  mockFxAccountsConfig
+    .expects("fetchConfigDocument")
+    .once()
+    .callsFake(() => {
+      deferInConfigDocument.resolve();
+      return deferConfigDocument.promise;
+    });
+
+  mockJwcrypto
+    .expects("generateJWE")
+    .once()
+    .returns(expectedEcosystemAnonId);
+
+  let p1 = telemetry.ensureEcosystemAnonId();
+  let p2 = telemetry.ensureEcosystemAnonId();
+
+  // Make sure we've entered fetchConfigDocument
+  await deferInConfigDocument.promise;
+  // Let it go.
+  deferConfigDocument.resolve({ ecosystem_anon_id_keys: ["testKey"] });
+
+  Assert.equal(await p1, expectedEcosystemAnonId);
+  Assert.equal(await p2, expectedEcosystemAnonId);
+
+  // And all the `.once()` calls on the mocks are checking we only did the
+  // work once.
+  mockedUpdate.verify();
+  mockProfile.verify();
+  mockFxAccountsConfig.verify();
+  mockJwcrypto.verify();
+  mockProfileClient.verify();
+});
+
 add_task(async function test_ensureEcosystemAnonId_clientRace() {
   const expectedEcosystemAnonId = "bbbbbbbbbbbb";
   const expectedErrrorMessage = "test error at 'setEcosystemAnonId'";
@@ -290,11 +311,11 @@ add_task(async function test_ensureEcosystemAnonId_clientRace() {
           ecosystemAnonId: expectedEcosystemAnonId,
         });
 
-      const actualEcosystemAnonId = await telemetry.ensureEcosystemAnonId(true);
+      const actualEcosystemAnonId = await telemetry.ensureEcosystemAnonId();
       Assert.equal(expectedEcosystemAnonId, actualEcosystemAnonId);
     } else {
       try {
-        await telemetry.ensureEcosystemAnonId(true);
+        await telemetry.ensureEcosystemAnonId();
       } catch (e) {
         Assert.equal(expectedErrrorMessage, e.message);
       }