Bug 64150 - dispatchEvent should raise an exception if the event object is already being dispatched
Summary: dispatchEvent should raise an exception if the event object is already being ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: UI Events (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dominic Cooney
URL:
Keywords:
Depends on:
Blocks: 63934
  Show dependency treegraph
 
Reported: 2011-07-07 23:53 PDT by Dominic Cooney
Modified: 2011-07-13 19:26 PDT (History)
3 users (show)

See Also:


Attachments
WIP patch (10.26 KB, patch)
2011-07-08 00:23 PDT, Dominic Cooney
no flags Details | Formatted Diff | Diff
Patch (12.61 KB, patch)
2011-07-08 01:12 PDT, Dominic Cooney
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (1.33 MB, application/zip)
2011-07-08 01:45 PDT, WebKit Review Bot
no flags Details
Patch (13.91 KB, patch)
2011-07-12 06:26 PDT, Dominic Cooney
dglazkov: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dominic Cooney 2011-07-07 23:53:14 PDT
This came up in the context of bug 63934.

DOM Events says calling dispatchEvent with an event that is being dispatched should raise DISPATCH_REQUEST_ERR.
<http://www.w3.org/TR/2011/WD-DOM-Level-3-Events-20110531/#events-EventTarget-dispatchEven

For context, the status quo in WebKit is that calling dispatchEvent and passing an event that is in the process of being dispatched resets the event’s target. Subsequent event listeners of the original event dispatch see an unrelated event.target, despite event.target being a readonly attribute that DOM Events says "must be the EventTarget object on which dispatchEvent is called." Firefox 5.0 makes dispatchEvent of an event that is being dispatched a no-op.
Comment 1 Dominic Cooney 2011-07-08 00:23:47 PDT
Created attachment 100086 [details]
WIP patch
Comment 2 Dominic Cooney 2011-07-08 01:12:38 PDT
Created attachment 100090 [details]
Patch
Comment 3 WebKit Review Bot 2011-07-08 01:45:54 PDT
Comment on attachment 100090 [details]
Patch

Attachment 100090 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9000461

New failing tests:
http/tests/misc/slow-loading-mask.html
http/tests/navigation/javascriptlink-frames.html
fast/blockflow/japanese-rl-selection.html
svg/transforms/text-with-mask-with-svg-transform.svg
fast/backgrounds/background-leakage.html
fast/box-shadow/scaled-box-shadow.html
fast/backgrounds/repeat/negative-offset-repeat.html
svg/W3C-SVG-1.1/struct-use-01-t.svg
transforms/2d/hindi-rotated.html
http/tests/websocket/tests/hixie76/websocket-event-target.html
svg/repaint/filter-repaint.svg
fast/blockflow/japanese-lr-selection.html
Comment 4 WebKit Review Bot 2011-07-08 01:45:59 PDT
Created attachment 100094 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 5 Dominic Cooney 2011-07-08 01:55:41 PDT
http/tests/websocket/tests/hixie76/websocket-event-target.html failures look relevant; I will investigate.
Comment 6 Dominic Cooney 2011-07-08 01:56:05 PDT
Comment on attachment 100090 [details]
Patch

Clearing review: ? to investigate WebSocket test failures.
Comment 7 Dominic Cooney 2011-07-12 06:26:29 PDT
Created attachment 100475 [details]
Patch
Comment 8 Dimitri Glazkov (Google) 2011-07-12 08:55:41 PDT
Comment on attachment 100475 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=100475&action=review

> Source/WebCore/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=64150

It would be really good if you included here a link to the part of the spec you're implementing.
Comment 9 Darin Adler 2011-07-12 09:53:02 PDT
Comment on attachment 100475 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=100475&action=review

> Source/WebCore/dom/Event.h:174
> +        bool isBeingDispatched() const { return eventPhase(); }

Even though the implementation is different, this seems redundant with the dispatched() function. Do we really need both?
Comment 10 Dominic Cooney 2011-07-12 16:39:32 PDT
(In reply to comment #9)
> (From update of attachment 100475 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=100475&action=review
> 
> > Source/WebCore/dom/Event.h:174
> > +        bool isBeingDispatched() const { return eventPhase(); }
> 
> Even though the implementation is different, this seems redundant with the dispatched() function. Do we really need both?

The meanings *should* be different, so I think we need both:

dispatched()—has event dispatch started for this event at any point in the past, ie can you no longer call init*Event.

isBeingDispatched()—is event dispatch processing this event right now.

So the state transitions are different:

!dispatched() → dispatched() [terminal state]
!isBeingDispatched() → isBeingDispatched() → !isBeingDispatched() → …

However since bug 63934 is about the event target being reset to 0, which dispatched() depends on, I expect dispatched() is broken. If it is OK I will work on that in the context of bug 63934.
Comment 11 Dominic Cooney 2011-07-12 16:40:55 PDT
Adding Darin in reply to comment 9:

<https://bugs.webkit.org/show_bug.cgi?id=64150#c10>
Comment 12 Dominic Cooney 2011-07-13 19:26:23 PDT
Committed r90972: <http://trac.webkit.org/changeset/90972>