Skip to content

Commit 5b5021d

Browse files
patrickanglepatrickangle
committed
No breakpoints hit on github.com, and some are invalid
https://bugs.webkit.org/show_bug.cgi?id=235607 Reviewed by Yusuke Suzuki. Source/JavaScriptCore: 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. Canonical link: https://commits.webkit.org/246715@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@288996 268f45cc-cd09-0410-ab3c-d52691b4dbfc
1 parent 237db45 commit 5b5021d

File tree

7 files changed

+152
-6
lines changed

7 files changed

+152
-6
lines changed

‎LayoutTests/ChangeLog‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,17 @@
1+
2022-02-02 Patrick Angle <pangle@apple.com>
2+
3+
No breakpoints hit on github.com, and some are invalid
4+
https://bugs.webkit.org/show_bug.cgi?id=235607
5+
6+
Reviewed by Yusuke Suzuki.
7+
8+
Add test cases for resolving breakpoints on lines that begin with the end of multi-line strings, comments, and
9+
template strings.
10+
11+
* inspector/debugger/breakpoints/resolved-dump-all-pause-locations-expected.txt:
12+
* inspector/debugger/breakpoints/resolved-dump-all-pause-locations.html:
13+
* inspector/debugger/breakpoints/resources/dump-multiline.js: Added.
14+
115
2022-02-02 Patrick Griffis <pgriffis@igalia.com>
216

317
CSP: Fix returned WebAssembly error type when blocked

‎LayoutTests/inspector/debugger/breakpoints/resolved-dump-all-pause-locations-expected.txt‎

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2814,3 +2814,82 @@ PAUSES AT: 3:0
28142814
4
28152815

28162816

2817+
-- Running test case: Debugger.resolvedBreakpoint.dumpAllLocations.Multiline
2818+
2819+
INSERTING AT: 0:0
2820+
PAUSES AT: 1:4
2821+
-> 0 #function test() {
2822+
=> 1 |var x;
2823+
2 }
2824+
3
2825+
4 // Strings
2826+
2827+
INSERTING AT: 1:5
2828+
PAUSES AT: 2:0
2829+
0 function test() {
2830+
-> 1 v#ar x;
2831+
=> 2 |}
2832+
3
2833+
4 // Strings
2834+
5 let multiline1 = "test\
2835+
2836+
INSERTING AT: 2:1
2837+
PAUSES AT: 5:0
2838+
0 function test() {
2839+
1 var x;
2840+
-> 2 }#
2841+
3
2842+
4 // Strings
2843+
=> 5 |let multiline1 = "test\
2844+
6 string", multiline2 = test();
2845+
7
2846+
8 // Template Strings
2847+
2848+
INSERTING AT: 5:1
2849+
PAUSES AT: 6:9
2850+
2 }
2851+
3
2852+
4 // Strings
2853+
-> 5 l#et multiline1 = "test\
2854+
=> 6 string", |multiline2 = test();
2855+
7
2856+
8 // Template Strings
2857+
9 let multiline3 = `test
2858+
2859+
INSERTING AT: 6:10
2860+
PAUSES AT: 9:0
2861+
3
2862+
4 // Strings
2863+
5 let multiline1 = "test\
2864+
-> 6 string", m#ultiline2 = test();
2865+
7
2866+
8 // Template Strings
2867+
=> 9 |let multiline3 = `test
2868+
10 string`, multiline4 = test();
2869+
11
2870+
12 // Comments
2871+
2872+
INSERTING AT: 9:1
2873+
PAUSES AT: 10:9
2874+
6 string", multiline2 = test();
2875+
7
2876+
8 // Template Strings
2877+
-> 9 l#et multiline3 = `test
2878+
=> 10 string`, |multiline4 = test();
2879+
11
2880+
12 // Comments
2881+
13 /* test
2882+
2883+
INSERTING AT: 10:10
2884+
PAUSES AT: 14:11
2885+
7
2886+
8 // Template Strings
2887+
9 let multiline3 = `test
2888+
-> 10 string`, m#ultiline4 = test();
2889+
11
2890+
12 // Comments
2891+
13 /* test
2892+
=> 14 comment */ |let multiline5 = test();
2893+
15
2894+
2895+

‎LayoutTests/inspector/debugger/breakpoints/resolved-dump-all-pause-locations.html‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
<script src="resources/dump-general.js"></script>
99
<script src="resources/dump-functions.js"></script>
1010
<script src="resources/dump-unicode.js"></script>
11+
<script src="resources/dump-multiline.js"></script>
1112
<script>
1213
function test()
1314
{
@@ -28,6 +29,11 @@
2829
scriptRegex: /dump-unicode\.js$/,
2930
});
3031

32+
window.addDumpAllPauseLocationsTestCase(suite, {
33+
name: "Debugger.resolvedBreakpoint.dumpAllLocations.Multiline",
34+
scriptRegex: /dump-multiline\.js$/,
35+
});
36+
3137
suite.runTestCasesAndFinish();
3238
}
3339
</script>

‎LayoutTests/inspector/debugger/breakpoints/resources/dump-multiline.js‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
function test() {
2+
var x;
3+
}
4+
5+
// Strings
6+
let multiline1 = "test\
7+
string", multiline2 = test();
8+
9+
// Template Strings
10+
let multiline3 = `test
11+
string`, multiline4 = test();
12+
13+
// Comments
14+
/* test
15+
comment */ let multiline5 = test();

‎Source/JavaScriptCore/ChangeLog‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,29 @@
1+
2022-02-02 Patrick Angle <pangle@apple.com>
2+
3+
No breakpoints hit on github.com, and some are invalid
4+
https://bugs.webkit.org/show_bug.cgi?id=235607
5+
6+
Reviewed by Yusuke Suzuki.
7+
8+
Added test case in: inspector/debugger/breakpoints/resolved-dump-all-pause-locations.html
9+
10+
Previously not all line terminations resulted in setting the `m_lineStart` to the current m_code, which meant
11+
that the location for pause-able locations and stack traces were inaccurate when they were on a line that
12+
terminated multi-line comments, strings, or template strings. We now always update m_lineStart when shifting for
13+
a line terminator, instead of only when the terminator appears outside a string or comment.
14+
15+
* debugger/Breakpoint.cpp:
16+
(JSC::Breakpoint::resolve):
17+
- The existing assertions were somewhat in conflict with each other. If we permit the line number to increase,
18+
there is no guarantee that the column number will remain the same or increase, which can now more easily occur
19+
with multi-line strings. Instead, we should make sure that the overall offset has increase.
20+
21+
* parser/Lexer.cpp:
22+
(JSC::Lexer<T>::shiftLineTerminator):
23+
(JSC::Lexer<T>::lexWithoutClearingLineTerminator):
24+
(JSC::Lexer<T>::scanTemplateString):
25+
- Make sure the token start offset and start line offset are correctly set for strings.
26+
127
2022-02-02 Yusuke Suzuki <ysuzuki@apple.com>
228

329
[JSC] YarrJIT m_checkedOffset should be pre-computed and stored to Yarr op

‎Source/JavaScriptCore/debugger/Breakpoint.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ bool Breakpoint::resolve(unsigned lineNumber, unsigned columnNumber)
6565
ASSERT(isLinked());
6666
ASSERT(!isResolved());
6767
ASSERT(lineNumber >= m_lineNumber);
68-
ASSERT(columnNumber >= m_columnNumber);
68+
ASSERT(columnNumber >= m_columnNumber || lineNumber > m_lineNumber);
6969

7070
m_lineNumber = lineNumber;
7171
m_columnNumber = columnNumber;

‎Source/JavaScriptCore/parser/Lexer.cpp‎

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -712,6 +712,7 @@ void Lexer<T>::shiftLineTerminator()
712712
shift();
713713

714714
++m_lineNumber;
715+
m_lineStart = m_code;
715716
}
716717

717718
template <typename T>
@@ -2472,6 +2473,8 @@ JSTokenType Lexer<T>::lexWithoutClearingLineTerminator(JSToken* tokenRecord, Opt
24722473
m_buffer8.shrink(0);
24732474
break;
24742475
case CharacterQuote: {
2476+
auto startLineNumber = m_lineNumber;
2477+
auto startLineStartOffset = currentLineStartOffset();
24752478
StringParseResult result = StringCannotBeParsed;
24762479
if (lexerFlags.contains(LexerFlags::DontBuildStrings))
24772480
result = parseString<false>(tokenData, strictMode);
@@ -2484,8 +2487,10 @@ JSTokenType Lexer<T>::lexWithoutClearingLineTerminator(JSToken* tokenRecord, Opt
24842487
}
24852488
shift();
24862489
token = STRING;
2487-
break;
2488-
}
2490+
m_atLineStart = false;
2491+
fillTokenInfo(tokenRecord, token, startLineNumber, currentOffset(), startLineStartOffset, currentPosition());
2492+
return token;
2493+
}
24892494
case CharacterIdentifierStart: {
24902495
if constexpr (ASSERT_ENABLED) {
24912496
UChar32 codePoint;
@@ -2506,7 +2511,6 @@ JSTokenType Lexer<T>::lexWithoutClearingLineTerminator(JSToken* tokenRecord, Opt
25062511
shiftLineTerminator();
25072512
m_atLineStart = true;
25082513
m_hasLineTerminatorBeforeToken = true;
2509-
m_lineStart = m_code;
25102514
goto start;
25112515
case CharacterHash: {
25122516
// Hashbang is only permitted at the start of the source text.
@@ -2567,7 +2571,6 @@ JSTokenType Lexer<T>::lexWithoutClearingLineTerminator(JSToken* tokenRecord, Opt
25672571
shiftLineTerminator();
25682572
m_atLineStart = true;
25692573
m_hasLineTerminatorBeforeToken = true;
2570-
m_lineStart = m_code;
25712574
if (!lastTokenWasRestrKeyword())
25722575
goto start;
25732576

@@ -2699,6 +2702,9 @@ JSTokenType Lexer<T>::scanTemplateString(JSToken* tokenRecord, RawStringsBuildMo
26992702
ASSERT(!m_error);
27002703
ASSERT(m_buffer16.isEmpty());
27012704

2705+
int startingLineStartOffset = currentLineStartOffset();
2706+
int startingLineNumber = lineNumber();
2707+
27022708
// Leading backquote ` (for template head) or closing brace } (for template trailing) are already shifted in the previous token scan.
27032709
// So in this re-scan phase, shift() is not needed here.
27042710
StringParseResult result = parseTemplateLiteral(tokenData, rawStringsBuildMode);
@@ -2711,7 +2717,7 @@ JSTokenType Lexer<T>::scanTemplateString(JSToken* tokenRecord, RawStringsBuildMo
27112717

27122718
// Since TemplateString always ends with ` or }, m_atLineStart always becomes false.
27132719
m_atLineStart = false;
2714-
fillTokenInfo(tokenRecord, token, m_lineNumber, currentOffset(), currentLineStartOffset(), currentPosition());
2720+
fillTokenInfo(tokenRecord, token, startingLineNumber, currentOffset(), startingLineStartOffset, currentPosition());
27152721
return token;
27162722
}
27172723

0 commit comments

Comments
 (0)