WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
164412
Object with numerical keys with gaps gets filled by NaN values
https://bugs.webkit.org/show_bug.cgi?id=164412
Summary
Object with numerical keys with gaps gets filled by NaN values
David Gasperoni
Reported
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)
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
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
David Gasperoni
Comment 1
2016-11-04 06:27:55 PDT
Created
attachment 293883
[details]
Screenshot highlighting the issue
David Gasperoni
Comment 2
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.
Blaze Burg
Comment 3
2016-12-13 15:17:00 PST
Does this reproduce outside of Web Inspector (say, with `jsc`) ?
Blaze Burg
Comment 4
2016-12-13 21:31:04 PST
<
rdar://problem/29655632
>
David Gasperoni
Comment 5
2016-12-14 03:35:08 PST
Created
attachment 297080
[details]
Test case New test case highlighting the issue.
David Gasperoni
Comment 6
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 👨💻
David Gasperoni
Comment 7
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
David Gasperoni
Comment 8
2017-03-23 11:59:44 PDT
Still an issue in Safari Technology Preview release 26.
GSkachkov
Comment 9
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.
GSkachkov
Comment 10
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.
GSkachkov
Comment 11
2017-03-31 12:27:10 PDT
Created
attachment 305999
[details]
Patch Patch
Mark Lam
Comment 12
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.
GSkachkov
Comment 13
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!
GSkachkov
Comment 14
2017-04-01 01:22:43 PDT
Patch landed
https://trac.webkit.org/changeset/214714
with fixed Mark Lam's comments.
Michael Saboff
Comment 15
2017-04-03 17:06:54 PDT
***
Bug 170264
has been marked as a duplicate of this bug. ***
Alexey Proskuryakov
Comment 16
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?
Saam Barati
Comment 17
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.
Ryan King
Comment 18
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
David Gasperoni
Comment 19
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.
Dominic Szablewski
Comment 20
2017-04-13 14:29:43 PDT
***
Bug 170807
has been marked as a duplicate of this bug. ***
MikeM
Comment 21
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().
Mauricio Andrades
Comment 22
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.
David Gasperoni
Comment 23
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.
Mauricio Andrades
Comment 24
2017-06-05 15:37:46 PDT
Brilliant. Great idea thanks!
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug