Bug 170442 - Safari 10.1 JSON.parse returns incorrect object for numeric keys with decimal values
Summary: Safari 10.1 JSON.parse returns incorrect object for numeric keys with decimal...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: Safari 10
Hardware: Macintosh macOS 10.12
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-04-04 01:49 PDT by Martin Wiesinger
Modified: 2017-04-26 11:49 PDT (History)
8 users (show)

See Also:


Attachments
Patch (2.84 KB, patch)
2017-04-14 00:19 PDT, GSkachkov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Wiesinger 2017-04-04 01:49:25 PDT
Additional invalid key/value pairs are added if the object contains numeric keys with a decimal number for the first two keys whose 1st decimal place isn't 0.

JSON.parse('{"0":97.1,"1000":96.5,"2000":96.1,"3000":97.4,"4000":90.4}')
> {0: 97.1, 1: NaN, 2: NaN, 1000: 96.5, 2000: 96.1, 3000: 97.4, 4000: 90.4}
If these are parseable to integers, this doesn't happen.
JSON.parse('{"0":97.0,"1000":96.0,"2000":96.1,"3000":97.4,"4000":90.4}')
> {0: 97, 1000: 96, 2000: 96.1, 3000: 97.4, 4000: 90.4}
Nor does the problem occur if we replace the numeric values with strings.
JSON.parse('{"0":"97.1","1000":"96.5","2000":"96.1","3000":"97.4","4000":"90.4"}')
> {0: "97.1", 1000: "96.5", 2000: "96.1", 3000: "97.4", 4000: "90.4"}
Comment 1 Alexey Proskuryakov 2017-04-04 17:39:02 PDT
<rdar://problems/31442949>
Comment 2 GSkachkov 2017-04-13 13:30:14 PDT
I can't reproduce this issue on current version of JSC, and it looks like duplicate of the fixed https://bugs.webkit.org/show_bug.cgi?id=164412
Comment 3 Saam Barati 2017-04-13 19:03:23 PDT
(In reply to GSkachkov from comment #2)
> I can't reproduce this issue on current version of JSC, and it looks like
> duplicate of the fixed https://bugs.webkit.org/show_bug.cgi?id=164412

Could you add a test for this to make sure we don't regress it in the future?
Comment 4 GSkachkov 2017-04-14 00:19:59 PDT
Created attachment 307100 [details]
Patch

Added additional tests
Comment 5 Yusuke Suzuki 2017-04-14 01:28:56 PDT
Comment on attachment 307100 [details]
Patch

r=me
Comment 6 WebKit Commit Bot 2017-04-14 02:08:55 PDT
Comment on attachment 307100 [details]
Patch

Clearing flags on attachment: 307100

Committed r215360: <http://trac.webkit.org/changeset/215360>
Comment 7 WebKit Commit Bot 2017-04-14 02:08:57 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 PiwEL 2017-04-26 09:01:27 PDT
It's the same problem even when the first key isn't 0, for JSON.parse and JSON.stringify.

JSON.stringify({ 8: 13.2, 111544: 132})
> "{\"0\":null,\"1\":null,\"2\":null,\"3\":null,\"4\":null,\"5\":null,\"6\":null,\"7\":null,\"8\":13.2,\"111544\":132}"

If these are parseable to integers, this doesn't happen either.
JSON.stringify({ 8: 132, 111544: 132})
> "{\"8\":132,\"111544\":132}"


And in the other way :
JSON.parse("{\"8\":13.2,\"111544\":132}");
> {0: NaN, 1: NaN, 2: NaN, 3: NaN, 4: NaN, 5: NaN, 6: NaN, 7: NaN, 8: 13.2, 111544: 132}

I guess tests for theses cases should also be added, but I'm sorry, I'm new here and I don't know how to do it.
Comment 9 Saam Barati 2017-04-26 11:49:32 PDT
(In reply to PiwEL from comment #8)
> It's the same problem even when the first key isn't 0, for JSON.parse and
> JSON.stringify.
> 
> JSON.stringify({ 8: 13.2, 111544: 132})
> > "{\"0\":null,\"1\":null,\"2\":null,\"3\":null,\"4\":null,\"5\":null,\"6\":null,\"7\":null,\"8\":13.2,\"111544\":132}"
> 
> If these are parseable to integers, this doesn't happen either.
> JSON.stringify({ 8: 132, 111544: 132})
> > "{\"8\":132,\"111544\":132}"
> 
> 
> And in the other way :
> JSON.parse("{\"8\":13.2,\"111544\":132}");
> > {0: NaN, 1: NaN, 2: NaN, 3: NaN, 4: NaN, 5: NaN, 6: NaN, 7: NaN, 8: 13.2, 111544: 132}
> 
> I guess tests for theses cases should also be added, but I'm sorry, I'm new
> here and I don't know how to do it.
Thanks for giving us this test case. We already added a test similar to this.