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.
<rdar://problem/36461309>
Created attachment 334793 [details] Patch
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; }
Created attachment 335204 [details] Patch
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
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
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
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
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
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
Created attachment 335219 [details] Patch
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.
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
Created attachment 338813 [details] Patch for landing
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
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
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
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
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
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
Created attachment 339265 [details] Patch for landing
Created attachment 339266 [details] Patch
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`?
*** Bug 178198 has been marked as a duplicate of this bug. ***
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!
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.
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.
Created attachment 339480 [details] Patch
(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.
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.
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?
Created attachment 339761 [details] Patch
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. :)
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.
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. :) 👍
Comment on attachment 339761 [details] Patch r=me
Comment on attachment 339761 [details] Patch Clearing flags on attachment: 339761 Committed r231659: <https://trac.webkit.org/changeset/231659>
All reviewed patches have been landed. Closing bug.