1908019 - Store sidebar UI state in a pref that acts as a fallback

Status: VERIFIED FIXED

(Firefox :: Sidebar, defect, P1)

Description

[Ed]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:128.0) Gecko/20100101 Firefox/128.0

Steps to reproduce:

• enabled the 'History' option of 'Clear history when Firefox closes'

• enabled the bookmarks sidebar

• restarted FF

Actual results:

The bookmarks sidebar was not enabled.

Expected results:

The bookmarks sidebar should have been enabled even though empty. This was how it was under the same conditions in previous versions.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Firefox::Bookmarks & History' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 2

[Dana Lucaci, Desktop QA]

Hello, Ed! I tried to reproduce this issue on Firefox 128.0 (2024-07-04) using Windows 10, but with no success. Are there any additional steps (settings) or something that I'm missing? Does this issue happen with a new profile?
Thank you!

Comment 3

[Ed]

No other steps; it does still happen with a new profile. I'm using Win11 - can you test on that? I filed this bug as a result of helping someone in another forum who said it started after he updated to Win11.

Comment 4

[Ed]

The problem does not occur in FF v127 portable.

Comment 5

[Ed]

The problem also happens in FF v128 portable, but not in v127.

Comment 6

[Mark Banner (:standard8)]

Are you able to test this on the official Firefox desktop versions? This could be an issue that is specific to portable, so would need reporting to them.

Comment 7

[Ed]

Using official FF release versions 126 and 127:

• the problem does not occur on v.126

• the problem does occur on v.127 and all other versions/platforms/channels equal to or greater than 127

• I opened the standalone profile manager and started v.126 with the v.127 profile and the problem did occur

Comment 8

[Mark Banner (:standard8)]

Thanks, looks like this might be related to the work done in bug 1885894, so moving across to the sidebar component.

(In reply to Ed from comment #7)

• I opened the standalone profile manager and started v.126 with the v.127 profile and the problem did occur

Note that profile downgrade is not supported, and hence this scenario is never guaranteed to work.

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

:nsharpley, since you are the author of the regressor, bug 1885894, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 10

[:Gijs (he/him)]

This is a consequence of the "clear history" setting combined with the sidebar state moving from xulstore to session store. I can easily reproduce. Clearing history will also clear session history, which ends us up in https://searchfox.org/mozilla-central/rev/5756c5a3dea4f2896cdb3c8bb15d0ced5e2bf690/browser/components/sessionstore/SessionStore.sys.mjs#2568-2610 which will just wipe the file from disk and, if invoked prior to shutdown, will also erase all the in-memory data.

Honestly I'm not 100% sure what to do about this.

XUL store isn't per-window, and sidebar state is per-window. So it was always a bad fit that led to confusing state for users (if you opened multiple windows and some had an open sidebar and some didn't, what sidebar is open after a restart). So moving it was the right idea, I think.

However, this particular consequence is unwanted. It's just not clear to me how to avoid it, short of rewriting the session purging code I referenced to avoid clearing the sidebar data. That is pretty doable for the non-shutdown state, but we really primarily care about the shutdown state, where going from "delete this file" to "delete this file except for this bit" is not entirely trivial.

I guess we could create an artificial minimal state that carries over the sidebar state (something like {windows: [{sidebar: blah}]} and nothing else; we probably need a bit more to make session store actually load it happily) and write that to disk instead, which might be the easiest remedy?

Moving to sessionstore given that that is where any fix would have to live.

Comment 11

[Sarah Clements [:sclements]]

Clearing this need info for Nikki, since while she's technically the regressor I don't think this is on her specifically to fix. Per Gijs' comment, it'd be worth discussing this in the next session restore triage meeting today and figure out a plan for.

Comment 12

[Sam Foster [:sfoster] (he/him)]

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

However, this particular consequence is unwanted. It's just not clear to me how to avoid it, short of rewriting the session purging code I referenced to avoid clearing the sidebar data. That is pretty doable for the non-shutdown state, but we really primarily care about the shutdown state, where going from "delete this file" to "delete this file except for this bit" is not entirely trivial.

onPurgeSessionHistory seems to go through the motions of emptying out the session state and then ensuring lazy.SessionSaver.run(); runs, which replaces whatever we last saved with this empty state. So I think would could safely be a bit more selective to retain UI state for open windows, while purging tabs/closed tabs, cookies etc. as well as the closed windows which we do currently.

Comment 14

[Sarah Clements [:sclements]]

After seeing a few more bugs related to moving all of sidebar state to session restore (see bug 1909109 about being in permanently private browsing), I think we need to pursue the solution of keeping a pref to store sidebar UI state if it doesn't exist. It'd break the per-window sidebar state but it's better than having to play whack-a-mole with different edge cases and quirks of session restore.

Comment 16

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1885894

Comment 17

[Liz Henry (:lizzard) (relman/hg->git project)]

Marking this fix-optional for 130, as we are about to head into the last week of 130 beta.

Comment 18

[IanM]

Is this going to be fixed in fiefox131?

Comment 19

[Sarah Clements [:sclements]]

Sorry, we may not be able to get this into 131 but we'll get it fixed for 132.

Comment 21

[Sarah Clements [:sclements]]

This is going to slip to 133 as I've had other things I've needed to prioritize (it's been discussed with Gijs and product/UX and we're in agreement about it).

Comment 22

[Sarah Clements [:sclements]]

Attachment: WIP: Bug 1908019 - set up pref and backupSetState functions

Comment 23

[Jonathan Sudiaman [:jsudiaman]]

Attachment: Bug 1908019 - Store sidebar UI state in a pref that acts as a fallback r?#sidebar-reviewers!

Refactor sidebar state persistence logic outside of SessionStore and into SidebarController and SidebarManager. Expose an API for session store to update state. If session store data is not available, use the backup state instead. Works for both "Never remember history" and "Use custom settings for history".

Comment 24

[IanM]

Is this a work-around? If so, how do I use 'backup state'?

Comment 25

[Jonathan Sudiaman [:jsudiaman]]

(In reply to IanM from comment #24)

Is this a work-around? If so, how do I use 'backup state'?

Hi Ian,

This is a patch that we need to review before it gets shipped.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a7a3ec0f3d35
Store sidebar UI state in a pref that acts as a fallback r=sidebar-reviewers,sessionstore-reviewers,sfoster

Comment 28

[Sandor Molnar[:smolnar]]

Backed out for causing multiple failures

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

Push with failures

Failure log

Failure log

There's a lot of different failure, please check on push with failures.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/47f93788c763
Store sidebar UI state in a pref that acts as a fallback r=sidebar-reviewers,sessionstore-reviewers,sfoster

Comment 30

[Sandor Molnar[:smolnar]]

Backed out for causing perma bc failures @ browser_sidebar_keys.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/678eb88910a76627bf321c20d5d260efca38aee6

Push with failures

Failure log -> TEST-UNEXPECTED-FAIL | browser/base/content/test/sidebar/browser_sidebar_keys.js | Sidebar widget background should appear unchecked

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/214c931a0501
Store sidebar UI state in a pref that acts as a fallback r=sidebar-reviewers,sessionstore-reviewers,sfoster

Comment 32

[Norisz Fay [:noriszfay]]

Backed out for causing bc failure on browser_keyboard_tests.js

Backout link

Push with failures

Failure log

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1954a3e2d9ab
Store sidebar UI state in a pref that acts as a fallback r=sidebar-reviewers,sessionstore-reviewers,sfoster

Comment 34

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/1954a3e2d9ab

Comment 36

[Florin Bilt]

(In reply to Sandor Molnar[:smolnar] from comment #30)

Backed out for causing perma bc failures @ browser_sidebar_keys.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/678eb88910a76627bf321c20d5d260efca38aee6

Push with failures

Failure log -> TEST-UNEXPECTED-FAIL | browser/base/content/test/sidebar/browser_sidebar_keys.js | Sidebar widget background should appear unchecked

Perfherder has detected a browsertime performance change from push 678eb88910a76627bf321c20d5d260efca38aee6.

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)	Performance Profiles
3%	speedometer Elm-TodoMVC/CompletingAllItems/Sync	windows11-64-nightlyasrelease-qr	fission webrender	37.59 -> 36.60	Before/After
2%	speedometer Elm-TodoMVC/CompletingAllItems	windows11-64-nightlyasrelease-qr	fission webrender	39.34 -> 38.43	Before/After

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests.

If you need the profiling jobs you can trigger them yourself from treeherder job view or ask a sheriff to do that for you.

You can run these tests on try with ./mach try perf --alert 2732

For more information on performance sheriffing please see our FAQ.

Comment 37

[Ardelean Oana, Desktop Test Engineering [:oardelean]]

Reproducible on Firefox 132.0 on Windows 10.
Issue is verified as fixed on Firefox 133.0b4 and Firefox Nightly 134.0a1 on Windows 10, Ubuntu 22, macOS 14.