648471 - If the replacer is an array, only get the list of property names to use once, and use non-enumerable properties

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Noted during review in bug 636079:

  var count = 0;
  var a = [];
  Object.defineProperty(a, 0, { get: function() { count++; return "0"; } });
  assertEq(JSON.stringify({ "0": { "0": 5 } }, a), '{"0":{"0":5}}');
  assertEq(count, 1);

We reget properties from the replacer array every time we hit an object, but per spec we should only get them once, at the start of the JSON.stringify recursive algorithm.

Also, step 4b(ii) refers to array index properties, not merely to enumerable array index properties (implicitly demonstrated above, where ignoring non-enumerables would result in stringification to "{}").

Comment 1

[Jeff Walden [:Waldo]]

There's a fair bit of spec vagueness here, so I sent mail to es5-discuss about the replacer-is-array semantics:

https://mail.mozilla.org/pipermail/es5-discuss/2011-April/003976.html

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Patch and tests

I'm not convinced there's really a better name than |outputValue| in those two places, so I've left it as is.  If that single variable name is the impediment to someone understanding the code, that person probably has bigger problems to overcome to comprehend this code.

Comment 3

[Jeff Walden [:Waldo]]

Note that the algorithm for handling boxed String and Number objects is wrong, but it's the algorithm that exists now.  Bug 643537 has a patch that fixes it (seems best to keep the concerns separate).

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Fix thinko

Comment 5

[Paul Biggar]

Comment on attachment 526004 [details] [diff] [review]
Fix thinko

Review of attachment 526004 [details] [diff] [review]:

This looks right to me, but it was tricky to follow - even with the standard for reference. Can you:

- add a short description of each step
- reference the email sent to ES5, and include the algorithm that you implemented as a comment, since we wont be able to refer to the standard due to vagueness.

r+ with those changes.

Comment 6

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/22580db92b77

Comment 7

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/22580db92b77

Comment 8

[Jeff Walden [:Waldo]]

Somehow I wrote a test that never got added to patches here -- found it when cleaning out my forest of Mozilla trees, pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/f2a3c41ee77f
https://hg.mozilla.org/integration/mozilla-inbound/rev/1986bfb7f0b6

Comment 9

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/f2a3c41ee77f
https://hg.mozilla.org/mozilla-central/rev/1986bfb7f0b6