Bug 188778 – Web Inspector: support breakpoints for timers and animation-frame events

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

We should also provide support for breaking whenever a `setTimeout`, `setInterval`, or `requestAnimationFrame` is fired.

[Devin Rousso]

Created attachment 347612 [details]
Patch

[EWS Watchlist]

Comment on attachment 347612 [details]
Patch

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

New failing tests:
inspector/dom-debugger/event-breakpoint-with-navigation.html

[EWS Watchlist]

Created attachment 347620 [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

[Devin Rousso]

Created attachment 347622 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/43572345>

[Devin Rousso]

Created attachment 347702 [details]
Patch

Remove old protocol methods since they don't really do what we want

[Matt Baker]

Comment on attachment 347702 [details]
Patch

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

Looks good! r-, only because this lacks compatibility checks for older backends. We shouldn't present these new breakpoint types in that case.

> Source/WebCore/inspector/agents/InspectorDOMDebuggerAgent.cpp:401
> +    String eventName = oneShot ? "setTimeout" : "setInterval";

I think you want to use the new user-defined literal for ASCIILiteral here:

String eventName = oneShot ? "setTimeout"_s : "setInterval"_s;

> Source/WebCore/inspector/agents/InspectorDOMDebuggerAgent.cpp:413
> +    String eventName = "requestAnimationFrame";

Ditto.

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:960
> +                this._pauseReasonTreeOutline = this.createContentTreeOutline(true);

I'd use a named const above this line, like we do elsewhere in this function.

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:1135
> +                this._pauseReasonTreeOutline = this.createContentTreeOutline(true);

Ditto.

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:1316
> +        if (popover instanceof WI.XHRBreakpointPopover && popover.result === WI.InputPopover.Result.Committed) {

It looks like we aren't using InputPopover anywhere right now. Maybe the check below for `url` is enough, and we can remove the check for Result.Committed.

> Source/WebInspectorUI/UserInterface/Views/EventBreakpointPopover.js:80
> +        this._domEventNameInputElement.placeholder = "click";

When adding a symbolic breakpoint in Xcode, the placeholder for the Symbol and Library fields are prefixed with "Example: ". Maybe we should do:

this._domEventNameInputElement.placeholder = "Example: \"click\"";

> LayoutTests/inspector/dom-debugger/event-animation-frame-breakpoints-expected.txt:7
> +Firing "requestAnimationFrame" on window...

I find all the ellipses at the end of each log message distracting. How about just a period?

> LayoutTests/inspector/dom-debugger/event-animation-frame-breakpoints.html:14
> +    requestAnimationFrame(handleWindow_requestAnimationFrame, 100);

requestAnimationFrame only takes a callback. The interval is determined by the engine.

> LayoutTests/inspector/dom-debugger/event-animation-frame-breakpoints.html:55
> +            description: "Check that debugger does the not pause for disabled breakpoints.",

"Check that debugger does not pause for disabled breakpoints."

> LayoutTests/inspector/dom-debugger/event-listener-breakpoints.html:69
> +            description: "Check that debugger does the not pause for disabled breakpoints.",

"Check that debugger does not pause for disabled breakpoints."

[Devin Rousso]

Comment on attachment 347702 [details]
Patch

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

(In reply to Matt Baker from comment #7)
> Looks good! r-, only because this lacks compatibility checks for older
> backends. We shouldn't present these new breakpoint types in that case.

This was the reason that I removed `setInstrumentationBreakpoint` and `removeInstrumentationBreakpoint`.  That way, we can check for the existence of the new functions I created.  See the usage of the following:
 - `WI.DOMDebuggerManager.supportsEventAnimationFrameBreakpoints`
 - `WI.DOMDebuggerManager.supportsEventListenerBreakpoints`
 - `WI.DOMDebuggerManager.supportsEventTimerBreakpoints`

>> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:1316
>> +        if (popover instanceof WI.XHRBreakpointPopover && popover.result === WI.InputPopover.Result.Committed) {
> 
> It looks like we aren't using InputPopover anywhere right now. Maybe the check below for `url` is enough, and we can remove the check for Result.Committed.

This is used by `WI.XHRBreakpointPopover`, so I'd like to not mess with it too much.  Based on what I see, however, I think we could drop this pretty safely (in another patch).

>> LayoutTests/inspector/dom-debugger/event-animation-frame-breakpoints-expected.txt:7
>> +Firing "requestAnimationFrame" on window...
> 
> I find all the ellipses at the end of each log message distracting. How about just a period?

I find them useful to quickly indicate that something will be triggered/fired.  I've seen that "..." is used in our tests right before we do something to the page that will normally trigger an event.  I think it's useful to have, but I don't have a strong preference.

>> LayoutTests/inspector/dom-debugger/event-animation-frame-breakpoints.html:14
>> +    requestAnimationFrame(handleWindow_requestAnimationFrame, 100);
> 
> requestAnimationFrame only takes a callback. The interval is determined by the engine.

Oops.  Copypasta :P

[Devin Rousso]

Created attachment 347716 [details]
Patch

[EWS Watchlist]

Comment on attachment 347716 [details]
Patch

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

New failing tests:
http/wpt/workers/name-property-enhanced.html

[EWS Watchlist]

Created attachment 347755 [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 347768 [details]
[Image] After Patch is applied

[Matt Baker]

Comment on attachment 347716 [details]
Patch

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

> Source/JavaScriptCore/inspector/protocol/DOMDebugger.json:34
> +                { "name": "eventName", "type": "string", "description": "Animation frame event name to stop on." }

Does this take an eventName in case we choose to add breakpoints for canceling and scheduling later on?

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:456
> +        }

That's a lot of checks! This will only get worse if we ever add support for more instrumentation points (cancelAnimationFrame, etc). Why was setInstrumentationBreakpoint with an enum for the type (animationFrameFired, timerFired, etc) discarded? It seems like a lot of code would be simplified.

> Source/WebInspectorUI/UserInterface/Views/EventBreakpointPopover.js:72
> +            createOption(WI.unlocalizedString("requestAnimationFrame"), "requestAnimationFrame");

Shouldn't the option value be WI.EventBreakpoint.Type.AnimationFrame? Then you could eliminate the data massaging that happens in Popover.dismiss later on. Maybe I'm missing something.

> Source/WebInspectorUI/UserInterface/Views/EventBreakpointPopover.js:80
> +        this._domEventNameInputElement.placeholder = WI.UIString("Example: \"%s\"").format("click");

It looks like escaped quotes trips up the localizable string extractor. This works:

WI.UIString("Example: “%s”")

> Source/WebInspectorUI/UserInterface/Views/EventBreakpointPopover.js:102
> +        if (type === WI.EventBreakpoint.Type.Listener && WI.DOMDebuggerManager.supportsEventListenerBreakpoints())

The checks for support seem redundant. You already checked for backend support when creating the options.

[Devin Rousso]

Comment on attachment 347716 [details]
Patch

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

>> Source/JavaScriptCore/inspector/protocol/DOMDebugger.json:34
>> +                { "name": "eventName", "type": "string", "description": "Animation frame event name to stop on." }
> 
> Does this take an eventName in case we choose to add breakpoints for canceling and scheduling later on?

Yeah, as well as any other potentially future "event"s that may be added to the rAF system.

>> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:456
>> +        }
> 
> That's a lot of checks! This will only get worse if we ever add support for more instrumentation points (cancelAnimationFrame, etc). Why was setInstrumentationBreakpoint with an enum for the type (animationFrameFired, timerFired, etc) discarded? It seems like a lot of code would be simplified.

The main reason I wanted to split this out into different functions was to provide more dedicated paths for each "type" of event.  Additionally, the previous implementation of `setInstrumentationBreakpoint` wouldn't have worked for what we wanted to do, specifically since it didn't distinguish between `setTimeout` and `setInterval`.  I like your idea of using an enum!

>> Source/WebInspectorUI/UserInterface/Views/EventBreakpointPopover.js:72
>> +            createOption(WI.unlocalizedString("requestAnimationFrame"), "requestAnimationFrame");
> 
> Shouldn't the option value be WI.EventBreakpoint.Type.AnimationFrame? Then you could eliminate the data massaging that happens in Popover.dismiss later on. Maybe I'm missing something.

This is a bit future-forward in that if we decide to add other AnimationFrame breakpoint support, we'd have two <option> with the same value of `WI.EventBreakpoint.Type.AnimationFrame`.  It is for this reason that we don't use `WI.EventBreakpoint.Type.Timer` below for the `setTimeout` and `setInterval` <option>.

[Devin Rousso]

Created attachment 347824 [details]
Patch

Changed to use a single set/remove breakpoint command with enum for all event breakpoints

[Blaze Burg]

Comment on attachment 347824 [details]
Patch

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

Overall this is very solid. I have feedback on various parts. Please post a new patch.

> Source/JavaScriptCore/inspector/protocol/DOMDebugger.json:15
> +            "enum": ["animation-frame", "listener", "timer"],

Good idea.

> Source/JavaScriptCore/inspector/protocol/DOMDebugger.json:37
> +            "name": "setEventBreakpoint",

Is this going to work with older backends? Were both of these just added in trunk or have they been shipped yet?

> Source/WebCore/inspector/InspectorInstrumentation.cpp:-84
> -static const char* const timerFiredEventName = "timerFired";

LOL, sad.

> Source/WebCore/inspector/InspectorInstrumentation.cpp:1058
> +    if (InspectorDOMDebuggerAgent* domDebuggerAgent = instrumentingAgents.inspectorDOMDebuggerAgent())

I think it's fine to put the agent existence checks at each callsite. However, the current approach drops the difference between sync and async pausing. Is this expected? Does the new implementation work with things that used to be sync (rAF events)?

> Source/WebCore/inspector/agents/InspectorDOMDebuggerAgent.cpp:142
> +    if (breakpointType != animationFrameCategoryType && breakpointType != listenerCategoryType && breakpointType != timerCategoryType) {

Please use the generated enum values and parsing helper. Here's an example:

        std::optional<Protocol::Timeline::Instrument> instrumentType = Protocol::InspectorHelpers::parseEnumValueFromString<Protocol::Timeline::Instrument>(enumValueString);

> Source/WebCore/inspector/agents/InspectorDOMDebuggerAgent.cpp:152
> +    m_eventBreakpoints.add(formatEventBreakpointString(breakpointType, eventName));

Please store std::pair<BreakpointType, eventName> instead of requiring string allocations to do a hash lookup.

> Source/WebCore/inspector/agents/InspectorDOMDebuggerAgent.cpp:162
> +    if (breakpointType != animationFrameCategoryType && breakpointType != listenerCategoryType && breakpointType != timerCategoryType) {

Ditto.

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:434
> +        if (breakpoint.disabled) {

Please put legacy paths together at top level here; it's okay to check breakpoint.disabled again. It's hard to follow either code path (legacy or modern) right now.

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:956
> +            if (pauseData) {

Make this an early return if !pauseData?

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:1133
> +            if (pauseData) {

Ditto

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:1319
> +        if (popover instanceof WI.XHRBreakpointPopover && popover.result === WI.InputPopover.Result.Committed) {

Why?

> Source/WebInspectorUI/UserInterface/Views/EventBreakpointTreeElement.js:32
> +        let classNames = ["breakpoint", "event", breakpoint.type];

Please adjust the last one to be "breakpoint-for-${breakpoint.type}". This makes the type-dependent CSS class searchable to this call site.

> LayoutTests/inspector/dom-debugger/event-animation-frame-breakpoints.html:15
> +}

These two function names are confusing. How about trigger_requestAnimationFrame and calledBy_requestAnimationFrame ?

> LayoutTests/inspector/dom-debugger/resources/event-breakpoint-utilities.js:2
> +    window.teardown = function(resolve, reject) {

It's not safe to put these into the global namespace inside the Inspector as other utility files could pick the same names. The pattern used in other files is to namespace these under

InspectorTest.EventBreakpoint.<the thing>

Which is more wordy but guaranteed to not have this problem.

> LayoutTests/inspector/dom-debugger/resources/event-breakpoint-utilities.js:51
> +                InspectorTest.assert(!breakpoint.disabled, "Breakpoint should not be disabled initially.");

I think this should reference event.data.breakpoint.

[Devin Rousso]

Comment on attachment 347824 [details]
Patch

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

>> Source/JavaScriptCore/inspector/protocol/DOMDebugger.json:37
>> +            "name": "setEventBreakpoint",
> 
> Is this going to work with older backends? Were both of these just added in trunk or have they been shipped yet?

The event listener breakpoints work with older backends, but the instrumentation breakpoints do not.

>> Source/WebCore/inspector/InspectorInstrumentation.cpp:-84
>> -static const char* const timerFiredEventName = "timerFired";
> 
> LOL, sad.

Yeah.  :/  This is why I'm dropping the `*InstrumentationBreakpoint` functions, since they don't use the name most people would expect.

>> Source/WebCore/inspector/InspectorInstrumentation.cpp:1058
>> +    if (InspectorDOMDebuggerAgent* domDebuggerAgent = instrumentingAgents.inspectorDOMDebuggerAgent())
> 
> I think it's fine to put the agent existence checks at each callsite. However, the current approach drops the difference between sync and async pausing. Is this expected? Does the new implementation work with things that used to be sync (rAF events)?

The only sync events were the actual function calls (e.g. setTimeout, clearTimeout, setInterval, clearInterval, requestAnimationFrame, cancelAnimationFrame).  This patch focuses on the callback that will be invoked, which is always async.

>> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:956
>> +            if (pauseData) {
> 
> Make this an early return if !pauseData?

I was following the other cases, but I guess there's no reason to do that 😛

One thing that is nice about this is that it allows us to use let/const, since we are inside an if-block, not a case.

>> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:1319
>> +        if (popover instanceof WI.XHRBreakpointPopover && popover.result === WI.InputPopover.Result.Committed) {
> 
> Why?

I don't want to break the current functionality of `WI.XHRBreakpointPopver`.  I removed the if statement above that uses this.  `WI.XHRBreakpointPopver` uses `WI.InputPopoverResult` for some reason, and rather than make this patch more complex, I just want to keep supporting it as is.  I can address this in a followup.

[Matt Baker]

(In reply to Devin Rousso from comment #17)
> Comment on attachment 347824 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=347824&action=review
> 
> >> Source/JavaScriptCore/inspector/protocol/DOMDebugger.json:37
> >> +            "name": "setEventBreakpoint",
> > 
> > Is this going to work with older backends? Were both of these just added in trunk or have they been shipped yet?
> 
> The event listener breakpoints work with older backends, but the
> instrumentation breakpoints do not.

AFAIK, instrumentation breakpoints were never hooked up in the frontend. There shouldn't be any compatibility issues.

[Devin Rousso]

Created attachment 347907 [details]
Patch

Added `DefaultHash` and `HashTraits` to any enums used in Inspector protocol, since we need a way to hash them when used inside `HashMap`/`HashSet`

[EWS Watchlist]

This patch modifies the inspector protocol generator. Please ensure that you have rebaselined any generator test results (i.e., by running `Tools/Scripts/run-inspector-generator-tests --reset-results`)

[EWS Watchlist]

Attachment 347907 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:438:  [CppProtocolTypesHeaderGenerator._generate_hash_declarations] Instance of 'CppProtocolTypesHeaderGenerator' has no 'type_declarations_for_domain' member  [pylint/E1101] [5]
Total errors found: 1 in 41 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Blaze Burg]

Comment on attachment 347907 [details]
Patch

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

> Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:68
> +        sections.extend(self._generate_declarations_for_enum_conversion_methods(domains))

Seems fine.

> Source/JavaScriptCore/inspector/scripts/tests/generic/expected/commands-with-async-attribute.json-result:735
> +template<>

Per discussion on IRC, this can be simplified a lot:
- The HashTraits can be defined at the declaration of the map/set to use PairHashTraits<HashTraits<String>, StrongEnumHashTraits<EnumType>>. Thus we don't need to define custom hash traits.
- The generated DefaultHash<EnumType> can be implemented as:

struct DefaultHash<EnumType> {
typedef IntHash<EnumType> Hash;
}

[Blaze Burg]

Comment on attachment 347907 [details]
Patch

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

> LayoutTests/inspector/dom-debugger/event-timer-breakpoints.html:137
> +                .catch((reason) => {

This can simply be

.catch(InspectorTest.reportUnhandledRejection.bind(InspectorTest))

Bonus points if you fix reportUnhandledRejection to not use `this` so it doesn't need binding. ;-)

[Devin Rousso]

Comment on attachment 347907 [details]
Patch

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

>> LayoutTests/inspector/dom-debugger/event-timer-breakpoints.html:137
>> +                .catch((reason) => {
> 
> This can simply be
> 
> .catch(InspectorTest.reportUnhandledRejection.bind(InspectorTest))
> 
> Bonus points if you fix reportUnhandledRejection to not use `this` so it doesn't need binding. ;-)

Frankly, I'd rather just use the local `reject`, since that's going to resolve the current chain instead of abruptly exiting.  Another reason I wanted to manually call `InspectorTest.fail` was so that we didn't skip the rest of the promise chain (we sill `resolve`, so we would run the next test).  In this case, however, if we fail to `resume`, we should exit anyways.

I can look at the binding "issue" in a followup :)

[Devin Rousso]

Created attachment 347933 [details]
Patch

[EWS Watchlist]

Attachment 347933 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:433:  [CppProtocolTypesHeaderGenerator._generate_hash_declarations] Instance of 'CppProtocolTypesHeaderGenerator' has no 'type_declarations_for_domain' member  [pylint/E1101] [5]
Total errors found: 1 in 40 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 347933 [details]
Patch

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

New failing tests:
accessibility/smart-invert-reference.html

[EWS Watchlist]

Created attachment 347940 [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

[Blaze Burg]

Comment on attachment 347933 [details]
Patch

r=me, EWS failure looks unrelated.

[WebKit Commit Bot]

Comment on attachment 347933 [details]
Patch

Clearing flags on attachment: 347933

Committed r235248: <https://trac.webkit.org/changeset/235248>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.