Closed Bug 1016053 Opened 6 years ago Closed 6 years ago

document.createEvent("StorageEvent") is not supported

Categories

(Core :: DOM: Core & HTML, defect)

32 Branch
x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32
Tracking Status
firefox32 + fixed

People

(Reporter: markus.popp, Assigned: baku)

References

Details

(Keywords: regression)

Attachments

(2 files, 2 obsolete files)

Attached image Failed Ringmark test
In the latest Firefox Nightly (2014-05-26), one of the Web Storage Ring 0 Ringmark tests fails, see screenshot.

No error in Aurora, Beta & Release.
Regression window(m-i)
Good:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c9833f16a62
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0 ID:20140522233819
Bad:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2b9e7e71113
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0 ID:20140523003424
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=9c9833f16a62&tochange=f2b9e7e71113

Regressed by:
f2b9e7e71113	Andrea Marchesini — Bug 1014657 - Port DOMStorageEvent to WebIDL and remove nsIDOMStorageEvent, r=smaug, f=ms2ger
The relevant code is at https://github.com/facebook/rng.io/blob/master/tests/webstorage/test.js#L51, which tries to do XPConnect specific hacks that no longer apply.
Summary: Ringmark test (Ring 0, Web Storage) fails → document.createEvent("StorageEvent") is not supported
Hmm.  document.createEvent("StorageEvent") used to work.  Why does it not now?

Note also that per DOM spec at http://dom.spec.whatwg.org/#dom-document-createevent it shouldn't work.  Presumably that spec needs to be fixed.
Flags: needinfo?(bugs)
Flags: needinfo?(annevk)
But yes, the hackery in that test is a bit confusing to me...
We should fix it because a test relies on it? I guess we could just add anything we come across and vow not to add anything more... All the additional init*Event() methods kinda suck.
Flags: needinfo?(annevk)
Ringmark being crap as usual... Rick, you've got blame for this, please fix the test.
Flags: needinfo?(waldron.rick)
> We should fix it because a test relies on it?

This test _claims_ to be only testing things people actually care about in real life, last I checked.
Note that StorageEvent in HTML does not define an inti*Event() method.
This test needs a `storageevent` instance to verify some of the event's properties. The way it's generated is sketchy (and possibly not spec-compliant) and is the part of the code that is erroring out now. I would suggest modifying the test so that it generates a `storageevent` through actual localstorage changes.
Test could also use new StorageEvent().
Hmm perhaps it was too risky to remove createEvent support for StorageEvent.
Apparently blink does have it after all, it is just strict with case handling, unlink gecko or trident.


I think we should backout bug 1014657 for now, and then have a bit different patch for it.
Flags: needinfo?(bugs)
We can introduce it quickly without backout that bug. How does it sound?
Sounds fine too, but please re-test whether other engines have initStorageEvent.
Attached patch se.patch (obsolete) — Splinter Review
Attachment #8429207 - Flags: review?(bugs)
> This test needs a `storageevent` instance

Why?  The tests it performs could just as well be performed on StorageEvent.prototype per spec.  Of course WebKit doesn't follow the spec...
It seems this test is verifying that the event object passed to the event handler contains a number of properties. To the end developer, what generally matters is whether these props are available on the event object and have correct values. Whether these are set by the constructor or are already present on the prototype is usually not particularly important for common use cases.
(In reply to :Ms2ger from comment #6)
> Ringmark being crap as usual... Rick, you've got blame for this, please fix
> the test.

Wow, aren't you pleasant—a true representative of practitioners.
Flags: needinfo?(waldron.rick)
To make sure I get this right, I'll explain what the test is looking for and then this thread can advise the best way to do that test. The "Storage Events in Practice" test simply wants to confirm that StorageEvent objects contain these three properties: "key", "oldValue", "newValue". We didn't want to take the path of actually storing something, as that may have unwanted side effects*, which is why we create an object or look at the StorageEvent.prototype. When these were written, the following was true: 

- Chrome WebKit has a StorageEvent constructor
- Mobile Safari WebKit has a StorageEvent object that needs to be created
- Firefox has a StorageEvent prototype object



* Originally we had a test that confirmed that those properties behaved correctly by actually storing something, but we discarded this for the same reason.
>- Firefox has a StorageEvent prototype object

What does that mean, exactly?

Current Firefox has a StorageEvent constructor (in the sense that you can |new StorageEvent|).  There is also a StorageEvent.prototype, obviously.  Very old Firefox didn't have a constructible StorageEvent; I agree you needed createEvent there.

What I'm not sure about is why the test insists on creating StorageEvent objects via createEvent no matter what.  Seems to me like the right thing to do would be:

  try {
    event = new StorageEvent("something");
  } catch (e) {
    // Fall back on createEvent for browsers that don't follow the current spec, like
    // really old Firefox
  }

or something.  You could check typeof StorageEvent instead of try/catch, I guess; certainly in Firefox if that's "function" then you can construct with new.
And in particular, in old Firefox the test ended up doing 

  event = StorageEvent.prototype;

and leaving it like that, afaict.  So it wasn't even testing StorageEvent instances there...
Comment on attachment 8429207 [details] [diff] [review]
se.patch

>   if (aEventType.LowerCaseEqualsLiteral("mozmmsevent"))
>     return NS_NewDOMMozMmsEvent(aDOMEvent, aOwner, aPresContext, nullptr);
>+  if (aEventType.LowerCaseEqualsLiteral("storageevent")) {
>+    StorageEventInit dict;
>+    nsRefPtr<StorageEvent> event =
>+      StorageEvent::Constructor(aOwner, NS_LITERAL_STRING("storage"), dict);
>+    event.forget(aDOMEvent);
>+    return NS_OK;
>+  }

Event created using .createEvent shouldn't have .type set.
Could you perhaps add similar old style NS_New* method what those other classes have.
Attachment #8429207 - Flags: review?(bugs) → review-
Attached patch se.patch (obsolete) — Splinter Review
Attachment #8429207 - Attachment is obsolete: true
Attachment #8429421 - Flags: review?(bugs)
(In reply to Boris Zbarsky [:bz] from comment #19)
> >- Firefox has a StorageEvent prototype object
> 
> What does that mean, exactly?
> 
> Current Firefox has a StorageEvent constructor (in the sense that you can
> |new StorageEvent|).  There is also a StorageEvent.prototype, obviously. 
> Very old Firefox didn't have a constructible StorageEvent; I agree you
> needed createEvent there.
> 
> What I'm not sure about is why the test insists on creating StorageEvent
> objects via createEvent no matter what.  

No one is "insisting" on anything. The code in that test is written around the three items I included in my previous message—that was also in early 2012. 

> Seems to me like the right thing to
> do would be:
> 
>   try {
>     event = new StorageEvent("something");
>   } catch (e) {
>     // Fall back on createEvent for browsers that don't follow the current
> spec, like
>     // really old Firefox
>   }

I can assure you this was the _first_ thing we tried for _every_ constructor. Please, give me the benefit of the doubt here. 


> 
> or something.  You could check typeof StorageEvent instead of try/catch, I
> guess; certainly in Firefox if that's "function" then you can construct with
> new.

That's great news.
> I can assure you this was the _first_ thing we tried for _every_ constructor.

Can you explain how it fails in the case of this test, exactly?  Presumably the UAs where it fails have (typeof StorageEvent != "function") test true and either (typeof StorageEvent != "object") or ("key" in StorageEvent.prototype) test true.  Were there UAs like that in 2012?  Certainly Firefox wasn't such a UA back then (and incidentally we've had a constructible StorageEvent since January 2012 on trunk, June 2012 in releases...).
Also, all that is somewhat beside the point.  The real questions here are:

1)  Is the test going to continue requiring that createEvent work for this type of event in UAs where StorageEvent is a function?

2)  What should the DOM spec say here.  That is, what are the actual web compat requirements if we ignore the test?
(In reply to Boris Zbarsky [:bz] from comment #25)
> 2)  What should the DOM spec say here.  That is, what are the actual web
> compat requirements if we ignore the test?
Because the answer to this is unclear and we'll have the next uplift pretty soon, I'd 
like to take baku's patch.
Comment on attachment 8429421 [details] [diff] [review]
se.patch

>+StorageEvent::Constructor(EventTarget* aOwner,
>+                          nsIDOMEvent** aDOMEvent)
>+{
Please add NS_New*Event, and not this kind of ctor -
for the sake of consistency with other events.
Attachment #8429421 - Flags: review?(bugs) → review+
(In reply to Boris Zbarsky [:bz] from comment #25)
> Also, all that is somewhat beside the point.  The real questions here are:
> 
> 1)  Is the test going to continue requiring that createEvent work for this
> type of event in UAs where StorageEvent is a function?

Here's the real, unfiltered truth: we were instructed to take whatever steps were necessary to make Firefox and Opera look as good as possible in those tests so that some kind of partnership might blossom. It's not important _who_ gave that instruction, just accept that it happened. I'm probably out of line saying this, but 2 years later it's embarrassing that we even bothered to jump through these silly hoops; but those are hoops that web devs generally have to jump through (not that specifically, but hopefully you understand). All browsers should've been made to fail if `new StorageEvent()` didn't produce a storage event object.

To answer your question: no, it should not require createEvent to work in UAs where StorageEvent is a function.

> 
> 2)  What should the DOM spec say here.  That is, what are the actual web
> compat requirements if we ignore the test?

The test is going to be corrected. I don't think Firefox should make any changes if it's doing the correct thing.
Rick, I'm not trying to attack you here.  I'm trying to figure out whether there are compat constraints other than "old Firefox" on the test code at this point.

If there aren't and the test is going to get changed, so much the better.  ;)
> Please add NS_New*Event, and not this kind of ctor -
> for the sake of consistency with other events.

Bz, the patch is almost ready, do we want to land it or we want to keep the current behaviour?
Flags: needinfo?(bzbarsky)
Attached patch se.patchSplinter Review
Attachment #8429421 - Attachment is obsolete: true
I think smaug is right and we want to restore the createEvent bit for now.
Flags: needinfo?(bzbarsky)
Green on try: https://tbpl.mozilla.org/?tree=Try&rev=bfb9639fe1f1
m-i tree is closed: checkin-needed.
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/457400a5938c
Assignee: nobody → amarchesini
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/498162161226
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.