Bug 173053 – \n\r is not the same as \r\n

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Peter van der Zee]

What steps will reproduce the problem?
1. eval("'\\\n\r'")

What is the expected output?

Uncaught SyntaxError: Invalid or unexpected token

What do you see instead?

A parser bug! :)

Description:

The spec explicitly treats `\r\n` as a single newline token rather than two but individual `\n` and `\r` characters cause individual newline tokens. When back-to-back `\n\r` causes two tokens. This is normally not very relevant, only to the point of line number reporting, but there's an edge case concerning so called "newline escapes" in single/double strings; Escaping a newline there takes this distinction into account and `'\\\r\n'` should be fine while `'\\\n\r'` should trigger an error on an unescaped `\r`.

See also https://bugs.chromium.org/p/v8/issues/detail?id=6401

[Peter van der Zee]

r/newline escapes/line continuations

[Mark Lam]

Seems like a simple thing to fix.  I'll take a look.

[Mark Lam]

Created attachment 312213 [details]
proposed patch.

Let's try this on the EWS first.

[Build Bot]

Comment on attachment 312213 [details]
proposed patch.

Attachment 312213 [details] did not pass jsc-ews (mac):
Output: http://webkit-queues.webkit.org/results/3889531

New failing tests:
stress/template-literal-line-terminators.js.ftl-eager
stress/template-literal-line-terminators.js.no-cjit-validate-phases
jsc-layout-tests.yaml/js/script-tests/parse-backslash-before-newline.js.layout-dfg-eager-no-cjit
stress/template-literal-line-terminators.js.ftl-no-cjit-no-inline-validate
stress/template-literal-line-terminators.js.ftl-eager-no-cjit
stress/template-literal-line-terminators.js.ftl-no-cjit-b3o1
stress/template-literal-line-terminators.js.default
jsc-layout-tests.yaml/js/script-tests/parse-backslash-before-newline.js.layout-ftl-no-cjit
stress/template-literal-line-terminators.js.no-cjit-collect-continuously
stress/template-literal-line-terminators.js.ftl-eager-no-cjit-b3o1
stress/template-literal-line-terminators.js.ftl-no-cjit-no-put-stack-validate
jsc-layout-tests.yaml/js/script-tests/parse-backslash-before-newline.js.layout-no-ftl
stress/template-literal-line-terminators.js.dfg-eager-no-cjit-validate
jsc-layout-tests.yaml/js/script-tests/parse-backslash-before-newline.js.layout-ftl-eager-no-cjit
stress/template-literal-line-terminators.js.no-llint
stress/template-literal-line-terminators.js.ftl-no-cjit-validate-sampling-profiler
stress/template-literal-line-terminators.js.dfg-maximal-flush-validate-no-cjit
jsc-layout-tests.yaml/js/script-tests/parse-backslash-before-newline.js.layout-no-cjit
jsc-layout-tests.yaml/js/script-tests/parse-backslash-before-newline.js.layout
jsc-layout-tests.yaml/js/script-tests/parse-backslash-before-newline.js.layout-no-llint
stress/template-literal-line-terminators.js.no-ftl
stress/template-literal-line-terminators.js.dfg-eager
stress/template-literal-line-terminators.js.ftl-no-cjit-small-pool

[Build Bot]

Comment on attachment 312213 [details]
proposed patch.

Attachment 312213 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3889656

New failing tests:
js/parse-backslash-before-newline.html

[Build Bot]

Created attachment 312222 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 312213 [details]
proposed patch.

Attachment 312213 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3889688

New failing tests:
js/parse-backslash-before-newline.html

[Build Bot]

Created attachment 312223 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 312213 [details]
proposed patch.

Attachment 312213 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3889691

New failing tests:
js/parse-backslash-before-newline.html

[Build Bot]

Created attachment 312226 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 312213 [details]
proposed patch.

Attachment 312213 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3889701

New failing tests:
js/parse-backslash-before-newline.html

[Build Bot]

Created attachment 312229 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[Mark Lam]

Created attachment 312570 [details]
proposed patch w/ updated test baselines.

[Mark Lam]

Comment on attachment 312570 [details]
proposed patch w/ updated test baselines.

The patch applies properly: I tested applying it locally on another checkout.  The EWS patching system just does not like the \r in the patch.  Unfortunately, the nature of this fix is in the handling of \n\r.  So, I can't remove the use of \r in the test case.

Let's get the patch reviewed.

[Yusuke Suzuki]

Comment on attachment 312570 [details]
proposed patch w/ updated test baselines.

View in context: https://bugs.webkit.org/attachment.cgi?id=312570&action=review

> Source/JavaScriptCore/parser/Lexer.cpp:708
>      // Allow both CRLF and LFCR.

Let's change this comment.

> Source/JavaScriptCore/parser/Lexer.cpp:1416
>              ++m_lineNumber;

I think now this LineNumberAdder is not necessary. This is added to increnting one line number with <LF><CR> while normalizing them as <LF><LF>.
However, now, our Lexer does not accept `<LF><CR>` as one line terminator. Thus, I think we can discard this class!

[Mark Lam]

(In reply to Yusuke Suzuki from comment #15)
> Comment on attachment 312570 [details]
> proposed patch w/ updated test baselines.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=312570&action=review
> 
> > Source/JavaScriptCore/parser/Lexer.cpp:708
> >      // Allow both CRLF and LFCR.
> 
> Let's change this comment.

I deleted the comment.  I think the code now says what is allowed clearly enough.

> > Source/JavaScriptCore/parser/Lexer.cpp:1416
> >              ++m_lineNumber;
> 
> I think now this LineNumberAdder is not necessary. This is added to
> increnting one line number with <LF><CR> while normalizing them as <LF><LF>.
> However, now, our Lexer does not accept `<LF><CR>` as one line terminator.
> Thus, I think we can discard this class!

Thanks.  Fixed.

[Yusuke Suzuki]

How about this patch?

[Mark Lam]

(In reply to Yusuke Suzuki from comment #17)
> How about this patch?

I'm still working thru some test failures.  This is a low priority task for me, but I'm trying to wrap it up.

[Mark Lam]

Created attachment 314859 [details]
proposed patch.

This patch has passed the JSC and layout tests on my local runs.

[Keith Miller]

Comment on attachment 314859 [details]
proposed patch.

r=me.

[Mark Lam]

Thanks for the review.  Landed in r219263: <http://trac.webkit.org/r219263>.