StorageEvent implementation does not match the spec

RESOLVED FIXED in mozilla1.9.3a3

Status

()

Core
DOM: Events
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: sheppy, Assigned: mayhemer)

Tracking

({compat, dev-doc-complete})

Trunk
mozilla1.9.3a3
compat, dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.2 -
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

8 years ago
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

8 years ago
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(){
  console.log('fired'); 
};
localStorage.setItem('foo', 'bar');

logs 'fired' in IE8 but not in FF.

Am I missing something?

Comment 3

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

Comment 4

8 years ago
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.

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

Comment 6

8 years ago
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()
Yes, I realized it already : ) `addEventListener` works as expected. Thank you.

Comment 8

8 years ago
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

8 years ago
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."
(Assignee)

Updated

8 years ago
Assignee: nobody → honzab.moz
(Assignee)

Updated

8 years ago
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

Updated

8 years ago
Priority: -- → P2
Target Milestone: --- → mozilla1.9.2
Note, seems like the draft is going to change again.
http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2009-October/023623.html
(Assignee)

Comment 12

8 years ago
Created attachment 409548 [details] [diff] [review]
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.
Attachment #409548 - Flags: review?
(Assignee)

Updated

8 years ago
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.
(Assignee)

Comment 14

8 years ago
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 http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2009-October/023623.html ?
Do we know how stable StorageEvent API is?
(Assignee)

Comment 16

8 years ago
(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.
(Assignee)

Comment 17

8 years ago
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
Status: NEW → ASSIGNED
(Assignee)

Comment 18

8 years ago
Created attachment 411990 [details] [diff] [review]
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();'.
Attachment #411990 - Flags: review?(jst)
(Assignee)

Updated

8 years ago
Attachment #409548 - Flags: review?(jst)
(Assignee)

Updated

8 years ago
Whiteboard: [needs review jst]
Comment on attachment 411990 [details] [diff] [review]
v2

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

Comment 22

8 years ago
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.
Whiteboard: [needs review jst]
(Assignee)

Comment 23

8 years ago
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.
Attachment #412571 - Flags: review?(jst)
(Assignee)

Updated

8 years ago
Whiteboard: [needs review jst]
Comment on attachment 412571 [details] [diff] [review]
v2 update

r=jst
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
(Assignee)

Comment 26

7 years ago
Comment on attachment 409548 [details] [diff] [review]
v1

I just wanted to land this on m-c, but it seems it never got r+.
Attachment #409548 - Flags: review?(jst)
(Assignee)

Updated

7 years ago
Attachment #411990 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Attachment #412571 - Attachment is obsolete: true

Comment 27

7 years ago
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]
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!
Attachment #409548 - Flags: review?(jst) → review+
(Assignee)

Updated

7 years ago
blocking2.0: --- → ?
Whiteboard: [needs review jst]
(Assignee)

Comment 29

7 years ago
Created attachment 423514 [details] [diff] [review]
v1.1

Adjusted and cleaned up patch, ready to land.
Attachment #409548 - Attachment is obsolete: true
Attachment #423514 - Flags: review+
(Assignee)

Comment 30

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

http://hg.mozilla.org/mozilla-central/rev/f41cab7bf009
Attachment #423514 - Attachment description: v1.1 → v1.1 [Checkin comment 30]
(Assignee)

Updated

7 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Reporter)

Updated

7 years ago
Keywords: dev-doc-needed
Blocks: 467530
Flags: in-testsuite+
Five of the tests in test_storageSessionStorageEventCheckPropagation.html have been orange in every test run since this landed.
(Assignee)

Comment 32

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

http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=dda9eddd101e
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

7 years ago
Attachment #423514 - Attachment description: v1.1 [Checkin comment 30] → v1.1
(Assignee)

Comment 33

7 years ago
Test failures cause is described in comment 22.  The full version of the patch suffers from the same issue.  Working on the fix.
(Assignee)

Comment 34

7 years ago
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.
Attachment #423609 - Flags: review?(jst)

Updated

7 years ago
Attachment #423609 - Flags: review?(jst) → review+
(Assignee)

Comment 35

7 years ago
Created attachment 423824 [details] [diff] [review]
v1.2 [Checkin comment 36]

The full patch, passed tryserver.
Attachment #423514 - Attachment is obsolete: true
Attachment #423824 - Flags: review+
(Assignee)

Updated

7 years ago
Blocks: 542775
(Assignee)

Comment 36

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

http://hg.mozilla.org/mozilla-central/rev/ff2d4ff05af5
Attachment #423824 - Attachment description: v1.2 → v1.2 [Checkin comment 36]
(Assignee)

Updated

7 years ago
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
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.
Attachment #424727 - Flags: review?(jst)

Updated

7 years ago
Attachment #424727 - Flags: review?(jst) → review+
Followup pushed: http://hg.mozilla.org/mozilla-central/rev/77800011e573

Updated

7 years ago
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: 467530
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?
(Assignee)

Comment 40

7 years ago
(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
(Assignee)

Updated

7 years ago
Depends on: 614116
(Reporter)

Comment 41

6 years ago
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.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.