1824094 - [Intermittent] [Nightly] The “Sponsored” tag is no longer displayed for the Sponsored Top Sites when opening a New Firefox Window after the browser was previously restarted

Status: VERIFIED FIXED

(Firefox :: Top Sites, defect)

Description

[Carmen Fat [:cfat] - Ecosystem QA]

[Affected versions]:

• Firefox Nightly 113.0a1 - Build ID: 20230322211554

[Affected Platforms]:

• Windows 10 x64
• macOS 12.4
• Linux Ubuntu 20.04 x64

[Prerequisites]:

• Have a VPN client connected to a US server (for users located outside the US).

[Steps to reproduce]:

1. Open the browser from the prerequisites.
2. Open a new tab and make sure you have Sponsored Top Sites.
3. Restart the browser using the shortcuts (Ctrl+Shift+J then Ctrl+Alt+R).
4. Dismiss the Default browser message.
5. Press the Ctrl+N shortcut to open a New Firefox Window.
6. Observe the Sponsored Top Sites.

[Expected result]:

• The Top Sites display the “Sponsored” tag right under the tiles.

[Actual result]:

• The “Sponsored” tag is not displayed.

[Regression window]:

• I was able to reproduce the issue even back on the 94.0a1 Nighty build, so I assume the issue was introduced at the same time with the feature and it’s not a regression.

[Notes]:

• I am able to reproduce the issue only on the Nightly channel, on Beta and Release channels I did not reproduce it at all.
• I wasn’t able to reproduce the issue if closing and reopening the browser instead of restarting it through shortcuts.
• The issue is cleared if the tab is refreshed and doesn’t reproduce anymore when opening new tabs.
• Here is a screen recording of the issue.

Comment 1

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

Hi cfat - are you able to reproduce this with browser.startup.homepage.abouthome_cache.enabled set to false?

Comment 2

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

Actually, I've been able to reproduce, and this does appear to be influenced by the about:home startup cache pref.

Comment 3

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

Attachment: Bug 1824094 - Don't read from the about:home startup cache if the last session will be recovered. r?Gijs!

We were choosing not to read from the cache if the user has configured the browser
to restore the last session, but there's also the case where the last session might
be restored regardless - for example, to recover from a parent-process crash, after
an update applies, or if using the browser restart shortcut for developers.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/141c453cccac
Don't read from the about:home startup cache if the last session will be recovered. r=Gijs

Comment 5

[Narcis Beleuzu [:NarcisB]]

Backed out for bc failures on browser_no_startup_actions.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/c87710f5b27e20c4abc14854ffcd849c77d6cdb1
Log link: https://treeherder.mozilla.org/logviewer?job_id=410511284&repo=autoland&lineNumber=33426

Please also check this Tier2 perma failure

Comment 6

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

Comment on attachment 9325329 [details]
Bug 1824094 - Don't read from the about:home startup cache if the last session will be recovered. r?Gijs!

Beta/Release Uplift Approval Request

• User impact if declined: Users who have their session recovered after a parent process crash or applying an update might find that the first new window that they open has a slightly broken about:home.

This only impacts people who have the about:home startup cache enabled, which we hope to start doing a Nimbus rollout on for Firefox 112.

• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: See comment 0
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is a small, well understood patch to bypass the cache, and comes with an automated test.
• String changes made/needed: None.
• Is Android affected?: No

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f84682fa3ea9
Don't read from the about:home startup cache if the last session will be recovered. r=Gijs

Comment 8

[Norisz Fay [:noriszfay]]

Backed out for causing Mn failures on test_restore_manually_with_pinned_tabs.py

Backout link

Push with failures

Failure log

Comment 9

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

So the issue is that getting the startupType from SessionStartup is a race-y read, based on whether or not the Session JSON file has been pulled off of the disk yet. Accessing startupType too soon can result in it caching the NO_SESSION type before the file has been read: https://searchfox.org/mozilla-central/rev/f9beb753a84aa297713d1565dcd0c5e3c66e4174/browser/components/sessionstore/SessionStartup.sys.mjs#380-397

Investigating alternatives...

Comment 10

[:Gijs (out for now; he/him)]

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

So the issue is that getting the startupType from SessionStartup is a race-y read, based on whether or not the Session JSON file has been pulled off of the disk yet. Accessing startupType too soon can result in it caching the NO_SESSION type before the file has been read: https://searchfox.org/mozilla-central/rev/f9beb753a84aa297713d1565dcd0c5e3c66e4174/browser/components/sessionstore/SessionStartup.sys.mjs#380-397

😱

Can we file a bug on this and fix it separately (should maybe throw if trying to read it before that's happened, or use a promise or something)? Feels like a footgun we could run in separately (and also could potentially explain why we've seen people occasionally lose sessions!)

Comment 11

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

(In reply to :Gijs (he/him) from comment #10)

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

Can we file a bug on this and fix it separately (should maybe throw if trying to read it before that's happened, or use a promise or something)? Feels like a footgun we could run in separately (and also could potentially explain why we've seen people occasionally lose sessions!)

Yep, filed as bug 1825868.

ni'ing to Gijs for https://phabricator.services.mozilla.com/D173782#5742484

Comment 12

[:Gijs (out for now; he/him)]

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

ni'ing to Gijs for https://phabricator.services.mozilla.com/D173782#5742484

Thanks for the ping. Answered on phab.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7af6c9bb9b86
Don't read from the about:home startup cache if the last session will be recovered. r=Gijs

Comment 14

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/7af6c9bb9b86

Comment 15

[Carmen Fat [:cfat] - Ecosystem QA]

I was verifying this issue on the latest Firefox Nightly 113.0a1 (Build ID: 20230404093915) and can confirm that the issue is no longer reproducible by following the original STR from comment 0. However, while doing some extra testing, I noticed 2 things:

1. the “Sponsored” tag is still not displayed if restarting the browser while having 2 Firefox Windows opened at the same time.
2. the tiles are misaligned when dismissing a top site in the window where the issue above was reproducible.

Both of these issues can be seen in this video.

@mconley, could you please look at the behaviors mentioned above? I'd rather not mark this report as verified until knowing your input, since they might be related to other issues such as bug 1825868.

Comment 16

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

Hm, sorry for the delay here. That does look like unexpected behaviour indeed, and thanks for filing that as bug 1829348.

Comment 17

[Carmen Fat [:cfat] - Ecosystem QA]

For better tracking purposes, I will mark this report as Verified Fixed since the issue is no longer reproducible using the steps to reproduce from the Description. Since the issue mentioned at 1st point from comment 15 is no longer reproducible on my side, I have only filed Bug 1835654 for the 2nd behavior mentioned in the same comment.