Port Bug 1165277 to SeaMonkey - Use origin in SessionStorage

RESOLVED FIXED in seamonkey2.50

Status

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: frg, Assigned: frg)

Tracking

SeaMonkey 2.49 Branch
seamonkey2.50

SeaMonkey Tracking Flags

(seamonkey2.48 unaffected, seamonkey2.49esr fixed, seamonkey2.50 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

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.
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.
Assignee: nobody → frgrahl
Status: NEW → ASSIGNED
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
Attachment #8817873 - Flags: review?(philip.chee)
Attachment #8817873 - Flags: review?(iann_bugzilla)
Attachment #8817873 - Flags: approval-comm-aurora?
Posted file 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 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
Attachment #8817873 - Flags: review?(iann_bugzilla)
Attachment #8817873 - Flags: review+
Attachment #8817873 - Flags: approval-comm-aurora?
Attachment #8817873 - Flags: approval-comm-aurora+
> 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
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.50
Version: unspecified → SeaMonkey 2.49 Branch
Attachment #8817873 - Flags: review?(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);
}
Attachment #8817873 - Flags: review+
> 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?
Posted patch 1319114-SessionStorage.patch (obsolete) — Splinter Review
Not reopening the bug for it.
Attachment #8818012 - Flags: review?(philip.chee)
Comment on attachment 8818012 [details] [diff] [review]
1319114-SessionStorage.patch

This patch looks identical to the previous patch???
Sorry bad multitasking. Uploaded the wrong one.
Attachment #8818012 - Attachment is obsolete: true
Attachment #8818012 - Flags: review?(philip.chee)
Attachment #8818111 - Flags: review?(philip.chee)
Attachment #8818111 - Flags: review?(philip.chee) → review-
Attachment #8818111 - Flags: review- → review+
You need to log in before you can comment on or make changes to this bug.