1633913 - Eliminate re-construction of Top Sites and Pocket stories when about:home startup cache loads before Activity Stream initializes

Status: RESOLVED FIXED

(Firefox :: New Tab Page, task, P3)

Description

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

If the about:home startup cache loads first, and then Activity Stream initializes, it sends a slew of actions down to the about:home document's Redux instance. Some of these messages cause the Top Sites and Pocket stories to redraw.

We should make sure that these actions either aren't sent, or don't result in reconstructing these parts of the DOM, since the whole point of the about:home startup cache is to load from the disk and then stop.

Comment 1

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

For reference, here's the list of actions that the Redux store gets dispatched on it after the about:home static document loads:

INIT
PREFS_INITIAL_VALUES
DISCOVERY_STREAM_PERSONALIZATION_VERSION
DISCOVERY_STREAM_CONFIG_SETUP
DISCOVERY_STREAM_COLLECTION_DISMISSIBLE_TOGGLE
SECTION_REGISTER
SECTION_REGISTER
SECTION_UPDATE
SECTION_UPDATE
DISCOVERY_STREAM_PERSONALIZATION_LAST_UPDATED
DISCOVERY_STREAM_LAYOUT_UPDATE
DISCOVERY_STREAM_SPOCS_ENDPOINT
DISCOVERY_STREAM_SPOCS_PLACEMENTS
DISCOVERY_STREAM_FEED_UPDATE
DISCOVERY_STREAM_SPOCS_UPDATE
DISCOVERY_STREAM_FEEDS_UPDATE
UPDATE_SEARCH_SHORTCUTS
TOP_SITES_UPDATED
SECTION_UPDATE
AS_ROUTER_INITIALIZED
SCREENSHOT_UPDATED
SCREENSHOT_UPDATED
SCREENSHOT_UPDATED
SCREENSHOT_UPDATED
SCREENSHOT_UPDATED
SCREENSHOT_UPDATED
SCREENSHOT_UPDATED
SCREENSHOT_UPDATED
SCREENSHOT_UPDATED
SECTION_UPDATE_CARD
SECTION_UPDATE_CARD
SECTION_UPDATE_CARD
SECTION_UPDATE_CARD
SECTION_UPDATE_CARD
SECTION_UPDATE_CARD
SECTION_UPDATE_CARD
SECTION_UPDATE_CARD
SECTION_UPDATE_CARD
SECTION_UPDATE_CARD
SECTION_UPDATE_CARD

Comment 2

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

So the flash of the screenshots in the Top Sites section is because the TOP_SITES_UPDATED message comes down from the parent with its initial concept of what the TopSites are. The top site link objects don't include the screenshot properties, so those get wiped out, and we render the "no screenshot" case. Meanwhile, a request is sent in the background to get the screenshot URLs, and then the SCREENSHOT_UPDATED messages come down to alert the page when they arrive. Then we put the screenshots back.

That's what's causing the Top Sites flash, at least. I suspect it's something similar for Pocket, but I haven't started digging into there yet.

Comment 3

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

Hey Mardak,

How would you recommend I avoid this Top Site flash, given the conditions in comment 2? A few ideas come to mind:

1. Specially mark the first TOP_SITES_UPDATED and SCREENSHOT_UPDATED actions for when they're fired for the initialization of TopSitesFeed, and have the page with the cached about:home document ignore those actions.
2. Somehow merge the TOP_SITES_UPDATED links with the ones that already exist from the cache, so that the screenshots from the cached copy of the state get assigned to the initial links that come down from TOP_SITES_UPDATED, probably matching on guid

I think (1) is probably safest. Anything else I should consider?

Comment 4

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

(In reply to Mike Conley (:mconley) (:⚙️) (Extremely busy) from comment #3)

1. Specially mark the first TOP_SITES_UPDATED and SCREENSHOT_UPDATED actions for when they're fired for the initialization of TopSitesFeed, and have the page with the cached about:home document ignore those actions.
   ...
   I think (1) is probably safest. Anything else I should consider?

This also has the advantage of making it relatively easy to ignore more messages that only fire during startup that the cached document might not care about - we could, perhaps, put a action.meta.isStartup = true for startup actions that can be ignored by the cache, and have the rehydrationMiddleware ignore those actions if window.__FROM_STARTUP_CACHE__ is true.

Comment 5

[Ed Lee :Mardak]

The TopSites reducer does have a prevState.rows that would contain the screenshot value from cache, so TOP_SITES_UPDATED could copy over the value.
https://searchfox.org/mozilla-central/rev/3d39d3b7dd1b2be30692d4541ea681614e34c786/browser/components/newtab/common/Reducers.jsm#159,165-166

But I do see that SCREENSHOT_UPDATED has a different moz-page-thumb://…&revision=#### value every restart that would then cause a different background-image url value to be set (and flash) even though it's probably actually the same image from disk loaded.

How would action.meta.isStartup get set (and later not set)? We probably wouldn't want to ignore actions that were user triggered, e.g., dismissing a top site soon after starting firefox which might result in a second TOP_SITES_UPDATED. Similarly, the async SCREENSHOT_UPDATED actions that were triggered from the initial refresh would need to differentiate from later updates where some might have existed at startup and others actually fetched in the background.

Comment 6

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

(In reply to Ed Lee :Mardak from comment #5)

How would action.meta.isStartup get set (and later not set)?

My idea was to pass a new argument to TopSitesFeed's refresh method when called from the init method. That'll get plumbed through the various functions it calls so that when it ultimately dispatches the TOP_SITES_UPDATED message, it sets the action.meta.isStartup property. Similarly, when it requests the screenshots, the function closure will know that it's for a startup refresh, and can pass that property along in the screenshot callback when it dispatches SCREENSHOT_UPDATED.

Would that be acceptable? Or is there something better I can do here?

Comment 7

[Ed Lee :Mardak]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #6)

That'll get plumbed through the various functions it calls so that when it ultimately dispatches the TOP_SITES_UPDATED message, it sets the action.meta.isStartup property.

Hmm.. I guess that is doable although potentially quite messy as that would be quite the plumbing modifying BroadcastToContent to take an optional startup flag to customize the meta object for each desired call site:
https://searchfox.org/mozilla-central/rev/46e3b1ce2cc120a188f6940b5c6eab6b24530e4f/browser/components/newtab/common/Actions.jsm#224-227

I do see it would be desirable to have some way to exclude more actions when we're FROM_STARTUP_CACHE as this flashing top site is one of potentially several cases of "prefer the cached version." (This would be opposed to specially targeting TopSites Reducer to be smarter.)

Perhaps we can introduce a new content middleware or modify messageMiddleware that is configured to ignore some number of certain actions when FROM_STARTUP_CACHE?
https://searchfox.org/mozilla-central/rev/46e3b1ce2cc120a188f6940b5c6eab6b24530e4f/browser/components/newtab/content-src/lib/init-store.js#45-54

const STARTUP_CACHE_IGNORE = {
  TOP_SITES_UPDATED: 1,
  SCREENSHOT_UPDATED: Infinity,
};

messageMiddleware = store => next => action => {
  …
  if (window.__FROM_STARTUP_CACHE__ && STARTUP_CACHE_IGNORE[action.type]-- > 0)

Comment 8

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

(In reply to Ed Lee :Mardak from comment #7)

const STARTUP_CACHE_IGNORE = {
  TOP_SITES_UPDATED: 1,
  SCREENSHOT_UPDATED: Infinity,
};

Hm. I worry about this - suppose the user has the cached document open, and then in a separate tab, they update one of their screenshots to a custom screenshot? Maybe that's a bit contrived. I just worry that counting events is more fragile than explicitly opting in to which actions are part of the "startup" set. Is that worry unfounded?

potentially quite messy as that would be quite the plumbing modifying BroadcastToContent to take an optional startup flag to customize the meta object for each desired call site:

By quite messy, do you mean it'd be a pain to maintain, or a pain to implement? If it's a pain to implement, I'm willing to do it. I don't, however, want to land something that's a pain to maintain.

Comment 9

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

Also, is the set and number of messages during startup deterministic?

Comment 10

[Ed Lee :Mardak]

At least for SCREENSHOT_UPDATED, that should depend on the number of top sites that use screenshots (not rich icons).

The action.meta.isStartup change should be fine. There's many potential action creators that need to be updated for a general fix, so to start just the ones needed to fix this bug (only BroadcastToContent ?)

And to be clear, skipping some actions in the middleware can change behavior, e.g., showing stale content, but that's actually the desired startup behavior.

Comment 11

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

Attachment: Bug 1633913 - Annotate ActivityStream actions that occur during startup, and have the cached about:home document ignore them. r?Mardak!

Comment 12

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

Attachment: Bug 1633913 - Add a test to make sure no new Activity Stream actions get added to the startup window without the isStartup meta property. r?Mardak!

This also updates the head.js for the about:home startup cache tests to make
sure that Pocket stories exist during the test.

Depends on D80998

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9b4ac167f5fa
Annotate ActivityStream actions that occur during startup, and have the cached about:home document ignore them. r=Mardak
https://hg.mozilla.org/integration/autoland/rev/034094223ab6
Add a test to make sure no new Activity Stream actions get added to the startup window without the isStartup meta property. r=Mardak

Comment 14

[Cosmin Sabou [:CosminS]]

Backed out for turning Bug 1648918 into permafail.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=linux%2Cx64%2Cwebrender%2Cshippable%2Copt%2Ctalos%2Cperformance%2Ctests%2Ctest-linux64-shippable-qr%2Fopt-talos-other-e10s%2Ct%28o%29&group_state=expanded&fromchange=4a21afb65254da156757ac7dc17fa62668cabfbf&tochange=534ece4991d2fdafa5fc68ebd2b6ff3ae914c37e&selectedTaskRun=W0Nuf9H1ThG0ByN3w7La1g.0

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=308933722&repo=autoland&lineNumber=6023

Backout link: https://hg.mozilla.org/integration/autoland/rev/25be502ba95b9df36916bde91f8bdf34cbf2659f

Comment 15

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

Attachment: Bug 1633913 - Move the queueEarlyMessageMiddleware before the rehydrationMiddleware in the Activity Stream middleware chain. r?Mardak!

The queueEarlyMessageMiddlware is designed to hold any messages from the about:home/about:newtab
document until the first message from the parent process is received. This is important because
it's possible for the about:home/about:newtab document to finish loading before Activity Stream
has finished initializing.

The other changes in this patch series allow the rehydrationMiddlware to skip dispatching
actions for the cached about:home document during the startup window. Unfortunately, because
the rehydrationMiddleware has been earlier in the chain than queueEarlyMessageMiddlware, this
means that those ignored startup actions didn't trigger the early queued messages to be sent
to the parent. This prevented the timestamps.about_home_topsites_first_paint scalar from being
set, which broke a Talos test.

By moving the queueEarlyMessageMiddleware earlier, it has an opportunity to send those early
actions to the parent before the rehydrationMiddleware has an opportunity to discard actions.

Depends on D80999

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/290605d18b40
Annotate ActivityStream actions that occur during startup, and have the cached about:home document ignore them. r=Mardak
https://hg.mozilla.org/integration/autoland/rev/5c42bddc167a
Add a test to make sure no new Activity Stream actions get added to the startup window without the isStartup meta property. r=Mardak
https://hg.mozilla.org/integration/autoland/rev/0f8566feffd2
Move the queueEarlyMessageMiddleware before the rehydrationMiddleware in the Activity Stream middleware chain. r=Mardak

Comment 17

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/290605d18b40
https://hg.mozilla.org/mozilla-central/rev/5c42bddc167a
https://hg.mozilla.org/mozilla-central/rev/0f8566feffd2

Comment 18

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/290605d18b40
https://hg.mozilla.org/mozilla-central/rev/5c42bddc167a
https://hg.mozilla.org/mozilla-central/rev/0f8566feffd2