2072914 - New tab keeps the old search bar after the newtab urlbar feature gate changes

Status: NEW

(Firefox :: New Tab Page, defect)

Description

[Florian Quèze [:florian]]

ExternalComponentWrapper's loading effect has no staleness check. loadComponent() awaits a dynamic import() and then acts on refs, but React can tear the effect down and start a new one while that await is pending, and the old continuation still runs.

A search feature-gate flip does exactly that. newtabFeatureGate swaps the SEARCH component between UrlbarNewTabComponentRegistrant's moz-urlbar and SearchUIUtils's content-search-handoff-ui, which re-runs the effect on a preloaded about:newtab that is still mounting. Two things then go wrong:

• Default branch: the stale continuation still passes containerRef.current && !customElementRef.current, so it appends the superseded configuration's element -- after its own cleanup has already run. The new effect's continuation then sees a set ref and creates nothing. Which search bar you end up with depends only on which of the two imports resolves first, and the wrong one survives until the page is reloaded.
• react-bundle branch: the stale continuation appends a second <script> to document.head and overwrites scriptRef.current and cleanupRef.current, leaking a script element and a mounted React tree that nothing will ever clean up, and mounts into a shadow root that cleanup has already emptied and disconnected from document.l10n.

The trigger is wider than a gate flip. The REFRESH_EXTERNAL_COMPONENTS reducer returns components: action.data, a fresh array identity, so any registry update re-runs the effect even when no configuration changed; browser.nova.enabled alone produces one, although neither registrant's getComponents() reads it. And a single gate flip calls updated() on both SEARCH registrants, so two broadcasts arrive back to back and the second lands inside the first one's import window.

Suggested fix: give the effect a cancellation flag, set it in the cleanup function, and bail out after each await, rather than treating customElementRef.current as proof of what is in the DOM. The comment excluding props from the dependency array records the assumption that is wrong.

Found by reading the code during a flakiness burndown of browser/extensions/newtab/test/browser. No test currently reaches it: bug 2062212 turned the gate off in add_setup for browser_as_render.js, which was the one test exercising a mid-mount flip.

Comment 1

[Hackbot [:hackbot]]

ExternalComponentWrapper.jsx has no cancellation flag across await importModule(), so when a newtabFeatureGate flip changes components and re-runs the effect, the torn-down effect's continuation still satisfies containerRef.current && !customElementRef.current (its cleanup nulled only the second ref, at #191-194) and appends the superseded configuration's element, after which the live effect's continuation sees a set ref and creates nothing. The react-bundle branch fails worse, since a stale continuation appends a second <script> and overwrites scriptRef.current and cleanupRef.current, leaking a script element and a mounted React tree while mounting into a shadow root that cleanup has already emptied and disconnected from document.l10n.

Fix: Give the effect an effect-local cancelled flag set by the cleanup function and checked after every await, hold the script and mount-cleanup handles in effect-local variables rather than shared refs so a stale run cannot overwrite the live one, and key the effect on the resolved config instead of components identity, which also stops the spurious re-runs caused by refreshComponents() building a fresh array that the reducer stores verbatim. ExternalComponentWrapper.test.jsx needs a case with a deferred importModule that resolves after the dependencies change, since all 13 existing cases resolve the import before rerendering or unmounting and none exercise mountStrategy.

Suggested severity: S3. A user enrolled in or unenrolled from the gate while a newtab is preloading gets the wrong search bar, with which one winning decided by import ordering, and reloading the tab is the only recovery.

---

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, ai-triage-wrong-fix, ai-triage-shallow-fix.

Needinfo [e-mail] to have a patch generated for this bug, include any questions or directions as needed in your comment.