Bug 202713 – Web Inspector: Timelines: don't call willDispatchEvent/didDispatchEvent unless there is a listener for the event

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

`EventTarget::fireEventListeners` will early return if there are no event listeners for the type of the event being dispatched.  This is not the case for `DOMWindow::dispatchEvent`, which always attempts to dispatch both the capturing and bubbling phase of the event regardless of whether there are any event listeners for the dispatched event's type.

[Devin Rousso]

Created attachment 380485 [details]
Patch

[Chris Dumez]

Comment on attachment 380485 [details]
Patch

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

> Source/WebCore/page/DOMWindow.cpp:2182
> +    if (!hasEventListeners(event.type()))

This does not look right to me, given that firing an animationend event may cause a webkitAnimationEnd event listener to get called. See legacyType() in EventTarget.cpp, called from EventTarget::fireEventListeners().
When it comes to DOM / HTML objects, I would discourage from doing such things unless the spec explicitly states it is OK.

[Chris Dumez]

Comment on attachment 380485 [details]
Patch

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

>> Source/WebCore/page/DOMWindow.cpp:2182
>> +    if (!hasEventListeners(event.type()))
> 
> This does not look right to me, given that firing an animationend event may cause a webkitAnimationEnd event listener to get called. See legacyType() in EventTarget.cpp, called from EventTarget::fireEventListeners().
> When it comes to DOM / HTML objects, I would discourage from doing such things unless the spec explicitly states it is OK.

Seems like this would also be Web observable, if the JS calls dispatchEvent() on a window (which does not have a listener for |type|, then it would previously set the event's target to window. After your change, it would fail to update the Event's target, which the page's JS could observe by checking event.target after dispatching.

[Devin Rousso]

Comment on attachment 380485 [details]
Patch

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

>>> Source/WebCore/page/DOMWindow.cpp:2182
>>> +    if (!hasEventListeners(event.type()))
>> 
>> This does not look right to me, given that firing an animationend event may cause a webkitAnimationEnd event listener to get called. See legacyType() in EventTarget.cpp, called from EventTarget::fireEventListeners().
>> When it comes to DOM / HTML objects, I would discourage from doing such things unless the spec explicitly states it is OK.
> 
> Seems like this would also be Web observable, if the JS calls dispatchEvent() on a window (which does not have a listener for |type|, then it would previously set the event's target to window. After your change, it would fail to update the Event's target, which the page's JS could observe by checking event.target after dispatching.

That's a good point.  I didn't think about events or "legacy" events.  I'll make this patch more specific to Web Inspector.  Thanks!

[Devin Rousso]

Comment on attachment 380485 [details]
Patch

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

>>>> Source/WebCore/page/DOMWindow.cpp:2182
>>>> +    if (!hasEventListeners(event.type()))
>>> 
>>> This does not look right to me, given that firing an animationend event may cause a webkitAnimationEnd event listener to get called. See legacyType() in EventTarget.cpp, called from EventTarget::fireEventListeners().
>>> When it comes to DOM / HTML objects, I would discourage from doing such things unless the spec explicitly states it is OK.
>> 
>> Seems like this would also be Web observable, if the JS calls dispatchEvent() on a window (which does not have a listener for |type|, then it would previously set the event's target to window. After your change, it would fail to update the Event's target, which the page's JS could observe by checking event.target after dispatching.
> 
> That's a good point.  I didn't think about events or "legacy" events.  I'll make this patch more specific to Web Inspector.  Thanks!

s/events/custom

[Devin Rousso]

Created attachment 380539 [details]
Patch

[Truitt Savell]

Can we get this reviewed and landed today?

[Joseph Pecoraro]

Comment on attachment 380539 [details]
Patch

r=me

[Blaze Burg]

Comment on attachment 380539 [details]
Patch

LGTM

[WebKit Commit Bot]

Comment on attachment 380539 [details]
Patch

Clearing flags on attachment: 380539

Committed r250977: <https://trac.webkit.org/changeset/250977>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/56161305>