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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tim Evans]

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}'

Comment 1

[Ed Morley [:emorley]]

Attachment: Testcase

Testcase from URL field, attached in case remote copy becomes unavailable.

Comment 2

[Jeff Walden [:Waldo]]

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.

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Patch, might need tweaks and more tests, not yet ready

Comment 4

[Ed Morley [:emorley]]

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

Comment 5

[Jeff Walden [:Waldo]]

Attachment: Patch with tests for all bugs the patch is known to fix

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.

Comment 6

[Nicholas Nethercote [inactive]]

Please note bug 589664, which has a (probably bit-rotted) patch for speeding up the JSON parser.

Comment 7

[Nicholas Nethercote [inactive]]

Oh, you're talking about JSON.stringify, which is unrelated to the parser.

Comment 8

[Jeff Walden [:Waldo]]

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.

Comment 9

[Jeff Walden [:Waldo]]

Attachment: Updated

Comment 10

[Paul Biggar]

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").

Comment 11

[Jeff Walden [:Waldo]]

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.

Comment 12

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

http://hg.mozilla.org/mozilla-central/rev/67596937aa39