Bug 197362 – normalizeMapKey should normalize NaN to one PureNaN bit pattern to make MapHash same

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Andrea Puddu]

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

[Andrea Puddu]

I'm not sure but perhaps it's just a bug within the Set constructor.

[Andrea Giammarchi]

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);
```

[Andrea Puddu]

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.

[Andrea Puddu]

(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

[Yusuke Suzuki]

I think the problem may be we are producing non pure NaN double JSValue.

[Yusuke Suzuki]

(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.

[Yusuke Suzuki]

Created attachment 368498 [details]
Patch

[Saam Barati]

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?

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Committed r244760: <https://trac.webkit.org/changeset/244760>

[Radar WebKit Bug Importer]

<rdar://problem/50313976>

[Yusuke Suzuki]

*** Bug 197360 has been marked as a duplicate of this bug. ***

[Yusuke Suzuki]

Committed r244763: <https://trac.webkit.org/changeset/244763>