Bug 164412 - Object with numerical keys with gaps gets filled by NaN values
Summary: Object with numerical keys with gaps gets filled by NaN values
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords: InRadar
: 170807 (view as bug list)
Depends on:
Blocks:
 
Reported: 2016-11-04 06:27 PDT by David Gasperoni
Modified: 2017-06-05 15:37 PDT (History)
18 users (show)

See Also:


Attachments
Test case (1.78 KB, text/html)
2016-11-04 06:27 PDT, David Gasperoni
no flags Details
Screenshot highlighting the issue (200.87 KB, image/png)
2016-11-04 06:27 PDT, David Gasperoni
no flags Details
Test case (1.29 KB, text/html)
2016-12-14 03:35 PST, David Gasperoni
no flags Details
Patch (3.86 KB, patch)
2017-03-31 12:27 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 David Gasperoni 2016-11-04 06:27:07 PDT
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)
Comment 1 David Gasperoni 2016-11-04 06:27:55 PDT
Created attachment 293883 [details]
Screenshot highlighting the issue
Comment 2 David Gasperoni 2016-12-13 14:20:34 PST
This causes bugs, since I need to add a (normally pointless) !isNAN() check as a workaround, otherwise it would silently break logic.
Comment 3 Brian Burg 2016-12-13 15:17:00 PST
Does this reproduce outside of Web Inspector (say, with `jsc`) ?
Comment 4 Brian Burg 2016-12-13 21:31:04 PST
<rdar://problem/29655632>
Comment 5 David Gasperoni 2016-12-14 03:35:08 PST
Created attachment 297080 [details]
Test case

New test case highlighting the issue.
Comment 6 David Gasperoni 2016-12-14 03:40:59 PST
(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 πŸ‘¨β€πŸ’»
Comment 7 David Gasperoni 2016-12-14 03:47:29 PST
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
Comment 8 David Gasperoni 2017-03-23 11:59:44 PDT
Still an issue in Safari Technology Preview release 26.
Comment 9 GSkachkov 2017-03-30 05:59:05 PDT
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.
Comment 10 GSkachkov 2017-03-31 03:35:33 PDT
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.
Comment 11 GSkachkov 2017-03-31 12:27:10 PDT
Created attachment 305999 [details]
Patch

Patch
Comment 12 Mark Lam 2017-03-31 12:56:49 PDT
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.
Comment 13 GSkachkov 2017-04-01 01:21:42 PDT
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!
Comment 14 GSkachkov 2017-04-01 01:22:43 PDT
Patch landed https://trac.webkit.org/changeset/214714 with fixed Mark Lam's comments.
Comment 15 Michael Saboff 2017-04-03 17:06:54 PDT
*** Bug 170264 has been marked as a duplicate of this bug. ***
Comment 16 Alexey Proskuryakov 2017-04-03 17:41:51 PDT
> Patch landed https://trac.webkit.org/changeset/214714 with fixed Mark Lam's comments.

The bug remains open; should it be marked as resolved?
Comment 17 Saam Barati 2017-04-03 19:07:27 PDT
(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.
Comment 18 Ryan King 2017-04-10 23:23:55 PDT
Any idea when this will be pushed to Safari?
This is causing significant issues with our clients on 10.1
Comment 19 David Gasperoni 2017-04-10 23:29:03 PDT
(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.
Comment 20 Dominic Szablewski 2017-04-13 14:29:43 PDT
*** Bug 170807 has been marked as a duplicate of this bug. ***
Comment 21 MikeM 2017-04-22 14:43:11 PDT
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().
Comment 22 Mauricio Andrades 2017-06-05 14:20:51 PDT
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.
Comment 23 David Gasperoni 2017-06-05 15:24:24 PDT
(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.
Comment 24 Mauricio Andrades 2017-06-05 15:37:46 PDT
Brilliant. Great idea thanks!