Last Comment Bug 339445 - Session store should save/restore sessionStorage data (from DOMStorage)
: Session store should save/restore sessionStorage data (from DOMStorage)
Status: RESOLVED FIXED
: dev-doc-complete, html5, testcase
Product: Firefox
Classification: Client Software
Component: Session Restore (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: Firefox 3.1b1
Assigned To: Simon Bünzli
:
Mentors:
http://www.whatwg.org/specs/web-apps/...
Depends on: PrivateBrowsing 442048 455070 458954 462541
Blocks: 426045 450465 459041
  Show dependency treegraph
 
Reported: 2006-05-27 10:34 PDT by Nickolay_Ponomarev
Modified: 2009-06-11 12:18 PDT (History)
25 users (show)
mbeltzner: blocking‑firefox3-
reed: wanted‑firefox3+
mtschrep: wanted‑firefox3.5+
ehsan: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (1.17 KB, text/html)
2006-05-27 10:35 PDT, Nickolay_Ponomarev
no flags Details
WIP (SessionStore bits) (4.60 KB, patch)
2007-05-27 17:59 PDT, Simon Bünzli
no flags Details | Diff | Review
patch (3.24 KB, patch)
2008-06-26 08:42 PDT, Simon Bünzli
no flags Details | Diff | Review
tests (first try) (6.07 KB, patch)
2008-07-30 16:21 PDT, Simon Bünzli
no flags Details | Diff | Review
updated and with test included (10.87 KB, patch)
2008-08-19 10:17 PDT, Simon Bünzli
no flags Details | Diff | Review
unbitrotted and with self-contained test (9.74 KB, patch)
2008-09-05 15:41 PDT, Simon Bünzli
asaf: review+
Details | Diff | Review
nits fixed (9.64 KB, patch)
2008-09-24 10:12 PDT, Simon Bünzli
no flags Details | Diff | Review

Description Nickolay_Ponomarev 2006-05-27 10:34:26 PDT
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.
Comment 1 Nickolay_Ponomarev 2006-05-27 10:35:13 PDT
Created attachment 223553 [details]
testcase
Comment 2 Simon Bünzli 2007-05-27 17:59:44 PDT
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).
Comment 3 (not reading, please use seth@sspitzer.org instead) 2007-06-03 09:57:31 PDT
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.
Comment 4 Mike Beltzner [:beltzner, not reading bugmail] 2007-06-19 11:56:40 PDT
Dietrich, can we get a SWAG as to the work involved to do this?
Comment 5 Dietrich Ayala (:dietrich) 2007-06-19 16:00:20 PDT
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?
Comment 6 Simon Bünzli 2007-06-20 08:18:54 PDT
(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.
Comment 7 Mike Beltzner [:beltzner, not reading bugmail] 2007-06-25 11:13:57 PDT
Definitely wanted, but can't block on it without a better estimate of the implementation cost.
Comment 8 Simon Bünzli 2008-06-26 08:42:30 PDT
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.
Comment 9 Simon Bünzli 2008-07-30 16:21:49 PDT
Created attachment 331796 [details] [diff] [review]
tests (first try)
Comment 10 Simon Bünzli 2008-08-19 10:17:11 PDT
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.
Comment 11 Simon Bünzli 2008-09-05 15:41:15 PDT
Created attachment 337136 [details] [diff] [review]
unbitrotted and with self-contained test
Comment 12 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-09-24 08:56:06 PDT
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.
Comment 13 Simon Bünzli 2008-09-24 10:12:22 PDT
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.
Comment 14 :Ehsan Akhgari (busy, don't ask for review please) 2008-09-28 02:34:01 PDT
http://hg.mozilla.org/mozilla-central/rev/1d4c7cf0c7f2
Comment 15 :Ehsan Akhgari (busy, don't ask for review please) 2008-09-28 02:37:57 PDT
Tests checked in as http://hg.mozilla.org/mozilla-central/rev/948b5a201326
Comment 16 Reed Loden [:reed] (use needinfo?) 2008-09-28 02:38:18 PDT
Ehsan, please note that Simon's last name is Bünzli, not Bunzli. :)
Comment 17 :Ehsan Akhgari (busy, don't ask for review please) 2008-09-28 02:43:19 PDT
(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 IU 2008-09-29 12:54:56 PDT
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 IU 2008-09-29 12:57:57 PDT
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.
Comment 20 Simon Bünzli 2008-10-03 17:02:41 PDT
(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!
Comment 21 :Ehsan Akhgari (busy, don't ask for review please) 2008-10-03 17:14:12 PDT
(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!  :-)
Comment 22 Simon Bünzli 2008-10-04 04:01:28 PDT
(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.
Comment 23 :Ehsan Akhgari (busy, don't ask for review please) 2008-10-11 11:22:32 PDT
(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.
Comment 24 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2009-06-11 11:18:45 PDT
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.
Comment 25 Simon Bünzli 2009-06-11 11:54:17 PDT
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).
Comment 26 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2009-06-11 12:18:28 PDT
(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

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