455070 - Make sessionStorage object conform the WHATWG spec

Status: RESOLVED FIXED

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

Description

[Honza Bambas (:mayhemer)]

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.

Comment 1

[Honza Bambas (:mayhemer)]

Attachment: fix v1

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

Comment 2

[Honza Bambas (:mayhemer)]

err, GetOriginForCodebase  -->  GetCodebasePrincipal.

Comment 3

[Honza Bambas (:mayhemer)]

Attachment: fix v2

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

Comment 4

[Honza Bambas (:mayhemer)]

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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

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)?

Comment 6

[Honza Bambas (:mayhemer)]

Attachment: v1.1

Up-to-date patch, just session storage patch.

Comment 8

[Johnny Stenback (:jst)]

Do we need this for 1.9.1?

Comment 9

[Johnny Stenback (:jst)]

Minusing until we know whether this is a must have for 1.9.1.

Comment 10

[Honza Bambas (:mayhemer)]

> Do we need this for 1.9.1?

No, this is dependent on (and actually complementary to) localStorage
implementation which unfortunately missed the milestone.

Comment 11

[Honza Bambas (:mayhemer)]

Attachment: v1.2

Merged against the new localStorage patch. Prepared for 1.9.2. All tests has been run and passed.

Comment 12

[Honza Bambas (:mayhemer)]

Attachment: inderdiff 1.1 -> 1.2

Comment 13

[Honza Bambas (:mayhemer)]

Attachment: v2

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.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Would be awesome to make this available to workers too

Comment 15

[Jeff Walden [:Waldo]]

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

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 17

[Honza Bambas (:mayhemer)]

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.

Comment 18

[Honza Bambas (:mayhemer)]

Requesting again b1.9.1 because we should preserve Storage as sessionStorage's constructor. See bug 492219.

Comment 19

[Johnny Stenback (: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.

Comment 20

[Honza Bambas (:mayhemer)]

Attachment: v2 (merged)

Patch merged against recent trunk and built upon patches for bug 492219 and bug 487695.

Comment 21

[Johnny Stenback (: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.

Comment 22

[Honza Bambas (:mayhemer)]

Attachment: v2 (merged iid changes) [Checkin comment 34]

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.

Comment 23

[Johnny Stenback (:jst)]

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

Looks good. r+sr=jst

Comment 24

[Mike Beltzner [:beltzner, not reading bugmail]]

Honza: ETA on the branchsafe patch?

Comment 25

[Honza Bambas (:mayhemer)]

(In reply to comment #24)
> Honza: ETA on the branchsafe patch?

Should be today.

Comment 26

[Honza Bambas (:mayhemer)]

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

http://hg.mozilla.org/mozilla-central/rev/ef25b4175b78

Comment 27

[Honza Bambas (:mayhemer)]

Build failure, reopening.

Comment 28

[(not currently active) Ted Mielczarek]

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

Comment 29

[Honza Bambas (:mayhemer)]

Attachment: v2 for 1.9.1 [Checkin comment 38]

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.

Comment 30

[Peter Van der Beken [:peterv]]

I landed a fix for the dependency problem: http://hg.mozilla.org/mozilla-central/rev/ac136abf6fd3

Comment 31

[Boris Zbarsky [: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.

Comment 32

[Johnny Stenback (: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

Comment 33

[Honza Bambas (:mayhemer)]

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

Comment 34

[Honza Bambas (:mayhemer)]

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

http://hg.mozilla.org/mozilla-central/rev/b2e48c0aa965

Relanded.

Comment 35

[Honza Bambas (:mayhemer)]

Backed out again, tests fail on windows.

Comment 36

[Honza Bambas (:mayhemer)]

Attachment: v2.1 [Checkin comment 37]

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.

Comment 37

[Honza Bambas (:mayhemer)]

Comment on attachment 378696 [details] [diff] [review]
v2.1 [Checkin comment 37]

http://hg.mozilla.org/mozilla-central/rev/f1c65acaccf3

Comment 38

[Honza Bambas (:mayhemer)]

Comment on attachment 378577 [details] [diff] [review]
v2 for 1.9.1 [Checkin comment 38]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/4b8e9d27a9ad
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/564aec224cee

Comment 39

[Johnny Stenback (:jst)]

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.

Comment 40

[Honza Bambas (:mayhemer)]

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

Comment 41

[Boris Zbarsky [:bzbarsky]]

This caused bug 494453.

Comment 42

[Mike Shaver (:shaver emeritus)]

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.

Comment 43

[Mike Beltzner [:beltzner, not reading bugmail]]

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.

Comment 44

[Johnny Stenback (:jst)]

Followup bug 495337 filed for the parts of this that was backed out.