[SessionStore] allow SessionStore to work without writing data to disk

RESOLVED FIXED

Status

()

RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: zeniko, Assigned: zeniko)

Tracking

({privacy, verified1.8.1.2})

2.0 Branch
privacy, verified1.8.1.2
Points:
---
Bug Flags:
blocking1.8.1.2 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

12 years ago
sessionstore.js should only be written when either (1) resume_from_crash is true or (2) we need the data for session resuming.

This allows privacy concerned people to disable crash recovering while keeping full Undo Close Tab functionality, i.e. the recommended way to disable Session Restore would become setting sessionstore.resume_from_crash to |false| and _not_ setting sessionstore.enabled to |false| (which really is only for extensions providing the same functionality in a different way).
(Assignee)

Comment 1

12 years ago
Created attachment 249625 [details] [diff] [review]
make writing to disk dependent on resume_from_crash
Assignee: nobody → zeniko
Status: NEW → ASSIGNED
Attachment #249625 - Flags: review?(gavin.sharp)
(Assignee)

Comment 2

12 years ago
Branch drivers: fixing this bug on the branch would help those users who don't want crash recovery (for privacy reasons) but still want to be able to reopen accidentally closed tabs.
Flags: blocking1.8.1.2?

Updated

12 years ago
Keywords: privacy

Updated

12 years ago
Flags: blocking1.8.1.2? → blocking1.8.1.2+
Attachment #249625 - Flags: review?(dietrich)
Comment on attachment 249625 [details] [diff] [review]
make writing to disk dependent on resume_from_crash

This works ok, assuming default prefs. However, after applying this patch, if browser.startup.page=3 and resume_from_crash=false, sessions are not restored and this error is in the console:

Error: this._sessionFile has no properties
Source File: file:///Users/dayala/moz/branch/build/dist/BonEcho.app/Contents/MacOS/components/nsSessionStore.js
Line: 1720
Attachment #249625 - Flags: review?(dietrich) → review-
(Assignee)

Comment 4

12 years ago
Created attachment 250222 [details] [diff] [review]
fix v2

Yeah, of course we need a reference to the session files first before trying to remove the referenced files... sorry for that.
Attachment #249625 - Attachment is obsolete: true
Attachment #250222 - Flags: review?(dietrich)
Attachment #249625 - Flags: review?(gavin.sharp)
Comment on attachment 250222 [details] [diff] [review]
fix v2

thanks for fixing that, r+.
Attachment #250222 - Flags: review?(dietrich) → review+
(Assignee)

Updated

12 years ago
Whiteboard: [checkin needed]
(Assignee)

Comment 6

12 years ago
Gavin: may I ask you for another checkin? Thanks.

And while you're at it: bug 340895 could also need some CVS love... ;-)
(In reply to comment #6)
> Gavin: may I ask you for another checkin? Thanks.
> 
> And while you're at it: bug 340895 could also need some CVS love... ;-)
> 

i checked this in:

Checking in browser/components/sessionstore/src/nsSessionStore.js;
/cvsroot/mozilla/browser/components/sessionstore/src/nsSessionStore.js,v  <--  nsSessionStore.js
new revision: 1.56; previous revision: 1.55
done

i'll do 340895 as well.
(Assignee)

Updated

12 years ago
Whiteboard: [checkin needed] → [baking]
(Assignee)

Updated

12 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Assignee)

Comment 8

12 years ago
Comment on attachment 250222 [details] [diff] [review]
fix v2

Drivers: See comment #2 for why this low-risk patch should be included on the branch.
Attachment #250222 - Flags: approval1.8.1.2?

Comment 9

12 years ago
Comment on attachment 250222 [details] [diff] [review]
fix v2

Approved for the 1.8 branch, a=jay for drivers.  Let's get this landed asap and get QA some time to test it thoroughly.
Attachment #250222 - Flags: approval1.8.1.2? → approval1.8.1.2+
(Assignee)

Comment 10

12 years ago
Dietrich: the patch should directly apply to the branch as well. Could you please check it in and mark this bug fixed1.8.1.2? Thanks.
Whiteboard: [baking] → [checkin needed (1.8 branch)]
Checking in browser/components/sessionstore/src/nsSessionStore.js;
/cvsroot/mozilla/browser/components/sessionstore/src/nsSessionStore.js,v  <--  nsSessionStore.js
new revision: 1.5.2.42; previous revision: 1.5.2.41
done
Keywords: fixed1.8.1.2
Whiteboard: [checkin needed (1.8 branch)]

Comment 12

12 years ago
Please add test steps so QA can regress.  thanks.
Whiteboard: [need testcase]
(Assignee)

Comment 13

12 years ago
Steps to reproduce:
1. Set the hidden pref browser.sessionstore.resume_from_crash to false
2. Make sure that there's no file sessionstore.js in your profile folder
3. Set Firefox to load the previously opened windows and tabs at startup
4. Close Firefox
5. Verify that sessionstore.js exists
6. Restart Firefox
7. sessionstore.js should be gone again
8. Set the pref from step 1 to true again
9. sessionstore.js exists once more
Component: General → Session Restore
QA Contact: general → session.restore
Verified using steps in comment #13: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.2pre) Gecko/20070206 BonEcho/2.0.0.2pre

Also tried on Mac and Linux (ubuntu).
Keywords: fixed1.8.1.2 → verified1.8.1.2
Whiteboard: [need testcase]
You need to log in before you can comment on or make changes to this bug.