Closed Bug 501423 Opened 11 years ago Closed 10 years ago

StorageEvent implementation does not match the spec


(Core :: DOM: Events, defect)

Not set





(Reporter: sheppy, Assigned: mayhemer)




(Keywords: compat, dev-doc-complete)


(3 files, 4 obsolete files)

Our current implementation of the StorageEvent for DOM storage does not match that specified by the latest draft of the specification.

The spec is here:

Our implementation has only one property, "domain", which is not included in the spec, and is missing several useful properties.
Note, the spec is still just a draft so anything in it may change.
Firefox (3.5.2) doesn't seem to fire storage event at all.

document.onstorage = function(){
localStorage.setItem('foo', 'bar');

logs 'fired' in IE8 but not in FF.

Am I missing something?
So what if you add event listener using document.addEventListener("storage", listener, false);
And indeed, HTML5 doesn't say that document should have onstorage property.
Olli, HTML5 doesn't mention anything about storage at all, from what I can see. However, Storage spec says that "storage" event should be fired on a document:

When the setItem(), removeItem(), and clear() methods are called on a Storage object x that is associated with a local storage area, if the methods did something, then in every HTMLDocument object whose Window  object's localStorage  attribute's Storage object is associated with the same storage area, other than x, a storage event must be fired, as described below.

<> says that onstorage property
should be in body and window (not sure if we support that since our storage event is based on an earlier draft). does not mention onstorage property at all.
The event should be dispatch to document, sure, but to catch that event on
document level you need to use addEventListener()
Yes, I realized it already : ) `addEventListener` works as expected. Thank you.
I want to emphasize the main issue here.
StorageEvent for DOM storage doesn't include all the attributes,
especially the source attribute which give you a reference to the window firing the event.
Safari and IE8 support it but only FF doesn't:

From IEBlog:
"Finally, we also included improved support of the updated onstorage HTML 5.0 event returned when the storage is changed. We now return the URI when the local storage is changed, so that handlers for pages can know who carried out the latest transaction in the storage space in addition to providing the source to the window of the origin. Furthering the good news, the HTML 5.0 Working Group has incorporated the clear method, which we shipped in Beta 1, into the draft. This essentially allows for script to clear all items accessible in its storage space without having to iterate though the keys."
Assignee: nobody → honzab.moz
Flags: blocking1.9.2?
We should fix this for 1.9.2, it's a web compatibility bug, and one that's pretty easy to fix it seems.
Flags: blocking1.9.2? → blocking1.9.2+
Keywords: compat
Priority: -- → P2
Target Milestone: --- → mozilla1.9.2
Note, seems like the draft is going to change again.
Attached patch v1 (obsolete) — Splinter Review
First version. All tests are working. This needs change to nsIDocShell interface (I'll create _1_9_2 version for it).

jst, feel free to ask on IRC for explanation of the code. Anyway, I should probably add more comments.
Attachment #409548 - Flags: review?
Attachment #409548 - Flags: review? → review?(jst)
I'm leaning towards not taking this for 1.9.2 after all. The patch is pretty big, and it's not clear that it's worth the risk for 1.9.2, and per talking to Honza it can't really be done in a less invasive way. Honza is investigating whether or not any other browsers implement this new event per spec yet, to help gauge the impact of shipping one more release w/o taking this.
Results of tests in other browsers:
- IE8.0.6: implements the event, but a different way. The event seems not to have any properties, event handler is attached to document (not window), it fires even in the window that changed the storage, and it's targeted by domain not by origin; so, it's close to what we have now
- Safari: 4.0.3: Fully implements the event, but fires it even in the same window
- Opera 10.01 and the latest chrome: don't know what localStorage is

So, only Safari is further then we are.

I'll try to make the patch smaller, however, all tests related to dom storage are green with this patch and I want to vote for this change as it is really useful from performance and clean-code point of view.
Has HTML5 been updated to cover ?
Do we know how stable StorageEvent API is?
(In reply to comment #15)
> Has HTML5 been updated to cover
> ?
> Do we know how stable StorageEvent API is?

Yes, it has been.
There are some ways to make this patch smaller by removing/changing following features:

- I introduced sessions storage "forking"; purpose is to distinguish the event originator and do not fire the event in the same window that introduced the change, it means that each window now strongly refers its own wrapper (fork) from a single session storage shared inside a single docshell. by removing this, we would fire event for a sessionStorage change even in the originating window (same as Safari does)

- dom storage now strongly refers its document's URI; to pass this parameter to the storage I had to change interfaces, there was no other reason to change them. this is needed for one of the storage attributes; as an alternative we could take URI from the principal ("generally the document URI"), it would remove lot of changes, but I'm not sure if the principal holds exactly the correct property demanded for the storage event
Attached patch v2 (obsolete) — Splinter Review
Smaller patch with two drawbacks:
- session storage events are fired also in the same window that changed the storage
- url attribute of the event is the principal URI and not the document URI; it means that when open an iframe with address javascrip:something(); and code in that iframe changes the storage, parent will get event with uri == '' but should get 'javascript:something();'.
Attachment #411990 - Flags: review?(jst)
Attachment #409548 - Flags: review?(jst)
Whiteboard: [needs review jst]
Comment on attachment 411990 [details] [diff] [review]

Patch looks good. r=jst
Attachment #411990 - Flags: review?(jst) → review+
Building with this patch I realized that the following line in nsGlobalWindow.h:

  typedef nsTArray<nsCOMPtr<nsIDOMStorageEvent>> nsDOMStorageEventArray;

needs a space between the two '>'s to make gcc happy.
And the ';' after the definition of NS_PIDOMSTORAGE_IID needs to be removed.
I found a problem with the smaller patch. When I was locally running tests for the patch I was running them in a different order then it is being run when the whole site is executed. If the sessionStorage tests are run first then we share the same sessionStorage object also in storageevent tests. This leads to storage event test failure, the event.documentURI attribute is wrong, filled with principal URI from the first test run.

See the log:


- use the first version of the patch
- let the attribute documentURI unimplemented
- get the URI from the current subject principal

I would like to try the third solution. It is already checked that the subject principal is of the same origin as the one the storage object has previously been created for (it is an explicit check in nsDocShell::GetSessionStorageForPrincipal) and the documentURI attribute is not used for any security checks, it has just an informal value.
Whiteboard: [needs review jst]
Attached patch v2 update (obsolete) — Splinter Review
This is an update for the event.URL attribute implementation. I take the URL from the subject's principal.
Attachment #412571 - Flags: review?(jst)
Whiteboard: [needs review jst]
Comment on attachment 412571 [details] [diff] [review]
v2 update

Attachment #412571 - Flags: review?(jst) → review+
Ok, so it turns out that latest update isn't quite right either, and given that, plus the size of the patch, and how close we are to the deadline here I think the right thing to do here is to postpone fixing this until the release after 1.9.2. So, blocking-, we'll get this in for the next release (with plenty of time to hash out any issues etc).
Flags: blocking1.9.2+ → blocking1.9.2-
Priority: P2 → --
Target Milestone: mozilla1.9.2 → mozilla1.9.3a1
Version: 1.9.1 Branch → Trunk
Comment on attachment 409548 [details] [diff] [review]

I just wanted to land this on m-c, but it seems it never got r+.
Attachment #409548 - Flags: review?(jst)
Attachment #411990 - Attachment is obsolete: true
Attachment #412571 - Attachment is obsolete: true
I've just installed Firefox 3.6 RC1, hoping to find the proper support for StorageEvent (by "proper" I mean access to its attributes, at least three most important: key, oldValue, newValue).
Safari supports it for more than half the year, it's a shame Firefox has local storage build in, but in such a **** way...
As almost all newest versions of major browsers support localStorage now (including beta versions of Opera and Chrome), in my company we decided to start to use it in our applications, and I guess we are not alone - so I predict the year 2010 will bring real boost in client side storage usage. So IMHO it's really important to have it working properly in Firefox 3.6!
Without those 3 attributes in event object, you are forced to use some nasty hacks. Having "change" event without information what has changed is really pointless - it's like registering one global "onclick" event, without information in this event what element has been clicked. Can you imagine programming it?
Comment on attachment 409548 [details] [diff] [review]

This looks good, I only found one comment that you should, and meant to, update...

- In nsDocShell::GetSessionStorageForPrincipal():

+    if (aCreate) {
+        // XXX Change this comment...
+        // We are demanded to create a new storage object. This indicates

... We are *asked* to..., plus make whatever other changes you had in mind when you added the XXX :)

Attachment #409548 - Flags: review?(jst) → review+
blocking2.0: --- → ?
Whiteboard: [needs review jst]
Attached patch v1.1 (obsolete) — Splinter Review
Adjusted and cleaned up patch, ready to land.
Attachment #409548 - Attachment is obsolete: true
Attachment #423514 - Flags: review+
Comment on attachment 423514 [details] [diff] [review]
Attachment #423514 - Attachment description: v1.1 → v1.1 [Checkin comment 30]
Closed: 10 years ago
Resolution: --- → FIXED
Keywords: dev-doc-needed
Blocks: FF2SM
Flags: in-testsuite+
Five of the tests in test_storageSessionStorageEventCheckPropagation.html have been orange in every test run since this landed.
Backed out due to a reglar test failure (thought locally passing in a debug build).
Resolution: FIXED → ---
Attachment #423514 - Attachment description: v1.1 [Checkin comment 30] → v1.1
Test failures cause is described in comment 22.  The full version of the patch suffers from the same issue.  Working on the fix.
Attached patch v1.2 interdiffSplinter Review
This is fix for the test failures (not for the build bustage).  I was not forking the sessionStorage object when it had already been existing in the hash table of a docshell even docshell had been asked to create it (for a new window).  Deleting just this one return let the fork be performed.

Note that the full patch makes nsGlobalWindow keep reference to its sessionStorage object, so, we never ask docshell to create sessionStorage for the same window more then ones.
Attachment #423609 - Flags: review?(jst)
Attachment #423609 - Flags: review?(jst) → review+
The full patch, passed tryserver.
Attachment #423514 - Attachment is obsolete: true
Attachment #423824 - Flags: review+
Blocks: 542775
Comment on attachment 423824 [details] [diff] [review]
v1.2 [Checkin comment 36]
Attachment #423824 - Attachment description: v1.2 → v1.2 [Checkin comment 36]
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
This checkin introduced these 12 lines of build warnings:

> dom/base/nsGlobalWindow.h: In constructor ‘nsGlobalWindow::nsGlobalWindow(nsGlobalWindow*)’:
> dom/base/nsGlobalWindow.h:782: warning: ‘nsGlobalWindow::mFocusMethod’ will be initialized after
> dom/base/nsGlobalWindow.h:773: warning:   ‘nsDataHashtable<nsStringHashKey, int>* nsGlobalWindow::mPendingStorageEventsObsolete’
> dom/base/nsGlobalWindow.cpp:642: warning:   when initialized here

> dom/src/storage/nsDOMStorage.h: In constructor ‘nsDOMStorage::nsDOMStorage()’:
> dom/src/storage/nsDOMStorage.h:253: warning: ‘nsDOMStorage::mItemsCached’ will be initialized after
> dom/src/storage/nsDOMStorage.h:250: warning:   ‘nsPIDOMStorage::nsDOMStorageType nsDOMStorage::mStorageType’
> dom/src/storage/nsDOMStorage.cpp:550: warning:   when initialized here

> dom/src/storage/nsDOMStorage.h: In copy constructor ‘nsDOMStorage::nsDOMStorage(nsDOMStorage&)’:
> dom/src/storage/nsDOMStorage.h:253: warning: ‘nsDOMStorage::mItemsCached’ will be initialized after
> dom/src/storage/nsDOMStorage.h:234: warning:   ‘nsCString nsDOMStorage::mDomain’
> dom/src/storage/nsDOMStorage.cpp:563: warning:   when initialized here

The attached followup patch fixes this by tweaking a few constructor init lists, to make their ordering match the ordering in the corresponding class definition.
Attachment #424727 - Flags: review?(jst)
Attachment #424727 - Flags: review?(jst) → review+
Blocks: 548496
Attachment #424727 - Attachment description: followup: reorder constructor init lists to fix build warning → followup: reorder constructor init lists to fix build warning [Checkin: Comment 38]
No longer blocks: FF2SM
Target Milestone: mozilla1.9.3a1 → mozilla1.9.3a3
blocking2.0: ? → ---
So there is now the ObsoleteEvent. When is that dispatched and why is it there?
(In reply to comment #39)
> So there is now the ObsoleteEvent. When is that dispatched and why is it there?

GlobalStorage object invokes ObsoleteEvent, it is just for backward compat (will be removed with globalStorage object some day.)

Depends on: CVE-2011-0052
You need to log in before you can comment on or make changes to this bug.