960740 - Add a 'sanity check' to SessionStore.js to avoid writing bad data

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: General, defect, P2)

Description

[Marina Samuel [:emtwo]]

This would include checking the data for minimum requirements (at least 1 window, at least 1 tab, etc)

Comment 1

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

I will have a stab at this

Comment 2

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

Attachment: Sanity check in saveState before writing session data

This puts a simple sanity check around the actual writing of state data to disk. I just check for windows.length and a truthy selectedWindow and only write and update the timestamp if it passes. That catches the main error condition I've been able to log which is a session state of {"windows":[], "selectedWindow":0}. Clearly we could do further validation.. how much is enough?

Comment 3

[Marina Samuel [:emtwo]]

Comment on attachment 8363263 [details] [diff] [review]
Sanity check in saveState before writing session data

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

I think this is the best way to go for now since the {windows: [], selectedWindow: 0} is the only problem we've seen. If any other write issue comes up we can address it accordingly. Also, I'm hoping bug 959396 will fix some of this.

Comment 4

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

Attachment: Sanity check in saveState before writing session data

Patch updated to add \n to the dump string. Carrying msamuel's r+. Landed on fx-team: https://hg.mozilla.org/integration/fx-team/rev/e0cd2d1ff7b7

Comment 5

[Ryan VanderMeulen [:RyanVM]]

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

Comment 6

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/fb2b1405f493

Comment 7

[Manuela Muntean [Away]]

Could you please give some guidance in order for the QA to verify this? Thanks!

Comment 8

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

I was able to reproduce this before by variations of switching from desktop to metro with multiple tabs open, only about:start, and then back again. The bug was compounded by bug 959396, which means it may be more difficult to reproduce now. If you can start in either mode with/without the restore tabs option and switch successfully from desktop to metro and back again while getting the expected session behavior with regard to tabs, then it is confirmed as fixed.

Comment 9

[Manuela Muntean [Away]]

> I was able to reproduce this before by variations of switching from desktop
> to metro with multiple tabs open, only about:start, and then back again. The
> bug was compounded by bug 959396, which means it may be more difficult to
> reproduce now. If you can start in either mode with/without the restore tabs
> option and switch successfully from desktop to metro and back again while
> getting the expected session behavior with regard to tabs, then it is
> confirmed as fixed.

Verified as fixed, for iteration #23, using the above indications, with both latest Nightly and Aurora on Win 8 64-bit.