Bug 147664

<rdar://problem/22278809>

I checked how breakpoints in Xcode behave when both a Condition and Ignore count are specified. In Xcode the Condition is not evaluated until the hit count exceeds the ignore count. For example: BREAKPOINT ---------- Line: 2 Condition: "i & 1" Ignore Count: 5 Log message action: "@i@" SAMPLE CODE ----------- 01: for (int i = 0; i < 10; ++i) { > 02: // Do stuff 03: } OUTPUT ------ 5 7 9

Created attachment 260180 [details] [Patch] WIP Just need to write a test

Created attachment 260181 [details] [Video] Using the "Ignore" option

Comment on attachment 260180 [details] [Patch] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=260180&action=review Needs tests. Would it be worth updating the frontend with "# Skips Remaining" events? > Source/JavaScriptCore/inspector/protocol/Debugger.json:52 > + { "name": "ignoreCount", "type": "integer", "optional": 0, "description": "Number of times to ignore this breakpoint, before stopping on the breakpoint and running actions." } Nit: `"optional": 0` should instead be `"optional": true`

(In reply to comment #5) > Comment on attachment 260180 [details] > [Patch] WIP > > View in context: > https://bugs.webkit.org/attachment.cgi?id=260180&action=review > > Needs tests. > > Would it be worth updating the frontend with "# Skips Remaining" events? > > > Source/JavaScriptCore/inspector/protocol/Debugger.json:52 > > + { "name": "ignoreCount", "type": "integer", "optional": 0, "description": "Number of times to ignore this breakpoint, before stopping on the breakpoint and running actions." } > > Nit: `"optional": 0` should instead be `"optional": true` In fact, I think that the generator should throw an exception here, since it type checks this.

Comment on attachment 260180 [details] [Patch] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=260180&action=review This looks good, just needs tests. Ask if you need help. Setting r- to remove it from the review queue. > Source/JavaScriptCore/debugger/Breakpoint.h:44 > + , hitCount(0) Please use brace initializers in the declaration. > Source/JavaScriptCore/inspector/ScriptBreakpoint.h:89 > + int ignoreCount; Here too. Even if all constructors initialize the value (right now), it's a good habit to take up. BTW, this should also be unsigned to match Breakpoint's field. >>> Source/JavaScriptCore/inspector/protocol/Debugger.json:52 >>> + { "name": "ignoreCount", "type": "integer", "optional": 0, "description": "Number of times to ignore this breakpoint, before stopping on the breakpoint and running actions." } >> >> Nit: `"optional": 0` should instead be `"optional": true` > > In fact, I think that the generator should throw an exception here, since it type checks this. Filed: https://bugs.webkit.org/show_bug.cgi?id=148679 > Source/WebInspectorUI/UserInterface/Models/Breakpoint.js:395 > + const match = /^[0-9]*$/.exec(event.target.value); const [match,] = ... ignoreCount = match ? parseInt(match, 10) : 0 > Source/WebInspectorUI/UserInterface/Models/Breakpoint.js:450 > + // COMPATIBILITY (iOS 9): Legacy backends don't support breakpoint ignore count. Since support This is fine for now, but I filed a bug about how we can do it better: https://bugs.webkit.org/show_bug.cgi?id=148680

Comment on attachment 260180 [details] [Patch] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=260180&action=review >> Source/JavaScriptCore/inspector/ScriptBreakpoint.h:89 >> + int ignoreCount; > > Here too. Even if all constructors initialize the value (right now), it's a good habit to take up. > > BTW, this should also be unsigned to match Breakpoint's field. I agree, however there is a bit of a ripple effect here. InspectorDebuggerAgent APIs deal with ints, likely requiring casts. InspectorObjectBase also lacks function templates for setInteger (for which a FIXME exists). We should do this work under a separate issue. >> Source/WebInspectorUI/UserInterface/Models/Breakpoint.js:395 >> + const match = /^[0-9]*$/.exec(event.target.value); > > const [match,] = ... > ignoreCount = match ? parseInt(match, 10) : 0 I do like this style, but is there a particular reason to prefer this? >> Source/WebInspectorUI/UserInterface/Models/Breakpoint.js:450 >> + // COMPATIBILITY (iOS 9): Legacy backends don't support breakpoint ignore count. Since support > > This is fine for now, but I filed a bug about how we can do it better: https://bugs.webkit.org/show_bug.cgi?id=148680 I'll add a // FIXME referring to the bug.

Comment on attachment 260180 [details] [Patch] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=260180&action=review >>> Source/WebInspectorUI/UserInterface/Models/Breakpoint.js:395 >>> + const match = /^[0-9]*$/.exec(event.target.value); >> >> const [match,] = ... >> ignoreCount = match ? parseInt(match, 10) : 0 > > I do like this style, but is there a particular reason to prefer this? NIT: I almost feel like just calling "parseInt(event.target.value)" on the value would be simpler. Then you could check for "isNaN(ignoreCount)" and "ignoreCount < 0" and set ignoreCount to 0 if either is true. > Source/WebInspectorUI/UserInterface/Models/Breakpoint.js:461 > + this._ignoreCountInput.max = 1000000; NIT: Is there any particular reason that you are specifying a maximum here?

Created attachment 262111 [details] [Patch] Proposed Fix

(In reply to comment #9) > Comment on attachment 260180 [details] > [Patch] WIP > > View in context: > https://bugs.webkit.org/attachment.cgi?id=260180&action=review > > >>> Source/WebInspectorUI/UserInterface/Models/Breakpoint.js:395 > >>> + const match = /^[0-9]*$/.exec(event.target.value); > >> > >> const [match,] = ... > >> ignoreCount = match ? parseInt(match, 10) : 0 > > > > I do like this style, but is there a particular reason to prefer this? > > NIT: I almost feel like just calling "parseInt(event.target.value)" on the > value would be simpler. Then you could check for "isNaN(ignoreCount)" and > "ignoreCount < 0" and set ignoreCount to 0 if either is true. It's simpler. I'll change it. > > Source/WebInspectorUI/UserInterface/Models/Breakpoint.js:461 > > + this._ignoreCountInput.max = 1000000; > > NIT: Is there any particular reason that you are specifying a maximum here? It was arbitrary. We don't range check other numeric values before shipping over the protocol, so I removed it.

Comment on attachment 262111 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=262111&action=review Nice! What should the behavior be across reloads. Is the breakpoint still "alive" or does its ignore/hit count reset to 0? > Source/JavaScriptCore/debugger/Breakpoint.h:70 > + unsigned hitCount {0}; Style: We've been converging on style with spaces, like "unsigned foo { 0 };". > Source/JavaScriptCore/debugger/Debugger.cpp:424 > + if (breakpoint->ignoreCount >= ++breakpoint->hitCount) > + return false; Style: As elegant as this is, I think we tend to break up the increment and comparison into multiple lines for maximum clarity. > Source/JavaScriptCore/inspector/ScriptBreakpoint.h:89 > + int ignoreCount {0}; Style: Same style here. > Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:308 > + options->getInteger(ASCIILiteral("ignoreCount"), ignoreCount); We should verify that ignoreCount >= 0, and 0 if missing. > Source/WebInspectorUI/UserInterface/Models/Breakpoint.js:156 > + this._ignoreCount = ignoreCount; Nit: console.assert(ignoreCount >= 0)

(In reply to comment #12) > Comment on attachment 262111 [details] > [Patch] Proposed Fix > > View in context: > https://bugs.webkit.org/attachment.cgi?id=262111&action=review > > Nice! > > What should the behavior be across reloads. Is the breakpoint still "alive" > or does its ignore/hit count reset to 0? I hadn't considered this! The hit count should definitely reset to zero on reload. I'll make sure this is the case in the patch for landing.

Comment on attachment 262111 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=262111&action=review >> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:308 >> + options->getInteger(ASCIILiteral("ignoreCount"), ignoreCount); > > We should verify that ignoreCount >= 0, and 0 if missing. InspectorObjectBase::getInteger doesn't modify the output variable if the name is missing, so the value will be zero by default. I'll change ignoreCount to unsigned.

(In reply to comment #14) > Comment on attachment 262111 [details] > [Patch] Proposed Fix > > View in context: > https://bugs.webkit.org/attachment.cgi?id=262111&action=review > > >> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:308 > >> + options->getInteger(ASCIILiteral("ignoreCount"), ignoreCount); > > > > We should verify that ignoreCount >= 0, and 0 if missing. > > InspectorObjectBase::getInteger doesn't modify the output variable if the > name is missing, so the value will be zero by default. I'll change > ignoreCount to unsigned. This value may come in through the protocol. An endpoint can send any integer value.

Created attachment 262351 [details] [Patch] Proposed Fix Added a second test, addressed style issues

Comment on attachment 262351 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=262351&action=review r=me > Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:355 > + int ignoreCount = 0; Seems this one should be unsigned as well. > LayoutTests/inspector/debugger/breakpoint-ignore-after-reload-expected.txt:4 > +inside breakpointBasic > +Was inside breakpoint after page reload. This worries me that the test might be flakey. I think there is a preexisting issue that reloadPage can cause the test to have flakey output. If you run this test 100 times `run-webkit-tests --iterations=100 inspector/debugger/breakpoint-ignore-after-reload.html` is it flakey? We might need to mark it as such if it is.

(In reply to comment #17) > Comment on attachment 262351 [details] > [Patch] Proposed Fix > > View in context: > https://bugs.webkit.org/attachment.cgi?id=262351&action=review > > r=me > > > Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:355 > > + int ignoreCount = 0; > > Seems this one should be unsigned as well. > > > LayoutTests/inspector/debugger/breakpoint-ignore-after-reload-expected.txt:4 > > +inside breakpointBasic > > +Was inside breakpoint after page reload. > > This worries me that the test might be flakey. I think there is a > preexisting issue that reloadPage can cause the test to have flakey output. > If you run this test 100 times `run-webkit-tests --iterations=100 > inspector/debugger/breakpoint-ignore-after-reload.html` is it flakey? We > might need to mark it as such if it is. It ran 1000 times without problems. Is the reloadPage flakiness related to the frontend test harness resending results?

Created attachment 262373 [details] [Patch] Proposed Fix

Comment on attachment 262373 [details] [Patch] Proposed Fix Clearing flags on attachment: 262373 Committed r190542: <http://trac.webkit.org/changeset/190542>

All reviewed patches have been landed. Closing bug.