Bug 174739 – Web Inspector: capture an async stack trace when web content calls addEventListener

• Component: Web Inspector
• Status: RESOLVED FIXED

[Matt Baker]

Summary:
Capture an async stack trace when web content calls addEventListener.

- Add a new WebCore::AsynchronousCallType::EventListener enum value (this enum added in XXXX)
- Instrument EventTarget
- Add instrumentation end points to PageDebuggerAgent

Note:
Requires no new UI.

[Matt Baker]

Created attachment 316160 [details]
Patch

[Matt Baker]

Patch won't apply to TOT until the blocking issue lands.

[Radar WebKit Bug Importer]

<rdar://problem/33468197>

[Radar WebKit Bug Importer]

<rdar://problem/33468189>

[Matt Baker]

Created attachment 316244 [details]
Patch

[Blaze Burg]

Comment on attachment 316244 [details]
Patch

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

r- because it needs rebasing.

> Source/WebCore/dom/EventTarget.cpp:-72
> -    return ensureEventTargetData().eventListenerMap.add(eventType, WTFMove(listener), { options.capture, options.passive, options.once });

This code seems kinda touchy for performance. Maybe you should check with Ryosuke to learn how to check whether this is a serious perf regression (adding hooks here on the hot path).

> Source/WebCore/dom/EventTarget.cpp:72
> +    bool listenerIsScriptDebuggable = listener->type() == EventListener::JSEventListenerType && !listener->wasCreatedFromMarkup();

Move this next to the use site. Also this name is wonky. What about listenerExecutesJavaScript or listenerCanBeDebugged?

The !listener->wasCreatedFromMarkup() exclusion deserves some explanation here or in the ChangeLog. Why can't we debug that?

> Source/WebCore/dom/EventTarget.cpp:76
> +    if (listenerIsScriptDebuggable) {

To minimize perf impact you should guard the extra work here (iterating the listeners) on InspectorInstrumentation::hasFrontends().

> Source/WebCore/dom/EventTarget.cpp:118
> +    auto& listenersForType = eventListeners(eventType);

To minimize perf impact you should guard the extra work here (iterating the listeners) on InspectorInstrumentation::hasFrontends().

> Source/WebCore/dom/EventTarget.cpp:120
> +        if (registeredListener->callback() == listener && registeredListener->useCapture() == options.capture) {

What is the purpose of this check for usesCapture?

> Source/WebCore/inspector/InspectorInstrumentation.cpp:354
> +void InspectorInstrumentation::didRegisterEventListenerImpl(InstrumentingAgents& instrumentingAgents, const RegisteredEventListener& listener, ScriptExecutionContext& context)

Applies throughout: please rename to add (so we have add/remove method pairs inside inspector).

> Source/WebCore/inspector/PageDebuggerAgent.cpp:176
> +    int listenerIdentifier = m_nextEventListenerIdentifier++;

Please use an unsigned type for the identifier key. Adding a typedef would be nice too, so the HashMap type parameters are a little less vague.

> Source/WebCore/inspector/PageDebuggerAgent.cpp:179
> +    didScheduleAsyncCall(scriptState, static_cast<int>(AsynchronousCallType::EventListener), listenerIdentifier, listener.isOnce());

what!? don't cast away the enum type here. Do it inside the methods if they need to use the value for hashing or whatever. Please fix the existing call sites, this is not good.

> Source/WebCore/inspector/PageDebuggerAgent.cpp:184
> +    auto eventListenerIterator = m_registeredEventListeners.find(&listener);

Nit: you can just do `auto it = ...`. There's only one iterator, naming it is not helpful.

> Source/WebCore/inspector/PageDebuggerAgent.cpp:195
> +    auto eventListenerIterator = m_registeredEventListeners.find(&listener);

Ditto.

[Matt Baker]

Comment on attachment 316244 [details]
Patch

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

`

>> Source/WebCore/dom/EventTarget.cpp:-72
>> -    return ensureEventTargetData().eventListenerMap.add(eventType, WTFMove(listener), { options.capture, options.passive, options.once });
> 
> This code seems kinda touchy for performance. Maybe you should check with Ryosuke to learn how to check whether this is a serious perf regression (adding hooks here on the hot path).

Will investigate. At the very least the call to "auto& listenersForType = ..." should be moved to InspectorInstrumentation and guarded on the presence of frontends.

>> Source/WebCore/dom/EventTarget.cpp:72
>> +    bool listenerIsScriptDebuggable = listener->type() == EventListener::JSEventListenerType && !listener->wasCreatedFromMarkup();
> 
> Move this next to the use site. Also this name is wonky. What about listenerExecutesJavaScript or listenerCanBeDebugged?
> 
> The !listener->wasCreatedFromMarkup() exclusion deserves some explanation here or in the ChangeLog. Why can't we debug that?

I agree this is a poor name. Maybe `listenerCreatedFromScript` would be more appropriate. The reason for ignoring listeners created from markup (<a onclick="didClick()">), is that no script call stack is associated with the event listener's registration.

>> Source/WebCore/dom/EventTarget.cpp:76
>> +    if (listenerIsScriptDebuggable) {
> 
> To minimize perf impact you should guard the extra work here (iterating the listeners) on InspectorInstrumentation::hasFrontends().

EventTarget::eventListeners is public, so this step can be moved into InspectorInstrumentation. EventTarget will need to be passed to the instrumentation instead of ScriptExecutionContext (which can be retrieved from the former).

>> Source/WebCore/dom/EventTarget.cpp:120
>> +        if (registeredListener->callback() == listener && registeredListener->useCapture() == options.capture) {
> 
> What is the purpose of this check for usesCapture?

The same callback can be registered a second time so long as the `capture` option differs. See EventListenerMap::add.

[Matt Baker]

Comment on attachment 316244 [details]
Patch

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

>> Source/WebCore/inspector/PageDebuggerAgent.cpp:176
>> +    int listenerIdentifier = m_nextEventListenerIdentifier++;
> 
> Please use an unsigned type for the identifier key. Adding a typedef would be nice too, so the HashMap type parameters are a little less vague.

The identifier is signed to be consistent with requestAnimationFrame and DOMTimer identifiers handed out in WebCore.

[Blaze Burg]

Comment on attachment 316244 [details]
Patch

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

>>> Source/WebCore/dom/EventTarget.cpp:72
>>> +    bool listenerIsScriptDebuggable = listener->type() == EventListener::JSEventListenerType && !listener->wasCreatedFromMarkup();
>> 
>> Move this next to the use site. Also this name is wonky. What about listenerExecutesJavaScript or listenerCanBeDebugged?
>> 
>> The !listener->wasCreatedFromMarkup() exclusion deserves some explanation here or in the ChangeLog. Why can't we debug that?
> 
> I agree this is a poor name. Maybe `listenerCreatedFromScript` would be more appropriate. The reason for ignoring listeners created from markup (<a onclick="didClick()">), is that no script call stack is associated with the event listener's registration.

Perhaps in a future patch, we could capture the call stack for the operation that creates the associated markup (i.e., the innerHTML setter, DOM mutation APIs, or loading HTML in a particular document).

>>> Source/WebCore/dom/EventTarget.cpp:120
>>> +        if (registeredListener->callback() == listener && registeredListener->useCapture() == options.capture) {
>> 
>> What is the purpose of this check for usesCapture?
> 
> The same callback can be registered a second time so long as the `capture` option differs. See EventListenerMap::add.

oh, interesting.

[Matt Baker]

Created attachment 316504 [details]
Patch

[Matt Baker]

EventTarget instrumentation changed to do no work when no Inspector frontends aren't present.

[Matt Baker]

(In reply to Matt Baker from comment #11)
> EventTarget instrumentation changed to do no work when no Inspector
> frontends aren't present.

>_<

Only does work when Inspector frontends are present.

[Blaze Burg]

Comment on attachment 316504 [details]
Patch

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

What happens if the page or frame containing the event listeners navigates? I don't see that we are clearing that anywhere.

> Source/WebCore/dom/EventTarget.cpp:71
> +    bool listenerCreatedFromScript = listener->type() == EventListener::JSEventListenerType && !listener->wasCreatedFromMarkup();

Move this local next to the use site. It isn't needed prior to the first early exit.

> Source/WebCore/inspector/PageDebuggerAgent.cpp:169
> +    auto& listener = eventListeners.last();

Please use the concrete type here. Auto is fine for the collection above.

[Matt Baker]

Comment on attachment 316504 [details]
Patch

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

>> Source/WebCore/dom/EventTarget.cpp:71
>> +    bool listenerCreatedFromScript = listener->type() == EventListener::JSEventListenerType && !listener->wasCreatedFromMarkup();
> 
> Move this local next to the use site. It isn't needed prior to the first early exit.

This needs to come before we WTFMove the listener.

[Matt Baker]

(In reply to Brian Burg from comment #13)
> Comment on attachment 316504 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=316504&action=review
> 
> What happens if the page or frame containing the event listeners navigates?
> I don't see that we are clearing that anywhere.

PageDebuggerAgent overrides didClearAsyncStackData, which is called when the global object is cleared. This is unnecessarily complex — PageDebuggerAgent can just clear its data in didClearMainFrameWindowObject:

void PageDebuggerAgent::didClearMainFrameWindowObject()
{
    didClearGlobalObject();

+    m_registeredEventListeners.clear();
+    m_nextEventListenerIdentifier = 1;
}

[Matt Baker]

(In reply to Matt Baker from comment #15)
> (In reply to Brian Burg from comment #13)
> > Comment on attachment 316504 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=316504&action=review
> > 
> > What happens if the page or frame containing the event listeners navigates?
> > I don't see that we are clearing that anywhere.
> 
> PageDebuggerAgent overrides didClearAsyncStackData, which is called when the
> global object is cleared. This is unnecessarily complex — PageDebuggerAgent
> can just clear its data in didClearMainFrameWindowObject:
> 
> void PageDebuggerAgent::didClearMainFrameWindowObject()
> {
>     didClearGlobalObject();
> 
> +    m_registeredEventListeners.clear();
> +    m_nextEventListenerIdentifier = 1;
> }

Nevermind. The hook `didClearAsyncStackTraceData()` is needed, since there are multiple things in the InspectorDebuggerAgent that can cause async data top be cleared:

- Window object cleared
- Agent disabled
- Change to async stack trace depth setting

[Matt Baker]

Created attachment 316685 [details]
Patch

[Blaze Burg]

Comment on attachment 316685 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 316685 [details]
Patch

Clearing flags on attachment: 316685

Committed r220036: <http://trac.webkit.org/changeset/220036>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.