JSON.stringify ignores inherited properties in replacer array

RESOLVED FIXED in Firefox 44

Status

()

Core
JavaScript: Standard Library
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: anba, Assigned: Waldo)

Tracking

(Blocks: 1 bug)

Trunk
mozilla44
Points:
---

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
Test case:
---
Object.prototype[0] = "a";
JSON.stringify({a: 0, b: 1}, [,])
---

Expected: Returns the string "{\"a\":0}"
Actual: Returns the string ""
Woo, they rewrote the spec algorithm for this!  Maybe now we can replace our implementation consistent with this comment copied from our code now:

            /*
             * Step 4b: The spec algorithm is unhelpfully vague about the exact
             * steps taken when the replacer is an array, regarding the exact
             * sequence of [[Get]] calls for the array's elements, when its
             * overall length is calculated, whether own or own plus inherited
             * properties are considered, and so on.  A rewrite was proposed in
             * <https://mail.mozilla.org/pipermail/es5-discuss/2011-April/003976.html>,
             * whose steps are copied below, and which are implemented here.
             *
             * i.   Let PropertyList be an empty internal List.
             * ii.  Let len be the result of calling the [[Get]] internal
             *      method of replacer with the argument "length".
             * iii. Let i be 0.
             * iv.  While i < len:
             *      1. Let item be undefined.
             *      2. Let v be the result of calling the [[Get]] internal
             *         method of replacer with the argument ToString(i).
             *      3. If Type(v) is String then let item be v.
             *      4. Else if Type(v) is Number then let item be ToString(v).
             *      5. Else if Type(v) is Object then
             *         a. If the [[Class]] internal property of v is "String"
             *            or "Number" then let item be ToString(v).
             *      6. If item is not undefined and item is not currently an
             *         element of PropertyList then,
             *         a. Append item to the end of PropertyList.
             *      7. Let i be i + 1.
             */
Or, hm.  That algorithm is actually exactly the one in the spec, if you squint.  Rather, it looks like we have some mis-optimizations in place, more or less.  Might as well fix them, now that I've stared that hard at this.
Assignee: nobody → jwalden+bmo
Status: NEW → ASSIGNED
Created attachment 8677140 [details] [diff] [review]
Only optimize filter-list creation for JSON.stringify if the replacer array has no extra indexed properties, including on its prototype chain

I also went and renumbered the steps whose numbers are off since ES5.
Attachment #8677140 - Flags: review?(arai.unmht)
(Reporter)

Comment 4

2 years ago
Comment on attachment 8677140 [details] [diff] [review]
Only optimize filter-list creation for JSON.stringify if the replacer array has no extra indexed properties, including on its prototype chain

Review of attachment 8677140 [details] [diff] [review]:
-----------------------------------------------------------------

+1 on updating the step numbers! :-)

::: js/src/json.cpp
@@ +574,1 @@
>                  len = Min(len, replacer->as<ArrayObject>().getDenseInitializedLength());

I think this optimization needs to be removed completely:
---
s=new String();
a=[s,,];
s.toString=()=>{ a[1] = "b"; return "a" };
JSON.stringify({a:0, b:0}, a); // Should return "{\"a\":0,\"b\":0}"
---

Yay, side-effects! :-/
Created attachment 8677165 [details] [diff] [review]
Oof
Attachment #8677165 - Flags: review?(arai.unmht)
Attachment #8677140 - Attachment is obsolete: true
Attachment #8677140 - Flags: review?(arai.unmht)
Comment on attachment 8677165 [details] [diff] [review]
Oof

Review of attachment 8677165 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good :)

::: js/src/json.cpp
@@ +599,1 @@
>                      /* Step 4b(iv)(4). */

Step 4b(iii)(5)(e).

@@ +599,3 @@
>                      /* Step 4b(iv)(4). */
>                      int32_t n;
> +                    if (item.isNumber() && ValueFitsInInt32(item, &n) && INT_FITS_IN_JSID(n)) {

|item.isNumber()| seems to be redundant.

@@ +629,2 @@
>                  if (!p) {
>                      /* Step 4b(iv)(6)(a). */

Step 4b(iii)(5)(g)(i)  ;)
Attachment #8677165 - Flags: review?(arai.unmht) → review+

Comment 7

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/6dc0a7e6f791
https://hg.mozilla.org/mozilla-central/rev/6dc0a7e6f791
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox44: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.