Bug 215794 – Web Inspector: allow special JavaScript breakpoints to be configured

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

[Devin Rousso]

This would allow developers to do things like:
 - ignore the first N pauses
 - evaluate JavaScript inside every microtask without pausing
 - evaluate JavaScript whenever an exception is thrown without pausing

[Devin Rousso]

Created attachment 407543 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/68119056>

[Blaze Burg]

Comment on attachment 407543 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:41
> +        This is needed when the JavaScript being paused in is native code, such as when pausing for

Nit: grammaro

> Source/JavaScriptCore/ChangeLog:72
> +        * debugger/Breakpoint.h:

I think this should move up to near other debugger/*.h changes.

> Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp:219
> +JSValue DebuggerCallFrame::evaluateWithScopeExtension(const String& script, JSObject* scopeExtensionObject, NakedPtr<Exception>& exception, ClimbToValidFrame climbToValidFrame)

Its weird that there are no uses of ClimbToValidFrame::No.

If this behavior is new, I think it should be covered by a test. Feel free to split that from the main patch, it doesn't seem essential to me.

[Blaze Burg]

Comment on attachment 407543 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:41
> +        This is needed when the JavaScript being paused in is native code, such as when pausing for

Nit: grammaro

> Source/JavaScriptCore/ChangeLog:72
> +        * debugger/Breakpoint.h:

I think this should move up to near other debugger/*.h changes.

> Source/JavaScriptCore/debugger/Debugger.cpp:-692
> -{

Hooray!

> Source/JavaScriptCore/debugger/Debugger.h:40
>  

I'm on the fence about whether this is better or worse than an RAII object that uses scopes explicitly.

> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:274
> +    m_pauseOnAssertionsBreakpoint = nullptr;

Nice.

> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:905
> +    if (stateString == "all") {

Kind of weird to not use an enum here.

> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:918
> +    m_debugger.setPauseOnAllExceptionsBreakpoint(WTFMove(allExceptionsBreakpoint));

This is intentionally clearing breakpoints not sent in the payload, right?

> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:965
> +        OVERRIDE_PAUSE_ON_EXCEPTIONS_BREAKPOINTS(m_debugger, nullptr);

So much clearer what this is doing now.

> Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js:1014
> +localizedStrings["Reset Breakpoint"] = "Reset Breakpoint";

Nit: please add more context (specifically, 'Reset' could act like a verb or an adjective)

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:130
> +            await new Promise((resolve, reject) => queueMicrotask(resolve));

Maybe this async flush should happen as part of registerInitializationPromise. It seems silly to do this at every caller where we might need to access controllers/managers.

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:1291
> +            target.DebuggerAgent[command].invoke(commandArguments);

Nice.

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:-1167
> -            if (!breakpoint.disabled && !this.breakpointsDisabledTemporarily)

Did you retain the behavior that setting a breakpoint will enable breakpoints if disabled?

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:-1205
> -                target.DebuggerAgent.setPauseOnMicrotasks(!breakpoint.disabled);

ooOOoo. So much nicer.

> Source/WebInspectorUI/UserInterface/Models/JavaScriptBreakpoint.js:28
> +    constructor(sourceCodeLocation, {contentIdentifier, resolved, disabled, condition, actions, ignoreCount, autoContinue} = {})

I can't find any uses of this additional option. Can this be reverted?

> Source/WebInspectorUI/UserInterface/Models/JavaScriptBreakpoint.js:159
> +            // COMPATIBILITY (iOS 14): Debugger.setPauseOnDebuggerStatements did not have an "options" parameter yet.

What is this for? I can't expand the context because patch does not apply to ToT.

> Source/WebInspectorUI/UserInterface/Views/BreakpointPopover.js:90
> +            contextMenu.appendItem(WI.UIString("Reset Breakpoint"), () => {

Ditto above comment about localizing this.

> Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:1957
>          let treeElement = event.data.element;

I don't think this is used any more.

> Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:1963
> +        if (selectedTreeElement.representedObject !== WI.debuggerManager.assertionFailuresBreakpoint && selectedTreeElement.representedObject.removable)

What is this line for?

[Devin Rousso]

Comment on attachment 407543 [details]
Patch

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

>>> Source/JavaScriptCore/ChangeLog:72
>>> +        * debugger/Breakpoint.h:
>> 
>> I think this should move up to near other debugger/*.h changes.
> 
> I think this should move up to near other debugger/*.h changes.

I moved it down because it was basically a non-functional change, but I see your point.

>> Source/JavaScriptCore/debugger/Debugger.h:40
>>  
> 
> I'm on the fence about whether this is better or worse than an RAII object that uses scopes explicitly.

🤦‍♂️ yeah a RAII scope object would be better I'll do that

>> Source/JavaScriptCore/debugger/DebuggerCallFrame.cpp:219
>> +JSValue DebuggerCallFrame::evaluateWithScopeExtension(const String& script, JSObject* scopeExtensionObject, NakedPtr<Exception>& exception, ClimbToValidFrame climbToValidFrame)
> 
> Its weird that there are no uses of ClimbToValidFrame::No.
> 
> If this behavior is new, I think it should be covered by a test. Feel free to split that from the main patch, it doesn't seem essential to me.

`climbToValidFrame` is defaulted to `No` in the header so that we don't have to update all the other callers.

I am tempted to entirely remove `ClimbToValidFrame` though because I think the only other caller is from `Inspector::JavaScriptCallFrame`, which is called from `InjectedScriptSource.js` which is already in JavaScript, so it shouldn't ever need to climb to a valid frame.

This is covered by the tests added in this patch.  Evaluating breakpoint condition/actions does not work without this for any special breakpoint that uses `Debugger::breakProgram` (which is why I added this).

>> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:905
>> +    if (stateString == "all") {
> 
> Kind of weird to not use an enum here.

AFAICT we don't generate `parseEnumValueFromString` for this because it's inlined into the command parameter in the JSON

>> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:918
>> +    m_debugger.setPauseOnAllExceptionsBreakpoint(WTFMove(allExceptionsBreakpoint));
> 
> This is intentionally clearing breakpoints not sent in the payload, right?

only one (or neither) of "All Exceptions" and "Uncaught Exceptions" can be active at a time

>> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:130
>> +            await new Promise((resolve, reject) => queueMicrotask(resolve));
> 
> Maybe this async flush should happen as part of registerInitializationPromise. It seems silly to do this at every caller where we might need to access controllers/managers.

All other call sites are doing work with something that is inherently asynchronous (e.g. `WI.ObjectStore.prototype.getAll`.  This case is the special one in that we have a co-dependency between `WI.debuggerManager` and the special `WI.JavaScriptBreakpoint`s (which are loaded/initialized synchronously).  I'd rather leave the decision to delay or not up to the callers.

> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:1247
> +            if (!specificTarget)
> +                this._debuggerStatementsBreakpointSetting.value = this._debuggerStatementsBreakpoint.toJSON();

NOTE: I could make this much nicer by just having a `let setting` and doing this after the `switch`

>> Source/WebInspectorUI/UserInterface/Controllers/DebuggerManager.js:-1167
>> -            if (!breakpoint.disabled && !this.breakpointsDisabledTemporarily)
> 
> Did you retain the behavior that setting a breakpoint will enable breakpoints if disabled?

Ah I'd moved this to `WI.DebuggerManager.prototype.addBreakpoint` but it should also happen for `WI.Breakpoint.Event.DisabledStateDidChange`.  I'll move it into `_updateSpecialBreakpoint`.  Good catch!

>> Source/WebInspectorUI/UserInterface/Models/JavaScriptBreakpoint.js:28
>> +    constructor(sourceCodeLocation, {contentIdentifier, resolved, disabled, condition, actions, ignoreCount, autoContinue} = {})
> 
> I can't find any uses of this additional option. Can this be reverted?

I think we should use this inside `WI.DebuggerManager.prototype._createSpecialBreakpoint` so that we can avoid dispatching `WI.JavaScriptBreakpoint.Event.ResolvedStateDidChange`.

>> Source/WebInspectorUI/UserInterface/Models/JavaScriptBreakpoint.js:159
>> +            // COMPATIBILITY (iOS 14): Debugger.setPauseOnDebuggerStatements did not have an "options" parameter yet.
> 
> What is this for? I can't expand the context because patch does not apply to ToT.

this is for `WI.JavaScriptBreakpoint.prototype.get editable`, which basically controls whether the "Edit Breakpoint" contextmenu item is shown :)

>> Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:1957
>>          let treeElement = event.data.element;
> 
> I don't think this is used any more.

Nice catch!

>> Source/WebInspectorUI/UserInterface/Views/SourcesNavigationSidebarPanel.js:1963
>> +        if (selectedTreeElement.representedObject !== WI.debuggerManager.assertionFailuresBreakpoint && selectedTreeElement.representedObject.removable)
> 
> What is this line for?

I dunno actually 🤔  I just adjusted the code that was already here.  Let me investigate as to whether this is actually needed.

[Devin Rousso]

Created attachment 407846 [details]
Patch

[EWS Watchlist]

This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.

[Devin Rousso]

Created attachment 407873 [details]
Patch

typo

[Blaze Burg]

Comment on attachment 407873 [details]
Patch

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

r=me

> Source/JavaScriptCore/debugger/Debugger.h:75
> +    class TemporarilyDisableExceptionBreakpoints {

Nit: can't this be defined entirely in the .cpp file? Is it used outside this class?

[Devin Rousso]

Comment on attachment 407873 [details]
Patch

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

>> Source/JavaScriptCore/debugger/Debugger.h:75
>> +    class TemporarilyDisableExceptionBreakpoints {
> 
> Nit: can't this be defined entirely in the .cpp file? Is it used outside this class?

in fact, it is _only_ used outside of this class 😅

[EWS]

Committed r266534: <https://trac.webkit.org/changeset/266534>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 407873 [details].