1288898 - Document::createEvent sometimes creates events with the initialized flag set

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Because EventDispatcher::CreateEvent() calls constructors for some arguments, and doesn't reset the flag. (In particular: DeviceOrientationEvent, HashChangeEvent, and PopStateEvent.)

This is tested in wpt's dom/events/EventTarget-dispatchEvent.html. (It is currently broken; I'm fixing it in <https://github.com/w3c/web-platform-tests/pull/3326>.)

Comment 1

[Andrew Overholt [:overholt]]

Olli, wdyt?

Comment 2

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

Attachment: event_creation_init.diff

Ms2ger, I assume wpt will have tests for this stuff.

Comment 3

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

Attachment: event_creation_init.diff

Comment 4

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

MarkUninitialized() is perhaps a bit silly, but document.createEvent() is legacy method and no new event types should be created that way, so quick and dirty approach is fine here.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8775734 [details] [diff] [review]
event_creation_init.diff

>   if (aEventType.LowerCaseEqualsLiteral("deviceorientationevent")) {
>     DeviceOrientationEventInit init;
>-    return DeviceOrientationEvent::Constructor(aOwner, EmptyString(), init);
>+    RefPtr<Event> event =
>+      DeviceOrientationEvent::Constructor(aOwner, EmptyString(), init);
>+    event->MarkUninitialized();
>+    return event.forget();
>   }

Hmm, looks like that this causes same crash with:

var event = new DeviceOrientationEvent("", {});

, isn't it? And this is not good for maintenance.

Why don't you call MarkUninitialized() automatically in Construnctor() when event name is EmptyString()?
https://dxr.mozilla.org/mozilla-central/rev/db3ed1fdbbeaf5ab1e8fe454780146e7499be3db/dom/bindings/Codegen.py#16578-16592

Comment 6

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

Because empty string is valid for initializing an Event. But createEvent() shouldn't initialize events.

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

The test won't start passing due to <https://github.com/w3c/web-platform-tests/pull/3326>; feel free to land without a test for now.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Olli Pettay [:smaug] from comment #6)
> Because empty string is valid for initializing an Event. But createEvent()
> shouldn't initialize events.

Yeah, I understand that. But what's the cause of this crash?

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Oops, I misread "creates" as "crashes"... Okay, I'll give r+.

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8775734 [details] [diff] [review]
event_creation_init.diff

>   static nsIContent* GetShadowRelatedTarget(nsIContent* aCurrentTarget,
>                                             nsIContent* aRelatedTarget);
> 
>+  void MarkUninitialized()
>+  {
>+    mEvent->mMessage = eVoidEvent;
>+    mEvent->mSpecifiedEventTypeString.Truncate();
>+    mEvent->mSpecifiedEventType = nullptr;
>+  }
> protected:

nit: please insert an empty line before |protected:|.

Sorry for misunderstanding the purpose of this bug.

Comment 11

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

Attachment: +new line

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/86a982b73d1b
Document::createEvent sometimes creates events with the initialized flag set, r=masayuki

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/86a982b73d1b