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.

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

err, GetOriginForCodebase --> GetCodebasePrincipal.

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

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

Do we need this for 1.9.1?

Minusing until we know whether this is a must have for 1.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.

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

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.

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.

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

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.

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

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.

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 on attachment 378343 [details] [diff] [review] v2 (merged iid changes) [Checkin comment 34] Looks good. r+sr=jst

Honza: ETA on the 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

Build failure, reopening.

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

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.

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

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

(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 on attachment 378343 [details] [diff] [review] v2 (merged iid changes) [Checkin comment 34] http://hg.mozilla.org/mozilla-central/rev/b2e48c0aa965 Relanded.

Backed out again, tests fail on windows.

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 on attachment 378696 [details] [diff] [review] v2.1 [Checkin comment 37] http://hg.mozilla.org/mozilla-central/rev/f1c65acaccf3

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

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.

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.

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.

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