Bug 197362

Summary: normalizeMapKey should normalize NaN to one PureNaN bit pattern to make MapHash same
Product: WebKit Reporter: Andrea Puddu <me>
Component: JavaScriptCoreAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Major CC: andrea.giammarchi, ews-watchlist, fpizlo, keith_miller, mark.lam, me, msaboff, saam, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: Safari 12   
Hardware: Mac   
OS: macOS 10.13   
Attachments:
Description Flags
Modified testcase for Set.prototype.size
none
Patch saam: review+

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>