Object with numerical keys with gaps gets filled by NaN values

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[David Gasperoni]

Created attachment 293882 [details]
Test case

For some reason an object that I instantiate with sub-objects which have numbered keys, they get a whole bunch of extra keys with NaN values.

See attached test case. (and screenshot)

[David Gasperoni]

Created attachment 293883 [details]
Screenshot highlighting the issue

[David Gasperoni]

This causes bugs, since I need to add a (normally pointless) !isNAN() check as a workaround, otherwise it would silently break logic.

[BJ Burg]

Does this reproduce outside of Web Inspector (say, with `jsc`) ?

[BJ Burg]

<rdar://problem/29655632>

[David Gasperoni]

Created attachment 297080 [details]
Test case

New test case highlighting the issue.

[David Gasperoni]

(In reply to comment #3)
> Does this reproduce outside of Web Inspector (say, with `jsc`) ?

Yes, I added a new test case that shows the issue as I discovered it: our e-commerce site was showing price 0 when changing the quantity from a dropdown, because we have an object containing some price ranges ("from this quantity and up is this price", something like that). I cannot tell you where is the cause of this because, hey, I'm just a web developer 👨‍💻

[David Gasperoni]

Just for posterity: the results of the tests in Safari TP 19 for me are as follows:

Test Results:

Quote for 10 pieces: NaN.
Quote for 25 pieces: NaN.
Quote for 50 pieces: NaN.
Quote for 100 pieces: 0.45.
Quote for 150 pieces: NaN.
Quote for 200 pieces: NaN.
Quote for 250 pieces: 0.3.
Quote for 500 pieces: NaN.
Quote for 1000 pieces: NaN.
Quote for 2000 pieces: 0.28.

Inspector console:
>Object.keys(priceRanges).length
>506

[David Gasperoni]

Still an issue in Safari Technology Preview release 26.

[GSkachkov]

I managed to reproduce this issue in jsc:

```
var Ranges = {
    "250" : 0.5,
    "1000": 0.1,
    "abc": 0.5
  };
print(Object.keys(Ranges));
print(Object.keys(Ranges).length);
```
I suspect something wrong when put_by_val_direct op code is used.

[GSkachkov]

I found that during execution of convertDoubleToArrayStorage in JSObject it does not check if value pNaN and materialized is as NaN value. I hope to prepare patch today.

[GSkachkov]

Created attachment 305999 [details]
Patch

Patch

[Mark Lam]

Comment on attachment 305999 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:11
> +        The issue is appear when during invoking convertDoubleToArrayStorage when array is filled by 
> +        pNaN and method converting it to real NaN. To fix issue we need to check value and clear it
> +        if in pNaN.

Please add a comment here that says that "a PNaN in a Double array is a hole, and Double arrays cannot have NaN values."

> JSTests/stress/object-number-properties.js:35
> +assert(Object.keys(foo).length, 4);

Can you also some explicit asserts to ensure that the actual values also remain the expected values (especially NaNs) after an indexing type conversion to ArrayStorage, and didn't switch to a hole or something?  Thanks.

[GSkachkov]

Thanks(In reply to Mark Lam from comment #12)
> Comment on attachment 305999 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=305999&action=review
> 
> > Source/JavaScriptCore/ChangeLog:11
> > +        The issue is appear when during invoking convertDoubleToArrayStorage when array is filled by 
> > +        pNaN and method converting it to real NaN. To fix issue we need to check value and clear it
> > +        if in pNaN.
> 
> Please add a comment here that says that "a PNaN in a Double array is a
> hole, and Double arrays cannot have NaN values."
> 
> > JSTests/stress/object-number-properties.js:35
> > +assert(Object.keys(foo).length, 4);
> 
> Can you also some explicit asserts to ensure that the actual values also
> remain the expected values (especially NaNs) after an indexing type
> conversion to ArrayStorage, and didn't switch to a hole or something? 
> Thanks.

Thanks foe review!

[GSkachkov]

Patch landed https://trac.webkit.org/changeset/214714 with fixed Mark Lam's comments.

[Michael Saboff]

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

[Alexey Proskuryakov]

> Patch landed https://trac.webkit.org/changeset/214714 with fixed Mark Lam's comments.

The bug remains open; should it be marked as resolved?

[Saam Barati]

(In reply to Alexey Proskuryakov from comment #16)
> > Patch landed https://trac.webkit.org/changeset/214714 with fixed Mark Lam's comments.
> 
> The bug remains open; should it be marked as resolved?

Yes. I'm doing that now.

[Ryan King]

Any idea when this will be pushed to Safari?
This is causing significant issues with our clients on 10.1

[David Gasperoni]

(In reply to Ryan King from comment #18)
> Any idea when this will be pushed to Safari?
> This is causing significant issues with our clients on 10.1

My suggestion would be to workaround it for now: add checks like isNan() on the keys if possible. I do no speak for the Safari team but I doubt a bugfix will be released so soon.

[Dominic Szablewski]

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

[MikeM]

Given that this bug is present in Safari, Firefox and Chrome running on iOS 10.3 and 10.3.1 (the most recent stable release), please could someone familiar with WebKit internals give a full description of how to avoid it. 

What are the necessary conditions in terms of JavaScript code for the bug to manifest?

Here is an example showing how a basic array operation fails which would not be obvious from how the bug is discussed in the comments above

var arr = [0, 2147483648];
arr.shift();              // arr is [2147483648]
arr[1] = 1;               // arr is [2147483648, 1]
console.log(arr); 

// arr should be [2147483648, 1], but is [2147483648] on Safari

The bug does not appear if 2147483648 is 2147483647, or if arr = arr.slice() is added after the arr.shift().

[Mauricio Andrades]

Hi Guys,

How could I get the list of versions this affected? I need to set up some exclusions based on the userAgent for versions with this bug.

[David Gasperoni]

(In reply to Mauricio Andrades from comment #22)
> Hi Guys,
> 
> How could I get the list of versions this affected? I need to set up some
> exclusions based on the userAgent for versions with this bug.

Hi, I guess you could also just write a two-three liner test for it… construct and object with numerical keys and check the length of it.

[Mauricio Andrades]

Brilliant. Great idea thanks!