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

VERIFIED FIXED in Firefox 28

Status

defect
P2
normal
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: emtwo, Assigned: sfoster)

Tracking

unspecified
Firefox 29
x86_64
Windows 8
Dependency tree / graph

Firefox Tracking Flags

(firefox28 verified)

Details

(Whiteboard: [beta28] [feature] p=2)

Attachments

(1 attachment, 1 obsolete attachment)

This would include checking the data for minimum requirements (at least 1 window, at least 1 tab, etc)
Whiteboard: [triage]
Whiteboard: [triage] → [triage] [feature] p=0
Whiteboard: [triage] [feature] p=0 → [beta28] [feature] p=0
Whiteboard: [beta28] [feature] p=0 → [beta28] [feature] p=2
I will have a stab at this
Assignee: nobody → sfoster
Blocks: metrov1it23
No longer blocks: metrov1backlog
Status: NEW → ASSIGNED
Priority: -- → P2
QA Contact: jbecerra
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?
Attachment #8363263 - Flags: review?(msamuel)
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.
Attachment #8363263 - Flags: review?(msamuel) → review+
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
Attachment #8363263 - Attachment is obsolete: true
Attachment #8365230 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/e0cd2d1ff7b7
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Could you please give some guidance in order for the QA to verify this? Thanks!
Flags: needinfo?(sfoster)
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.
Flags: needinfo?(sfoster)
> 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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.