Closed Bug 339445 Opened 14 years ago Closed 11 years ago

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

Categories

(Firefox :: Session Restore, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 3.1b1

People

(Reporter: asqueella, Assigned: zeniko)

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-complete, html5, testcase)

Attachments

(2 files, 5 obsolete files)

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.
Attached file testcase
Keywords: testcase
Component: General → Session Restore
QA Contact: general → session.restore
Attached patch WIP (SessionStore bits) (obsolete) — Splinter Review
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).
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]
(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]
Depends on: 442048
Attached patch patch (obsolete) — Splinter Review
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
Flags: wanted-firefox3.1+
Attached patch tests (first try) (obsolete) — Splinter Review
Attachment #326922 - Flags: review?(gavin.sharp)
Attachment #326922 - Flags: review?(mano)
Attachment #326922 - Flags: review?(gavin.sharp)
Blocks: 450465
Attached patch updated and with test included (obsolete) — Splinter Review
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)
OS: Windows XP → All
Hardware: PC → All
Whiteboard: [swag: 2d] → [has patch][needs review mano]
Attachment #334509 - Flags: review?(dietrich)
Attachment #334509 - Attachment is obsolete: true
Attachment #337136 - Flags: review?(dietrich)
Attachment #334509 - Flags: review?(mano)
Attachment #334509 - Flags: review?(dietrich)
Blocks: 426045
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+
Attached patch nits fixedSplinter Review
(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
Keywords: checkin-needed
Whiteboard: [has patch][needs review mano] → [has patch]
Depends on: PrivateBrowsing
http://hg.mozilla.org/mozilla-central/rev/1d4c7cf0c7f2
Status: ASSIGNED → RESOLVED
Closed: 12 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?
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?
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.
(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!  :-)
Depends on: 458498
(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.
Keywords: dev-doc-needed
Depends on: 458954
No longer depends on: 458498
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
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 → ---
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
Closed: 12 years ago11 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
It looks like a bug has been introduced in Firefox 57 / Quantum and now when we close a tab, a window or the browser that has set a session storage item and restore the web page, the session storage item is not restored. I noticed that the last version that restored correctly a session storage item was Firefox 56 (tested on Linux 64-bit release). You can test the session storage here: https://www.w3schools.com/html/tryit.asp?filename=tryhtml5_webstorage_session
I also noticed a related problem that was already in versions before Firefox 57. On some websites, if we set a session storage item manually via the browser console and we close the tab, window or browser, when we restore the web page, the session storage item is restored correctly. However, in the browser console, in "Storage" tab, "Session Storage", if we right click on a session storage item and select "Delete All", the session storage item is deleted but when we close the tab, window or browser and restore the web page, the session storage item is restored even if it was deleted! (in fact, to really delete a session storage item in the browser console, we have to select "Delete <key_name>" instead of "Delete all" or execute sessionStorage.removeItem(<key_name>) in the browser console).
I hope these problems will be fixed soon.
Can anyone reopen this bug? (I think it is better than to create a new bug report for the same problem that has been reintroduced)
@baptx in b.m.o it's actually preferred to track the reintroduced problem in a new bug (also to file different bugs for different - even if possibly related - problems and to be very specific with steps to reproduce <https://developer.mozilla.org/en-US/docs/Mozilla/QA/Bug_writing_guidelines>).
For people interested, here is my bug report: https://bugzilla.mozilla.org/show_bug.cgi?id=1501594
You need to log in before you can comment on or make changes to this bug.