1217069 - JSON.stringify ignores inherited properties in replacer array

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[André Bargull [:anba]]

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

Expected: Returns the string "{\"a\":0}"
Actual: Returns the string ""

Comment 1

[Jeff Walden [:Waldo]]

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.
             */

Comment 2

[Jeff Walden [:Waldo]]

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.

Comment 3

[Jeff Walden [:Waldo]]

Attachment: 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.

Comment 4

[André Bargull [:anba]]

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! :-/

Comment 5

[Jeff Walden [:Waldo]]

Attachment: Oof

Comment 6

[Tooru Fujisawa [:arai]]

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

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6dc0a7e6f791

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6dc0a7e6f791