2073421 - TopSitesFeed keeps broadcasting a top sites row after uninit(), so toggling feeds.system.topsites can leave every about:newtab showing the old row

Status: RESOLVED FIXED

(Firefox :: New Tab Page, defect)

Description

[Florian Quèze [:florian]]

TopSitesFeed.refresh() computes the row asynchronously and lets only the newest refresh dispatch:

const refreshId = ++this._refreshGeneration;
...
if (refreshId !== this._refreshGeneration) { return; }

_refreshGeneration is a per-instance field, and uninit() does not stop a refresh that is already in flight. So when feeds.system.topsites goes false and then true, Store.onPrefChanged uninits the feed and constructs a new one, and the store holds that new TopSitesFeed while the old, uninitialized one is still inside getLinksWithDefaults(). The old one then dispatches TOP_SITES_UPDATED like any live feed -- broadcasting whenever the trigger that started it asked to, which refreshDefaults and PLACES_HISTORY_CLEARED both do -- and passes its own guard, because the new instance never touched its counter. Whichever instance finishes last wins in content, so the row every open about:newtab shows can be the one built from the configuration that has just been replaced.

An in-flight Contile fetch is a second way in: it calls _readDefaults() when it completes, which starts a whole refresh on a feed the store no longer holds.

Reachability is narrow: nothing in shipping code writes feeds.system.topsites. The customize panel's Shortcuts toggle writes feeds.topsites (data-preference="feeds.topsites" in ContentSection.jsx), a separate content-side section pref, and only feeds.system.topsites is a key in Store's _feedFactories, so only it recreates the feed. Every other non-test reference to it is a read. It takes about:config, an enterprise policy or a test to reach this, hence S4 and no user-facing STR.

A test that does flip the pref caught it. browser/extensions/newtab/test/browser/browser_topsites_contextMenu_options.js and browser_topsites_section.js had a helper that toggled it to make the feed re-read prefs the test had just pushed. Instrumenting refresh() showed two feed instances dispatching per test, each with its own _refreshGeneration; delaying the dispatch of any instance whose uninit() has run fails both files 6/6 with Wait for the https://www.youtube.com/ top site tile - timed out after 50 tries, and they pass 6/6 once the helper stops recreating the feed.

Fix: uninit() marks the instance, and the refresh treats that mark like a superseded generation, both before starting work and before dispatching -- no store dispatch, no broadcast.

Comment 1

[Hackbot [:hackbot]]

TopSitesFeed.sys.mjs uninit() drops observers but never marks the instance or bumps _refreshGeneration, so a discarded feed stays inside an in-flight refresh() and passes its own per-instance guard at #2100, dispatching TOP_SITES_UPDATED after Store.sys.mjs has already replaced it, with feed.store still assigned because uninitFeed only deletes the map entry. Whichever instance finishes last decides the row in every open about:newtab, and ContileIntegration.refresh() is a second way in, calling _readDefaults() on a feed the store no longer holds.

Fix: Set a destroyed marker in uninit(), clear it in init(), and treat it as a superseded generation in refresh() both before starting work and before dispatch, in the _linksWithDefaults write-back guard at #1961, and in ContileIntegration.refresh() ahead of _readDefaults(). Then drop the feed-recreating toggleTopsitesPref() helper in head.js and cover the two-instance case in test_TopSitesFeed.js.

Suggested severity: S4. Every write to feeds.system.topsites in the tree is test code, so reaching this takes about:config, an enterprise policy or a test, and the live feed's next refresh replaces the stale row.

---

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.

Comment 2

[Florian Quèze [:florian]]

Attachment: Bug 2073421 - Stop TopSitesFeed dispatching a top sites row after uninit(), r=#home-newtab-reviewers.

refresh() lets only the newest refresh dispatch by comparing against
_refreshGeneration, but that counter is per instance and uninit() does not touch
it. When feeds.system.topsites is toggled, Store.onPrefChanged uninits the feed
and constructs a new one while the old instance is still inside
getLinksWithDefaults(), so the old one passes its own generation check and
broadcasts a row built from the configuration that has just been replaced. An
in-flight Contile fetch is a second way in, through the _readDefaults() it calls
when it completes.

Mark the instance in uninit() and treat that mark like a superseded generation.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/afe7d1b18c52
https://hg.mozilla.org/integration/autoland/rev/42e006243ffe
Stop TopSitesFeed dispatching a top sites row after uninit(), r=mconley.

Comment 4

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/42e006243ffe