Closed Bug 636079 Opened 9 years ago Closed 9 years ago

JSON.stringify replacer function calls keys twice when values are replaced

Categories

(Core :: JavaScript Engine, defect, major)

defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla5

People

(Reporter: tim.evans, Assigned: Waldo)

References

()

Details

(Keywords: regression, Whiteboard: fixed-in-tracemonkey)

Attachments

(2 files, 2 obsolete files)

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_6_6; en-US) AppleWebKit/534.13 (KHTML, like Gecko) Chrome/9.0.597.102 Safari/534.13
Build Identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b11) Gecko/20100101 Firefox/4.0b11

When the value returned to the replacer function is not the same as the value it was, it will call the value once more (it does NOT recurse, however).

Reproducible: Always

Steps to Reproduce:
1. Call JSON.stringify on an object with a replacer function as the second argument.
2. Augment one of the values, returning the new result.
3. The replacer function is called with that key (again).
Actual Results:  
The string returned has values that are doubly augmented.

ie. With '{"a":0,"b":1,"c":2}' as the input (and increasing the values by 1) the result is '{"a":2,"b":3,"c":4}' 

Expected Results:  
The string returned values that are augmented only once.

ie. With '{"a":0,"b":1,"c":2}' as the input (and increasing the values by 1) the result is '{"a":1,"b":2,"c":3}'
Attached file Testcase
Testcase from URL field, attached in case remote copy becomes unavailable.
I ran across this a couple days back.  I have a semi-rewrite, semi-reorganization of the JSON code which fixes it.  Not for 4.0 (way too late for that), but definitely for very shortly into the next cycle.
Assignee: general → jwalden+bmo
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
OS: Mac OS X → All
Hardware: x86 → All
Edit, post mid-air collision:

The following is no doubt redundant now, but seeing as I spent the last 30 mins doing this, might as well post it still.

---

Confirmed using Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b12pre) Gecko/20110222 Firefox/4.0b12pre ID:20110222191848.

Chrome dev 10.latest -> WORKS  {"a":1,"b":2,"c":3}
IE 8                 -> WORKS  {"a":1,"b":2,"c":3}
Firefox 3.6.13       -> BROKEN {"a":0,"b":1,"c":2}
Minefield 2010-06-24 -> WORKS  {"a":1,"b":2,"c":3}
Minefield 2010-06-25 -> BROKEN {"a":2,"b":3,"c":4}

Last good nightly: 2010-06-24 
First bad nightly: 2010-06-25
Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=68d98f30eda0&tochange=3a9f81291788

Likely candidate for the break in Fx4...

Saint Wesonga — Bug 512447 - JSON.stringify does not correct handle replacer functions. r=sayrer
http://hg.mozilla.org/mozilla-central/rev/42371f85bbfb

(This testcase failed in 3.6.13 as well, albeit differently, so presume not blocking).
Keywords: regression
Version: unspecified → Trunk
Blocks: 512447
Let's try this for size.

This code will almost certainly need a lot of refactoring for perf if our JSON implementation is to do well on the tests at the URL in bug 557354, but best to get it correct first before we start fine-tuning for that.
Attachment #514500 - Attachment is obsolete: true
Attachment #516073 - Flags: review?(sayrer)
Please note bug 589664, which has a (probably bit-rotted) patch for speeding up the JSON parser.
Oh, you're talking about JSON.stringify, which is unrelated to the parser.
Comment on attachment 516073 [details] [diff] [review]
Patch with tests for all bugs the patch is known to fix

Shifting review, sayrer's busy, and he suggests it's a good idea to get more people up to speed on the code anyway, a sensible idea.

ES5 §15.12 is the place to look to understand what all this code should do, and I think does post-patch.
Attachment #516073 - Flags: review?(sayrer) → review?(pbiggar)
Blocks: 505238
Blocks: 645922
Attached patch UpdatedSplinter Review
Attachment #516073 - Attachment is obsolete: true
Attachment #516073 - Flags: review?(pbiggar)
Attachment #524536 - Flags: review?(pbiggar)
Comment on attachment 524536 [details] [diff] [review]
Updated

We did this review in person, and decided to:

Add comments indicating the Spec steps in JO.

Add a comment to isFilteredValue use in both JO and JA, explaining why we don't just have undefined to contend with here, and how we're not actually deviating from the spec.

File a follow-on bug to pull replacer to the start of Stringify.

Rename keySource to PropertyList.

Use a better name than outputValue when appropriate (the spec uses "value").
Attachment #524536 - Flags: review?(pbiggar) → review+
Blocks: 648471
http://hg.mozilla.org/tracemonkey/rev/67596937aa39

The followup is bug 648471.  Also, I left outputValue for the moment because I wasn't sure of a better name, and what we call that the spec calls a few different things (we have one variable where the spec has several intermediate variables).  I'll think about better options when I do the followup.
Whiteboard: fixed-in-tracemonkey
Target Milestone: --- → mozilla2.2
http://hg.mozilla.org/mozilla-central/rev/67596937aa39
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Blocks: 662231
You need to log in before you can comment on or make changes to this bug.