898775 - Fix the browser.sessionstore.resume_from_crash preference

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

The resume_from_crash preference has been in sessionstore since v1, probably. Its meaning has changed a lot since then and I think we should clean this up.

When it first got introduced, resume_from_crash=false basically meant that nothing will be written to disk. No session data will be stored, and no help will be given in case of a crash.

> // when crash recovery is disabled, session data is not written to disk
> XPCOMUtils.defineLazyGetter(this, "_resume_from_crash", function () {
>   this._prefBranch.addObserver("sessionstore.resume_from_crash", this, true);
>   return this._prefBranch.getBoolPref("sessionstore.resume_from_crash");
> });

This comment is certainly not true anymore. With resume_from_crash=false session data containing only app tabs will be written to disk and will automatically be restored on next startup. Interesting is that we do save the whole session including pinned and unpinned tabs when quitting:

> saveState: function ssi_saveState(aUpdateAll) {
>   let pinnedOnly = this._loadState == STATE_RUNNING && !this._resume_from_crash;
>   let oState = this._getCurrentState(aUpdateAll, pinnedOnly);

On next startup we will have a deferred session, i.e. we restore pinned tabs automatically and the user can decide to restore the rest of the session manually. If we crash, we only have pinned tabs and cannot offer the user to manually restore the last session. That's weird and inconsistent.

My suggestion is to fix resume_from_crash to behave like the name suggests. We shouldn't exclude pinned tabs when collecting the session, we should just not resume a session automatically after a crash with resume_from_crash=false.

I think resume_from_crash was introduced back then mostly because of privacy reasons? The preference was added before we even had the private browsing mode. Now we have private windows so I think that's the preferred way to go undercover if you need to.

The pref of course also allows a user to opt-out of maybe annoying crash dialogs if you don't care about your session and you happen to just shutdown the OS and not apps first. It is also used in a couple of test suites that don't want Firefox to restore stuff when crashing so it seems like we should keep that pref around.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

CC'ing a couple of people that might have an opinion about this.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Fix the browser.sessionstore.resume_from_crash preference

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

FTR, try is green:

https://tbpl.mozilla.org/?tree=Try&rev=968ed298941e

No big surprise, our startup path test coverage is bad.

Comment 4

[Steven MacLeod [:smacleod]]

Comment on attachment 782172 [details] [diff] [review]
Fix the browser.sessionstore.resume_from_crash preference

Review of attachment 782172 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! Just one thing:

It appears you missed a use of |pinnedOnly| in SessionStore.jsm:
|if (pinnedOnly) {|
http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/SessionStore.jsm#3840

Comment 5

[Steven MacLeod [:smacleod]]

(In reply to Steven MacLeod [:smacleod] from comment #4) 
> It appears you missed a use of |pinnedOnly| in SessionStore.jsm:
> |if (pinnedOnly) {|
> http://mxr.mozilla.org/mozilla-central/source/browser/components/
> sessionstore/src/SessionStore.jsm#3840

Disregard this, you remove it in Bug 898755

Comment 6

[David Teller [:Yoric] - still alive but not very active]

I'll try and review this on Thursday.

Comment 7

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 782172 [details] [diff] [review]
Fix the browser.sessionstore.resume_from_crash preference

Review of attachment 782172 [details] [diff] [review]:
-----------------------------------------------------------------

Just to be sure I understand: the intended behavior of resume_from_crash is implemented in nsSessionStartup._onSessionFileRead, isn't it?
If we don't use the session file, why do we bother reading it at all?

Regardless, the patch looks good and the session restore startup desperately needs this kind of cleanups, thanks.

::: browser/components/sessionstore/src/SessionWorker.js
@@ +121,5 @@
> +        File.move(this.path, this.backupPath);
> +      } catch (ex if isNoSuchFileEx(ex)) {
> +        // Ignore exceptions about non-existent files.
> +      } catch (ex) {
> +        // Throw the exception after we wrote the state to disk

Nit: "Wait until we have written the state to disk before throwing the exception[...]"

::: browser/components/sessionstore/test/unit/test_no_backup_first_write.js
@@ -1,2 @@
> -/* Any copyright is dedicated to the Public Domain.
> -   http://creativecommons.org/publicdomain/zero/1.0/ */

I don't understand what meaningful property we are testing here.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Rajchenbach Teller [:Yoric] from comment #7)
> Just to be sure I understand: the intended behavior of resume_from_crash is
> implemented in nsSessionStartup._onSessionFileRead, isn't it?

Yes.

> If we don't use the session file, why do we bother reading it at all?

We always restore pinned tabs and offer a manual restore of all non-pinned tabs later with resume_from_crash=false, which means we have to read it.

> ::: browser/components/sessionstore/test/unit/test_no_backup_first_write.js
> > -/* Any copyright is dedicated to the Public Domain.
> > -   http://creativecommons.org/publicdomain/zero/1.0/ */
> 
> I don't understand what meaningful property we are testing here.

The test is being removed because the 'backupOnFirstWrite' option isn't supported for _SessionFile.write() anymore. We'll always create a backup now.

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

FTR, I'll land this at the beginning of the next cycle to get some more coverage for this change. No need to rush it in.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/a07dcc3e414a

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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