Closed Bug 1824094 Opened 1 year ago Closed 1 year ago

[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

Categories

(Firefox :: Top Sites, defect)

Desktop
All
defect

Tracking

()

VERIFIED FIXED
113 Branch
Tracking Status
firefox113 --- verified

People

(Reporter: cfat, Assigned: mconley)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

[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.

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

Flags: needinfo?(cfat)

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

Flags: needinfo?(cfat)

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.

Assignee: nobody → mconley
Status: NEW → ASSIGNED
Blocks: 1614351
Blocks: 1824145
Pushed by mconley@mozilla.com:
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 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
Flags: needinfo?(mconley)
Attachment #9325329 - Flags: approval-mozilla-beta?
Flags: qe-verify+
Pushed by mconley@mozilla.com:
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

Backed out for causing Mn failures on test_restore_manually_with_pinned_tabs.py

Backout link

Push with failures

Failure log

Flags: needinfo?(mconley)
Flags: needinfo?(mconley)
Attachment #9325329 - Flags: approval-mozilla-beta?

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...

(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!)

Flags: needinfo?(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

Flags: needinfo?(mconley)
Flags: needinfo?(gijskruitbosch+bugs)

(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.

Flags: needinfo?(gijskruitbosch+bugs)
Pushed by mconley@mozilla.com:
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
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 113 Branch

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.

Flags: needinfo?(mconley)

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

Flags: needinfo?(mconley)
See Also: → 1835654

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.

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: