2043699 - Three consumers of `Utils.baseAttachmentsURL` duplicate a try/catch fallback to a hardcoded CDN URL

Status: RESOLVED FIXED

(Firefox :: Remote Settings Client, enhancement)

Description

[Mike Kaply [:mkaply]]

While working on bug 2007024 I noticed that three places in the tree have what look like copy-pasted versions of the same try/catch around Utils.baseAttachmentsURL():

• browser/extensions/newtab/lib/Wallpapers/WallpaperFeed.sys.mjs
• browser/extensions/newtab/lib/FrecencyBoostProvider/FrecencyBoostProvider.mjs
• toolkit/components/ml/actors/MLEngineParent.sys.mjs

They each declare their own RS_FALLBACK_BASE_URL constant (all set to the same firefox-settings-attachments.cdn.mozilla.net/ string), wrap the call in a try/catch, and fall back to the constant on any error. Even the log message text is nearly word-for-word identical.

The wallpaper one was added intentionally in bug 1950883 — when the user is briefly offline, building wallpaper URLs against the hardcoded value lets the browser self-heal once the network comes back, since <img> elements get retried automatically. That's a real and deliberate use of the pattern. The other two copies look like they were modeled on the wallpaper one without an equivalent payoff: FrecencyBoost converts favicons to data URIs immediately, and ML model downloads aren't retried by anything the image loader handles.

A few things that make this worth cleaning up:

The catch is indiscriminate — anything baseAttachmentsURL throws gets swallowed and replaced with the hardcoded URL, not just network errors. While I was working on the remote settings policy, this meant a deliberate "policy says no" throw would have been bypassed by the catches and traffic would still flow to the Mozilla CDN. (I ended up not gating baseAttachmentsURL in that patch, but the bypass shape is still there for anything similar in the future.)

Nothing else in the platform falls back this way. Attachments.sys.mjs's downloadAsBytes, for example, wraps the same failure in a ServerInfoError and re-throws. So the "fall back to a hardcoded URL" convention isn't really a platform convention — it's three feature-level consumers doing the same thing without a shared decision.

And the value of the round-trip itself is questionable. baseAttachmentsURL makes a network request to read the server's capabilities, then in-memory-caches the result for the rest of the process. The returned CDN URL hasn't changed in years and matches the hardcoded fallback the consumers already have. So the platform fetches a value to learn something the callers already know.

Probably the cleanest direction is to persist that value (or just hardcode it once at the platform level), make baseAttachmentsURL synchronous, and let the three try/catches go away. Failing that, the catches could at least be narrowed to network errors so other failure modes propagate instead of being silently papered over.

Related: bug 1950883 (origin of the pattern), bug 2007024 (where I tripped over it), bug 2035221 (separate wallpaper coupling that I'm splitting out).

Comment 1

[Mathieu Leplatre [:leplatrem]]

Thank you Mike for catching our attention on this!

This pattern is definitely not suitable.

The returned CDN URL hasn't changed in years and matches the hardcoded fallback the consumers already have. So the platform fetches a value to learn something the callers already know.

That's true for our central PROD setup, but fetching it from the server also brings us some value. For example, being able to QA some changes by switching environments, or letting Firefox Entreprise connect to a on-premise server that serves its own attachments.

The cleanest direction is to persist that value (or just hardcode it once at the platform level)

I like the idea of persisting the value on first sync instead of memory. Remote Settings has to be able to run at least once anyway.
Hardcoding seems less flexible than what we need, at least to my understanding.

Comment 2

[Mike Kaply [:mkaply]]

Totally zoned on QA environments. good call.

I do like the idea of persisting it on first sync.

Comment 3

[Mathieu Leplatre [:leplatrem]]

Attachment: Bug 2043699 - Remove fallback URIs for Remote Settings attachments r?acottner

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/0e1e88227a25
https://hg.mozilla.org/integration/autoland/rev/47c06e334a52
Remove fallback URIs for Remote Settings attachments r=acottner,ai-platform-reviewers,omc-reviewers,dmose,jbowser

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/7d9672e1090c
https://hg.mozilla.org/integration/autoland/rev/57361c957ca2
Revert "Bug 2043699 - Remove fallback URIs for Remote Settings attachments r=acottner,ai-platform-reviewers,omc-reviewers,dmose,jbowser" for causing bc failures @ browser_search_telemetry_domain_categorization_download_timer

Comment 6

[amarc]

Backed out for causing bc failures

• Backout link
• Push with failures
• Failure Log
• Failure Log TV failure

Comment 7

[Atila Butkovits]

There is also this Puppeteer

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/f9519bc75219
https://hg.mozilla.org/integration/autoland/rev/6622fc1641da
Remove fallback URIs for Remote Settings attachments r=acottner,ai-platform-reviewers,omc-reviewers,dmose,jbowser,search-reviewers

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/a9f1fb1da4b1
https://hg.mozilla.org/integration/autoland/rev/d944e1275280
Revert "Bug 2043699 - Remove fallback URIs for Remote Settings attachments r=acottner,ai-platform-reviewers,omc-reviewers,dmose,jbowser,search-reviewers" for causing Pup failure

Comment 10

[Norisz Fay [:noriszfay]]

Backed out for causing Pup failure

Backout link

Push with failures

Failure log

Comment 11

[Mathieu Leplatre [:leplatrem]]

I don't understand how all these Puppeteer failures could be related to the changes of this patch.

I feel stuck. Scott, since you introduced some of these hard-coded URLs, could you please help me removing them?

Comment 12

[Mike Kaply [:mkaply]]

Why the Puppeteer test fails

It's the new retry-with-backoff loop added to Utils.baseAttachmentsURL() in services/settings/Utils.sys.mjs. That loop is the regression — and the failure is a timing/shutdown race, not anything the test author did wrong.

The new code (Utils.sys.mjs):

const { retries = 3, retryWaitMsec = 1000 } = options;
...
while (retried <= retries) {
  try { ... break; }
  catch (error) {
    retried++;
    if (retried > retries) { throw error; }
    await new Promise(resolve => setTimeout(resolve, retryWaitMsec * retried));
  }
}

On a failed server fetch this sleeps 1000 + 2000 + 3000 = 6000ms before giving up. The old code did a single fetch that failed instantly (consumers then fell back to the hardcoded CDN URL — the very thing this bug removed).

The trigger is always-on, not newtab/ML-specific. Downloader.download() (Attachments.sys.mjs:554) calls baseAttachmentsURL() with no options → default retries = 3. So every Remote Settings attachment download goes through the retry loop now — including search-config icons via ConfigSearchEngine.attachments.download() that fire during a normal Firefox startup. In the Puppeteer CI environment there's no reachable Remote Settings server, so the fetch fails and the loop spins the full ~6s.

Why that breaks "tmp profile should be cleaned up":

1. Puppeteer launches Firefox (BiDi, no CDP), opens a page; startup attachment downloads kick off the 6s backoff, sitting behind the profileBeforeChange shutdown blocker in services/settings (RemoteSettingsWorker/Database).
2. The test calls close(). For Firefox+BiDi, BrowserLauncher.closeBrowser() takes the else-branch (BrowserLauncher.ts:309): it races graceful shutdown against timer(5000).
3. The 6s backoff exceeds the 5s graceful-close budget → the timer wins → Puppeteer force-kills the process (browserProcess.close()).
4. Profile cleanup (onProcessExit → cleanUserDataDir → rm) is an exit-event callback that await close() doesn't await, so it loses the race against the test's immediate readdirSync. The temp profile is still there → toHaveLength(0) sees length 1.

The took 6455ms ≈ 6000ms backoff + overhead is the tell.

Fix options

The hot/shutdown-sensitive download path must not gain a multi-second stall where it used to fail fast. Smallest, most faithful fix:

• Option A (recommended, minimal): default retries = 0 in baseAttachmentsURL, making retries opt-in. This restores the old single-fetch timing exactly (which the test passed against), while callers who genuinely want resilience can pass { retries }. One-line change.
• Option B (more robust): keep the default retries but make the backoff abortable — tie the setTimeout to an AbortSignal wired to AsyncShutdown.profileBeforeChange so shutdown is never delayed. More code, but keeps transient-failure resilience everywhere.

I'd go with A given this is a regression and the retry was added blanket-style with no caller actually requesting it.

Comment 13

[Mathieu Leplatre [:leplatrem]]

Thank you Mike! I agree with you, let's go with Option A.

My assistant was a lot less helpful than yours 😅

Why it failed

Two TEST-UNEXPECTED-FAILs, both in remote/test/puppeteer/.../launcher.spec.js:

1. Puppeteer.launch tmp profile should be cleaned up (line 17567) — a temp profile dir
   was left behind:

   expect(received).toHaveLength(expected)
   Expected length: 0 / Received length: 1
   Received array: ["puppeteer_dev_firefox_profile-M6rvAk"]
   

2. Puppeteer.launch userDataDir option restores preferences (line 17761):

   ENOENT: no such file or directory, open '/tmp/pptr_tmp_folder-FuGnaT/prefs.js'
   

The common root cause is one line earlier (line 17573):

/tmp/pptr_tmp_folder-FuGnaT/prefs.js:1: prefs parse error: expected ';' after ')'

Firefox refused to parse the prefs.js puppeteer wrote, so the profile never settled and was
neither restored (test 2) nor cleaned up (test 1).

The key point for your patch

These failures are in the puppeteer browser-launcher specs. Your commit 1880142d3dcf8
only touches services/settings/ (Attachments/Utils), toolkit/components/ml/, newtab, and a
couple of test files — nothing in remote/, nothing that writes prefs.js or profile
handling. So these are unrelated/intermittent CI failures, not something your code
introduced. They're what's making the task red and "rejecting" the push, but they're not caused
by removing the Remote Settings fallback URIs.

Recommended next steps

• Re-trigger / retry that puppeteer-test chunk — prefs.js parse + leftover-profile failures
  here are classic intermittents.
• Confirm against a clean baseline (treeherder) that these same launcher.spec tests are
  failing/orange without your patch.

Comment 14

[Mathieu Leplatre [:leplatrem]]

:noriszfay, am I right to think that Pup tests are fixed here?
https://treeherder.mozilla.org/jobs?repo=try&revision=78e4d399a27c9b6c0a2ec6f4ebff2e65758ff2ce

Comment 15

[Norisz Fay [:noriszfay]]

Since the test is green and has passed it could very well be fixed I think.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/c0ca49e2b3c8
https://hg.mozilla.org/integration/autoland/rev/6587600eabcb
Remove fallback URIs for Remote Settings attachments r=acottner,ai-platform-reviewers,omc-reviewers,dmose,jbowser,search-reviewers

Comment 17

[Silaghi Andreea]

https://hg.mozilla.org/mozilla-central/rev/6587600eabcb