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
Created attachment 407543 [details] Patch
<rdar://problem/68119056>
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.
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?
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.
Created attachment 407846 [details] Patch
This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.
Created attachment 407873 [details] Patch typo
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?
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 😅
Committed r266534: <https://trac.webkit.org/changeset/266534> All reviewed patches have been landed. Closing bug and clearing flags on attachment 407873 [details].