commit 69b667350d2edf2795f703b81bc946946bd2e7db
parent e2521cc605c1c7c0fd5c84a3bf0853e40f3cc944
Author: scottdowne <sdowne@mozilla.com>
Date: Wed, 5 Nov 2025 18:19:07 +0000
Bug 1997475 - Newtab weather retry change. r=home-newtab-reviewers,reemhamz,nanj
Differential Revision: https://phabricator.services.mozilla.com/D270777
Diffstat:
2 files changed, 157 insertions(+), 15 deletions(-)
diff --git a/browser/extensions/newtab/lib/WeatherFeed.sys.mjs b/browser/extensions/newtab/lib/WeatherFeed.sys.mjs
@@ -46,6 +46,7 @@ import {
const CACHE_KEY = "weather_feed";
const WEATHER_UPDATE_TIME = 10 * 60 * 1000; // 10 minutes
const MERINO_PROVIDER = ["accuweather"];
+const RETRY_DELAY_MS = 60 * 1000; // 1 minute in ms.
const MERINO_CLIENT_KEY = "HNT_WEATHER_FEED";
const PREF_WEATHER_QUERY = "weather.query";
@@ -63,6 +64,7 @@ export class WeatherFeed {
this.lastUpdated = null;
this.locationData = {};
this.fetchTimer = null;
+ this.retryTimer = null;
this.fetchIntervalMs = 30 * 60 * 1000; // 30 minutes
this.timeoutMS = 5000;
this.lastFetchTimeMs = 0;
@@ -100,10 +102,12 @@ export class WeatherFeed {
return;
}
- lazy.clearTimeout(this.fetchTimer);
+ this.clearTimeout(this.fetchTimer);
+ this.clearTimeout(this.retryTimer);
this.merino = null;
this.suggestions = null;
this.fetchTimer = 0;
+ this.retryTimer = 0;
}
async fetch() {
@@ -177,10 +181,12 @@ export class WeatherFeed {
}
restartFetchTimer(ms = this.fetchIntervalMs) {
- lazy.clearTimeout(this.fetchTimer);
- this.fetchTimer = lazy.setTimeout(() => {
+ this.clearTimeout(this.fetchTimer);
+ this.clearTimeout(this.retryTimer);
+ this.fetchTimer = this.setTimeout(() => {
this.fetch();
}, ms);
+ this.retryTimer = null; // tidy
}
async fetchLocationAutocomplete() {
@@ -312,7 +318,7 @@ export class WeatherFeed {
* This thin wrapper around the fetch call makes it easier for us to write
* automated tests that simulate responses.
*/
- async _fetchHelper(retries = 3, queryOverride = null) {
+ async _fetchHelper(maxRetries = 1, queryOverride = null) {
this.restartFetchTimer();
const weatherQuery = this.store.getState().Prefs.values[PREF_WEATHER_QUERY];
@@ -341,24 +347,39 @@ export class WeatherFeed {
}
}
- let suggestions;
- let retry = 0;
-
- while (retry++ < retries && !suggestions?.length) {
+ const attempt = async (retry = 0) => {
try {
- suggestions = await this.merino.fetch({
+ // Because this can happen after a timeout,
+ // we want to ensure if it was called later after a teardown,
+ // we don't throw. If we throw, we end up in another retry.
+ if (!this.merino) {
+ return [];
+ }
+ return await this.merino.fetch({
query,
providers: MERINO_PROVIDER,
timeoutMs: 7000,
otherParams,
});
- } catch (error) {
- // We don't need to do anything with this right now.
+ } catch (e) {
+ // If we get an error, we try again in 1 minute,
+ // and give up if we try more than maxRetries number of times.
+ if (retry >= maxRetries) {
+ return [];
+ }
+ await new Promise(res => {
+ // store the timeout so it can be cancelled elsewhere
+ this.retryTimer = this.setTimeout(() => {
+ this.retryTimer = null; // cleanup once it fires
+ res();
+ }, RETRY_DELAY_MS);
+ });
+ return attempt(retry + 1);
}
- }
+ };
- // results from the API or empty array if null
- return suggestions ?? [];
+ // results from the API or empty array
+ return await attempt();
}
async _fetchNormalizedLocation() {
@@ -399,7 +420,7 @@ export class WeatherFeed {
}
/**
- * Creating a thin wrapper around MerinoClient, PersistentCache, and Date.
+ * Creating a thin wrapper around external tools.
* This makes it easier for us to write automated tests that simulate responses.
*/
WeatherFeed.prototype.MerinoClient = (...args) => {
@@ -414,3 +435,9 @@ WeatherFeed.prototype.PersistentCache = (...args) => {
WeatherFeed.prototype.Date = () => {
return Date;
};
+WeatherFeed.prototype.setTimeout = (...args) => {
+ return lazy.setTimeout(...args);
+};
+WeatherFeed.prototype.clearTimeout = (...args) => {
+ return lazy.clearTimeout(...args);
+};
diff --git a/browser/extensions/newtab/test/xpcshell/test_WeatherFeed.js b/browser/extensions/newtab/test/xpcshell/test_WeatherFeed.js
@@ -497,3 +497,118 @@ add_task(async function test_detect_location_with_geolocation() {
sandbox.restore();
}
});
+
+function setupFetchHelperHarness(
+ sandbox,
+ outcomes /* e.g. ['reject','resolve'] */
+) {
+ // Prevent the “next fetch” scheduling inside fetchHelper().
+ sandbox.stub(WeatherFeed.prototype, "restartFetchTimer").returns(undefined);
+
+ // Stub the timeout and capture the retry callback.
+ let timeoutCallback = null;
+ const setTimeoutStub = sandbox
+ .stub(WeatherFeed.prototype, "setTimeout")
+ .callsFake(cb => {
+ timeoutCallback = cb;
+ return 1;
+ });
+
+ const feed = new WeatherFeed();
+
+ // Minimal store so fetchHelper can read prefs.
+ feed.store = {
+ dispatch: sinon.spy(),
+ getState() {
+ return { Prefs: { values: {} } };
+ },
+ };
+
+ const fetchStub = sinon.stub();
+
+ // Fail or pass each fetch.
+ outcomes.forEach((outcome, index) => {
+ if (outcome === "reject") {
+ fetchStub.onCall(index).rejects(new Error(`fail${index}`));
+ } else if (outcome === "resolve") {
+ fetchStub.onCall(index).resolves([{ city_name: "RetryCity" }]);
+ }
+ });
+ feed.merino = { fetch: fetchStub };
+
+ return {
+ feed,
+ setTimeoutStub,
+ triggerRetry: () => timeoutCallback && timeoutCallback(),
+ };
+}
+
+add_task(async function test_fetchHelper_retry_resolve() {
+ const sandbox = sinon.createSandbox();
+
+ const { feed, setTimeoutStub, triggerRetry } = setupFetchHelperHarness(
+ sandbox,
+ ["reject", "resolve"]
+ );
+
+ // After retry success, fetchHelper should resolve to RetryCity.
+ const promise = feed._fetchHelper(1, "q");
+
+ // Let the first attempt run and schedule the retry.
+ await Promise.resolve();
+
+ Assert.equal(feed.merino.fetch.callCount, 1);
+ Assert.equal(setTimeoutStub.callCount, 1);
+ Assert.ok(
+ setTimeoutStub.calledWith(sinon.match.func, 60 * 1000),
+ "retry waits 60s (virtually)"
+ );
+
+ // Fire the retry.
+ triggerRetry();
+ const results = await promise;
+
+ Assert.equal(feed.merino.fetch.callCount, 2, "retried exactly once");
+ Assert.deepEqual(
+ results,
+ [{ city_name: "RetryCity" }],
+ "returned retry result"
+ );
+
+ sandbox.restore();
+});
+
+add_task(async function test_fetchHelper_retry_reject() {
+ const sandbox = sinon.createSandbox();
+
+ const { feed, setTimeoutStub, triggerRetry } = setupFetchHelperHarness(
+ sandbox,
+ ["reject", "reject"]
+ );
+
+ // After retry also fails, fetchHelper should resolve to [].
+ const promise = feed._fetchHelper(1, "q");
+
+ // Let the first attempt run and schedule the retry.
+ await Promise.resolve();
+
+ Assert.equal(feed.merino.fetch.callCount, 1);
+ Assert.equal(setTimeoutStub.callCount, 1);
+ Assert.ok(
+ setTimeoutStub.calledWith(sinon.match.func, 60 * 1000),
+ "retry waits 60s (virtually)"
+ );
+
+ // Fire the retry.
+ triggerRetry();
+ const results = await promise;
+
+ Assert.equal(
+ feed.merino.fetch.callCount,
+ 2,
+ "retried exactly once then gave up"
+ );
+ Assert.deepEqual(results, [], "returns empty array after exhausting retries");
+
+ sandbox.restore();
+});
