Closed Bug 455070 Opened 12 years ago Closed 11 years ago

Make sessionStorage object conform the WHATWG spec

Categories

(Core :: DOM: Core & HTML, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9.1

People

(Reporter: mayhemer, Assigned: mayhemer)

References

()

Details

(Keywords: fixed1.9.1, html5)

Attachments

(2 files, 8 obsolete files)

that is:
- change the getItem method to return DOMString (use nsIDOMStorage2 interface for the storage)
- change mapping of session storage by an origin and not by a domain as it is now
- compare principals instead of just string origins on some places

This bug will probably invalidate bug 394351, if it were not already invalid.
Blocks: 339445
Attached patch fix v1 (obsolete) — Splinter Review
Origin is extracted using script security manager and its method GetOriginForCodebase and subsequent call of GetOrigin on the resulting principal that returns 'file://' string from file: URIs. It is probably not a proper solution for bug 357323, I guess.

This patch is dependent on localStorage implementation (bug 422526).
Attachment #338670 - Flags: review?(dcamp)
err, GetOriginForCodebase  -->  GetCodebasePrincipal.
Summary: Make sessionStorage object conform the WHATWG spec → Make sessionStorage and localStrorage conform the WHATWG spec
Summary: Make sessionStorage and localStrorage conform the WHATWG spec → Make sessionStorage and localStorage conform the WHATWG spec
Attached patch fix v2 (obsolete) — Splinter Review
- using new WHATWG-compliant interface nsIDOMStorage2
- mapping session storage by an origin
- turning 'todo's in localStorage tests to regular tests + other cosmetic fixes
- added new test for localStorage mapping distinguished by schema
- dom_quickstubs.qsconf updated to work JUST with the new nsIDOMStorage2 interface, needed because js engine/xpconnect are unable to figure out which interface to delegate to when there are methods with identical name in a single DOM class even JS wrapped native is correctly QIed to interface I want; there is not any quick stubs optimization for old nsIDOMStorage interface (globalStorage)
Attachment #338670 - Attachment is obsolete: true
Attachment #339782 - Flags: review?(dcamp)
Attachment #338670 - Flags: review?(dcamp)
Comment on attachment 339782 [details] [diff] [review]
fix v2

Forget to mention: I change nsIDocShell interface because:
  1. I need to pass whole URI when adding new session storage to extract the origin
  2. I believe these two methods, I change, have to be in an "internal" or a "private" non-scriptable interface; when some extension developer play with sessionStorage objects using these two methods he IMHO does something wrong or malignant.
nsIDocShell is anything but private and nonscriptable.  If you don't want these methods scriptable, please mark them so.

Are these various domstorage specs really stable enough that we want to put the interfaces in the SDK (which implies frozen interface)?
Attachment #339782 - Attachment is obsolete: true
Attachment #339782 - Flags: review?(dcamp)
Summary: Make sessionStorage and localStorage conform the WHATWG spec → Make sessionStorage object conform the WHATWG spec
Attached patch v1.1 (obsolete) — Splinter Review
Up-to-date patch, just session storage patch.
Attachment #339854 - Flags: review?(dcamp)
Duplicate of this bug: 394351
Flags: wanted1.9.1?
Flags: blocking1.9.1?
Keywords: html5
Do we need this for 1.9.1?
Minusing until we know whether this is a must have for 1.9.1.
Flags: wanted1.9.1?
Flags: wanted1.9.1+
Flags: blocking1.9.1?
Flags: blocking1.9.1-
> Do we need this for 1.9.1?

No, this is dependent on (and actually complementary to) localStorage
implementation which unfortunately missed the milestone.
Attached patch v1.2 (obsolete) — Splinter Review
Merged against the new localStorage patch. Prepared for 1.9.2. All tests has been run and passed.
Attachment #339854 - Attachment is obsolete: true
Attachment #351223 - Flags: review?(dcamp)
Attachment #339854 - Flags: review?(dcamp)
Attached patch inderdiff 1.1 -> 1.2 (obsolete) — Splinter Review
Whiteboard: [needs review dcamp]
Attached patch v2 (obsolete) — Splinter Review
Deps on the new localStorage patch. I remove the nsIDocShell_1_9_1 interface because this patch will not land on 1.9.1 but on 1.9.2 (hopefully). I changed sessionStorage management to work with principals beside only URIs.
Attachment #351223 - Attachment is obsolete: true
Attachment #351224 - Attachment is obsolete: true
Attachment #357719 - Flags: review?(dcamp)
Attachment #351223 - Flags: review?(dcamp)
Would be awesome to make this available to workers too
That would make workers shared-something, tho, wouldn't it?  What is the result of simultaneous changes in a worker and in a webpage?  Doesn't this break the run-to-completion assumption that the only observable changes (excluding Date changes) are those caused by the executing script itself?  (I assume the navigator object added in a worker is wholly distinct from the navigator object in any other web page or worker, but that might be wrong.)
Hmm.. interesting point that it's shared-something. Implementation wise this should be ok, but it does mean that we're relying on scripts not to have races. I'll bring this up with W3C/WHATWG.
What exactly means "shared-something"? Workers already see the localStorage through the WorkerUtils interface (actually through the WorkerGlobalScope). The object (localStorage and same would be sessionStorage) is bound to the origin of the script that invoked a method of the worker (the one that posted a message to that worker, as I understand). It is doable to make it available, imo.
No longer blocks: 456912
Blocks: 492219
Requesting again b1.9.1 because we should preserve Storage as sessionStorage's constructor. See bug 492219.
Flags: blocking1.9.1- → blocking1.9.1?
Attachment #357719 - Flags: review?(dcamp) → review?(jst)
Our implementation of workers don't expose any storage objects to worker scripts. I don't understand why we would need to block 1.9.1 on this bug.
Attached patch v2 (merged) (obsolete) — Splinter Review
Patch merged against recent trunk and built upon patches for bug 492219 and bug 487695.
Attachment #357719 - Attachment is obsolete: true
Attachment #377849 - Flags: review?(jst)
Attachment #357719 - Flags: review?(jst)
Whiteboard: [needs review dcamp] → [needs review jst]
I agree that we should block on this now that I understand more about it. We never shipped anything with sessionStorage support, so the API change isn't as big of a deal, and shipping this now would mean we'd have a *really* hard time changing our API to be conformant with the spec later on. Blocking.

Honza, the last attachment is good to go, right? For the branch, we'll need some new interfaces as we can't change interfaces there any more, but other than that it looks good. I wish it was a smaller patch, but I guess there's not much we can do about that as most of the changes look necessary. Let me know if there's anything we could avoid doing for 1.9.1, just to keep the size down given how close to release we are.
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Target Milestone: --- → mozilla1.9.1
This is patch as it should be landed on the trunk. It's identical to "v2 (merged)" just updated iid changes from bug 492219.

I will create patch for 1.9.1 with new _1_9_1 interfaces where I make idl changes.
Attachment #377849 - Attachment is obsolete: true
Attachment #378343 - Flags: review?(jst)
Attachment #377849 - Flags: review?(jst)
Comment on attachment 378343 [details] [diff] [review]
v2 (merged iid changes) [Checkin comment 34]

Looks good. r+sr=jst
Attachment #378343 - Flags: superreview+
Attachment #378343 - Flags: review?(jst)
Attachment #378343 - Flags: review+
Whiteboard: [needs review jst] → [can land]
Honza: ETA on the branchsafe patch?
Whiteboard: [can land] → [needs 1.9.1 branchsafe patch]
(In reply to comment #24)
> Honza: ETA on the branchsafe patch?

Should be today.
Comment on attachment 378343 [details] [diff] [review]
v2 (merged iid changes) [Checkin comment 34]

http://hg.mozilla.org/mozilla-central/rev/ef25b4175b78
Attachment #378343 - Attachment description: v2 (merged iid changes) → v2 (merged iid changes) [Checkin comment 24]
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Build failure, reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
From the build log this looks like some bad dependencies with quickstubs. Perhaps you could touch base with jorendorff to see if this is something we can fix without having to clobber the world?
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1242808071.1242808274.29083.gz
Attachment #378343 - Attachment description: v2 (merged iid changes) [Checkin comment 24] → v2 (merged iid changes) [Backed out]
I'm adding new _1_9_1 interfaces for docshell that works as a sessionStorage manager and for dom storage window (implemented by nsGlobalWindow) to change sessionStorage attribute.

This file modifies dom_quickstubs.qsconf so there should be no problem after landing.
Attachment #378577 - Flags: superreview?(bzbarsky)
Comment on attachment 378577 [details] [diff] [review]
v2 for 1.9.1 [Checkin comment 38]

jst's probably the right reviewer here, since he's familiar with the patch and all.
Attachment #378577 - Flags: superreview?(bzbarsky) → superreview?(jst)
Comment on attachment 378577 [details] [diff] [review]
v2 for 1.9.1 [Checkin comment 38]

- In nsDocShell::GetSessionStorageForPrincipal():

+    nsDocShell* topDocShell = static_cast<nsDocShell*>(topItem.get());

Should this not QI to nsIDocShell_MOZILLA_1_9_1_SessionStorage rather than casting to nsDocShell?

Looks good with that fixed. r+sr=jst
Attachment #378577 - Flags: superreview?(jst)
Attachment #378577 - Flags: superreview+
Attachment #378577 - Flags: review+
(In reply to comment #32)
> Should this not QI to nsIDocShell_MOZILLA_1_9_1_SessionStorage rather than
> casting to nsDocShell?

It would be better, but C++ function overloading handles this well. I'll fix it before land.

Thanks for review.
Status: REOPENED → ASSIGNED
Attachment #378343 - Attachment description: v2 (merged iid changes) [Backed out] → v2 (merged iid changes) [Checkin comment 34]
Status: ASSIGNED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Whiteboard: [needs 1.9.1 branchsafe patch] → [needs 1.9.1 landing]
Backed out again, tests fail on windows.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I didn't clear the sessionStorage object at the start of the test. There were some data left from some other test. Fixing my test.
Attachment #378343 - Attachment is obsolete: true
Comment on attachment 378696 [details] [diff] [review]
v2.1 [Checkin comment 37]

http://hg.mozilla.org/mozilla-central/rev/f1c65acaccf3
Attachment #378696 - Attachment description: v2.1 → v2.1 [Checkin comment 37]
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Keywords: fixed1.9.1
Whiteboard: [needs 1.9.1 landing]
Had to land:

http://hg.mozilla.org/mozilla-central/rev/865b5be2ce4d
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/027583c55155

to fix orange on linux, it was due to a simple character case problem in a few paths in a test.
(In reply to comment #39)
> to fix orange on linux, it was due to a simple character case problem in a few
> paths in a test.

Thanks! I didn't have a linux machine to test on...
This caused bug 494453.
Depends on: 494502
Could this have also caused Bug 494543?  One regression range implicates it, still confirming on 1.9.1 -- we're getting an NS_ERROR_DOM_SECURITY_ERR that's breaking the dell.com cart.

Sites that were written to the last beta could see changed behaviour here, right?  What about sites that were written to the old localStorage API?  Not having beta coverage for this makes me sad.
Blocks: 495112
Note that a partial backout of this bug was done in bug 495112, so you'll want to follow there. Honza should file a follow up for whatever next-set-of-steps are to get this into shape.
Blocks: 495337
Followup bug 495337 filed for the parts of this that was backed out.
Blocks: 511635
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.