Bug 202713

Summary:

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

Product:

WebKit

Reporter:

Devin Rousso <hi>

Component:

Web Inspector

Assignee:

Devin Rousso <hi>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bburg, cdumez, commit-queue, dbates, esprehn+autocc, ews-watchlist, graouts, hi, inspector-bugzilla-changes, joepeck, kangil.han, rniwa, tsavell, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

See Also:

https://bugs.webkit.org/show_bug.cgi?id=202487

Attachments:

Description	Flags
Patch	none
Patch	none

Devin Rousso

Reported 2019-10-08 17:10:23 PDT

`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.

Attachments
Patch (12.18 KB, patch) 2019-10-08 17:47 PDT, Devin Rousso	no flags	Details Formatted Diff Diff
Patch (12.37 KB, patch) 2019-10-09 10:10 PDT, Devin Rousso	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Devin Rousso

Comment 1 2019-10-08 17:47:38 PDT

Created attachment 380485 [details] Patch

Chris Dumez

Comment 2 2019-10-08 18:45:52 PDT

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 3 2019-10-08 18:53:51 PDT

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 4 2019-10-09 09:28:07 PDT

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 5 2019-10-09 09:28:23 PDT

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

Comment 6 2019-10-09 10:10:36 PDT

Created attachment 380539 [details] Patch

Truitt Savell

Comment 7 2019-10-10 09:58:12 PDT

Can we get this reviewed and landed today?

Joseph Pecoraro

Comment 8 2019-10-10 10:10:12 PDT

Comment on attachment 380539 [details] Patch r=me

Blaze Burg

Comment 9 2019-10-10 10:12:55 PDT

Comment on attachment 380539 [details] Patch LGTM