Bug 188778 - Web Inspector: support breakpoints for timers and animation-frame events
Summary: Web Inspector: support breakpoints for timers and animation-frame events
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on: 183138
Blocks:
  Show dependency treegraph
 
Reported: 2018-08-20 22:32 PDT by Devin Rousso
Modified: 2018-08-23 14:36 PDT (History)
12 users (show)

See Also:


Attachments
Patch (93.76 KB, patch)
2018-08-20 22:52 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-sierra-wk2 (3.32 MB, application/zip)
2018-08-21 00:53 PDT, Build Bot
no flags Details
Patch (95.06 KB, patch)
2018-08-21 02:02 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (105.32 KB, patch)
2018-08-21 14:40 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (105.98 KB, patch)
2018-08-21 15:42 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews113 for mac-sierra (3.06 MB, application/zip)
2018-08-21 19:35 PDT, Build Bot
no flags Details
[Image] After Patch is applied (94.09 KB, image/png)
2018-08-21 22:23 PDT, Devin Rousso
no flags Details
Patch (107.01 KB, patch)
2018-08-22 11:56 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (134.18 KB, patch)
2018-08-22 23:27 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (124.50 KB, patch)
2018-08-23 10:35 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-sierra-wk2 (3.31 MB, application/zip)
2018-08-23 11:38 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2018-08-20 22:32:44 PDT
We should also provide support for breaking whenever a `setTimeout`, `setInterval`, or `requestAnimationFrame` is fired.
Comment 1 Devin Rousso 2018-08-20 22:52:28 PDT
Created attachment 347612 [details]
Patch
Comment 2 Build Bot 2018-08-21 00:53:10 PDT Comment hidden (obsolete)
Comment 3 Build Bot 2018-08-21 00:53:11 PDT Comment hidden (obsolete)
Comment 4 Devin Rousso 2018-08-21 02:02:11 PDT
Created attachment 347622 [details]
Patch
Comment 5 Radar WebKit Bug Importer 2018-08-21 12:20:01 PDT
<rdar://problem/43572345>
Comment 6 Devin Rousso 2018-08-21 14:40:31 PDT
Created attachment 347702 [details]
Patch

Remove old protocol methods since they don't really do what we want
Comment 7 Matt Baker 2018-08-21 15:09:21 PDT
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."
Comment 8 Devin Rousso 2018-08-21 15:15:39 PDT
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
Comment 9 Devin Rousso 2018-08-21 15:42:34 PDT
Created attachment 347716 [details]
Patch
Comment 10 Build Bot 2018-08-21 19:35:10 PDT Comment hidden (obsolete)
Comment 11 Build Bot 2018-08-21 19:35:12 PDT Comment hidden (obsolete)
Comment 12 Devin Rousso 2018-08-21 22:23:20 PDT
Created attachment 347768 [details]
[Image] After Patch is applied
Comment 13 Matt Baker 2018-08-22 10:54:04 PDT
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.
Comment 14 Devin Rousso 2018-08-22 11:05:29 PDT
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>.
Comment 15 Devin Rousso 2018-08-22 11:56:40 PDT
Created attachment 347824 [details]
Patch

Changed to use a single set/remove breakpoint command with enum for all event breakpoints
Comment 16 Brian Burg 2018-08-22 12:43:42 PDT
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.
Comment 17 Devin Rousso 2018-08-22 14:44:33 PDT
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.
Comment 18 Matt Baker 2018-08-22 14:55:41 PDT
(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.
Comment 19 Devin Rousso 2018-08-22 23:27:47 PDT
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`
Comment 20 Build Bot 2018-08-22 23:30:19 PDT Comment hidden (obsolete)
Comment 21 Build Bot 2018-08-22 23:30:34 PDT Comment hidden (obsolete)
Comment 22 Brian Burg 2018-08-23 09:46:00 PDT
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;
}
Comment 23 Brian Burg 2018-08-23 09:52:34 PDT
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. ;-)
Comment 24 Devin Rousso 2018-08-23 10:29:01 PDT
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 :)
Comment 25 Devin Rousso 2018-08-23 10:35:45 PDT
Created attachment 347933 [details]
Patch
Comment 26 Build Bot 2018-08-23 10:38:37 PDT Comment hidden (obsolete)
Comment 27 Build Bot 2018-08-23 11:38:03 PDT Comment hidden (obsolete)
Comment 28 Build Bot 2018-08-23 11:38:05 PDT Comment hidden (obsolete)
Comment 29 Brian Burg 2018-08-23 13:35:59 PDT
Comment on attachment 347933 [details]
Patch

r=me, EWS failure looks unrelated.
Comment 30 WebKit Commit Bot 2018-08-23 14:36:47 PDT
Comment on attachment 347933 [details]
Patch

Clearing flags on attachment: 347933

Committed r235248: <https://trac.webkit.org/changeset/235248>
Comment 31 WebKit Commit Bot 2018-08-23 14:36:49 PDT
All reviewed patches have been landed.  Closing bug.