1938752 - Firefox restore previous session/tabs

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect, P2)

Description

[Claudio Salvai]

Attachment: Proof-Of-Concept.mp4

Steps to reproduce:

Firefox version 133.0.3

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:133.0) Gecko/20100101 Firefox/133.0
1)Start Firefox and open two or more tabs.
2)Close Firefox.
3)Open a Private Window [IMPORTANT! -> you must open a Private Window WITHOUT opening Firefox normally first, otherwise the bug won´t trigger].
4)Open Firefox again, tabs will be restored

Proof of Concept video attached.

Actual results:

Previous tabs will be restored

Expected results:

Firefox should open a new fresh session

Comment 1

[Claudio Salvai]

I've made some for analysis and found out the bug has been introduced between version
118.0b9 [working]
and
119.0 [bug]

Comment 2

[BugBot [:suhaib / :marco]]

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

Comment 3

[Claudio Salvai]

I'm no code expert, but diffing the source code between these versions, i found these differences that might have something to do with this bug

firefox-118.0\browser\components\sessionstore\test\browser_480893.js ->
BrowserTestUtils.loadURIString(browser, "about:sessionrestore");

firefox-119.0\browser\components\sessionstore\test\browser_480893.js ->
BrowserTestUtils.startLoadingURIString(browser, "about:sessionrestore");

Comment 4

[Oana Botisan, Desktop QA]

I can reproduce this issue using Firefox 135.0a1 and Firefox 134.0b10 on Windows 10.

Comment 5

[BugBot [:suhaib / :marco]]

The severity field is not set for this bug.
:sfoster, could you have a look please?

For more information, please visit BugBot documentation.

Comment 6

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

I can reproduce this. When we start up with a private window, we unconditionally save the previous session state for possibly restoring later. And then when a non-private window is opened in the same session, Session Restore see's that deferred state and restores it - without consulting the pref or any of other other criteria we would normally check at startup to see if session restore needs to happen.

Here's where we save the initial (previous) state when the private window is opened.

And here's where we restore that previous state when the first non-private window is opened.

I'm not sure this is a good first bug as the STR are a bit complex and I imagine we'll want to add or change tests to verify. But maybe a good-second-bug?

Comment 7

[Matt Kwee]

Hi, my project team is interested in working on a fix for this bug. We are first-time contributors, and have read the relevant documentation on contributing. Are there any other documents we should read before we continue? Thanks!

Comment 8

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

(In reply to Matt Kwee from comment #7)

Hi, my project team is interested in working on a fix for this bug. We are first-time contributors, and have read the relevant documentation on contributing. Are there any other documents we should read before we continue? Thanks!

This probably not the best bug to tackle as your introduction to Firefox development. We normally suggest tackling something fairly trivial first to work through the kinks of getting your environment set up, working with our version control and code review tools and process.

Once you have an initial patch reviewed and landed, this bug is fair game. Session restore and this particular use case needs some careful thought. A solution seems possible, but unlike a good-first-bug, it hasn't already been mapped out for you. You'll need to familiarize yourself with how Session Restore works, how its tested, replicate the bug for yourself and puzzle out the best fix that won't break the other use cases, and if its possible to write an automated test that catches future regressions.

We do have some documentation for the platform feature, the pieces that are Firefox specific are not documented outside the code itself.

If I've not scared you off and when you've fixed a good-first-bug, flag me again if you have questions and I can mark this bug as in-progress so we don't have 2 people trying to fix it at the same time.

Comment 9

[Matt Kwee]

(Reply to Sam Foster, comment #8

Thanks for your guidance, Sam. After working on a more trivial bug, my team has created a basic patch for this issue, modifying the problematic code. After packaging and installing the modified version, I hand-tested it to verify that the unexpected behavior described in this thread is gone. Additionally, running the tests in the "browser/components/sessionrestore/test" directory via the command

./mach mochitest browser/components/sessionstore/test/browser
does not appear to cause any new tests to fail compared to the test output before making the changes. We haven't managed to write a test case that works, though - we don't quite understand the framework well enough to get it to pass, prepatch or postpatch. Is there anything else we should know or do before committing/submitting the patch via moz-phab? Thanks!

Comment 10

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

(In reply to Matt Kwee from comment #9)

./mach mochitest browser/components/sessionstore/test/browser
does not appear to cause any new tests to fail compared to the test output before making the changes. We haven't managed to write a test case that works, though - we don't quite understand the framework well enough to get it to pass, prepatch or postpatch.

This is a bit of a tricky case. To confirm the bug is fixed we really want to put the application startup under test and confirm that with a previous saved session, launching Firefox by opening a private window, then subsequently opening a non-private window doesn't unexpectedly restore that session. As the mochitest framework runs within the same Firefox process, its not much help here. We'll need a marionette test. There are a few already that should serve as good reference examples at browser/components/sessionstore/test/marionette/. There are docs for writing and running Marionette tests. As you'll see, the tests all subclass MarionetteTestCase, which has a self.marionette property which is the "client" used to talk to the Firefox process. That has API docs here.

For Session Restore specifically, there's a SessionStoreTestCase class which does some of the boilerplate setup for you. test_restore_manually.py looks like it has some of the pieces you'll need as this bug needs similar steps:

1. Open a couple of tabs and/or windows so there's session data to save, ensuring browser.startup.page is set to 1 i.e. don't restore automatically. That's the startup_page argument that gets passed to the setUp method.
2. quit the application and re-start it with a private window (i.e. firefox.exe -private equivalent). I'm looking and I don't actually see a easy way to do this. I'm asking around and will comment when I know more
3. Open a new non-private window and confirm that the previous session does not get restored. Its tough to assert on asynchronous things not happening. We may need to see if there's an event or observer notification we can catch to know that moment has passed. Failing that, wait a couple of seconds to be reasonably sure no new windows or other restored things have shown up.

Comment 11

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

(In reply to Sam Foster [:sfoster] (he/him) from comment #10)

2. quit the application and re-start it with a private window (i.e. firefox.exe -private equivalent). I'm looking and I don't actually see a easy way to do this. I'm asking around and will comment when I know more

Ok, I've been pointed at test_cli_arguments.py as an example of how to do this. I'm starting to think writing this test should be a follow-up bug as it balloons the complexity a bit. So if you want to develop a patch that fixes the bug as described, I can file the follow-up bug to implement the marionette test that goes with. And if that's something you want to tackle next, you have that option. Or we can leave it on file for one of the team to pick up when time permits.

Comment 12

[Matt Kwee]

Attachment: Bug 1938752 - Fix previous session being unexpectedly restored after opening private window first. r?sfoster

Comment 13

[Matt Kwee]

Attachment: Bug 1938752 - Prevent unexpected session restoration when private window opens before normal window. r?#sessionstore-reviewers,sfoster

Comment 14

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

To test this:

1. Set browser.startup.page pref to 1 ("Open previous windows and tabs is unchecked in about:preferences")
2. Open any website on a non-private window (that isn't about:newtab)
3. Close this browser window to quit the browser
4. Launch with a private window, e.g. ./mach run --private-window or using the taskbar context menu
5. Open a non-private window.

Expected results:

• The previous session is not restored

Actual results

• The tab opened in step 2 gets restored into the window from step 5.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/48564078bcf0
https://hg.mozilla.org/integration/autoland/rev/e46dbc31ecc0
Prevent unexpected session restoration when private window opens before normal window. r=sfoster,sessionstore-reviewers

Comment 16

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/e46dbc31ecc0

Comment 17

[Simona Badau, Desktop Test Engineering]

Reproduced the initial issue using Firefox 147.0.1 on both Windows 11 and Ubuntu 24.04.

Verified as fixed using Firefox 148.0b4 on Windows 11 and Ubuntu 24.04 — the previous session is no longer restored when following the steps to reproduce from Comment 14.