Bug 181580

Summary:

Web Inspector: ASSERT_NOT_REACHED in PageDebuggerAgent::didAddEventListener when page adds attribute event listener

Product:

WebKit

Reporter:

Joseph Pecoraro <joepeck>

Component:

Web Inspector

Assignee:

Matt Baker <mattbaker>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

bburg, cdumez, commit-queue, dbates, ddkilzer, esprehn+autocc, ews-watchlist, hi, inspector-bugzilla-changes, joepeck, kangil.han, mattbaker, rniwa, ross.kirsling, timothy, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

185398

Attachments:

Description	Flags
Patch	none
Patch	none
Archive of layout-test-results from ews103 for mac-sierra	none
Archive of layout-test-results from ews106 for mac-sierra-wk2	none
Archive of layout-test-results from ews112 for mac-sierra	none
Patch	none
Patch for landing	none
Archive of layout-test-results from ews102 for mac-sierra	none
Archive of layout-test-results from ews104 for mac-sierra-wk2	none
Archive of layout-test-results from ews114 for mac-sierra	none
Patch for landing	none
Patch	none
Patch	none
Patch	none

Joseph Pecoraro

Reported 2018-01-11 22:44:53 PST

ASSERT_NOT_REACHED in PageDebuggerAgent::didAddEventListener when page adds attribute event listener Test: <body> <script> (function() { let b = document.body.appendChild(document.createElement("button")); b.textContent = "button"; b.onclick = () => { console.log("target 2.0"); }; b.addEventListener("click", () => { console.log("capture") }, true); b.onclick = () => { console.log("target 2.1"); }; b.addEventListener("click", () => { console.log("bubble") }, false); b.onclick = () => { console.log("target 2.2"); }; })(); </script> Steps to Reproduce: 1. Inspect test page in a debug build 2. Reload => ASSERT Notes: • My theory for that is that EventTarget::setAttributeEventListener calls eventListenerMap.replace(...) and soon after InspectorInstrumentation::didAddEventListener -> PageDebuggerAgent::didAddEventListener. PageDebuggerAgent::didAddEventListener apparently assumes the newly added event listener is the last event listener in the target.eventListeners() list. That does not match with the EventListenerMap::replace implementation, so I suspect that causes problems.

Attachments
Patch (21.99 KB, patch) 2018-02-28 18:16 PST, Matt Baker	no flags	Details Formatted Diff Diff
Patch (48.22 KB, patch) 2018-03-07 12:28 PST, Matt Baker	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews103 for mac-sierra (2.19 MB, application/zip) 2018-03-07 13:38 PST, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews106 for mac-sierra-wk2 (2.56 MB, application/zip) 2018-03-07 13:42 PST, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews112 for mac-sierra (2.92 MB, application/zip) 2018-03-07 14:07 PST, EWS Watchlist	no flags	Details
Patch (48.07 KB, patch) 2018-03-07 14:17 PST, Matt Baker	no flags	Details Formatted Diff Diff
Patch for landing (48.03 KB, patch) 2018-04-25 15:59 PDT, Matt Baker	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews102 for mac-sierra (2.41 MB, application/zip) 2018-04-25 17:02 PDT, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews104 for mac-sierra-wk2 (3.64 MB, application/zip) 2018-04-25 18:22 PDT, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews114 for mac-sierra (3.15 MB, application/zip) 2018-04-25 19:27 PDT, EWS Watchlist	no flags	Details
Patch for landing (48.13 KB, patch) 2018-05-01 20:46 PDT, Matt Baker	no flags	Details Formatted Diff Diff
Patch (48.09 KB, patch) 2018-05-01 20:55 PDT, Matt Baker	no flags	Details Formatted Diff Diff
Patch (46.89 KB, patch) 2018-05-03 15:53 PDT, Matt Baker	no flags	Details Formatted Diff Diff
Patch (14.54 KB, patch) 2018-05-07 15:47 PDT, Matt Baker	no flags	Details Formatted Diff Diff
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2018-01-11 22:46:11 PST

<rdar://problem/36461309>

Matt Baker

Comment 2 2018-02-28 18:16:25 PST

Created attachment 334793 [details] Patch

Blaze Burg

Comment 3 2018-03-01 08:39:23 PST

Comment on attachment 334793 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=334793&action=review I'd like to see a new version of the patch. > Source/WebCore/dom/EventTarget.cpp:84 > + InspectorInstrumentation::didAddEventListener(*this, eventType, *listenerPointer, options.capture); Shouldn't we be comparing all of the options? Add operator== to the struct if you need to. > Source/WebCore/inspector/agents/page/PageDebuggerAgent.cpp:181 > + auto* registeredListener = eventListeners[listenerIndex].get(); This is more idiomatic to write as: auto position = eventListeners.findMatching[&](...); if (position == notFound) return; auto registeredListener = eventListeners.at(position); > LayoutTests/ChangeLog:10 > + listener tests into their own test suite, and cleanup the test utilities Nit: clean up > LayoutTests/ChangeLog:11 > + for logging asynchronous stack traces. In the future, the catch-all test What does 'catch-all test file' refer to? > LayoutTests/inspector/debugger/async-stack-trace-event-listener.html:18 > + document.body.onclick = previousListener; I don't think we should rely on hoisting like this. Just move previousListener assignment up higher, or use `let previousListener` to stub it out. > LayoutTests/inspector/debugger/async-stack-trace-event-listener.html:56 > + InspectorTest.AsyncStackTrace.addTestCase({ Because you haven't said anything, I have no reason to think that this helper will be used outside of this test suite. And so, it doesn't need to be in its own file if that is the case. > LayoutTests/inspector/debugger/async-stack-trace-event-listener.html:59 > + expression: "testAttributeEventListener()" I can't see any reason for these test cases to be out-of-line. If they are out-of-line, then I think the convention in most of our tests is to name them triggerXXX(). > LayoutTests/inspector/debugger/resources/async-stack-trace.js:1 > +TestPage.registerInitializer(() => { Is this new code, or moved from somewhere else? I don't see a corresponding deletion. If it is new, you should say something more concrete than "clean up utilities"; this adds a new test case factory method. > LayoutTests/inspector/debugger/resources/async-stack-trace.js:4 > + function getActiveStackTrace() { Why is this at global scope in the inspector page? > LayoutTests/inspector/debugger/resources/async-stack-trace.js:19 > + function logActiveStackTrace() { Ditto. > LayoutTests/inspector/debugger/resources/async-stack-trace.js:71 > + initializeTestSuite(suite) { What? This is not okay. We could have more than one suite in the same file, and this would break. It's also awkward. We don't need to save the suite in an ivar, it should just be a parameter to addTestCase. > LayoutTests/inspector/debugger/resources/async-stack-trace.js:74 > + addTestCase({name, description, expression}) { I would rename this to addTestCaseForAsyncStackTrace or something like that. Otherwise, they will look the same in a code search. Also, it should be a static method per the above comment. > LayoutTests/inspector/debugger/resources/async-stack-trace.js:79 > + test(resolve, reject) { I'd prefer an async function in new code if we don't need to use non-once event listeners. i.e., async test() { let done = WI.debuggerManager.awaitEvent(WI.DebuggerManager.Event.Paused) .then(() => { // do logging }) .then(() => WI.debuggerManager.awaitEvent(WI.DebuggerManager.Event.Resumed)); InspectorTest.evaluateInPage(expression); await done; }

Matt Baker

Comment 4 2018-03-07 12:28:44 PST

Created attachment 335204 [details] Patch

EWS Watchlist

Comment 5 2018-03-07 13:38:35 PST

Comment on attachment 335204 [details] Patch Attachment 335204 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/6843354 New failing tests: inspector/debugger/async-stack-trace-basic.html

EWS Watchlist

Comment 6 2018-03-07 13:38:37 PST

Created attachment 335212 [details] Archive of layout-test-results from ews103 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-sierra Platform: Mac OS X 10.12.6

EWS Watchlist

Comment 7 2018-03-07 13:42:57 PST

Comment on attachment 335204 [details] Patch Attachment 335204 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/6843369 New failing tests: inspector/debugger/async-stack-trace-basic.html

EWS Watchlist

Comment 8 2018-03-07 13:42:59 PST

Created attachment 335213 [details] Archive of layout-test-results from ews106 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6

EWS Watchlist

Comment 9 2018-03-07 14:07:35 PST

Comment on attachment 335204 [details] Patch Attachment 335204 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/6843481 New failing tests: inspector/debugger/async-stack-trace-basic.html

EWS Watchlist

Comment 10 2018-03-07 14:07:37 PST

Created attachment 335217 [details] Archive of layout-test-results from ews112 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-sierra Platform: Mac OS X 10.12.6

Matt Baker

Comment 11 2018-03-07 14:17:49 PST

Created attachment 335219 [details] Patch

Matt Baker

Comment 12 2018-03-07 14:29:42 PST

Comment on attachment 334793 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=334793&action=review >> Source/WebCore/dom/EventTarget.cpp:84 >> + InspectorInstrumentation::didAddEventListener(*this, eventType, *listenerPointer, options.capture); > > Shouldn't we be comparing all of the options? Add operator== to the struct if you need to. Two EventListeners with the same callback can both be registered, as long as the capture value differs. This check is made internally by EventListenerMap::add when it calls findListener (EventListenerMap.cpp:135). PageDebuggerAgent duplicates this logic, since EventListenerMap provides no way to lookup a RegisteredEventListener. >> LayoutTests/ChangeLog:11 >> + for logging asynchronous stack traces. In the future, the catch-all test > > What does 'catch-all test file' refer to? I was referring to async-stack-trace.html, where all the tests were lumped together. My change log comment was unclear. >> LayoutTests/inspector/debugger/async-stack-trace-event-listener.html:59 >> + expression: "testAttributeEventListener()" > > I can't see any reason for these test cases to be out-of-line. If they are out-of-line, then I think the convention in most of our tests is to name them triggerXXX(). I adopted the triggerXXX() convention. Looking over our recent tests, the majority that follow this pattern are written out-of-line (except for the simplest expressions, like `setTimeout(foo)`. I think it makes the test suite as a whole more legible >> LayoutTests/inspector/debugger/resources/async-stack-trace.js:74 >> + addTestCase({name, description, expression}) { > > I would rename this to addTestCaseForAsyncStackTrace or something like that. Otherwise, they will look the same in a code search. Also, it should be a static method per the above comment. Good point. >> LayoutTests/inspector/debugger/resources/async-stack-trace.js:79 >> + test(resolve, reject) { > > I'd prefer an async function in new code if we don't need to use non-once event listeners. i.e., > > async test() { > let done = WI.debuggerManager.awaitEvent(WI.DebuggerManager.Event.Paused) > .then(() => { // do logging }) > .then(() => WI.debuggerManager.awaitEvent(WI.DebuggerManager.Event.Resumed)); > > InspectorTest.evaluateInPage(expression); > await done; > } Cool. I've made this change.

WebKit Commit Bot

Comment 13 2018-04-25 15:10:11 PDT

Comment on attachment 335219 [details] Patch Rejecting attachment 335219 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 335219, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: 974b6b9c81f3356caace01ab9a0625eda9 e60d689504ac735c2a6c8593096a44e4a792aa1c M Source Current branch master is up to date. ERROR: Not all changes have been committed into SVN, however the committed ones (if any) seem to be successfully integrated into the working tree. Please see the above messages for details. Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Updating OpenSource Current branch master is up to date. Total errors found: 0 in 0 files Full output: http://webkit-queues.webkit.org/results/7459603

Matt Baker

Comment 14 2018-04-25 15:59:46 PDT

Created attachment 338813 [details] Patch for landing

EWS Watchlist

Comment 15 2018-04-25 17:02:13 PDT

Comment on attachment 338813 [details] Patch for landing Attachment 338813 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/7461158 New failing tests: inspector/debugger/async-stack-trace-event-listener.html

EWS Watchlist

Comment 16 2018-04-25 17:02:14 PDT

Created attachment 338829 [details] Archive of layout-test-results from ews102 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-sierra Platform: Mac OS X 10.12.6

EWS Watchlist

Comment 17 2018-04-25 18:22:20 PDT

Comment on attachment 338813 [details] Patch for landing Attachment 338813 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/7462208 New failing tests: inspector/debugger/async-stack-trace-event-listener.html

EWS Watchlist

Comment 18 2018-04-25 18:22:22 PDT

Created attachment 338843 [details] Archive of layout-test-results from ews104 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6

EWS Watchlist

Comment 19 2018-04-25 19:27:40 PDT

Comment on attachment 338813 [details] Patch for landing Attachment 338813 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/7462879 New failing tests: inspector/debugger/async-stack-trace-event-listener.html

EWS Watchlist

Comment 20 2018-04-25 19:27:42 PDT

Created attachment 338848 [details] Archive of layout-test-results from ews114 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-sierra Platform: Mac OS X 10.12.6

Matt Baker

Comment 21 2018-05-01 20:46:49 PDT

Created attachment 339265 [details] Patch for landing

Matt Baker

Comment 22 2018-05-01 20:55:22 PDT

Created attachment 339266 [details] Patch

Devin Rousso

Comment 23 2018-05-02 17:00:40 PDT

Comment on attachment 339266 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=339266&action=review > Source/WebCore/dom/EventTarget.cpp:78 > + auto* listenerPointer = listener.ptr(); Is there a reason you need to save this to a variable? > Source/WebCore/inspector/agents/page/PageDebuggerAgent.cpp:173 > + auto position = eventListeners.findMatching([&](auto& registeredListener) { Style: I think we prefer to add a " " between the capture and parameters. [&] (auto& registeredListener) { > LayoutTests/ChangeLog:13 > + Additionally, tests for async stack traces have been split up into multiple > + files and rewritten to improve clarity and maintainability. I like that you've added these new tests, but are they strictly necessary for this patch? > LayoutTests/inspector/debugger/async-stack-trace-event-listener-expected.txt:3 > +Uncaught exception in Inspector page: undefined is not an object (evaluating 'InspectorTest.AsyncStackTrace.addSimpleTestCase') Did you mean `addAsyncStackTraceTestCase`?

Ross Kirsling

Comment 24 2018-05-02 18:49:58 PDT

*** Bug 178198 has been marked as a duplicate of this bug. ***

Matt Baker

Comment 25 2018-05-03 14:36:23 PDT

Comment on attachment 339266 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=339266&action=review >> Source/WebCore/dom/EventTarget.cpp:78 >> + auto* listenerPointer = listener.ptr(); > > Is there a reason you need to save this to a variable? The value of the underlying pointer needs to be cached because WTFMove(listener) transfers ownership of the Ref<EventListener> to the event listener map. The address stored in the pointer is still valid, but the original r-value no longer owns it. >> Source/WebCore/inspector/agents/page/PageDebuggerAgent.cpp:173 >> + auto position = eventListeners.findMatching([&](auto& registeredListener) { > > Style: I think we prefer to add a " " between the capture and parameters. > > [&] (auto& registeredListener) { It looks like we use a mix of both, although including a space may be more common. I'll see if our style guidelines have anything to say about this. >> LayoutTests/ChangeLog:13 >> + files and rewritten to improve clarity and maintainability. > > I like that you've added these new tests, but are they strictly necessary for this patch? Not necessary, but as I was adding new tests anyway it seemed like a good time to clean house. >> LayoutTests/inspector/debugger/async-stack-trace-event-listener-expected.txt:3 >> +Uncaught exception in Inspector page: undefined is not an object (evaluating 'InspectorTest.AsyncStackTrace.addSimpleTestCase') > > Did you mean `addAsyncStackTraceTestCase`? Oops!

Blaze Burg

Comment 26 2018-05-03 15:46:10 PDT

Comment on attachment 339266 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=339266&action=review >>> Source/WebCore/dom/EventTarget.cpp:78 >>> + auto* listenerPointer = listener.ptr(); >> >> Is there a reason you need to save this to a variable? > > The value of the underlying pointer needs to be cached because WTFMove(listener) transfers ownership of the Ref<EventListener> to the event listener map. The address stored in the pointer is still valid, but the original r-value no longer owns it. In general this is dicey to do and you should take a local reference. In this case, I don't think anything is going to happen in between assignment and use. That said, to match other code I would take a copyRef() on the first usage and WTFMove it on the last usage.

Blaze Burg

Comment 27 2018-05-03 15:51:32 PDT

Comment on attachment 339266 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=339266&action=review Please remove unrelated test changes from this patch. It's distracting and should be separate. I don't want a mistake in a test to get this other consequential fix rolled out. > Source/WebCore/dom/EventTarget.cpp:84 > + InspectorInstrumentation::didAddEventListener(*this, eventType, *listenerPointer, options.capture); Please just pass the entirety of options since we may decide to care about other options later when hash consing to look up an existing listener. >>> Source/WebCore/inspector/agents/page/PageDebuggerAgent.cpp:173 >>> + auto position = eventListeners.findMatching([&](auto& registeredListener) { >> >> Style: I think we prefer to add a " " between the capture and parameters. >> >> [&] (auto& registeredListener) { > > It looks like we use a mix of both, although including a space may be more common. I'll see if our style guidelines have anything to say about this. We don't seem to have a style guide rule for this, and style checker doesn't parse capture lists well if at all. JSC tends to add a space and WebKit doesn't.

Matt Baker

Comment 28 2018-05-03 15:53:41 PDT

Created attachment 339480 [details] Patch

Matt Baker

Comment 29 2018-05-03 15:57:16 PDT

(In reply to Brian Burg from comment #27) > Comment on attachment 339266 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=339266&action=review > > Please remove unrelated test changes from this patch. It's distracting and > should be separate. I don't want a mistake in a test to get this other > consequential fix rolled out. > > > Source/WebCore/dom/EventTarget.cpp:84 > > + InspectorInstrumentation::didAddEventListener(*this, eventType, *listenerPointer, options.capture); > > Please just pass the entirety of options since we may decide to care about > other options later when hash consing to look up an existing listener. TIL that hash consing is a thing. Will change.

Matt Baker

Comment 30 2018-05-04 17:16:21 PDT

Comment on attachment 339266 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=339266&action=review >>> Source/WebCore/dom/EventTarget.cpp:84 >>> + InspectorInstrumentation::didAddEventListener(*this, eventType, *listenerPointer, options.capture); >> >> Please just pass the entirety of options since we may decide to care about other options later when hash consing to look up an existing listener. > > TIL that hash consing is a thing. Will change. There are two issues with this: 1) Options structs are nested inside EventTarget. Nested classes/structs cannot be forward declared, so this would require including EventTarget.h in PageDebuggerAgent.h. 2) EventTarget::removeEventListener is passed ListenerOptions, not AddEventListenerOptions, so we'd just be passing a bool anyway. Our only other options would be to get a copy of RegisteredEventListener::Options, which contains everything in AddEventListenerOptions, but that would require a separate lookup to retrieve the RegisteredEventListener from the listeners map. For these reasons I think it's more trouble than it's worth.

Blaze Burg

Comment 31 2018-05-04 21:06:38 PDT

Comment on attachment 339266 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=339266&action=review >>>> Source/WebCore/dom/EventTarget.cpp:84 >>>> + InspectorInstrumentation::didAddEventListener(*this, eventType, *listenerPointer, options.capture); >>> >>> Please just pass the entirety of options since we may decide to care about other options later when hash consing to look up an existing listener. >> >> TIL that hash consing is a thing. Will change. > > There are two issues with this: > > 1) Options structs are nested inside EventTarget. Nested classes/structs cannot be forward declared, so this would require including EventTarget.h in PageDebuggerAgent.h. > 2) EventTarget::removeEventListener is passed ListenerOptions, not AddEventListenerOptions, so we'd just be passing a bool anyway. Our only other options would be to get a copy of RegisteredEventListener::Options, which contains everything in AddEventListenerOptions, but that would require a separate lookup to retrieve the RegisteredEventListener from the listeners map. > > For these reasons I think it's more trouble than it's worth. The first issue is not a problem, EventTarget is included in many places. Whats the difference between ListenerOptions and AddEventListenerOptions?

Matt Baker

Comment 32 2018-05-07 15:47:06 PDT

Created attachment 339761 [details] Patch

Ross Kirsling

Comment 33 2018-05-07 16:04:14 PDT

Comment on attachment 339761 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=339761&action=review > Source/WebCore/dom/EventTarget.cpp:142 > + auto listenerPointer = listener.copyRef(); > eventTargetData()->eventListenerMap.replace(eventType, *existingListener, listener.releaseNonNull(), { }); > > - InspectorInstrumentation::didAddEventListener(*this, eventType); > + InspectorInstrumentation::didAddEventListener(*this, eventType, *listenerPointer, false); Yay, thanks for addressing my concern. :)

Matt Baker

Comment 34 2018-05-07 16:04:54 PDT

Comment on attachment 339266 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=339266&action=review >>>>> Source/WebCore/dom/EventTarget.cpp:84 >>>>> + InspectorInstrumentation::didAddEventListener(*this, eventType, *listenerPointer, options.capture); >>>> >>>> Please just pass the entirety of options since we may decide to care about other options later when hash consing to look up an existing listener. >>> >>> TIL that hash consing is a thing. Will change. >> >> There are two issues with this: >> >> 1) Options structs are nested inside EventTarget. Nested classes/structs cannot be forward declared, so this would require including EventTarget.h in PageDebuggerAgent.h. >> 2) EventTarget::removeEventListener is passed ListenerOptions, not AddEventListenerOptions, so we'd just be passing a bool anyway. Our only other options would be to get a copy of RegisteredEventListener::Options, which contains everything in AddEventListenerOptions, but that would require a separate lookup to retrieve the RegisteredEventListener from the listeners map. >> >> For these reasons I think it's more trouble than it's worth. > > The first issue is not a problem, EventTarget is included in many places. > > Whats the difference between ListenerOptions and AddEventListenerOptions? AddEventListenerOptions derives from ListenerOptions, and adds additional options that are set when adding an event listener. ListenerOptions only holds the capture flag. ListenerOptions is passed to EventTarget::removeEventListener, since only the capture flag is used to disambiguate to otherwise identical event listeners (when looking up the listener to remove). I suggest leaving the instrumentation API as-is.

Matt Baker

Comment 35 2018-05-07 16:05:56 PDT

Comment on attachment 339761 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=339761&action=review >> Source/WebCore/dom/EventTarget.cpp:142 >> + InspectorInstrumentation::didAddEventListener(*this, eventType, *listenerPointer, false); > > Yay, thanks for addressing my concern. :) 👍

Blaze Burg

Comment 36 2018-05-10 13:32:13 PDT

Comment on attachment 339761 [details] Patch r=me

WebKit Commit Bot

Comment 37 2018-05-10 13:59:12 PDT

Comment on attachment 339761 [details] Patch Clearing flags on attachment: 339761 Committed r231659: <https://trac.webkit.org/changeset/231659>

WebKit Commit Bot

Comment 38 2018-05-10 13:59:15 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.