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

RESOLVED FIXED in mozilla6

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

Trunk
mozilla6
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 1 obsolete attachment)

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 "{}").
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
Created attachment 525810 [details] [diff] [review]
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.
Attachment #525810 - Flags: review?(pbiggar)
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).
Created attachment 526004 [details] [diff] [review]
Fix thinko
Attachment #525810 - Attachment is obsolete: true
Attachment #525810 - Flags: review?(pbiggar)
Attachment #526004 - Flags: review?(pbiggar)

Comment 5

6 years ago
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.
Attachment #526004 - Flags: review?(pbiggar) → review+
http://hg.mozilla.org/tracemonkey/rev/22580db92b77
Whiteboard: fixed-in-tracemonkey
Target Milestone: mozilla5 → mozilla6
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/22580db92b77
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
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
https://hg.mozilla.org/mozilla-central/rev/f2a3c41ee77f
https://hg.mozilla.org/mozilla-central/rev/1986bfb7f0b6
You need to log in before you can comment on or make changes to this bug.