diff --git a/services/fxaccounts/FxAccountsProfile.jsm b/services/fxaccounts/FxAccountsProfile.jsm index 333d9ba26cc..b63cd64c1d7 100644 --- a/services/fxaccounts/FxAccountsProfile.jsm +++ b/services/fxaccounts/FxAccountsProfile.jsm @@ -73,12 +73,18 @@ function hasChanged(oldData, newData) { this.FxAccountsProfile = function (options = {}) { this._cachedProfile = null; + this._cachedAt = 0; // when we saved the cached version. + this._currentFetchPromise = null; + this._isNotifying = false; // are we sending a notification? this.fxa = options.fxa || fxAccounts; this.client = options.profileClient || new FxAccountsProfileClient({ fxa: this.fxa, serverURL: options.profileServerUrl, }); + // An observer to invalidate our _cachedAt optimization. We use a weak-ref + // just incase this.tearDown isn't called in some cases. + Services.obs.addObserver(this, ON_PROFILE_CHANGE_NOTIFICATION, true); // for testing if (options.channel) { this.channel = options.channel; @@ -86,11 +92,25 @@ this.FxAccountsProfile = function (options = {}) { } this.FxAccountsProfile.prototype = { + // If we get subsequent requests for a profile within this period, don't bother + // making another request to determine if it is fresh or not. + PROFILE_FRESHNESS_THRESHOLD: 120000, // 2 minutes + + observe(subject, topic, data) { + // If we get a profile change notification from our webchannel it means + // the user has just changed their profile via the web, so we want to + // ignore our "freshness threshold" + if (topic == ON_PROFILE_CHANGE_NOTIFICATION && !this._isNotifying) { + log.debug("FxAccountsProfile observed profile change"); + this._cachedAt = 0; + } + }, tearDown: function () { this.fxa = null; this.client = null; this._cachedProfile = null; + Services.obs.removeObserver(this, ON_PROFILE_CHANGE_NOTIFICATION); }, _getCachedProfile: function () { @@ -100,7 +120,9 @@ this.FxAccountsProfile.prototype = { }, _notifyProfileChange: function (uid) { + this._isNotifying = true; Services.obs.notifyObservers(null, ON_PROFILE_CHANGE_NOTIFICATION, uid); + this._isNotifying = false; }, // Cache fetched data if it is different from what's in the cache. @@ -111,6 +133,7 @@ this.FxAccountsProfile.prototype = { return Promise.resolve(null); // indicates no change (but only tests care) } this._cachedProfile = profileData; + this._cachedAt = Date.now(); return this.fxa.getSignedInUser() .then(userData => { log.debug("notifying profile changed for user ${uid}", userData); @@ -120,10 +143,20 @@ this.FxAccountsProfile.prototype = { }, _fetchAndCacheProfile: function () { - return this.client.fetchProfile() - .then(profile => { - return this._cacheProfile(profile).then(() => profile); + if (!this._currentFetchPromise) { + this._currentFetchPromise = this.client.fetchProfile().then(profile => { + return this._cacheProfile(profile).then(() => { + return profile; + }); + }).then(profile => { + this._currentFetchPromise = null; + return profile; + }, err => { + this._currentFetchPromise = null; + throw err; }); + } + return this._currentFetchPromise }, // Returns cached data right away if available, then fetches the latest profile @@ -133,11 +166,15 @@ this.FxAccountsProfile.prototype = { return this._getCachedProfile() .then(cachedProfile => { if (cachedProfile) { - // Note that _fetchAndCacheProfile isn't returned, so continues - // in the background. - this._fetchAndCacheProfile().catch(err => { - log.error("Background refresh of profile failed", err); - }); + if (Date.now() > this._cachedAt + this.PROFILE_FRESHNESS_THRESHOLD) { + // Note that _fetchAndCacheProfile isn't returned, so continues + // in the background. + this._fetchAndCacheProfile().catch(err => { + log.error("Background refresh of profile failed", err); + }); + } else { + log.trace("not checking freshness of profile as it remains recent"); + } return cachedProfile; } return this._fetchAndCacheProfile(); @@ -146,4 +183,9 @@ this.FxAccountsProfile.prototype = { return profile; }); }, + + QueryInterface: XPCOMUtils.generateQI([ + Ci.nsIObserver, + Ci.nsISupportsWeakReference, + ]), }; diff --git a/services/fxaccounts/tests/xpcshell/test_profile.js b/services/fxaccounts/tests/xpcshell/test_profile.js index 4cabb68cd7f..e0c92c15c44 100644 --- a/services/fxaccounts/tests/xpcshell/test_profile.js +++ b/services/fxaccounts/tests/xpcshell/test_profile.js @@ -164,6 +164,137 @@ add_test(function fetchAndCacheProfile_ok() { }); }); +// Check that a second profile request when one is already in-flight reuses +// the in-flight one. +add_task(function fetchAndCacheProfileOnce() { + // A promise that remains unresolved while we fire off 2 requests for + // a profile. + let resolveProfile; + let promiseProfile = new Promise(resolve => { + resolveProfile = resolve; + }); + let numFetches = 0; + let client = mockClient(mockFxa()); + client.fetchProfile = function () { + numFetches += 1; + return promiseProfile; + }; + let profile = CreateFxAccountsProfile(null, client); + + let request1 = profile._fetchAndCacheProfile(); + let request2 = profile._fetchAndCacheProfile(); + + // should be one request made to fetch the profile (but the promise returned + // by it remains unresolved) + do_check_eq(numFetches, 1); + + // resolve the promise. + resolveProfile({ avatar: "myimg"}); + + // both requests should complete with the same data. + let got1 = yield request1; + do_check_eq(got1.avatar, "myimg"); + let got2 = yield request1; + do_check_eq(got2.avatar, "myimg"); + + // and still only 1 request was made. + do_check_eq(numFetches, 1); +}); + +// Check that sharing a single fetch promise works correctly when the promise +// is rejected. +add_task(function fetchAndCacheProfileOnce() { + // A promise that remains unresolved while we fire off 2 requests for + // a profile. + let rejectProfile; + let promiseProfile = new Promise((resolve,reject) => { + rejectProfile = reject; + }); + let numFetches = 0; + let client = mockClient(mockFxa()); + client.fetchProfile = function () { + numFetches += 1; + return promiseProfile; + }; + let profile = CreateFxAccountsProfile(null, client); + + let request1 = profile._fetchAndCacheProfile(); + let request2 = profile._fetchAndCacheProfile(); + + // should be one request made to fetch the profile (but the promise returned + // by it remains unresolved) + do_check_eq(numFetches, 1); + + // reject the promise. + rejectProfile("oh noes"); + + // both requests should reject. + try { + yield request1; + throw new Error("should have rejected"); + } catch (ex if ex == "oh noes") {} + try { + yield request2; + throw new Error("should have rejected"); + } catch (ex if ex == "oh noes") {} + + // but a new request should work. + client.fetchProfile = function () { + return Promise.resolve({ avatar: "myimg"}); + }; + + let got = yield profile._fetchAndCacheProfile(); + do_check_eq(got.avatar, "myimg"); +}); + +// Check that a new profile request within PROFILE_FRESHNESS_THRESHOLD of the +// last one doesn't kick off a new request to check the cached copy is fresh. +add_task(function fetchAndCacheProfileAfterThreshold() { + let numFetches = 0; + let client = mockClient(mockFxa()); + client.fetchProfile = function () { + numFetches += 1; + return Promise.resolve({ avatar: "myimg"}); + }; + let profile = CreateFxAccountsProfile(null, client); + profile.PROFILE_FRESHNESS_THRESHOLD = 1000; + + yield profile.getProfile(); + do_check_eq(numFetches, 1); + + yield profile.getProfile(); + do_check_eq(numFetches, 1); + + yield new Promise(resolve => { + do_timeout(1000, resolve); + }); + + yield profile.getProfile(); + do_check_eq(numFetches, 2); +}); + +// Check that a new profile request within PROFILE_FRESHNESS_THRESHOLD of the +// last one *does* kick off a new request if ON_PROFILE_CHANGE_NOTIFICATION +// is sent. +add_task(function fetchAndCacheProfileBeforeThresholdOnNotification() { + let numFetches = 0; + let client = mockClient(mockFxa()); + client.fetchProfile = function () { + numFetches += 1; + return Promise.resolve({ avatar: "myimg"}); + }; + let profile = CreateFxAccountsProfile(null, client); + profile.PROFILE_FRESHNESS_THRESHOLD = 1000; + + yield profile.getProfile(); + do_check_eq(numFetches, 1); + + Services.obs.notifyObservers(null, ON_PROFILE_CHANGE_NOTIFICATION, null); + + yield profile.getProfile(); + do_check_eq(numFetches, 2); +}); + add_test(function tearDown_ok() { let profile = CreateFxAccountsProfile();