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

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Nickolay_Ponomarev]

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]

Attachment: testcase

Comment 2

[Simon Bünzli]

Attachment: 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 [e-mail] instead)]

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]]

Dietrich, can we get a SWAG as to the work involved to do this?

Comment 5

[Dietrich Ayala (:dietrich)]

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]

(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]]

Definitely wanted, but can't block on it without a better estimate of the implementation cost.

Comment 8

[Simon Bünzli]

Attachment: 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]

Attachment: tests (first try)

Comment 10

[Simon Bünzli]

Attachment: 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]

Attachment: unbitrotted and with self-contained test

Comment 12

[Asaf Romano (gone)]

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]

Attachment: 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

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/1d4c7cf0c7f2

Comment 15

[(no longer active)]

Tests checked in as http://hg.mozilla.org/mozilla-central/rev/948b5a201326

Comment 16

[Reed Loden [:reed]]

Ehsan, please note that Simon's last name is Bünzli, not Bunzli. :)

Comment 17

[(no longer active)]

(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]

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]

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]

(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

[(no longer active)]

(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]

(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

[(no longer active)]

(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

[u279076]

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]

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

[u279076]

(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

Comment 27

[baptx]

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.

Comment 28

[baptx]

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)

Comment 29

[Nickolay_Ponomarev]

@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>).

Comment 30

[baptx]

For people interested, here is my bug report: https://bugzilla.mozilla.org/show_bug.cgi?id=1501594