Bug 182232

Summary: [JSC] Relax line terminators in String to make JSON subset of JS
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: New BugsAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, keith_miller, mark.lam, msaboff, rniwa, saam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews101 for mac-sierra
none
Archive of layout-test-results from ews107 for mac-sierra-wk2
none
Patch
none
Archive of layout-test-results from ews103 for mac-sierra
none
Archive of layout-test-results from ews104 for mac-sierra-wk2
none
Patch keith_miller: review+

Description Yusuke Suzuki 2018-01-29 00:22:04 PST
[JSC] Relax line terminators in String to make JSON subset of JS
Comment 1 Yusuke Suzuki 2018-01-29 00:25:49 PST
Created attachment 332515 [details]
Patch
Comment 2 EWS Watchlist 2018-01-29 01:25:47 PST
Comment on attachment 332515 [details]
Patch

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

New failing tests:
sputnik/Conformance/07_Lexical_Conventions/7.3_Line_Terminators/S7.3_A2.3.html
sputnik/Conformance/07_Lexical_Conventions/7.3_Line_Terminators/S7.3_A2.4.html
Comment 3 EWS Watchlist 2018-01-29 01:25:48 PST
Created attachment 332521 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 4 EWS Watchlist 2018-01-29 01:30:23 PST
Comment on attachment 332515 [details]
Patch

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

New failing tests:
sputnik/Conformance/07_Lexical_Conventions/7.3_Line_Terminators/S7.3_A2.3.html
sputnik/Conformance/07_Lexical_Conventions/7.3_Line_Terminators/S7.3_A2.4.html
Comment 5 EWS Watchlist 2018-01-29 01:30:24 PST
Created attachment 332523 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 6 Yusuke Suzuki 2018-01-29 01:37:28 PST
Created attachment 332525 [details]
Patch
Comment 7 EWS Watchlist 2018-01-29 02:36:09 PST
Comment on attachment 332525 [details]
Patch

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

New failing tests:
sputnik/Conformance/07_Lexical_Conventions/7.3_Line_Terminators/S7.3_A2.3.html
Comment 8 EWS Watchlist 2018-01-29 02:36:10 PST
Created attachment 332530 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 9 EWS Watchlist 2018-01-29 02:41:41 PST
Comment on attachment 332525 [details]
Patch

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

New failing tests:
sputnik/Conformance/07_Lexical_Conventions/7.3_Line_Terminators/S7.3_A2.3.html
Comment 10 EWS Watchlist 2018-01-29 02:41:42 PST
Created attachment 332531 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 11 Yusuke Suzuki 2018-01-29 02:48:31 PST
Created attachment 332532 [details]
Patch
Comment 12 Keith Miller 2018-01-29 10:33:17 PST
Comment on attachment 332532 [details]
Patch

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

r=me with a nit.

> Source/JavaScriptCore/parser/Lexer.cpp:1361
> -            if (atEnd() || isLineTerminator(m_current)) {
> +            if (atEnd() || m_current == '\r' || m_current == '\n') {

Why'd you make this change? Isn't isLineTerminator() always equivalent given the above check (I think LChar/UChar are always unsigned)?

It might also we worth adding:

static_assert(std::is_unsigned<T>::value, "Lexer expects an unsigned character type"); 

somewhere.
Comment 13 Yusuke Suzuki 2018-01-29 23:32:49 PST
Comment on attachment 332532 [details]
Patch

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

Thanks

>> Source/JavaScriptCore/parser/Lexer.cpp:1361
>> +            if (atEnd() || m_current == '\r' || m_current == '\n') {
> 
> Why'd you make this change? Isn't isLineTerminator() always equivalent given the above check (I think LChar/UChar are always unsigned)?
> 
> It might also we worth adding:
> 
> static_assert(std::is_unsigned<T>::value, "Lexer expects an unsigned character type"); 
> 
> somewhere.

isLineTerminator is different. It returns true for \u2028 and \u2029. The spec change allows \u2028 and \u2029 inside string.
I've added static_assert.
Comment 14 Yusuke Suzuki 2018-01-29 23:34:49 PST
Committed r227775: <https://trac.webkit.org/changeset/227775>
Comment 15 Radar WebKit Bug Importer 2018-01-29 23:35:36 PST
<rdar://problem/37018683>
Comment 16 Yusuke Suzuki 2018-01-30 04:55:22 PST
Committed r227780: <https://trac.webkit.org/changeset/227780>
Comment 17 Keith Miller 2018-01-30 10:10:57 PST
Comment on attachment 332532 [details]
Patch

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

>>> Source/JavaScriptCore/parser/Lexer.cpp:1361
>>> +            if (atEnd() || m_current == '\r' || m_current == '\n') {
>> 
>> Why'd you make this change? Isn't isLineTerminator() always equivalent given the above check (I think LChar/UChar are always unsigned)?
>> 
>> It might also we worth adding:
>> 
>> static_assert(std::is_unsigned<T>::value, "Lexer expects an unsigned character type"); 
>> 
>> somewhere.
> 
> isLineTerminator is different. It returns true for \u2028 and \u2029. The spec change allows \u2028 and \u2029 inside string.
> I've added static_assert.

Right but the call to isLineTerminator is gated by the if (m_current < 0xE). So \u2028 and \u2029 wouldn't make it do the call. Maybe I'm missing something though?
Comment 18 Saam Barati 2018-01-30 14:25:40 PST
(In reply to Keith Miller from comment #17)
> Comment on attachment 332532 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=332532&action=review
> 
> >>> Source/JavaScriptCore/parser/Lexer.cpp:1361
> >>> +            if (atEnd() || m_current == '\r' || m_current == '\n') {
> >> 
> >> Why'd you make this change? Isn't isLineTerminator() always equivalent given the above check (I think LChar/UChar are always unsigned)?
> >> 
> >> It might also we worth adding:
> >> 
> >> static_assert(std::is_unsigned<T>::value, "Lexer expects an unsigned character type"); 
> >> 
> >> somewhere.
> > 
> > isLineTerminator is different. It returns true for \u2028 and \u2029. The spec change allows \u2028 and \u2029 inside string.
> > I've added static_assert.
> 
> Right but the call to isLineTerminator is gated by the if (m_current < 0xE).
> So \u2028 and \u2029 wouldn't make it do the call. Maybe I'm missing
> something though?

You're not missing anything. But I agree with Yusuke that it'd be a bit confusing to call that function in this context. (I guess unless we added some kind of assert stating it's definitely not those two unicode values)