1319114 - Port Bug 1165277 to SeaMonkey - Use origin in SessionStorage

Status: RESOLVED FIXED

(SeaMonkey :: Session Restore, defect)

Description

[Frank-Rainer Grahl (:frg)]

suite/common/src/nsSessionStore.js uses principal.jarprefix in   _serializeSessionStorage. The pricipal no longer has one and this will fail now. We need to port Bug 1165277 to SeaMonkey.

Comment 1

[Frank-Rainer Grahl (:frg)]

This now became more serious in 2.50 because getSessionStorageForPrincipal was removed in bug 1316075 so saving and restoring now is broken and may result in a blank webpage after restore if the sessionstore.json contains storage entries.

Comment 2

[Frank-Rainer Grahl (:frg)]

Attachment: 1319114-SessionStorage.patch

Mostly copy + paste from browser.

[Approval Request Comment]
Regression caused by (bug #): Bug 1165277 and Bug 1316075
User impact if declined: session storage saving not working.
Testing completed (on m-c, etc.): c-a c-c
Risk to taking this patch (and alternatives if risky): none already broken.
String changes made by this patch: none

Comment 3

[Frank-Rainer Grahl (:frg)]

Attachment: sessionstore.zip

Tested on http://csh.rit.edu/~ryanw/chaos/WebStorageTest.html with a new profile. You can use the sessionstore.json in the zip to test it.

Comment 4

[Ian Neal]

Comment on attachment 8817873 [details] [diff] [review]
1319114-SessionStorage.patch

>+++ b/suite/common/src/nsSessionStore.js

>       try {
>+        let storageManager = aDocShell.QueryInterface(Components.interfaces.nsIDOMStorageManager);
>+        storage = storageManager.getStorage(window, principal);
>+        storage.length; // See Bug 1232955 - storage.length can throw, catch that failure.
>+
>+        if (storage && storage.length)
>           storageItemCount = storage.length;
>       }
>       catch (ex) { /* sessionStorage might throw if it's turned off, see bug 458954 */ }

With the if statement in the try/catch you should not need the "storage.length;" line (keep the comment though)

r/a=me with that addressed

Comment 5

[Frank-Rainer Grahl (:frg)]

> you should not need the "storage.length;" line

Yes. I left it inside in case so that an error points at the right line but you are right. You won't see it in console because of the catch and so its unneeded. I also restored the if to the old one and removed it there. It will throw one line later in the assignment if it hits the mentioned bug.

https://hg.mozilla.org/comm-central/rev/e2419e87ba072551ee2587d136e288fe9b700073
https://hg.mozilla.org/releases/comm-aurora/rev/e37ec19f90290a727b4c00bb466852c45a70d73f

Comment 6

[Philip Chee]

Comment on attachment 8817873 [details] [diff] [review]
1319114-SessionStorage.patch

(In reply to Frank-Rainer Grahl from comment #5)
>> you should not need the "storage.length;" line
> 
> Yes. I left it inside in case so that an error points at the right line but
> you are right. You won't see it in console because of the catch and so its
> unneeded. I also restored the if to the old one and removed it there. It
> will throw one line later in the assignment if it hits the mentioned bug.

Oops! Ian is too fast for me.

> +      let window = aDocShell.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
> +                                              .getInterface(Components.interfaces.nsIDOMWindow);

> +      let window = aDocShell.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
> +                                            .getInterface(Components.interfaces.nsIDOMWindow);

The . in .getInterface() should vertically align with .QueryInterface()

>       catch (ex) { /* sessionStorage might throw if it's turned off, see bug 458954 */ }

> With the if statement in the try/catch you should not need the 
> "storage.length;" line (keep the comment though)

> Yes. I left it inside in case so that an error points at the right line but 
> you are right. You won't see it in console because of the catch and so its 
> unneeded. I also

How about:
catch (ex) {
/* sessionStorage might throw if it's turned off, see bug 458954
 * storage.length can throw, See Bug 1232955 */
  Components.utils.reportError(ex);
}

Comment 7

[Frank-Rainer Grahl (:frg)]

> The . in .getInterface() should vertically align with .QueryInterface()

You are right. I probably looked 3 or 4 times at the statement to correctly align it and missed the closing bracket every time... Will prepare a nit fix. 

> How about:

This is during session storing. If storage is turned off we would report 'false' errors. Proper fix might be to check if its turned off first but is it worth it and are prefs still active when this is called during shutdown?

Comment 8

[Frank-Rainer Grahl (:frg)]

Attachment: 1319114-SessionStorage.patch

Not reopening the bug for it.

Comment 9

[Philip Chee]

Comment on attachment 8818012 [details] [diff] [review]
1319114-SessionStorage.patch

This patch looks identical to the previous patch???

Comment 10

[Frank-Rainer Grahl (:frg)]

Attachment: 1319114-nitfix.patch

Sorry bad multitasking. Uploaded the wrong one.

Comment 11

[Frank-Rainer Grahl (:frg)]

https://hg.mozilla.org/comm-central/rev/65fbe3d4ae2495e9e9999c741aff410ba9634f9c