1825868 - Accessing SessionStartup.sessionType getter too early will result in failure to restore a session

Status: NEW

(Firefox :: Session Restore, defect, P2)

Description

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

I ran into this while debugging a patch for bug 1824094.

SessionStartup loads early on in the lifetime of the browser, and one of the things it does after initializing is to do an off-main-thread read of the SessionFile in order to see if there's anything there to restore: https://searchfox.org/mozilla-central/rev/6e4dc22cb30c9ffbfee917268c42b17ab8534b91/browser/components/sessionstore/SessionStartup.sys.mjs#134-137

However, between that initialization and the time that the file is read in and handled, callers can still attempt to access the SessionStartup.sessionType synchronous getter.

The problem here is that if we're still in the midst of loading the file, this getter will return NO_SESSION, and then cache the value, so that even if there was a session, SessionStartup will claim one doesn't exist. That sounds like a great way of losing a session.

We should probably make sessionType something that returns a Promise, and update all of the callsites.

Comment 1

[:Gijs (he/him)]

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

We should probably make sessionType something that returns a Promise, and update all of the callsites.

This might be "fun" because it feeds into willRestore() which has a number of other callers, which also need making async, etc. etc...

Comment 2

[:Gijs (he/him)]

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

The problem here is that if we're still in the midst of loading the file, this getter will return NO_SESSION, and then cache the value, so that even if there was a session, SessionStartup will claim one doesn't exist. That sounds like a great way of losing a session.

Hm, tbf, this can also only happen if the user doesn't have session restore enabled, right? https://searchfox.org/mozilla-central/rev/6e4dc22cb30c9ffbfee917268c42b17ab8534b91/browser/components/sessionstore/SessionStartup.sys.mjs#386-387,393

Comment 3

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

Yes, that's true - it looks like that first branch can be computed correctly synchronously (it's not waiting for anything from the SessionFile).

Comment 4

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

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

For more information, please visit auto_nag documentation.