501423 - StorageEvent implementation does not match the spec

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Eric Shepherd [:sheppy]]

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: http://www.w3.org/TR/webstorage/#the-storage-event

Our implementation has only one property, "domain", which is not included in the spec, and is missing several useful properties.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Note, the spec is still just a draft so anything in it may change.

Comment 2

[Juriy "kangax" Zaytsev]

Firefox (3.5.2) doesn't seem to fire storage event at all.

document.onstorage = function(){
  console.log('fired'); 
};
localStorage.setItem('foo', 'bar');

logs 'fired' in IE8 but not in FF.

Am I missing something?

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

So what if you add event listener using document.addEventListener("storage", listener, false);

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

And indeed, HTML5 doesn't say that document should have onstorage property.

Comment 5

[Juriy "kangax" Zaytsev]

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.

<http://www.w3.org/TR/webstorage/#localStorageEvent>

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

http://www.whatwg.org/specs/web-apps/current-work/ 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).

http://www.w3.org/TR/webstorage 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()

Comment 7

[Juriy "kangax" Zaytsev]

Yes, I realized it already : ) `addEventListener` works as expected. Thank you.

Comment 8

[mark]

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. http://www.w3.org/TR/webstorage/#the-storage-event
Safari and IE8 support it but only FF doesn't:
http://www.quirksmode.org/blog/archives/2009/06/html5_storage_t.html

Thanks

Comment 9

[mark]

From IEBlog: http://blogs.msdn.com/ie/archive/2008/10/06/updates-for-ajax-in-ie8-beta-2.aspx
"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."

Comment 10

[Johnny Stenback (:jst)]

We should fix this for 1.9.2, it's a web compatibility bug, and one that's pretty easy to fix it seems.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Note, seems like the draft is going to change again.
http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2009-October/023623.html

Comment 12

[Honza Bambas (:mayhemer)]

Attachment: v1

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.

Comment 13

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

Comment 14

[Honza Bambas (:mayhemer)]

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.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Has HTML5 been updated to cover http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2009-October/023623.html ?
Do we know how stable StorageEvent API is?

Comment 16

[Honza Bambas (:mayhemer)]

(In reply to comment #15)
> Has HTML5 been updated to cover
> http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2009-October/023623.html ?
> Do we know how stable StorageEvent API is?

Yes, it has been.

Comment 17

[Honza Bambas (:mayhemer)]

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

Comment 18

[Honza Bambas (:mayhemer)]

Attachment: v2

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 www.foo.com open an iframe with address javascrip:something(); and code in that iframe changes the storage, parent will get event with uri == 'www.foo.com' but should get 'javascript:something();'.

Comment 19

[Johnny Stenback (:jst)]

Comment on attachment 411990 [details] [diff] [review]
v2

Patch looks good. r=jst

Comment 20

[Johnny Stenback (:jst)]

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.

Comment 21

[Johnny Stenback (:jst)]

And the ';' after the definition of NS_PIDOMSTORAGE_IID needs to be removed.

Comment 22

[Honza Bambas (:mayhemer)]

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:
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1258289924.54.1258290701.28965.gz

solutions:

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

Comment 23

[Honza Bambas (:mayhemer)]

Attachment: v2 update

This is an update for the event.URL attribute implementation. I take the URL from the subject's principal.

Comment 24

[Johnny Stenback (:jst)]

Comment on attachment 412571 [details] [diff] [review]
v2 update

r=jst

Comment 25

[Johnny Stenback (:jst)]

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

Comment 26

[Honza Bambas (:mayhemer)]

Comment on attachment 409548 [details] [diff] [review]
v1

I just wanted to land this on m-c, but it seems it never got r+.

Comment 27

[Grzegorz Borkowski]

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 28

[Johnny Stenback (:jst)]

Comment on attachment 409548 [details] [diff] [review]
v1

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

r=jst!

Comment 29

[Honza Bambas (:mayhemer)]

Attachment: v1.1

Adjusted and cleaned up patch, ready to land.

Comment 30

[Honza Bambas (:mayhemer)]

Comment on attachment 423514 [details] [diff] [review]
v1.1

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

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Five of the tests in test_storageSessionStorageEventCheckPropagation.html have been orange in every test run since this landed.

Comment 32

[Honza Bambas (:mayhemer)]

Backed out due to a reglar test failure (thought locally passing in a debug build).

http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=dda9eddd101e

Comment 33

[Honza Bambas (:mayhemer)]

Test failures cause is described in comment 22.  The full version of the patch suffers from the same issue.  Working on the fix.

Comment 34

[Honza Bambas (:mayhemer)]

Attachment: v1.2 interdiff

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.

Comment 35

[Honza Bambas (:mayhemer)]

Attachment: v1.2 [Checkin comment 36]

The full patch, passed tryserver.

Comment 36

[Honza Bambas (:mayhemer)]

Comment on attachment 423824 [details] [diff] [review]
v1.2 [Checkin comment 36]

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

Comment 37

[Daniel Holbert [:dholbert]]

Attachment: followup: reorder constructor init lists to fix build warning [Checkin: Comment 38]

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.

Comment 38

[Daniel Holbert [:dholbert]]

Followup pushed: http://hg.mozilla.org/mozilla-central/rev/77800011e573

Comment 39

[Olli Pettay [:smaug][[e-mail]]]

So there is now the ObsoleteEvent. When is that dispatched and why is it there?

Comment 40

[Honza Bambas (:mayhemer)]

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

See http://www.whatwg.org/specs/web-apps/2007-10-26/multipage/section-storage.html#storage3

Comment 41

[Eric Shepherd [:sheppy]]

Updated documentation:

https://developer.mozilla.org/en/DOM/event/StorageEvent (new page)
https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIDOMStorageEventObsolete (marked obsolete, renamed, etc)
https://developer.mozilla.org/en/XPCOM_Interface_Reference/NsIDOMStorageManager (updated)
https://developer.mozilla.org/en/nsIDocShell (updated)

Change mentioned on Firefox 4 for developers.