Bug 191677 – [JSC] Implement "well-formed JSON.stringify" proposal

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC] Implement "well-formed JSON.stringify" proposal

[Yusuke Suzuki]

Created attachment 354906 [details]
Patch

[Yusuke Suzuki]

Created attachment 354907 [details]
Patch

[EWS Watchlist]

Comment on attachment 354907 [details]
Patch

Attachment 354907 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10001803

New failing tests:
js/dom/JSON-stringify.html
js/dom/webidl-type-mapping.html
css3/escape-dom-api.html

[EWS Watchlist]

Created attachment 354910 [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

[EWS Watchlist]

Comment on attachment 354907 [details]
Patch

Attachment 354907 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10001810

New failing tests:
js/dom/JSON-stringify.html
js/dom/webidl-type-mapping.html
css3/escape-dom-api.html

[EWS Watchlist]

Created attachment 354911 [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

[EWS Watchlist]

Comment on attachment 354907 [details]
Patch

Attachment 354907 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10002068

New failing tests:
js/dom/JSON-stringify.html
js/dom/webidl-type-mapping.html
css3/escape-dom-api.html

[EWS Watchlist]

Created attachment 354913 [details]
Archive of layout-test-results from ews114 for mac-sierra

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

[EWS Watchlist]

Comment on attachment 354907 [details]
Patch

Attachment 354907 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10002075

New failing tests:
js/dom/JSON-stringify.html
js/dom/webidl-type-mapping.html
css3/escape-dom-api.html

[EWS Watchlist]

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

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

[Yusuke Suzuki]

Created attachment 355026 [details]
Patch

[EWS Watchlist]

Comment on attachment 355026 [details]
Patch

Attachment 355026 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10014029

New failing tests:
js/dom/JSON-stringify.html
js/dom/webidl-type-mapping.html
js/dom/JSON-parse.html

[EWS Watchlist]

Created attachment 355030 [details]
Archive of layout-test-results from ews102 for mac-sierra

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

[EWS Watchlist]

Comment on attachment 355026 [details]
Patch

Attachment 355026 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10014035

New failing tests:
js/dom/JSON-stringify.html
js/dom/webidl-type-mapping.html
js/dom/JSON-parse.html

[EWS Watchlist]

Created attachment 355032 [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

[EWS Watchlist]

Comment on attachment 355026 [details]
Patch

Attachment 355026 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10014105

New failing tests:
js/dom/JSON-stringify.html
js/dom/webidl-type-mapping.html

[EWS Watchlist]

Created attachment 355033 [details]
Archive of layout-test-results from ews112 for mac-sierra

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

[EWS Watchlist]

Comment on attachment 355026 [details]
Patch

Attachment 355026 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10014133

New failing tests:
js/dom/JSON-stringify.html
js/dom/webidl-type-mapping.html
js/dom/JSON-parse.html

[EWS Watchlist]

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

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

[EWS Watchlist]

Comment on attachment 355026 [details]
Patch

Attachment 355026 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10014819

New failing tests:
js/dom/JSON-stringify.html
js/dom/webidl-type-mapping.html
js/dom/JSON-parse.html

[EWS Watchlist]

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

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

[Yusuke Suzuki]

Created attachment 355191 [details]
Patch

[Yusuke Suzuki]

Created attachment 355192 [details]
Patch

[Yusuke Suzuki]

Created attachment 355194 [details]
Patch

[Yusuke Suzuki]

Ping?

[Yusuke Suzuki]

Ping?

[Ross Kirsling]

Looks like there's a test262 test for this too, but it's newer than what's currently in the WebKit repo:
https://github.com/tc39/test262/blob/master/test/built-ins/JSON/stringify/string-escape-unicode.js

[Yusuke Suzuki]

(In reply to Ross Kirsling from comment #27)
> Looks like there's a test262 test for this too, but it's newer than what's
> currently in the WebKit repo:
> https://github.com/tc39/test262/blob/master/test/built-ins/JSON/stringify/
> string-escape-unicode.js

Yes, and this patch passes this test.

[Yusuke Suzuki]

Ping?

[Yusuke Suzuki]

Ping?

[Ross Kirsling]

Informal r+.

This seems quite straightforward (and even matches closely with the SpiderMonkey implementation).

[Yusuke Suzuki]

Ping review?

[Yusuke Suzuki]

(In reply to Ross Kirsling from comment #31)
> Informal r+.
> 
> This seems quite straightforward (and even matches closely with the
> SpiderMonkey implementation).

Thanks ;)

[Yusuke Suzuki]

Ping?

[Darin Adler]

Comment on attachment 355194 [details]
Patch

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

> Source/WTF/wtf/text/StringBuilderJSON.cpp:85
> +            uint8_t upper = static_cast<uint32_t>(character) >> 8;

I don’t understand what value the static_cast<uint32_t> has here. It seems the code would do the right thing without that.

> Source/WTF/wtf/text/StringBuilderJSON.cpp:86
> +            uint8_t lower = static_cast<uint8_t>(character);

I don’t understand what value the static_cast<uint8_t> has here. It seems the code would do the right thing without that.

[Darin Adler]

Comment on attachment 355194 [details]
Patch

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

> Source/WTF/wtf/text/StringBuilderJSON.cpp:84
> +        if (U16_IS_SURROGATE_TRAIL(character) || next == end || !U16_IS_SURROGATE_TRAIL(*next)) {

Oops, this is wrong! Details to come.

[Darin Adler]

Comment on attachment 355194 [details]
Patch

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

>> Source/WTF/wtf/text/StringBuilderJSON.cpp:84
>> +        if (U16_IS_SURROGATE_TRAIL(character) || next == end || !U16_IS_SURROGATE_TRAIL(*next)) {
> 
> Oops, this is wrong! Details to come.

This needs to use U16_IS_TRAIL, not U16_IS_SURROGATE_TRAIL, on *next. Test cases would be string where the last two characters are:

1) A surrogate head.
2) A non-surrogate character with the bit 0x400 set.

That case would work incorrectly.

I also think I would write it more like this to make the logic clearer:

    bool isValidSurrogatePair = U16_IS_SURROGATE_HEAD(character) && next != end && U16_IS_TRAIL(*next);
    if (!isValidSurrogatePair)

[Yusuke Suzuki]

(In reply to Darin Adler from comment #37)
> Comment on attachment 355194 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=355194&action=review
> 
> >> Source/WTF/wtf/text/StringBuilderJSON.cpp:84
> >> +        if (U16_IS_SURROGATE_TRAIL(character) || next == end || !U16_IS_SURROGATE_TRAIL(*next)) {
> > 
> > Oops, this is wrong! Details to come.
> 
> This needs to use U16_IS_TRAIL, not U16_IS_SURROGATE_TRAIL, on *next. Test
> cases would be string where the last two characters are:
> 
> 1) A surrogate head.
> 2) A non-surrogate character with the bit 0x400 set.
> 
> That case would work incorrectly.
> 
> I also think I would write it more like this to make the logic clearer:
> 
>     bool isValidSurrogatePair = U16_IS_SURROGATE_HEAD(character) && next !=
> end && U16_IS_TRAIL(*next);
>     if (!isValidSurrogatePair)

Oops, super nice catch! Right. I'll fix this and update the patch soon.

[Yusuke Suzuki]

Created attachment 357828 [details]
Patch

[Yusuke Suzuki]

Committed r239537: <https://trac.webkit.org/changeset/239537>

[Radar WebKit Bug Importer]

<rdar://problem/46916301>