Last Comment Bug 648471 - If the replacer is an array, only get the list of property names to use once, and use non-enumerable properties
: If the replacer is an array, only get the list of property names to use once,...
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
-- normal (vote)
: mozilla6
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
: Jason Orendorff [:jorendorff]
Depends on: 636079
  Show dependency treegraph
Reported: 2011-04-07 20:44 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2011-10-12 03:16 PDT (History)
1 user (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch and tests (22.91 KB, patch)
2011-04-13 14:26 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details | Diff | Splinter Review
Fix thinko (22.81 KB, patch)
2011-04-14 08:17 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
paul.biggar: review+
Details | Diff | Splinter Review

Description User image Jeff Walden [:Waldo] (remove +bmo to email) 2011-04-07 20:44:31 PDT
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 User image Jeff Walden [:Waldo] (remove +bmo to email) 2011-04-12 14:54:01 PDT
There's a fair bit of spec vagueness here, so I sent mail to es5-discuss about the replacer-is-array semantics:
Comment 2 User image Jeff Walden [:Waldo] (remove +bmo to email) 2011-04-13 14:26:39 PDT
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.
Comment 3 User image Jeff Walden [:Waldo] (remove +bmo to email) 2011-04-13 14:36:38 PDT
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 User image Jeff Walden [:Waldo] (remove +bmo to email) 2011-04-14 08:17:05 PDT
Created attachment 526004 [details] [diff] [review]
Fix thinko
Comment 5 User image Paul Biggar 2011-04-27 05:29:42 PDT
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 User image Jeff Walden [:Waldo] (remove +bmo to email) 2011-04-28 17:07:18 PDT
Comment 7 User image Chris Leary [:cdleary] (not checking bugmail) 2011-05-02 16:00:25 PDT
cdleary-bot mozilla-central merge info:
Comment 8 User image Jeff Walden [:Waldo] (remove +bmo to email) 2011-10-11 18:05:21 PDT
Somehow I wrote a test that never got added to patches here -- found it when cleaning out my forest of Mozilla trees, pushed:

Note You need to log in before you can comment on or make changes to this bug.