Bug 178897 - DOMWindow::dispatchEvent() does not reset the event's dispatch flag
Summary: DOMWindow::dispatchEvent() does not reset the event's dispatch flag
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: UI Events (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar, WebExposed
Depends on:
Blocks:
 
Reported: 2017-10-26 16:53 PDT by Chris Dumez
Modified: 2017-11-15 12:41 PST (History)
7 users (show)

See Also:


Attachments
Patch (6.57 KB, patch)
2017-10-26 21:14 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-elcapitan (1.09 MB, application/zip)
2017-10-26 22:13 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-elcapitan (1.92 MB, application/zip)
2017-10-26 22:14 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (1.31 MB, application/zip)
2017-10-27 00:22 PDT, Build Bot
no flags Details
Patch (7.33 KB, patch)
2017-10-27 08:46 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2017-10-26 16:53:01 PDT
DOMWindow::dispatchEvent() does not reset the event's dispatch flag.
Comment 1 Chris Dumez 2017-10-26 21:14:48 PDT
Created attachment 325118 [details]
Patch
Comment 2 Build Bot 2017-10-26 22:13:17 PDT
Comment on attachment 325118 [details]
Patch

Attachment 325118 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/5008337

New failing tests:
inspector/model/remote-object-dom.html
Comment 3 Build Bot 2017-10-26 22:13:18 PDT
Created attachment 325127 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 4 Build Bot 2017-10-26 22:14:28 PDT
Comment on attachment 325118 [details]
Patch

Attachment 325118 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/5008218

New failing tests:
inspector/model/remote-object-dom.html
Comment 5 Build Bot 2017-10-26 22:14:30 PDT
Created attachment 325128 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 6 Build Bot 2017-10-27 00:22:33 PDT
Comment on attachment 325118 [details]
Patch

Attachment 325118 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/5009081

New failing tests:
inspector/model/remote-object-dom.html
Comment 7 Build Bot 2017-10-27 00:22:34 PDT
Created attachment 325129 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 8 Ryosuke Niwa 2017-10-27 01:13:12 PDT
Comment on attachment 325118 [details]
Patch

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

> Source/WebCore/page/DOMWindow.cpp:2017
> +    event.resetPropagationFlags();

Can we do this in EventDispatcher.cpp instead? That file should really have everything in https://dom.spec.whatwg.org/#concept-event-dispatch.
Comment 9 Chris Dumez 2017-10-27 08:45:37 PDT
(In reply to Ryosuke Niwa from comment #8)
> Comment on attachment 325118 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=325118&action=review
> 
> > Source/WebCore/page/DOMWindow.cpp:2017
> > +    event.resetPropagationFlags();
> 
> Can we do this in EventDispatcher.cpp instead? That file should really have
> everything in https://dom.spec.whatwg.org/#concept-event-dispatch.

I think you are referring to another code path (which likely already does the right thing). This particular DOMWindow::dispatchEvent(Event&, EventTarget*) is called from places like DOMWindow::dispatchLoadEvent(), not EventDispatcher.
Comment 10 Chris Dumez 2017-10-27 08:46:18 PDT
Created attachment 325163 [details]
Patch
Comment 11 Darin Adler 2017-10-27 11:03:41 PDT
Comment on attachment 325163 [details]
Patch

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

> Source/WebCore/page/DOMWindow.cpp:2017
> +    event.setCurrentTarget(nullptr);
> +    event.setEventPhase(Event::NONE);
> +    event.resetPropagationFlags();

It’s not a great pattern to have this list of three things that needs to be done; I wish there was some way to structure things so it was not possible to forget one of these. For example, is there any call site that needs to reset the propagation flags without also setting the event phase to none? If not, then it seems we should be calling one function that does both rather than calling two functions.

We are treating the Event object as a sort of "data holder" here; maybe EventDispatcher could provide public functions that can do the slightly higher level combinations correctly? Or maybe Event itself could have a bit more policy and bit less "setters for every field" in the public functions
Comment 12 Chris Dumez 2017-10-27 12:36:42 PDT
(In reply to Darin Adler from comment #11)
> Comment on attachment 325163 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=325163&action=review
> 
> > Source/WebCore/page/DOMWindow.cpp:2017
> > +    event.setCurrentTarget(nullptr);
> > +    event.setEventPhase(Event::NONE);
> > +    event.resetPropagationFlags();
> 
> It’s not a great pattern to have this list of three things that needs to be
> done; I wish there was some way to structure things so it was not possible
> to forget one of these. For example, is there any call site that needs to
> reset the propagation flags without also setting the event phase to none? If
> not, then it seems we should be calling one function that does both rather
> than calling two functions.
> 
> We are treating the Event object as a sort of "data holder" here; maybe
> EventDispatcher could provide public functions that can do the slightly
> higher level combinations correctly? Or maybe Event itself could have a bit
> more policy and bit less "setters for every field" in the public functions

EventDispatcher::dispatchEvent() and DOMWindow::dispatchEvent() both following the same pattern:
    event.setCurrentTarget(nullptr);
    event.resetPropagationFlags();
    event.setEventPhase(Event::NONE);

This matches pretty well the spec which says:
- Unset event’s dispatch flag, stop propagation flag, and stop immediate propagation flag.
- Set event’s eventPhase attribute to NONE.
- Set event’s currentTarget attribute to null.

The duplication between EventDispatcher::dispatchEvent() and DOMWindow::dispatchEvent() is unfortunate we should probably address this. However, I like that our code matches the specification fairly closely.
Comment 13 Chris Dumez 2017-10-27 12:50:32 PDT
FYI, calling initEvent() on an event that has already been dispatched has been supported for a while (in the regular EventDispatcher code path).

Firefox, Chrome, Safari and the specification all agree on this. Tested via:
http://jsbin.com/mekobayoxu/edit?html,output
Comment 14 WebKit Commit Bot 2017-10-27 12:56:07 PDT
Comment on attachment 325163 [details]
Patch

Clearing flags on attachment: 325163

Committed r224125: <https://trac.webkit.org/changeset/224125>
Comment 15 WebKit Commit Bot 2017-10-27 12:56:09 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Chris Dumez 2017-10-27 13:01:35 PDT
(In reply to Chris Dumez from comment #13)
> FYI, calling initEvent() on an event that has already been dispatched has
> been supported for a while (in the regular EventDispatcher code path).
> 
> Firefox, Chrome, Safari and the specification all agree on this. Tested via:
> http://jsbin.com/mekobayoxu/edit?html,output

I however started a discussion upstream out possibly updating the specification:
- https://github.com/whatwg/dom/issues/523

Feel free to pitch in.
Comment 17 Darin Adler 2017-10-28 15:16:53 PDT
I looked at this topic again and here are some additional inconsistencies I have noticed in our event handling code:

    IDBEventDispatcher::dispatch does not call resetPropagationFlags once it is done; I think it probably should.
    EventTarget::dispatchEvent does not call setCurrentTarget(null) once it is done; I think it probably should.

I understand how appealing it is to have code that exactly corresponds to what the specification calls for in a direct way, but I do think it makes it easier to get these details wrong, doing some of the resetting, but not all of it.
Comment 18 Darin Adler 2017-10-28 17:49:25 PDT
(In reply to Darin Adler from comment #17)
> I looked at this topic again and here are some additional inconsistencies I
> have noticed in our event handling code:
> 
>     IDBEventDispatcher::dispatch does not call resetPropagationFlags once it
> is done; I think it probably should.
>     EventTarget::dispatchEvent does not call setCurrentTarget(null) once it
> is done; I think it probably should.
> 
> I understand how appealing it is to have code that exactly corresponds to
> what the specification calls for in a direct way, but I do think it makes it
> easier to get these details wrong, doing some of the resetting, but not all
> of it.

I’m going to fix these.
Comment 19 Chris Dumez 2017-10-29 11:45:21 PDT
(In reply to Darin Adler from comment #18)
> (In reply to Darin Adler from comment #17)
> > I looked at this topic again and here are some additional inconsistencies I
> > have noticed in our event handling code:
> > 
> >     IDBEventDispatcher::dispatch does not call resetPropagationFlags once it
> > is done; I think it probably should.
> >     EventTarget::dispatchEvent does not call setCurrentTarget(null) once it
> > is done; I think it probably should.
> > 
> > I understand how appealing it is to have code that exactly corresponds to
> > what the specification calls for in a direct way, but I do think it makes it
> > easier to get these details wrong, doing some of the resetting, but not all
> > of it.
> 
> I’m going to fix these.

Thank you.
Comment 20 Radar WebKit Bug Importer 2017-11-15 12:41:46 PST
<rdar://problem/35567960>