Bug 183138 – Web Inspector: allow breakpoints to be set for specific event listeners

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

In addition to breaking on all "click" listeners, we should provide the ability to break on a specific event listener.

[Devin Rousso]

Created attachment 334671 [details]
Patch

[Devin Rousso]

Created attachment 334672 [details]
[Image] After Patch is applied

[Joseph Pecoraro]

Comment on attachment 334671 [details]
Patch

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

The patch itself seems to have been just a WIP, but I threw some comments on anyways, since this is a natural progression after Event Breakpoints.

> Source/WebCore/ChangeLog:44
> [phone]  Devin Rousso  <[e-mail]>

Nit: Double changelog.

> Source/WebInspectorUI/ChangeLog:31
> [phone]  Devin Rousso  <[e-mail]>

Nit: Double changelog.

> Source/JavaScriptCore/inspector/protocol/DOM.json:278
> +            "name": "setBreakpointForEventListener",

Note: We will want to prepare an internal change for these protocol modifications. So we'll want to do a coordinated landing.

> Source/WebCore/inspector/agents/InspectorDOMAgent.h:287
> +        RefPtr<EventListener> eventListener;

Does this now need to be cleared across page loads? Maybe it already does, I just want to make sure.

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:147
> +localizedStrings["Break on listeners for event with name:"] = "Break on listeners for event with name:";

Most of the frontend changes are duplicate of the other patch (I guess the double ChangeLogs should have made that obvious). So I'll hav limited frontend comments.

[Devin Rousso]

Created attachment 347283 [details]
Patch

[Devin Rousso]

Created attachment 347388 [details]
Patch

Rebase

[EWS Watchlist]

Comment on attachment 347388 [details]
Patch

Attachment 347388 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/8895135

New failing tests:
inspector/dom/event-listener-add-remove.html

[EWS Watchlist]

Created attachment 347404 [details]
Archive of layout-test-results from ews113 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Devin Rousso]

Created attachment 347407 [details]
Patch

Retry EWS

[EWS Watchlist]

Comment on attachment 347407 [details]
Patch

Attachment 347407 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/8896827

New failing tests:
inspector/dom/event-listener-add-remove.html

[EWS Watchlist]

Created attachment 347420 [details]
Archive of layout-test-results from ews117 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Devin Rousso]

Created attachment 347428 [details]
Patch

Try again ... not sure what's wrong

[Devin Rousso]

Created attachment 347432 [details]
Patch

Extra logging for EWS

[EWS Watchlist]

Comment on attachment 347432 [details]
Patch

Attachment 347432 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/8899353

New failing tests:
inspector/dom/event-listener-add-remove.html

[EWS Watchlist]

Created attachment 347434 [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 on attachment 347432 [details]
Patch

Attachment 347432 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/8899465

New failing tests:
inspector/dom/event-listener-add-remove.html

[EWS Watchlist]

Created attachment 347435 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 347432 [details]
Patch

Attachment 347432 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/8899508

New failing tests:
inspector/dom/event-listener-add-remove.html

[EWS Watchlist]

Created attachment 347437 [details]
Archive of layout-test-results from ews115 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Devin Rousso]

Created attachment 347446 [details]
Patch

My guess is that by calling `clear()` we are deleting `m_table`, which prevents our iteration

[EWS Watchlist]

Comment on attachment 347446 [details]
Patch

Attachment 347446 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/8903001

New failing tests:
inspector/dom/event-listener-add-remove.html

[EWS Watchlist]

Created attachment 347449 [details]
Archive of layout-test-results from ews113 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Devin Rousso]

Created attachment 347521 [details]
Patch

Mutation during Iteration 😅

[Joseph Pecoraro]

Comment on attachment 347521 [details]
Patch

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

r=me

> Source/JavaScriptCore/inspector/protocol/DOM.json:94
> +                { "name": "hasBreakpoint", "type": "boolean", "optional": true }

Offline: These will require a slight internal change so hold off on landing until we talk.

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:847
> +        for (auto& inspectorEventListener : m_eventListenerEntries.values()) {
> +            if (inspectorEventListener.matches(*info.node, info.eventType, listener.callback(), listener.useCapture())) {

Do you want to break inside this if to avoid looping over times after you've found a match?

> Source/WebCore/inspector/agents/InspectorDOMAgent.h:305
> +            return eventTarget.get() == &target && eventListener.get() == &listener && eventType == type && useCapture == capture;

Style: Lets break each condition into its own line to make this a little more readable.

> Source/WebCore/inspector/agents/InspectorDOMDebuggerAgent.cpp:369
> +    bool shouldPause = m_debuggerAgent->pauseOnNextStatementEnabled() || m_eventListenerBreakpoints.contains(listenerEventCategoryType + event.type());

Should we rename "listenerEventCategoryType" from "listener:" to "eventNameCategoryType" with a value of "event-name:"?

I found it confusing to distinction between actual event listener breakpoints and event name breakpoints, especially given the poorly named `InspectorDOMDebuggerAgent::setEventListenerBreakpoint` which is event name breakpoints and we are just keeping the legacy name for backwards compatibility.

> Source/WebCore/inspector/agents/InspectorDOMDebuggerAgent.cpp:383
> +    if (m_domAgent) {
> +        int eventListenerId = m_domAgent->idForEventListener(*event.currentTarget(), event.type(), registeredEventListener.callback(), registeredEventListener.useCapture());
> +        if (eventListenerId)
> +            eventData->setInteger("eventListenerId"_s, eventListenerId);
> +    }

Its a shame that we are doing potentially 2 O(n) iterations to lookup hasBreakpoints and id for the EventListener, but I think we can assume that the number of event listener breakpoints is pretty small.

> Source/WebInspectorUI/UserInterface/Controllers/DOMTreeManager.js:53
> +    get eventBreakpoints()

Maybe we name these `eventListenerBreakpoints` so that it doesn't get confused with `eventBreakpoints` on DOMDebuggerManager? I realize that the type are WI.EventBreakpoint.

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:1078
> +                let eventListener = eventBreakpoint.eventListener;
> +                if (eventListener && eventListener.eventListenerId === pauseData.eventListenerId) {

Would it be safe to drop this 2nd check, or just make it an assert?

    if (eventListener) {
        console.assert(eventListener.eventListenerId === pauseData.eventListenerId);
        ...
    }

> Source/WebInspectorUI/UserInterface/Views/EventListenerSectionGroup.js:59
> +            if (DOMAgent.setBreakpointForEventListener && DOMAgent.removeBreakpointForEventListener)

Good feature detection 👍!

> LayoutTests/inspector/dom/breakpoint-for-event-listener.html:47
> +                if (paused)
> +                    InspectorTest.fail("Should not pause twice.");

I don't think this can be reached given `singleFireEventListener` above. So you could probably drop this if you wanted.

> LayoutTests/inspector/dom/breakpoint-for-event-listener.html:53
> +                InspectorTest.assert(targetData.pauseReason === WI.DebuggerManager.PauseReason.EventListener, `Pause reason should be EventListener.`);
> +                InspectorTest.assert(targetData.pauseData.eventName === "click", `Pause data eventName should be "click".`);

You could make these InspectorTest.expectThat(..., `...`) so these show up in the output as PASS / FAIL lines.

> LayoutTests/inspector/dom/breakpoint-for-event-listener.html:112
> +                if (paused)
> +                    InspectorTest.fail("Should not pause twice.");

Ditto.

[Devin Rousso]

Comment on attachment 347521 [details]
Patch

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

>> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:847
>> +            if (inspectorEventListener.matches(*info.node, info.eventType, listener.callback(), listener.useCapture())) {
> 
> Do you want to break inside this if to avoid looping over times after you've found a match?

I don't think it's possible to have more than one, and even if it was, there's nothing we do about that, so yeah.

>> Source/WebCore/inspector/agents/InspectorDOMDebuggerAgent.cpp:369
>> +    bool shouldPause = m_debuggerAgent->pauseOnNextStatementEnabled() || m_eventListenerBreakpoints.contains(listenerEventCategoryType + event.type());
> 
> Should we rename "listenerEventCategoryType" from "listener:" to "eventNameCategoryType" with a value of "event-name:"?
> 
> I found it confusing to distinction between actual event listener breakpoints and event name breakpoints, especially given the poorly named `InspectorDOMDebuggerAgent::setEventListenerBreakpoint` which is event name breakpoints and we are just keeping the legacy name for backwards compatibility.

I think we should also change `instrumentationEventCategoryType` to just `instrumentationCategoryType`, since instrumentation "event"s aren't actually events.

>> Source/WebCore/inspector/agents/InspectorDOMDebuggerAgent.cpp:383
>> +    }
> 
> Its a shame that we are doing potentially 2 O(n) iterations to lookup hasBreakpoints and id for the EventListener, but I think we can assume that the number of event listener breakpoints is pretty small.

Not only that, but this is only based on the number of indexed/tracked event listeners, so it is even further limited by the number of event listeners sent to the frontend.  We only generate event listener IDs when `DOM.getEventListenersForNode` is called.

>> Source/WebInspectorUI/UserInterface/Controllers/DOMTreeManager.js:53
>> +    get eventBreakpoints()
> 
> Maybe we name these `eventListenerBreakpoints` so that it doesn't get confused with `eventBreakpoints` on DOMDebuggerManager? I realize that the type are WI.EventBreakpoint.

Agreed.

[Devin Rousso]

Created attachment 347554 [details]
Patch

[Devin Rousso]

Created attachment 347573 [details]
Patch

Somehow missed my previous mutation during iteration change

[WebKit Commit Bot]

Comment on attachment 347573 [details]
Patch

Clearing flags on attachment: 347573

Committed r235103: <https://trac.webkit.org/changeset/235103>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/43540620>

[Devin Rousso]

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