- Revision
- 266138
- Author
- [email protected]
- Date
- 2020-08-25 11:49:57 -0700 (Tue, 25 Aug 2020)
Log Message
Web Inspector: breakpoint condition should be evaluated before the ignore count
https://bugs.webkit.org/show_bug.cgi?id=215364
<rdar://problem/67310703>
Reviewed by Joseph Pecoraro.
Source/_javascript_Core:
Previously, when pausing, `JSC::Breakpoint` would check that it's `ignoreCount` before it
would even attempt to evaluate it's `condition`. This meant that a `JSC::Breakpoint` with
a `condition` of `foo === 42` and an `ignoreCount` of `3` would ignore the first three
pauses and then only pause if `foo === 42`. This is likely contrary to the expectation of
most users (especially since the `condition` input is before the `ignoreCount` input in
the Web Inspector frontend UI) in that they would probably expect to ignore the first
three pauses if `foo === 42`.
* debugger/Breakpoint.cpp:
(JSC::Breakpoint::shouldPause):
LayoutTests:
* inspector/debugger/resources/condition-ignoreCount.js: Added.
(trigger): Added.
* inspector/debugger/setBreakpoint-condition-ignoreCount.html: Added.
* inspector/debugger/setBreakpoint-condition-ignoreCount-expected.txt: Added.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (266137 => 266138)
--- trunk/LayoutTests/ChangeLog 2020-08-25 18:47:43 UTC (rev 266137)
+++ trunk/LayoutTests/ChangeLog 2020-08-25 18:49:57 UTC (rev 266138)
@@ -1,3 +1,16 @@
+2020-08-25 Devin Rousso <[email protected]>
+
+ Web Inspector: breakpoint condition should be evaluated before the ignore count
+ https://bugs.webkit.org/show_bug.cgi?id=215364
+ <rdar://problem/67310703>
+
+ Reviewed by Joseph Pecoraro.
+
+ * inspector/debugger/resources/condition-ignoreCount.js: Added.
+ (trigger): Added.
+ * inspector/debugger/setBreakpoint-condition-ignoreCount.html: Added.
+ * inspector/debugger/setBreakpoint-condition-ignoreCount-expected.txt: Added.
+
2020-08-25 Hector Lopez <[email protected]>
[ macOS Release ] webgl/2.0.0/conformance2/textures/canvas_sub_rectangle/tex-2d-rgb32f-rgb-float.html is a flaky failure
Added: trunk/LayoutTests/inspector/debugger/resources/condition-ignoreCount.js (0 => 266138)
--- trunk/LayoutTests/inspector/debugger/resources/condition-ignoreCount.js (rev 0)
+++ trunk/LayoutTests/inspector/debugger/resources/condition-ignoreCount.js 2020-08-25 18:49:57 UTC (rev 266138)
@@ -0,0 +1,7 @@
+window.CONDITION_TEST = 0;
+
+function trigger() {
+ ++window.CONDITION_TEST;
+
+ TestPage.dispatchEventToFrontend("TestPage_trigger");
+}
Added: trunk/LayoutTests/inspector/debugger/setBreakpoint-condition-ignoreCount-expected.txt (0 => 266138)
--- trunk/LayoutTests/inspector/debugger/setBreakpoint-condition-ignoreCount-expected.txt (rev 0)
+++ trunk/LayoutTests/inspector/debugger/setBreakpoint-condition-ignoreCount-expected.txt 2020-08-25 18:49:57 UTC (rev 266138)
@@ -0,0 +1,25 @@
+Tests for Debugger.setBreakpoint.
+
+
+== Running test suite: Debugger.setBreakpoint
+-- Running test case: Debugger.setBreakpoint.ConditionIgnoreCount
+Adding breakpoint...
+
+Triggering breakpoint...
+PASS: Should not pause.
+
+Triggering breakpoint...
+PASS: Should not pause.
+
+Triggering breakpoint...
+PASS: Should not pause.
+
+Triggering breakpoint...
+PASS: Should not pause.
+
+Triggering breakpoint...
+PASS: Should pause.
+
+Triggering breakpoint...
+PASS: Should pause.
+
Added: trunk/LayoutTests/inspector/debugger/setBreakpoint-condition-ignoreCount.html (0 => 266138)
--- trunk/LayoutTests/inspector/debugger/setBreakpoint-condition-ignoreCount.html (rev 0)
+++ trunk/LayoutTests/inspector/debugger/setBreakpoint-condition-ignoreCount.html 2020-08-25 18:49:57 UTC (rev 266138)
@@ -0,0 +1,78 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script src=""
+<script src=""
+<script>
+function test() {
+ function isHelperScript(script) {
+ return /resources\/condition-ignoreCount\.js/.test(script.url);
+ }
+
+ let suite = InspectorTest.createAsyncSuite("Debugger.setBreakpoint");
+
+ suite.addTestCase({
+ name: "Debugger.setBreakpoint.ConditionIgnoreCount",
+ description: "Check that `condition` is evaluated before `ignoreCount`.",
+ async test() {
+ let pauseCount = 0;
+
+ let listener = WI.debuggerManager.addEventListener(WI.DebuggerManager.Event.Paused, (event) => {
+ ++pauseCount;
+ WI.debuggerManager.resume();
+ });
+
+ let debuggerData = WI.debuggerManager.dataForTarget(WI.mainTarget);
+
+ let script = debuggerData.scripts.filter(isHelperScript)[0];
+ if (!script) {
+ await new Promise((resolve, reject) => {
+ let listener = WI.debuggerManager.addEventListener(WI.DebuggerManager.Event.ScriptAdded, (event) => {
+ script = event.data.script;
+ if (isHelperScript(script)) {
+ WI.debuggerManager.removeEventListener(WI.DebuggerManager.Event.ScriptAdded, listener);
+ resolve();
+ }
+ });
+ });
+ }
+
+ let breakpoint = new WI._javascript_Breakpoint(new WI.SourceCodeLocation(script, 4, 0), {
+ condition: `window.CONDITION_TEST > 2`,
+ ignoreCount: 2,
+ });
+
+ InspectorTest.log("Adding breakpoint...");
+ await Promise.all([
+ breakpoint.awaitEvent(WI._javascript_Breakpoint.Event.ResolvedStateDidChange),
+ WI.debuggerManager.addBreakpoint(breakpoint),
+ ]);
+
+ for (let i = 1; i <= 6; ++i) {
+ InspectorTest.newline();
+
+ InspectorTest.log("Triggering breakpoint...");
+ await Promise.all([
+ InspectorTest.awaitEvent("TestPage_trigger"),
+ InspectorTest.evaluateInPage(`trigger()`),
+ ]);
+
+ if (i <= 4)
+ InspectorTest.expectEqual(pauseCount, 0, "Should not pause.");
+ else
+ InspectorTest.expectEqual(pauseCount, i - 4, "Should pause.");
+ }
+
+ breakpoint.remove();
+ WI.debuggerManager.removeEventListener(WI.DebuggerManager.Event.Paused, listener);
+ },
+ });
+
+ suite.runTestCasesAndFinish();
+}
+</script>
+</head>
+<body _onload_="runTest()">
+ <p>Tests for Debugger.setBreakpoint.</p>
+</body>
+</html>
Modified: trunk/Source/_javascript_Core/ChangeLog (266137 => 266138)
--- trunk/Source/_javascript_Core/ChangeLog 2020-08-25 18:47:43 UTC (rev 266137)
+++ trunk/Source/_javascript_Core/ChangeLog 2020-08-25 18:49:57 UTC (rev 266138)
@@ -1,3 +1,22 @@
+2020-08-25 Devin Rousso <[email protected]>
+
+ Web Inspector: breakpoint condition should be evaluated before the ignore count
+ https://bugs.webkit.org/show_bug.cgi?id=215364
+ <rdar://problem/67310703>
+
+ Reviewed by Joseph Pecoraro.
+
+ Previously, when pausing, `JSC::Breakpoint` would check that it's `ignoreCount` before it
+ would even attempt to evaluate it's `condition`. This meant that a `JSC::Breakpoint` with
+ a `condition` of `foo === 42` and an `ignoreCount` of `3` would ignore the first three
+ pauses and then only pause if `foo === 42`. This is likely contrary to the expectation of
+ most users (especially since the `condition` input is before the `ignoreCount` input in
+ the Web Inspector frontend UI) in that they would probably expect to ignore the first
+ three pauses if `foo === 42`.
+
+ * debugger/Breakpoint.cpp:
+ (JSC::Breakpoint::shouldPause):
+
2020-08-25 Alexey Shvayka <[email protected]>
Invalid early error for object literal method named "__proto__"
Modified: trunk/Source/_javascript_Core/debugger/Breakpoint.cpp (266137 => 266138)
--- trunk/Source/_javascript_Core/debugger/Breakpoint.cpp 2020-08-25 18:47:43 UTC (rev 266137)
+++ trunk/Source/_javascript_Core/debugger/Breakpoint.cpp 2020-08-25 18:49:57 UTC (rev 266138)
@@ -75,10 +75,10 @@
bool Breakpoint::shouldPause(Debugger& debugger, JSGlobalObject* globalObject)
{
- if (++m_hitCount <= m_ignoreCount)
+ if (!debugger.evaluateBreakpointCondition(*this, globalObject))
return false;
- return debugger.evaluateBreakpointCondition(*this, globalObject);
+ return ++m_hitCount > m_ignoreCount;
}
} // namespace JSC
