- Revision
- 288996
- Author
- pan...@apple.com
- Date
- 2022-02-02 14:13:03 -0800 (Wed, 02 Feb 2022)
Log Message
No breakpoints hit on github.com, and some are invalid
https://bugs.webkit.org/show_bug.cgi?id=235607
Reviewed by Yusuke Suzuki.
Source/_javascript_Core:
Added test case in: inspector/debugger/breakpoints/resolved-dump-all-pause-locations.html
Previously not all line terminations resulted in setting the `m_lineStart` to the current m_code, which meant
that the location for pause-able locations and stack traces were inaccurate when they were on a line that
terminated multi-line comments, strings, or template strings. We now always update m_lineStart when shifting for
a line terminator, instead of only when the terminator appears outside a string or comment.
* debugger/Breakpoint.cpp:
(JSC::Breakpoint::resolve):
- The existing assertions were somewhat in conflict with each other. If we permit the line number to increase,
there is no guarantee that the column number will remain the same or increase, which can now more easily occur
with multi-line strings. Instead, we should make sure that the overall offset has increase.
* parser/Lexer.cpp:
(JSC::Lexer<T>::shiftLineTerminator):
(JSC::Lexer<T>::lexWithoutClearingLineTerminator):
(JSC::Lexer<T>::scanTemplateString):
- Make sure the token start offset and start line offset are correctly set for strings.
LayoutTests:
Add test cases for resolving breakpoints on lines that begin with the end of multi-line strings, comments, and
template strings.
* inspector/debugger/breakpoints/resolved-dump-all-pause-locations-expected.txt:
* inspector/debugger/breakpoints/resolved-dump-all-pause-locations.html:
* inspector/debugger/breakpoints/resources/dump-multiline.js: Added.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (288995 => 288996)
--- trunk/LayoutTests/ChangeLog 2022-02-02 21:53:20 UTC (rev 288995)
+++ trunk/LayoutTests/ChangeLog 2022-02-02 22:13:03 UTC (rev 288996)
@@ -1,3 +1,17 @@
+2022-02-02 Patrick Angle <pan...@apple.com>
+
+ No breakpoints hit on github.com, and some are invalid
+ https://bugs.webkit.org/show_bug.cgi?id=235607
+
+ Reviewed by Yusuke Suzuki.
+
+ Add test cases for resolving breakpoints on lines that begin with the end of multi-line strings, comments, and
+ template strings.
+
+ * inspector/debugger/breakpoints/resolved-dump-all-pause-locations-expected.txt:
+ * inspector/debugger/breakpoints/resolved-dump-all-pause-locations.html:
+ * inspector/debugger/breakpoints/resources/dump-multiline.js: Added.
+
2022-02-02 Patrick Griffis <pgrif...@igalia.com>
CSP: Fix returned WebAssembly error type when blocked
Modified: trunk/LayoutTests/inspector/debugger/breakpoints/resolved-dump-all-pause-locations-expected.txt (288995 => 288996)
--- trunk/LayoutTests/inspector/debugger/breakpoints/resolved-dump-all-pause-locations-expected.txt 2022-02-02 21:53:20 UTC (rev 288995)
+++ trunk/LayoutTests/inspector/debugger/breakpoints/resolved-dump-all-pause-locations-expected.txt 2022-02-02 22:13:03 UTC (rev 288996)
@@ -2814,3 +2814,82 @@
4
+-- Running test case: Debugger.resolvedBreakpoint.dumpAllLocations.Multiline
+
+INSERTING AT: 0:0
+PAUSES AT: 1:4
+ -> 0 #function test() {
+ => 1 |var x;
+ 2 }
+ 3
+ 4 // Strings
+
+INSERTING AT: 1:5
+PAUSES AT: 2:0
+ 0 function test() {
+ -> 1 v#ar x;
+ => 2 |}
+ 3
+ 4 // Strings
+ 5 let multiline1 = "test\
+
+INSERTING AT: 2:1
+PAUSES AT: 5:0
+ 0 function test() {
+ 1 var x;
+ -> 2 }#
+ 3
+ 4 // Strings
+ => 5 |let multiline1 = "test\
+ 6 string", multiline2 = test();
+ 7
+ 8 // Template Strings
+
+INSERTING AT: 5:1
+PAUSES AT: 6:9
+ 2 }
+ 3
+ 4 // Strings
+ -> 5 l#et multiline1 = "test\
+ => 6 string", |multiline2 = test();
+ 7
+ 8 // Template Strings
+ 9 let multiline3 = `test
+
+INSERTING AT: 6:10
+PAUSES AT: 9:0
+ 3
+ 4 // Strings
+ 5 let multiline1 = "test\
+ -> 6 string", m#ultiline2 = test();
+ 7
+ 8 // Template Strings
+ => 9 |let multiline3 = `test
+ 10 string`, multiline4 = test();
+ 11
+ 12 // Comments
+
+INSERTING AT: 9:1
+PAUSES AT: 10:9
+ 6 string", multiline2 = test();
+ 7
+ 8 // Template Strings
+ -> 9 l#et multiline3 = `test
+ => 10 string`, |multiline4 = test();
+ 11
+ 12 // Comments
+ 13 /* test
+
+INSERTING AT: 10:10
+PAUSES AT: 14:11
+ 7
+ 8 // Template Strings
+ 9 let multiline3 = `test
+ -> 10 string`, m#ultiline4 = test();
+ 11
+ 12 // Comments
+ 13 /* test
+ => 14 comment */ |let multiline5 = test();
+ 15
+
+
Modified: trunk/LayoutTests/inspector/debugger/breakpoints/resolved-dump-all-pause-locations.html (288995 => 288996)
--- trunk/LayoutTests/inspector/debugger/breakpoints/resolved-dump-all-pause-locations.html 2022-02-02 21:53:20 UTC (rev 288995)
+++ trunk/LayoutTests/inspector/debugger/breakpoints/resolved-dump-all-pause-locations.html 2022-02-02 22:13:03 UTC (rev 288996)
@@ -8,6 +8,7 @@
<script src=""
<script src=""
<script src=""
+<script src=""
<script>
function test()
{
@@ -28,6 +29,11 @@
scriptRegex: /dump-unicode\.js$/,
});
+ window.addDumpAllPauseLocationsTestCase(suite, {
+ name: "Debugger.resolvedBreakpoint.dumpAllLocations.Multiline",
+ scriptRegex: /dump-multiline\.js$/,
+ });
+
suite.runTestCasesAndFinish();
}
</script>
Added: trunk/LayoutTests/inspector/debugger/breakpoints/resources/dump-multiline.js (0 => 288996)
--- trunk/LayoutTests/inspector/debugger/breakpoints/resources/dump-multiline.js (rev 0)
+++ trunk/LayoutTests/inspector/debugger/breakpoints/resources/dump-multiline.js 2022-02-02 22:13:03 UTC (rev 288996)
@@ -0,0 +1,15 @@
+function test() {
+ var x;
+}
+
+// Strings
+let multiline1 = "test\
+string", multiline2 = test();
+
+// Template Strings
+let multiline3 = `test
+string`, multiline4 = test();
+
+// Comments
+/* test
+comment */ let multiline5 = test();
Modified: trunk/Source/_javascript_Core/ChangeLog (288995 => 288996)
--- trunk/Source/_javascript_Core/ChangeLog 2022-02-02 21:53:20 UTC (rev 288995)
+++ trunk/Source/_javascript_Core/ChangeLog 2022-02-02 22:13:03 UTC (rev 288996)
@@ -1,3 +1,29 @@
+2022-02-02 Patrick Angle <pan...@apple.com>
+
+ No breakpoints hit on github.com, and some are invalid
+ https://bugs.webkit.org/show_bug.cgi?id=235607
+
+ Reviewed by Yusuke Suzuki.
+
+ Added test case in: inspector/debugger/breakpoints/resolved-dump-all-pause-locations.html
+
+ Previously not all line terminations resulted in setting the `m_lineStart` to the current m_code, which meant
+ that the location for pause-able locations and stack traces were inaccurate when they were on a line that
+ terminated multi-line comments, strings, or template strings. We now always update m_lineStart when shifting for
+ a line terminator, instead of only when the terminator appears outside a string or comment.
+
+ * debugger/Breakpoint.cpp:
+ (JSC::Breakpoint::resolve):
+ - The existing assertions were somewhat in conflict with each other. If we permit the line number to increase,
+ there is no guarantee that the column number will remain the same or increase, which can now more easily occur
+ with multi-line strings. Instead, we should make sure that the overall offset has increase.
+
+ * parser/Lexer.cpp:
+ (JSC::Lexer<T>::shiftLineTerminator):
+ (JSC::Lexer<T>::lexWithoutClearingLineTerminator):
+ (JSC::Lexer<T>::scanTemplateString):
+ - Make sure the token start offset and start line offset are correctly set for strings.
+
2022-02-02 Yusuke Suzuki <ysuz...@apple.com>
[JSC] YarrJIT m_checkedOffset should be pre-computed and stored to Yarr op
Modified: trunk/Source/_javascript_Core/debugger/Breakpoint.cpp (288995 => 288996)
--- trunk/Source/_javascript_Core/debugger/Breakpoint.cpp 2022-02-02 21:53:20 UTC (rev 288995)
+++ trunk/Source/_javascript_Core/debugger/Breakpoint.cpp 2022-02-02 22:13:03 UTC (rev 288996)
@@ -65,7 +65,7 @@
ASSERT(isLinked());
ASSERT(!isResolved());
ASSERT(lineNumber >= m_lineNumber);
- ASSERT(columnNumber >= m_columnNumber);
+ ASSERT(columnNumber >= m_columnNumber || lineNumber > m_lineNumber);
m_lineNumber = lineNumber;
m_columnNumber = columnNumber;
Modified: trunk/Source/_javascript_Core/parser/Lexer.cpp (288995 => 288996)
--- trunk/Source/_javascript_Core/parser/Lexer.cpp 2022-02-02 21:53:20 UTC (rev 288995)
+++ trunk/Source/_javascript_Core/parser/Lexer.cpp 2022-02-02 22:13:03 UTC (rev 288996)
@@ -712,6 +712,7 @@
shift();
++m_lineNumber;
+ m_lineStart = m_code;
}
template <typename T>
@@ -2472,6 +2473,8 @@
m_buffer8.shrink(0);
break;
case CharacterQuote: {
+ auto startLineNumber = m_lineNumber;
+ auto startLineStartOffset = currentLineStartOffset();
StringParseResult result = StringCannotBeParsed;
if (lexerFlags.contains(LexerFlags::DontBuildStrings))
result = parseString<false>(tokenData, strictMode);
@@ -2484,8 +2487,10 @@
}
shift();
token = STRING;
- break;
- }
+ m_atLineStart = false;
+ fillTokenInfo(tokenRecord, token, startLineNumber, currentOffset(), startLineStartOffset, currentPosition());
+ return token;
+ }
case CharacterIdentifierStart: {
if constexpr (ASSERT_ENABLED) {
UChar32 codePoint;
@@ -2506,7 +2511,6 @@
shiftLineTerminator();
m_atLineStart = true;
m_hasLineTerminatorBeforeToken = true;
- m_lineStart = m_code;
goto start;
case CharacterHash: {
// Hashbang is only permitted at the start of the source text.
@@ -2567,7 +2571,6 @@
shiftLineTerminator();
m_atLineStart = true;
m_hasLineTerminatorBeforeToken = true;
- m_lineStart = m_code;
if (!lastTokenWasRestrKeyword())
goto start;
@@ -2699,6 +2702,9 @@
ASSERT(!m_error);
ASSERT(m_buffer16.isEmpty());
+ int startingLineStartOffset = currentLineStartOffset();
+ int startingLineNumber = lineNumber();
+
// Leading backquote ` (for template head) or closing brace } (for template trailing) are already shifted in the previous token scan.
// So in this re-scan phase, shift() is not needed here.
StringParseResult result = parseTemplateLiteral(tokenData, rawStringsBuildMode);
@@ -2711,7 +2717,7 @@
// Since TemplateString always ends with ` or }, m_atLineStart always becomes false.
m_atLineStart = false;
- fillTokenInfo(tokenRecord, token, m_lineNumber, currentOffset(), currentLineStartOffset(), currentPosition());
+ fillTokenInfo(tokenRecord, token, startingLineNumber, currentOffset(), startingLineStartOffset, currentPosition());
return token;
}
