Last Comment Bug 501423 - StorageEvent implementation does not match the spec
: StorageEvent implementation does not match the spec
: compat, dev-doc-complete
Product: Core
Classification: Components
Component: DOM: Events (show other bugs)
: Trunk
: All All
-- normal with 1 vote (vote)
: mozilla1.9.3a3
Assigned To: Honza Bambas (:mayhemer)
: Andrew Overholt [:overholt]
Depends on: CVE-2011-0052
Blocks: 542775 548496
  Show dependency treegraph
Reported: 2009-06-30 11:24 PDT by Eric Shepherd [:sheppy]
Modified: 2011-01-07 09:55 PST (History)
12 users (show)
jst: blocking1.9.2-
bugzillamozillaorg_serge_20140323: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

v1 (112.62 KB, patch)
2009-10-31 15:43 PDT, Honza Bambas (:mayhemer)
jst: review+
Details | Diff | Splinter Review
v2 (83.36 KB, patch)
2009-11-12 09:33 PST, Honza Bambas (:mayhemer)
jst: review+
Details | Diff | Splinter Review
v2 update (4.45 KB, patch)
2009-11-16 05:29 PST, Honza Bambas (:mayhemer)
jst: review+
Details | Diff | Splinter Review
v1.1 (110.78 KB, patch)
2010-01-26 05:20 PST, Honza Bambas (:mayhemer)
honzab.moz: review+
Details | Diff | Splinter Review
v1.2 interdiff (1.08 KB, patch)
2010-01-26 13:15 PST, Honza Bambas (:mayhemer)
jst: review+
Details | Diff | Splinter Review
v1.2 [Checkin comment 36] (110.91 KB, patch)
2010-01-27 11:00 PST, Honza Bambas (:mayhemer)
honzab.moz: review+
Details | Diff | Splinter Review
followup: reorder constructor init lists to fix build warning [Checkin: Comment 38] (1.94 KB, patch)
2010-02-01 21:17 PST, Daniel Holbert [:dholbert] (vacation, returning 2/27)
jst: review+
Details | Diff | Splinter Review

Description User image Eric Shepherd [:sheppy] 2009-06-30 11:24:54 PDT
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.
Comment 1 User image Olli Pettay [:smaug] 2009-07-01 06:30:18 PDT
Note, the spec is still just a draft so anything in it may change.
Comment 2 User image Juriy "kangax" Zaytsev 2009-08-15 23:30:26 PDT
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?
Comment 3 User image Olli Pettay [:smaug] 2009-09-15 14:26:53 PDT
So what if you add event listener using document.addEventListener("storage", listener, false);
Comment 4 User image Olli Pettay [:smaug] 2009-09-15 14:29:40 PDT
And indeed, HTML5 doesn't say that document should have onstorage property.
Comment 5 User image Juriy "kangax" Zaytsev 2009-09-15 20:06:50 PDT
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.

Comment 6 User image Olli Pettay [:smaug] 2009-09-16 01:55:41 PDT 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()
Comment 7 User image Juriy "kangax" Zaytsev 2009-09-16 05:44:46 PDT
Yes, I realized it already : ) `addEventListener` works as expected. Thank you.
Comment 8 User image mark 2009-10-03 12:09:50 PDT
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:

Comment 9 User image mark 2009-10-03 17:22:28 PDT
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."
Comment 10 User image Johnny Stenback (:jst, 2009-10-16 17:06:06 PDT
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 User image Olli Pettay [:smaug] 2009-10-16 17:12:01 PDT
Note, seems like the draft is going to change again.
Comment 12 User image Honza Bambas (:mayhemer) 2009-10-31 15:43:52 PDT
Created attachment 409548 [details] [diff] [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.
Comment 13 User image Johnny Stenback (:jst, 2009-11-04 13:49:58 PST
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 User image Honza Bambas (:mayhemer) 2009-11-05 03:43:57 PST
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 User image Olli Pettay [:smaug] 2009-11-05 05:21:37 PST
Has HTML5 been updated to cover ?
Do we know how stable StorageEvent API is?
Comment 16 User image Honza Bambas (:mayhemer) 2009-11-05 08:58:06 PST
(In reply to comment #15)
> Has HTML5 been updated to cover
> ?
> Do we know how stable StorageEvent API is?

Yes, it has been.
Comment 17 User image Honza Bambas (:mayhemer) 2009-11-09 05:49:50 PST
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 User image Honza Bambas (:mayhemer) 2009-11-12 09:33:55 PST
Created attachment 411990 [details] [diff] [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();'.
Comment 19 User image Johnny Stenback (:jst, 2009-11-13 15:28:46 PST
Comment on attachment 411990 [details] [diff] [review]

Patch looks good. r=jst
Comment 20 User image Johnny Stenback (:jst, 2009-11-13 16:03:08 PST
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 User image Johnny Stenback (:jst, 2009-11-13 16:16:29 PST
And the ';' after the definition of NS_PIDOMSTORAGE_IID needs to be removed.
Comment 22 User image Honza Bambas (:mayhemer) 2009-11-15 07:57:58 PST
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.
Comment 23 User image Honza Bambas (:mayhemer) 2009-11-16 05:29:09 PST
Created attachment 412571 [details] [diff] [review]
v2 update

This is an update for the event.URL attribute implementation. I take the URL from the subject's principal.
Comment 24 User image Johnny Stenback (:jst, 2009-11-16 13:32:05 PST
Comment on attachment 412571 [details] [diff] [review]
v2 update

Comment 25 User image Johnny Stenback (:jst, 2009-11-16 13:51:39 PST
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 User image Honza Bambas (:mayhemer) 2009-12-28 04:13:12 PST
Comment on attachment 409548 [details] [diff] [review]

I just wanted to land this on m-c, but it seems it never got r+.
Comment 27 User image Grzegorz Borkowski 2010-01-16 11:10:03 PST
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 User image Johnny Stenback (:jst, 2010-01-21 18:11:36 PST
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 :)

Comment 29 User image Honza Bambas (:mayhemer) 2010-01-26 05:20:42 PST
Created attachment 423514 [details] [diff] [review]

Adjusted and cleaned up patch, ready to land.
Comment 30 User image Honza Bambas (:mayhemer) 2010-01-26 05:35:30 PST
Comment on attachment 423514 [details] [diff] [review]
Comment 31 User image David Baron :dbaron: ⌚️UTC-8 2010-01-26 08:58:47 PST
Five of the tests in test_storageSessionStorageEventCheckPropagation.html have been orange in every test run since this landed.
Comment 32 User image Honza Bambas (:mayhemer) 2010-01-26 09:24:19 PST
Backed out due to a reglar test failure (thought locally passing in a debug build).
Comment 33 User image Honza Bambas (:mayhemer) 2010-01-26 09:39:34 PST
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 User image Honza Bambas (:mayhemer) 2010-01-26 13:15:27 PST
Created attachment 423609 [details] [diff] [review]
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 User image Honza Bambas (:mayhemer) 2010-01-27 11:00:48 PST
Created attachment 423824 [details] [diff] [review]
v1.2 [Checkin comment 36]

The full patch, passed tryserver.
Comment 36 User image Honza Bambas (:mayhemer) 2010-01-28 08:26:59 PST
Comment on attachment 423824 [details] [diff] [review]
v1.2 [Checkin comment 36]
Comment 37 User image Daniel Holbert [:dholbert] (vacation, returning 2/27) 2010-02-01 21:17:17 PST
Created attachment 424727 [details] [diff] [review]
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 User image Daniel Holbert [:dholbert] (vacation, returning 2/27) 2010-02-02 12:13:31 PST
Followup pushed:
Comment 39 User image Olli Pettay [:smaug] 2010-03-08 05:34:55 PST
So there is now the ObsoleteEvent. When is that dispatched and why is it there?
Comment 40 User image Honza Bambas (:mayhemer) 2010-03-08 14:04:13 PST
(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.)

Comment 41 User image Eric Shepherd [:sheppy] 2011-01-07 09:55:41 PST
Updated documentation: (new page) (marked obsolete, renamed, etc) (updated) (updated)

Change mentioned on Firefox 4 for developers.

Note You need to log in before you can comment on or make changes to this bug.