Bug 197362 - normalizeMapKey should normalize NaN to one PureNaN bit pattern to make MapHash same
Summary: normalizeMapKey should normalize NaN to one PureNaN bit pattern to make MapHa...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: Safari 12
Hardware: Macintosh macOS 10.13
: P2 Major
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
: 197360 (view as bug list)
Depends on:
Blocks:
 
Reported: 2019-04-29 00:54 PDT by Andrea Puddu
Modified: 2019-04-29 19:54 PDT (History)
10 users (show)

See Also:


Attachments
Modified testcase for Set.prototype.size (175 bytes, text/javascript)
2019-04-29 01:54 PDT, Andrea Puddu
no flags Details
Patch (7.48 KB, patch)
2019-04-29 15:15 PDT, Yusuke Suzuki
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrea Puddu 2019-04-29 00:54:22 PDT
Given this code:

`const uniqueSet = new Set([NaN, 0/0])`

The content of `uniqueSet` is:

`Set {NaN, NaN} (2)`


This behaviour violates the step 2.a. of the SameValueZero spec which states:

> "If x is NaN and y is NaN, return true."

https://www.ecma-international.org/ecma-262/#sec-samevaluezero
Comment 1 Andrea Puddu 2019-04-29 00:59:24 PDT
I'm not sure but perhaps it's just a bug within the Set constructor.
Comment 2 Andrea Giammarchi 2019-04-29 01:41:17 PDT
FWIW the issue is not the constructor, rather the `.add(NaN)` followed by `.add(0/0)` or even `.add(Intinity/Infinity)`.

Coming from here https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/runtime/SetPrototype.cpp#L102 I think the issue might be in here: https://github.com/WebKit/webkit/blob/dad62415730c8df3cbbe654eb15f0585e74af04c/Source/JavaScriptCore/runtime/HashMapImpl.h#L462-L470

where I suspect `areKeysEqual(exec, key, bucket->key())` might consider the globally available `NaN` and those produced by `0/0` or `Infinity/Infinity` different.

The same applies for `Map`

```
const m = new Map;
m.set(0/0, '0/0');
m.set(NaN, 'NaN');
m.get(0/0);
m.get(NaN);
```
Comment 3 Andrea Puddu 2019-04-29 01:54:17 PDT
Created attachment 368446 [details]
Modified testcase for Set.prototype.size

Attaching a modified version of:

`JSTests/es6/Set_Set.prototype.size.js`

I suggest to add this change to cover this particular case.

Sorry but I'm unable to run the `run-javascriptcore-tests` script from my machine due to the missing xcode dependency (cannot install the latest one on my macOS version), but I believe that with these changes it will fail.
Comment 4 Andrea Puddu 2019-04-29 02:06:44 PDT
(In reply to Andrea Giammarchi from comment #2)

> FWIW the issue is not the constructor, rather the `.add(NaN)` followed by `.add(0/0)` or even `.add(Intinity/Infinity)`. 

> where I suspect `areKeysEqual(exec, key, bucket->key())` might consider the globally available `NaN` and those produced by `0/0` or `Infinity/Infinity` different.

> The same applies for `Map`


Agreed! Thanks for investigating further
Comment 5 Yusuke Suzuki 2019-04-29 12:03:23 PDT
I think the problem may be we are producing non pure NaN double JSValue.
Comment 6 Yusuke Suzuki 2019-04-29 14:44:39 PDT
(In reply to Yusuke Suzuki from comment #5)
> I think the problem may be we are producing non pure NaN double JSValue.

No. produced NaN is one of the pure NaN. What we want is normalizing NaN into one canonicalized value to calculate hash.
Comment 7 Yusuke Suzuki 2019-04-29 15:15:24 PDT
Created attachment 368498 [details]
Patch
Comment 8 Saam Barati 2019-04-29 15:18:00 PDT
Comment on attachment 368498 [details]
Patch

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

r=me

> JSTests/stress/hash-table-with-normalized-nan.js:81
> +}

an we also have a test for Map?
Comment 9 Yusuke Suzuki 2019-04-29 15:18:48 PDT
Comment on attachment 368498 [details]
Patch

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

Thanks!

>> JSTests/stress/hash-table-with-normalized-nan.js:81
>> +}
> 
> an we also have a test for Map?

Sounds nice, added.
Comment 10 Yusuke Suzuki 2019-04-29 15:25:11 PDT
Committed r244760: <https://trac.webkit.org/changeset/244760>
Comment 11 Radar WebKit Bug Importer 2019-04-29 15:26:18 PDT
<rdar://problem/50313976>
Comment 12 Yusuke Suzuki 2019-04-29 15:32:12 PDT
*** Bug 197360 has been marked as a duplicate of this bug. ***
Comment 13 Yusuke Suzuki 2019-04-29 19:54:21 PDT
Committed r244763: <https://trac.webkit.org/changeset/244763>