2067322 - Bug 2065645 wasn't caught by automated tests

Status: NEW

(Firefox :: New Tab Page, defect)

Description

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Bug 2065645 caused a 100% reproducible crash on startup for developers running debug builds, but didn't cause any failures on CI testing of debug builds, so it wasn't caught and backed out as one would normally expect for a bug on that level. (This kind of incident is also not a great experience for the author of the regressing patch either, which I can attest to as the guilty party in bug 1670277.) So I thought that that should be investigated, because there's clearly a gap in test coverage somewhere.

I noticed the phrase “while spocs are loading” from bug 2065645 comment #2 which suggests a remote server interaction that might be different in testing, so I asked Claude — and I'm normally not the “I asked Claude and it said…” type, but it contains more information about Web frontend dev than I do — and it made some comments that appear to be mostly but not entirely correct:

Why CI never hit it

Four independent reasons, any one of which is sufficient:

1. The Discovery Stream feed is disabled outright in the test profile. testing/profiles/common/user.js:18 sets discoverystream.config = "[]". DiscoveryStreamFeed.config (browser/extensions/newtab/lib/DiscoveryStreamFeed.sys.mjs:189-210) JSON-parses that and computes config.enabled = config.enabled && … → undefined. So no sections layout, so CardSections never renders, so SectionContextMenu never renders. testing/profiles/profiles.json shows every harness that could load a newtab (mochitest, wpt, reftest, talos, raptor, valgrind) includes common.

2. Section titles are remote data, not local strings. DiscoveryStreamFeed.sys.mjs:1952-1955 builds each section as title: sectionData.title from the curated-recommendations response. With feeds.system.topstories=false (common/user.js:16) and unifiedAds.endpoint="" (:19), there is no response at all — so CI gets zero sections rather than sections with blank titles.

3. The specific blanking path needs a remotely-enabled feature. spocsLoading is Base.isSpocsOnDemandExpired, which returns early unless spocs.onDemand.enabled (Base.jsx:212-227). That comes from DiscoveryStreamFeed.spocsOnDemand (:692-701), which is showSponsoredStories && (trainhopConfig.spocsOnDemand?.enabled || pref). The in-tree pref default is false (ActivityStream.sys.mjs:1978-1983), so on-demand spocs are turned on remotely via a Nimbus newtabTrainhop enrollment (PrefsFeed.sys.mjs:256-258). CI points services.settings.server at data:,#remote-settings-dummy/v1 (common/user.js:71) and blanks app.normandy.api_url, so no enrollment ever lands and spocsLoading is permanently false. That also explains the timing: this is a feature currently rolling out to Nightly clients — a developer's local debug build enrolls, CI can't.

4. Debug CI doesn't even open a newtab at startup. testing/profiles/unittest-required/user.js:41 sets browser.startup.page = 0 (about:blank), unlike ./mach run.

Plus the point hackbot made: the jest test that renders the exact crashing configuration (source="", no title) passes because jest doesn't run Fluent, and the one browser test that does enable discoverystream.config (browser_discovery_render.js) only asserts on top sites.

Points 1 and 2 appear to be correct as-is; copying either one of those prefs from testing/profiles/*/user.js is enough to suppress the bug. Point 4 is slightly off; I also had to set datareporting.policy.dataSubmissionPolicyBypassNotification, but those two prefs were enough. And point 3 doesn't appear to work at all; I assume Claude could actively experiment to find out what it missed but I haven't tried that yet and I don't know if it matters.

Claude also suggested this, and I don't know if it's a good idea but I'll pass it along:

The generalizable gap: newtab surfaces whose content comes from the network or from Nimbus are structurally invisible to CI's Fluent assertions. The whole stories/sections UI — the largest consumer of data-l10n-args in newtab — is dark in every debug job. A browser-chrome test that stubs the sections feed (there's already browser/extensions/newtab/test/browser/topstories.json as precedent for canned feed data) and walks the rendered newtab asserting every data-l10n-id/data-l10n-args pair resolves, would catch this class of bug and cost one test file. That's narrower and more durable than trying to make the DS feed reachable in CI generally.

Comment 1

[Hackbot [:hackbot]]

Investigated the gap. Two of the four hypotheses in comment 0 don't hold, and the real gap is narrower — and cheaper to close — than "make the DS feed reachable in CI".

What actually blanked the title — not spocsLoading. Your doubt about point 3 was right: isSpocsOnDemandExpired early-returns unless discoverystream.spocs.onDemand, which defaults false, so no Nimbus enrolment was involved. The unconditional path is placeholderComponent in browser/extensions/newtab/content-src/lib/selectLayoutRender.mjs: before the stories feed lands it synthesises a section object carrying only layout and data — no title key at all. CardSections.jsx destructures title off it with no fallback, so SectionContextMenu saw undefined on the first paint of every newtab. Feed-built sections are exposed too: DiscoveryStreamFeed.sys.mjs gives subtitle a || "" fallback but gives title none.

Why CI missed it — not discoverystream.config. The newtab browser-chrome manifest already overrides the shared profile: browser.toml puts feeds.system.topstories back to true, and ten tests in the directory set discoverystream.config to an enabled object. The blocker is two lines further down — browser.toml sets discoverystream.sections.region-content-config to empty. That pref feeds showSectionLayout (ActivityStream.sys.mjs), which is the getValue for discoverystream.sections.enabled (same file), so it computes false suite-wide, and DiscoveryStreamBase.jsx renders CardGrid instead of CardSections. Nothing under test/browser/ sets sections.enabled, so CardSections/SectionContextMenu is never rendered in CI at all.

And opt jobs can't catch this class regardless. The MOZ_CRASH is #[cfg(debug_assertions)]. In automation MaybeReportErrorsToGecko (intl/l10n/Localization.h) does throw InvalidStateError, but on the DOM data-l10n-id path that rejection is consumed with IgnoreErrors() in dom/l10n/L10nMutations.cpp and never becomes a failure. Localization.h's own comment names bug 1741430 for exactly this.

Proposed fix, two pieces:

1. Browser-chrome, catches the class on debug jobs. A test under browser/extensions/newtab/test/browser/ that sets discoverystream.sections.enabled=true (or a non-empty region-content-config) plus an enabled discoverystream.config, stubs generateFeedUrl to topstories.json using the existing pattern in browser_discovery_card.js, and loads about:newtab. The pre-feed placeholder paint alone would have crashed the content process on debug and failed the job.

2. Jest, cheaper and more general. jest-setup.mjs already has an afterEach guard that fails any test logging console.error. A sibling guard could check, for every rendered element with a data-l10n-id, that the message's required $variables are present in its data-l10n-args. @fluent/bundle is already a newtab dependency, so newtab.ftl can be parsed directly — browser/components/storybook/.storybook/fluent-utils.mjs does this already. Fluent is currently absent from the jest environment entirely, which is why SectionContextMenu.test.jsx passed while rendering the crashing shape. A global guard would have caught this with no new test file, and would also cover the still-unguarded newtab-menu-content-tooltip call sites in TopSite.jsx, DSLinkMenu.jsx, AdBannerContextMenu.jsx and Card.jsx.

Worth doing alongside: give the placeholderComponent section a title, or make CardSection tolerate its absence.

Bug 2036122 asks for a narrower "about:newtab loads without crashing" test; (1) would subsume it for the sections surface.

This is read-only analysis — I have not built or run any of it.

---

Suggested severity: S4
No direct user impact — this is a test-coverage gap and the defect it missed is already fixed. Worth more than trivial only because the uncovered surface is the main newtab content area.

---

If you'd like to provide feedback on this comment, please use the 👍 or 👎 reaction.
If you want to categorize your feedback you can add one of the following tags: ai-triage-wrong-file, ai-triage-wrong-cause, ai-triage-hallucination, ai-triage-out-of-scope.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Analysis seems reasonable. I think, solution-wise, I'd go with both 1 and 2:

• an integration test for this particular subcomponent of newtab to make sure it gets some mochitest-browser testing
• the more generalized Jest unit test helper.

Comment 3

[Hackbot [:hackbot]]

Attachment: Bug 2067322 - Cover newtab's sections surface in browser-chrome, and fail jest on missing Fluent variables. r?mconley

Implements both pieces from comment 2.

1. test/browser/browser_card_sections.js — integration coverage for the surface.

Nothing under test/browser/ rendered CardSections/SectionContextMenu, so bug 2065645 could not have gone orange. The blocker is browser.toml's shared discoverystream.sections.region-content-config=, which is what discoverystream.sections.enabled is computed from; the new test sets that computed pref directly (showSectionLayout() only runs at feed init, so restoring a region would not recompute it) and stubs generateFeedUrl to a new topstories_sections.json.

The fixture is shaped like a curated-recommendations response, with one titled section and one the server sent no title for — DiscoveryStreamFeed gives subtitle a || "" fallback but gives title none, so that second section is a real exposure and it makes the no-title assertion deterministic on opt as well as debug.

2. A jest guard, in test/jest/l10n-args-guard.mjs + jest-setup.mjs.

Alongside the existing console.error guard, an afterEach now parses the four .ftl files about:newtab links and fails any test rendering a data-l10n-id whose message interpolates a variable data-l10n-args omits. That is why SectionContextMenu.test.jsx passed while rendering the crashing markup: jsdom treats the attributes as inert. Only Unknown variable resolver errors fail, so Gecko-only functions like PLATFORM() and ids from other sources (ASRouter) are not false positives; l10n-args-guard.test.jsx pins that down so the guard cannot rot into a no-op.

browser_card_sections.js does the same sweep against real Fluent via a new head.js content helper. It uses document.l10n.formatMessages() rather than reading a rendered aria-label back: the DOM translation path drops the error with IgnoreErrors() (L10nMutations.cpp), which is exactly how this shipped, while Localization::FormatMessages reports it on opt too because xpc::IsInAutomation() makes MaybeReportErrorsToGecko throw.

3. The variants bug 2065645 left open.

Turning the guard on failed 35 existing tests. 30 were the same defect at four more call sites: newtab-menu-content-tooltip needs $title, and JSON.stringify({ title: undefined }) is "{}", so the variable vanishes with nothing looking wrong at the call site. Bug 2065645 fixed only SectionContextMenu. TopSite, DSLinkMenu, AdBannerContextMenu and Card all still derive a title from data that can be absent — AdBannerContextMenu's spoc.title || spoc.sponsor || spoc.alt_text comes straight off the wire with no client-side guard.

Rather than teach five call sites the same rule, content-src/lib/context-menu-tooltip.mjs centralises it and returns spreadable attributes; ContextMenuButton's tooltip/tooltipArgs pair collapses into one tooltipProps (Card was its only user). Net effect is fewer lines than before.

The remaining 5 failures were Privacy.test.jsx fixtures setting a $count message id with no countArg; PrivacyMessages' countArgFor() always mints the two together, so those are test-side only and now supply the count.

Verification. npx jest: 125 suites / 2250 tests green. Reverting contextMenuTooltip to the pre-bug-2065645 behaviour makes the guard fail 33 tests, including <SectionContextMenu> should render a panel-list menu — the crashing configuration from the bug — and the new CardSections placeholder tests, which drive the real selectLayoutRender so they stay honest if the placeholder's shape changes. eslint/license/codespell clean.

The browser-chrome test is not executed locally (no objdir here). Pushed to try for debug + opt browser-chrome and the newtab unit-test job: https://treeherder.mozilla.org/jobs?repo=try&landoInstance=lando-prod-2025&landoCommitID=85781

Not addressed here: L10nMutations.cpp swallowing the rejection is bug 1741430, named in Localization.h's own comment. Bug 2036122 wants a narrower "about:newtab loads without crashing" test and is subsumed for this surface.

Comment 4

[Hackbot [:hackbot]]

Attachment: Bug 2067322 - Cover newtab's sections surface in browser-chrome, and fail jest on missing Fluent variables. r?mconley

Implements both pieces from comment 2.

1. test/browser/browser_card_sections.js — integration coverage for the surface.

Nothing under test/browser/ rendered CardSections/SectionContextMenu, so bug 2065645 could not have gone orange. The blocker is browser.toml's shared discoverystream.sections.region-content-config=, which is what discoverystream.sections.enabled is computed from; the new test sets that computed pref directly (showSectionLayout() only runs at feed init, so restoring a region would not recompute it) and stubs generateFeedUrl to a new topstories_sections.json.

The fixture is shaped like a curated-recommendations response, with one titled section and one the server sent no title for — DiscoveryStreamFeed gives subtitle a || "" fallback but gives title none, so that second section is a real exposure and it makes the no-title assertion deterministic on opt as well as debug.

2. A jest guard, in test/jest/l10n-args-guard.mjs + jest-setup.mjs.

Alongside the existing console.error guard, an afterEach now parses the four .ftl files about:newtab links and fails any test rendering a data-l10n-id whose message interpolates a variable data-l10n-args omits. That is why SectionContextMenu.test.jsx passed while rendering the crashing markup: jsdom treats the attributes as inert. Only Unknown variable resolver errors fail, so Gecko-only functions like PLATFORM() and ids from other sources (ASRouter) are not false positives; l10n-args-guard.test.jsx pins that down so the guard cannot rot into a no-op.

browser_card_sections.js does the same sweep against real Fluent via a new head.js content helper. It uses document.l10n.formatMessages() rather than reading a rendered aria-label back: the DOM translation path drops the error with IgnoreErrors() (L10nMutations.cpp), which is exactly how this shipped, while Localization::FormatMessages reports it on opt too because xpc::IsInAutomation() makes MaybeReportErrorsToGecko throw.

3. The variants bug 2065645 left open.

Turning the guard on failed 35 existing tests. 30 were the same defect at four more call sites: newtab-menu-content-tooltip needs $title, and JSON.stringify({ title: undefined }) is "{}", so the variable vanishes with nothing looking wrong at the call site. Bug 2065645 fixed only SectionContextMenu. TopSite, DSLinkMenu, AdBannerContextMenu and Card all still derive a title from data that can be absent — AdBannerContextMenu's spoc.title || spoc.sponsor || spoc.alt_text comes straight off the wire with no client-side guard.

Rather than teach five call sites the same rule, content-src/lib/context-menu-tooltip.mjs centralises it and returns spreadable attributes; ContextMenuButton's tooltip/tooltipArgs pair collapses into one tooltipProps (Card was its only user). Net effect is fewer lines than before.

The remaining 5 failures were Privacy.test.jsx fixtures setting a $count message id with no countArg; PrivacyMessages' countArgFor() always mints the two together, so those are test-side only and now supply the count.

Verification. npx jest: 125 suites / 2250 tests green. Reverting contextMenuTooltip to the pre-bug-2065645 behaviour makes the guard fail 33 tests, including <SectionContextMenu> should render a panel-list menu — the crashing configuration from the bug — and the new CardSections placeholder tests, which drive the real selectLayoutRender so they stay honest if the placeholder's shape changes. eslint/license/codespell clean.

The browser-chrome test is not executed locally (no objdir here). Pushed to try for debug + opt browser-chrome and the newtab unit-test job: https://treeherder.mozilla.org/jobs?repo=try&landoInstance=lando-prod-2025&landoCommitID=85781

Not addressed here: L10nMutations.cpp swallowing the rejection is bug 1741430, named in Localization.h's own comment. Bug 2036122 wants a narrower "about:newtab loads without crashing" test and is subsumed for this surface.

Comment 5

[BugBot [:suhaib / :marco]]

The severity field is not set for this bug.
:thecount, could you have a look please?

For more information, please visit BugBot documentation.