The default bug view has changed. See this FAQ.

Session store should save/restore sessionStorage data (from DOMStorage)

RESOLVED FIXED in Firefox 3.1b1

Status

()

Firefox
Session Restore
RESOLVED FIXED
11 years ago
8 years ago

People

(Reporter: Nickolay_Ponomarev, Assigned: Simon Bünzli)

Tracking

(Blocks: 1 bug, {dev-doc-complete, html5, testcase})

Trunk
Firefox 3.1b1
dev-doc-complete, html5, testcase
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 -
wanted-firefox3 +
wanted-firefox3.5 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

11 years ago
See http://www.whatwg.org/specs/web-apps/current-work/#sessionstorage

The note there says: "The lifetime of a browsing context can be unrelated to the lifetime of the actual user agent process itself, as the user agent may support resuming sessions after a restart." We're trying to support resuming sessions, so we ought to save sessionStorage data.

I will attach a testcase soon.
(Reporter)

Comment 1

11 years ago
Created attachment 223553 [details]
testcase
(Reporter)

Updated

11 years ago
Keywords: testcase
Component: General → Session Restore
QA Contact: general → session.restore
(Assignee)

Comment 2

10 years ago
Created attachment 266312 [details] [diff] [review]
WIP (SessionStore bits)

A simple patch might look about like this - and if the .value getter of nsIDOMStorageItem wouldn't throw that constantly, this could even work.

An alternative approach might be to always update on "dom-storage-changed" notifications. Unfortunately these currently don't allow to infer what docShell the modified DOMStorage belonged to (nor which value was changed specifically).

Updated

10 years ago
Attachment #266312 - Attachment is patch: true
Attachment #266312 - Attachment mime type: application/octet-stream → text/plain
From http://developer.mozilla.org/en/docs/DOM:Storage

"sessionStorage should also be capable of restoring data after the browser has crashed (and been restored), but due to bug 339445 that doesn't work in Firefox yet. Until this is resolved, the use of sessionStorage as a preventative measure is debatable."

Asking for blocking-firefox3?, to get it on the radar.
Flags: blocking-firefox3?
Dietrich, can we get a SWAG as to the work involved to do this?
simon's done the bulk of the work here already.

simon, what else needs to be done here? is there a bug # for the nsIDOMStorageItem.value problem, and does that problem block this bug?
Whiteboard: [swag: 2d]
(Assignee)

Comment 6

10 years ago
(In reply to comment #5)
> is there a bug # for the nsIDOMStorageItem.value problem

I don't know. I tried to figure out the code path where the problem arises, but after having recompiled the source several times in vain, I gave up. So I can't even tell you what the performance impact of the frequent storage checks might be...

(In reply to comment #5)
> does that problem block this bug?

Yup, without getting that value, you simply won't have anything meaningful to restore.
Definitely wanted, but can't block on it without a better estimate of the implementation cost.
Flags: blocking-firefox3? → blocking-firefox3-
Whiteboard: [swag: 2d] → [swag: 2d][wanted-firefox3]
Flags: wanted-firefox3+
Whiteboard: [swag: 2d][wanted-firefox3] → [swag: 2d]
(Assignee)

Updated

9 years ago
Depends on: 442048
(Assignee)

Comment 8

9 years ago
Created attachment 326922 [details] [diff] [review]
patch

I no longer see the issue from comment #2: This just slightly unbitrotted patch now works as expected for all non-secured sessionStorage items (and those wouldn't be stored with the default privacy settings, anyway).

So, besides the edge-case of bug 442048 and the fact that I've still not figured out how to write acceptable test cases, this patch would be ready for review.
Attachment #266312 - Attachment is obsolete: true
Keywords: html5

Updated

9 years ago
Flags: wanted-firefox3.1+
(Assignee)

Comment 9

9 years ago
Created attachment 331796 [details] [diff] [review]
tests (first try)
(Assignee)

Updated

9 years ago
Attachment #326922 - Flags: review?(gavin.sharp)
(Assignee)

Updated

9 years ago
Attachment #326922 - Flags: review?(mano)
(Assignee)

Updated

9 years ago
Attachment #326922 - Flags: review?(gavin.sharp)
(Reporter)

Updated

9 years ago
Blocks: 450465
(Assignee)

Comment 10

9 years ago
Created attachment 334509 [details] [diff] [review]
updated and with test included

Two changes since the last patch:
* sessionStorage data is now always copied when duplicating tabs (the privacy_level pref is overruled in that case as it's everywhere else).
* sessionStorage data is now saved per domain and no longer per path since that's what the back-end does, anyway.
Assignee: nobody → zeniko
Attachment #326922 - Attachment is obsolete: true
Attachment #331796 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #334509 - Flags: review?(mano)
Attachment #326922 - Flags: review?(mano)
(Assignee)

Updated

9 years ago
OS: Windows XP → All
Hardware: PC → All
Whiteboard: [swag: 2d] → [has patch][needs review mano]
(Assignee)

Updated

9 years ago
Attachment #334509 - Flags: review?(dietrich)
(Assignee)

Comment 11

9 years ago
Created attachment 337136 [details] [diff] [review]
unbitrotted and with self-contained test
Attachment #334509 - Attachment is obsolete: true
Attachment #337136 - Flags: review?(dietrich)
Attachment #334509 - Flags: review?(mano)
Attachment #334509 - Flags: review?(dietrich)
(Assignee)

Updated

9 years ago
Blocks: 426045
(Assignee)

Updated

9 years ago
Depends on: 455070
Comment on attachment 337136 [details] [diff] [review]
unbitrotted and with self-contained test

>diff -r 7352ef83055a -r 2eec93293469 browser/components/sessionstore/src/nsSessionStore.js
>--- a/browser/components/sessionstore/src/nsSessionStore.js	Fri Sep 05 14:51:24 2008 -0700
>+++ b/browser/components/sessionstore/src/nsSessionStore.js	Sat Sep 06 00:38:29 2008 +0200

>+  /**
>+   * update all sessionStorage "super cookies"

nit: Updates

>+   * @param aTabData
>+   *        Data object for a specific tab

nii The data...


>+  _serializeSessionStorage:
>+    function sss_serializeSessionStorage(aTabData, aHistory, aDocShell, aFullData) {
>+    let storageData = {};
>+    let hasContent = false;
>+    
>+    for (let i = 0; i < aHistory.count; i++) {
>+      let uri = aHistory.getEntryAtIndex(i, false).URI.clone();
>+      // sessionStorage is saved per domain (cf. nsDocShell::GetSessionStorageForURI)
>+      try {
>+        uri.path = "";
>+      }
>+      catch (ex) { /* among others, about:... URIs don't have a path */ }

should you rather check if it's a nIURL?

>+      if (storageData[uri.spec] || !(aFullData || this._checkPrivacyLevel(uri.schemeIs("https"))))
>+        continue;
>+      
>+      let storage = aDocShell.getSessionStorageForURI(uri);
>+      if (!storage || storage.length == 0)
>+        continue;
>+      

nit: trailing spaces, maybe in few other places.

>+  /**
>+   * restores all sessionStorage "super cookies"

>+   * @param aStorageData
>+   *        Storage data to be restored
>+   * @param aDocShell
>+   *        A tab's docshell (containing the sessionStorage)
>+   */
>+  _deserializeSessionStorage: function sss_deserializeSessionStorage(aStorageData, aDocShell) {
>+    let ioService = Cc["@mozilla.org/network/io-service;1"].getService(Ci.nsIIOService);
>+    for (let url in aStorageData) {
>+      let uri = ioService.newURI(url, null, null);
>+      let storage = aDocShell.getSessionStorageForURI(uri);
>+      for (let key in aStorageData[url]) {
>+        try {
>+          storage.setItem(key, aStorageData[url][key].value);
>+          if (uri.schemeIs("https"))
>+            storage.getItem(key).secure = aStorageData[url][key].secure || false;
>+        }
>+        catch (ex) { Cu.reportError(ex); } // don't let a single item stop recovering

Note here on when this could throw.

r=mano otherwise.
Attachment #337136 - Flags: review?(dietrich) → review+
(Assignee)

Comment 13

9 years ago
Created attachment 340154 [details] [diff] [review]
nits fixed

(In reply to comment #12)
> should you rather check if it's a nIURL?

I obviously should, yeah.

> Note here on when this could throw.

Done; and fixed all the other nits as well. Thanks for the review.
Attachment #337136 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
Whiteboard: [has patch][needs review mano] → [has patch]

Updated

9 years ago
Depends on: 248970
http://hg.mozilla.org/mozilla-central/rev/1d4c7cf0c7f2
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [has patch]
Target Milestone: --- → Firefox 3.1b1
Tests checked in as http://hg.mozilla.org/mozilla-central/rev/948b5a201326
Flags: in-testsuite+
Ehsan, please note that Simon's last name is Bünzli, not Bunzli. :)
(In reply to comment #16)
> Ehsan, please note that Simon's last name is Bünzli, not Bunzli. :)

My bad.  I checked these patches in from Windows, and the stupid console doesn't allow me to type or paste this character (ugh).  I'll try to check in on Linux from now on, which shouldn't cause this problem.  BTW, is there any way to correct hg logs in this case?

Comment 18

9 years ago
Ever since this check-in, both remembering my last session and opening my homepage on startup fail for me with certain extensions.  All I get is a single "Untitled" tab.  I have tried to find out what the cause it, but I am unable to.  Safemode does not help.  Even when installing extensions one-by-one, and noting the point at which it happened, I cannot identify a specific extension.  Bug what I do get is the following in Error Console.

Error: Security error = NS_ERROR_DOM_SECURITY_ERR
Source file: file:///C:/Program%20Files/Internet/firefox/components/nsSessionStore.js
Line: 1103

After the error has occurred. If I then use the session manager Session Manager extension and try to save a session, I get:

"This operation failed due to a file access error:
Security error"

Any ideas?

Comment 19

9 years ago
By the way, line 1103 of the Error Console message above is pointing:

      if (!storage || storage.length == 0)

which you can find in the suggested patch of Comment #12 above.
(Assignee)

Comment 20

9 years ago
(In reply to comment #19)
>       if (!storage || storage.length == 0)

Are you using a Private Browsing try-server build? If so, that's a known issue (see the bottom of bug 248970 comment #307).

(In reply to comment #17)
> the stupid console doesn't allow me to type or paste this character (ugh).

What code page are you using? This WFM with |chcp 850|. Then again, maybe that doesn't the right thing and pasting into |chcp 1252| works, even though the character looks wrong... Anyway, thanks for the checkins!
(In reply to comment #20)
> (In reply to comment #19)
> >       if (!storage || storage.length == 0)
> 
> Are you using a Private Browsing try-server build? If so, that's a known issue
> (see the bottom of bug 248970 comment #307).

AFAICT, this is *not* a Private Browsing specific problem per se; if the user has disabled DOM Storage via the pref, the same thing should happen IINM (that's the code path that PB is emulating).

> (In reply to comment #17)
> > the stupid console doesn't allow me to type or paste this character (ugh).
> 
> What code page are you using?

I'm not sure, the default one I guess, which is 437.

> This WFM with |chcp 850|. Then again, maybe that
> doesn't the right thing and pasting into |chcp 1252| works, even though the
> character looks wrong...

Not sure.  I never understood the code page stuff in Windows, let alone their interaction with the console, and the console's interaction with the underlying apps it's passing the typed texts as params to...

> Anyway, thanks for the checkins!

Hope to get your name right on future check-ins!  :-)
(Assignee)

Updated

9 years ago
Depends on: 458498
(Assignee)

Comment 22

9 years ago
(In reply to comment #21)
> AFAICT, this is *not* a Private Browsing specific problem per se

You're right. Instead of just try-catching that line, let's rather fix bug 458498 in some way first, though.
(Assignee)

Updated

9 years ago
Keywords: dev-doc-needed

Updated

9 years ago
Depends on: 458954

Updated

9 years ago
No longer depends on: 458498
(Assignee)

Updated

9 years ago
Blocks: 459041
(In reply to comment #18)
> Ever since this check-in, both remembering my last session and opening my
> homepage on startup fail for me with certain extensions.  All I get is a single
> "Untitled" tab.  I have tried to find out what the cause it, but I am unable
> to.  Safemode does not help.  Even when installing extensions one-by-one, and
> noting the point at which it happened, I cannot identify a specific extension. 
> Bug what I do get is the following in Error Console.

I just pushed the fix to bug 458954 which should solve this problem.  If you can still see this problem in tomorrow's nightlies, please file a new bug.
Depends on: 462541
Keywords: dev-doc-needed → dev-doc-complete
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b99) Gecko/20090605 Firefox/3.5b99

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b99) Gecko/20090605 Firefox/3.5b99

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b99) Gecko/20090605 Firefox/3.5b99

The test case attached to this bug no longer appears to work in the above builds.

STR:
1. Navigate to the test case
2. Trigger a crash using the crashme! extension
3. Restart Firefox
4. Restore the session

RESULT:
Step 1: 1st field = null, 2nd field = 123
Step 4: 1st field = null, 2nd field = 123

EXPECTED:
Step 1: 1st field = null, 2nd field = 123
Step 4: 1st field = 123, 2nd field = 123

I'm going to work on a regression range for this.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 25

8 years ago
This bug has been FIXED and is verified by a unit test which still passes. Please open a new bug for this regression (which I assume to be https only).
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago8 years ago
Resolution: --- → FIXED
(In reply to comment #25)
> This bug has been FIXED and is verified by a unit test which still passes.
> Please open a new bug for this regression (which I assume to be https only).

Filed: bug 497658
You need to log in before you can comment on or make changes to this bug.