1434252 - sessionstore gets lost on upgrade to Firefox 58

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect, P1)

Description

[Hannes Papenberg]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20180103231032

Steps to reproduce:

1. Automatic upgrade from 57.0.4 to 58.0.1 (64bit)
2. Firefox requested a restart to finalise the upgrade.


Actual results:

The old session with all tabs was gone.


Expected results:

The old tabs should have been restored. I made a backup of the sessionstore-backups folder and in there had a recovery.jsonlz4, which seems to be corrupted (size of ~21kb) and a recovery.baklz4, which was fine. (size ~874kb) Every attempt at making FF58 load the recovery.baklz4 (from sessionstore-backups directly, by renaming it to sessionstore.jsonlz4 and by decoding it to plain JSON and then saving it as sessionstore.jsonlz4) failed. It always created a new session. I then downgraded to 57.0.4, deleted sessionstore.jsonlz4 and cleared sessionstore-backups. Then I copied recovery.baklz4 into that folder and FF57 successfully fired up and restored all tabs. Subsequently I started another update to 58.0.1 and it failed in the same way again. I was again able to restore the tabs with the same procedure as above.

Comment 1

[Hannes Papenberg]

If necessary, I can provide a copy of the recovery.baklz4 privately.

Comment 2

[Hannes Papenberg]

I tested this again today, when I had to restart Firefox due to memory issues. I opened and closes a lot of tabs in the meantime, so maybe this changed something. FF had done an update to 58.0.1 again and after another restart, it updated to 58.0.2. I again gave it the old sessionstore.jsonlz4 and it again refused to read that file, dropping all previous sessions. Long story short: The issue is still ongoing and still valid for me.

Is there a tool to decode/encode/modify the file, so that I can see what the issue is? Maybe it is too big? To many open tabs? Some sort of buffer overflow?

Comment 3

[Hannes Papenberg]

Today I closed a large number of tabs, so that the sessionstore.jsonlz4 file came down from 874,733 byte to 656,411 byte, but still 58.0.2 would not read the jsonlz4 from 57.0.4.

Comment 4

[Graham Perrin]

(In reply to Hannes Papenberg from comment #2)

> … Is there a tool to decode/encode/modify the file, …

[How to decompress jsonlz4 files (Firefox bookmark backups) using the command line? - Unix & Linux Stack Exchange](https://unix.stackexchange.com/questions/326897/how-to-decompress-jsonlz4-files-firefox-bookmark-backups-using-the-command-lin)

> Maybe it is too big? To many open tabs? …

I doubt it. 58.0.2 here had no difficulty restoring from a 1,280-tab session that was saved by Waterfox 56.0.4_4 on FreeBSD-CURRENT. 

----

Consider Mozilla bug 1427007 - Firefox can't autosave correctly session on browser exit (the list of tabs and their state) (sometimes)

>> RESOLVED FIXED in Firefox 59

Comment 5

[Hannes Papenberg]

I decoded the jsonlz4 file and send it through PHPs json_decode() to check for validity and the file contains a valid, LZ4 compressed JSON string. What to do next? As I said, 57.0.4 can read and write that file correctly, 58.0.2 fails at reading that file. 

On IRC I was given the hint to create bookmarks for all tabs, then update and then re-open those bookmarks, but somehow I refuse to use such a workaround instead of having this issue correctly fixed...

What do you guys need from me to check this further?

Comment 6

[Mike de Boer [:mikedeboer]]

Hi Hannes, thanks for checking that the file contains valid JSON! Since your sessionstore file may contain privacy sensitive information, it wouldn't be wise to attach it to this bug, since this is a public bugtracker.
What you could possibly do, however, is send over the sessionstore.jsonlz4 to me via public email to check out what the problem may be. This is really the only way for us at this point to see whatever might be the problem, because I can't reproduce the problem you're having otherwise.

Comment 7

[Mike de Boer [:mikedeboer]]

I meant *private* email, of course. Ahum!

Comment 8

[Hannes Papenberg]

File is sent.

Comment 9

[Hannes Papenberg]

I just tested this with 59.0 and it still does not work.

Comment 10

[nicolas.canceill]

Hello, I can confirm this bug when upgrading from Firefox 57.0.4 (64-bit). I heavily rely on Firefox's ability to restore my tabs so this bug is actually preventing me from upgrading and forced me to downgrade back to 57.0.4 and restore tabs from backup session store.

Comment 11

[Hannes Papenberg]

Any news regarding this? It is pretty cumbersome to downgrade each time I restart FF...

Comment 12

[nicolas.canceill]

Hello all,

Is there anything we can do to at least have this bug CONFIRMED?

Comment 13

[Hannes Papenberg]

Any news about this?

Comment 14

[Mike de Boer [:mikedeboer]]

[Tracking Requested - why for this release]: Tracking for all affected version that we can still reach, because this issue causes dataloss in the shape of a failure to restore the users' session due to cookies that can not be set for hosts with a leading '.' (meant as wildcard).

Comment 15

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1434252 - nsICookieService may throw an error in certain circumstances, so let's make SessionCookies::restore infallible.

Please see bug 1462402 for more details and the root cause.
SessionCookies.jsm uses nsICookieService::CookieExists and nsICookieService::Add
to restore session-cookies. These currently throw for hostnames that contain a
leading '.' and maybe other cases, so we need to wrap the calls in a try..catch
block to prevent breakage, leading to an unrestored session.

Review commit: https://reviewboard.mozilla.org/r/244744/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/244744/

Comment 16

[Hannes Papenberg]

Thank you very much for finding out what the issue is. Am I right in assuming that when I'm activating my next update from FF57, it will update to the next version each time, so I have to go through FF58, FF59 and FF60 before I could use a potentially fixed version FF60.0.1? That would mean that I had to save the session data from FF57 somewhere before doing the updates.

Comment 17

[Mike de Boer [:mikedeboer]]

(In reply to Hannes Papenberg from comment #16)
> Thank you very much for finding out what the issue is. Am I right in
> assuming that when I'm activating my next update from FF57, it will update
> to the next version each time, so I have to go through FF58, FF59 and FF60
> before I could use a potentially fixed version FF60.0.1? That would mean
> that I had to save the session data from FF57 somewhere before doing the
> updates.

Yes, that's right. Can't make it any prettier, I'm afraid :(

Comment 18

[Ryan VanderMeulen [:RyanVM]]

There are no watershed releases between 57 and 60, so you should get a full update directly from 57 to the latest version. A quick local test confirms the same (also being careful to note that version 60 is currently being throttled at a 25% rollout rate, so not all users are being updated to it yet).

That said, please note that version 60.0.1 was already released and does not contain a fix for this bug. Aat this point, it's more likely that the first release to contain any fix will be Firefox 61 shipping in late June.

Comment 19

[Dão Gottwald [:dao]]

Comment on attachment 8976651 [details]
Bug 1434252 - nsICookieService may throw an error in certain circumstances, so let's make SessionCookies::restore infallible.

https://reviewboard.mozilla.org/r/244744/#review250992

Are you going to revert this when bug 1462402 gets fixed?

::: browser/components/sessionstore/SessionCookies.jsm:15
(Diff revision 1)
>  ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm", this);
>  
>  ChromeUtils.defineModuleGetter(this, "PrivacyLevel",
>    "resource://gre/modules/sessionstore/PrivacyLevel.jsm");
>  
> +const {utils: Cu} = Components;

Cu is already available here, see bug 767640.

Comment 20

[Mike de Boer [:mikedeboer]]

Comment on attachment 8976651 [details]
Bug 1434252 - nsICookieService may throw an error in certain circumstances, so let's make SessionCookies::restore infallible.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/244744/diff/1-2/

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8c7f77a84166
nsICookieService may throw an error in certain circumstances, so let's make SessionCookies::restore infallible. r=dao

Comment 22

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/8c7f77a84166

Comment 23

[Hannes Papenberg]

Thank you. Now the long wait for FF62

Comment 24

[Ryan VanderMeulen [:RyanVM]]

This doesn't seem like something we'd want to backport all the way to release without testing, but I think uplifting to Beta would make sense given where we are in the cycle. Please nominate whenever you're comfortable doing so.

Comment 25

[Mike de Boer [:mikedeboer]]

Comment on attachment 8976651 [details]
Bug 1434252 - nsICookieService may throw an error in certain circumstances, so let's make SessionCookies::restore infallible.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1331680
[User impact if declined]: failure to restore the users' session when it contains serialized session cookies that can not be restored.
[Is this code covered by automated tests?]: No, these should be added in bug 1462402; this patch is duct tape, really.
[Has the fix been verified in Nightly?]: No.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: n/a.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Because it's wrapping existing code with a try..catch block, which means it doesn't change its semantics and/ or its intended operation doesn't differ from before.
[String changes made/needed]: n/a.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8976651 [details]
Bug 1434252 - nsICookieService may throw an error in certain circumstances, so let's make SessionCookies::restore infallible.

Low-risk fix for session dataloss, approved for 61.0b7. Definitely hoping we can get some test coverage for this sooner rather than later, though.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/bbb2cb6e0d06

Comment 28

[Kanchan Kumari QA]

Verified as fixed on Fx 62.0a1 and Fx 61.0b7 with Win 10 x64 and Win 7 x64.

Comment 29

[Julien Cristau [:jcristau]]

Mind requesting uplift to esr60 when you feel comfortable?

Comment 30

[Mike de Boer [:mikedeboer]]

Comment on attachment 8976651 [details]
Bug 1434252 - nsICookieService may throw an error in certain circumstances, so let's make SessionCookies::restore infallible.

[Approval Request Comment]
See comment 25 for the earlier approval request.
Fix Landed on Version: 62, 61

Comment 31

[Julien Cristau [:jcristau]]

Comment on attachment 8976651 [details]
Bug 1434252 - nsICookieService may throw an error in certain circumstances, so let's make SessionCookies::restore infallible.

sessionrestore fix, verified in nightly and beta, approved for 60.1esr

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/a53b5d718e85

Comment 33

[Kanchan Kumari QA]

I have verified this bug fix on Fx 60.1.0esr (esr-localtest channel)while updating (manual/automatic update)from older esr versions. Session/Tabs get restored as expected.

Version 	60.1.0esr
Build ID 	20180621121604	
Update Channel 	esr-localtest
User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:60.0) Gecko/20100101 Firefox/60.0


Version 	60.1.0esr
Build ID 	20180621121604
Update Channel 	esr-localtest
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Firefox/60.0


Version 	60.1.0esr
Build ID 	20180621121604	
Update Channel 	esr-localtest
User Agent 	Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:60.0) Gecko/20100101 Firefox/60.0