⚠ Archived content — this site is no longer maintained.   Current WebKit documentation is at docs.webkit.org.

Changeset 291746 in webkit


Ignore:
Timestamp:
Mar 23, 2022, 9:40:09 AM (5 years ago)
Author:
Patrick Angle
Message:

No breakpoints hit on github.com, and some are invalid
​https://bugs.webkit.org/show_bug.cgi?id=235607

Reviewed by Yusuke Suzuki.

JSTests:

Add test for multi-line parsing errors.

  • stress/regress-88440831.js: Added.

Source/JavaScriptCore:

New test: JSTests/stress/regress-88440831.js
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 increased.

  • parser/Lexer.cpp:

(JSC::Lexer<T>::shiftLineTerminator):
(JSC::Lexer<T>::lexWithoutClearingLineTerminator):
(JSC::Lexer<T>::scanTemplateString):

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.

(test):

Location:
trunk
Files:
2 added
7 edited

Legend:

Unmodified
Added
Removed

  • trunk/JSTests/ChangeLog

    r291736 r291746  
     12022-03-23  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 for multi-line parsing errors.
     9
     10        * stress/regress-88440831.js: Added.
     11
    1122022-03-22  Yusuke Suzuki  <ysuzuki@apple.com>
    213

  • trunk/LayoutTests/ChangeLog

    r291742 r291746  
     12022-03-23  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        (test):
     15
    1162022-03-23  Ziran Sun  <zsun@igalia.com>
    217

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

    r289112 r291746  
    28152815
    28162816
     2817-- Running test case: Debugger.resolvedBreakpoint.dumpAllLocations.Multiline
     2818
     2819INSERTING AT: 0:0
     2820PAUSES AT: 1:4
     2821 ->   0    #function test() {
     2822 =>   1        |var x;
     2823      2    }
     2824      3
     2825      4    // Strings
     2826
     2827INSERTING AT: 1:5
     2828PAUSES 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
     2836INSERTING AT: 2:1
     2837PAUSES 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
     2848INSERTING AT: 5:1
     2849PAUSES 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
     2859INSERTING AT: 6:10
     2860PAUSES 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
     2872INSERTING AT: 9:1
     2873PAUSES 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
     2883INSERTING AT: 10:10
     2884PAUSES 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

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

    r289112 r291746  
    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>
    1213function test()
    … …  
    2930    });
    3031
     32    window.addDumpAllPauseLocationsTestCase(suite, {
     33        name: "Debugger.resolvedBreakpoint.dumpAllLocations.Multiline",
     34        scriptRegex: /dump-multiline\.js$/,
     35    });
     36
    3137    suite.runTestCasesAndFinish();
    3238}

  • trunk/Source/JavaScriptCore/ChangeLog

    r291745 r291746  
     12022-03-23  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        New test: JSTests/stress/regress-88440831.js
     9        Added test case in: inspector/debugger/breakpoints/resolved-dump-all-pause-locations.html
     10
     11        Previously not all line terminations resulted in setting the `m_lineStart` to the current m_code, which meant
     12        that the location for pause-able locations and stack traces were inaccurate when they were on a line that
     13        terminated multi-line comments, strings, or template strings. We now always update m_lineStart when shifting for
     14        a line terminator, instead of only when the terminator appears outside a string or comment.
     15
     16        * debugger/Breakpoint.cpp:
     17        (JSC::Breakpoint::resolve):
     18        - The existing assertions were somewhat in conflict with each other. If we permit the line number to increase,
     19        there is no guarantee that the column number will remain the same or increase, which can now more easily occur
     20        with multi-line strings. Instead, we should make sure that the overall offset has increased.
     21
     22        * parser/Lexer.cpp:
     23        (JSC::Lexer<T>::shiftLineTerminator):
     24        (JSC::Lexer<T>::lexWithoutClearingLineTerminator):
     25        (JSC::Lexer<T>::scanTemplateString):
     26
    1272022-03-23  Xan Lopez  <xan@igalia.com>
    228

  • trunk/Source/JavaScriptCore/debugger/Breakpoint.cpp

    r289112 r291746  
    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;

  • trunk/Source/JavaScriptCore/parser/Lexer.cpp

    r289112 r291746  
    713713
    714714    ++m_lineNumber;
     715    m_lineStart = m_code;
    715716}
    716717
    … …  
    21002101        if (m_current == '*') {
    21012102            shift();
     2103            auto startLineNumber = m_lineNumber;
     2104            auto startLineStartOffset = currentLineStartOffset();
    21022105            if (parseMultilineComment())
    21032106                goto start;
    21042107            m_lexErrorMessage = "Multiline comment was not closed properly"_s;
    21052108            token = UNTERMINATED_MULTILINE_COMMENT_ERRORTOK;
    2106             goto returnError;
     2109            m_error = true;
     2110            fillTokenInfo(tokenRecord, token, startLineNumber, currentOffset(), startLineStartOffset, currentPosition());
     2111            return token;
    21072112        }
    21082113        if (m_current == '=') {
    … …  
    24732478        break;
    24742479    case CharacterQuote: {
     2480        auto startLineNumber = m_lineNumber;
     2481        auto startLineStartOffset = currentLineStartOffset();
    24752482        StringParseResult result = StringCannotBeParsed;
    24762483        if (lexerFlags.contains(LexerFlags::DontBuildStrings))
    … …  
    24812488        if (UNLIKELY(result != StringParsedSuccessfully)) {
    24822489            token = result == StringUnterminated ? UNTERMINATED_STRING_LITERAL_ERRORTOK : INVALID_STRING_LITERAL_ERRORTOK;
    2483             goto returnError;
     2490            m_error = true;
     2491            fillTokenInfo(tokenRecord, token, startLineNumber, currentOffset(), startLineStartOffset, currentPosition());
     2492            return token;
    24842493        }
    24852494        shift();
    24862495        token = STRING;
    2487         break;
    2488         }
     2496        m_atLineStart = false;
     2497        fillTokenInfo(tokenRecord, token, startLineNumber, currentOffset(), startLineStartOffset, currentPosition());
     2498        return token;
     2499    }
    24892500    case CharacterIdentifierStart: {
    24902501        if constexpr (ASSERT_ENABLED) {
    … …  
    25072518        m_atLineStart = true;
    25082519        m_hasLineTerminatorBeforeToken = true;
    2509         m_lineStart = m_code;
    25102520        goto start;
    25112521    case CharacterHash: {
    … …  
    25682578        m_atLineStart = true;
    25692579        m_hasLineTerminatorBeforeToken = true;
    2570         m_lineStart = m_code;
    25712580        if (!lastTokenWasRestrKeyword())
    25722581            goto start;
    … …  
    27002709    ASSERT(m_buffer16.isEmpty());
    27012710
     2711    int startingLineStartOffset = currentLineStartOffset();
     2712    int startingLineNumber = lineNumber();
     2713
    27022714    // Leading backquote ` (for template head) or closing brace } (for template trailing) are already shifted in the previous token scan.
    27032715    // So in this re-scan phase, shift() is not needed here.
    … …  
    27122724    // Since TemplateString always ends with ` or }, m_atLineStart always becomes false.
    27132725    m_atLineStart = false;
    2714     fillTokenInfo(tokenRecord, token, m_lineNumber, currentOffset(), currentLineStartOffset(), currentPosition());
     2726    fillTokenInfo(tokenRecord, token, startingLineNumber, currentOffset(), startingLineStartOffset, currentPosition());
    27152727    return token;
    27162728}
Note: See TracChangeset for help on using the changeset viewer.