FF 3.5.2 native JSON.stringify replacer applied only to array items, not object properties

RESOLVED FIXED in mozilla1.9.3a5

Status

()

Core
JavaScript Engine
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: Luke Smith, Assigned: Robert Sayre)

Tracking

({regression})

Trunk
mozilla1.9.3a5
x86
All
regression
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.9.2 ?

Firefox Tracking Flags

(blocking2.0 beta1+)

Details

(Whiteboard: fixed-in-tracemonkey, URL)

Attachments

(1 attachment, 1 obsolete attachment)

fix
1.39 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
(Reporter)

Description

8 years ago
User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2

It appears that the replacer function passed as the second arg to JSON.stringify only allows value replacement to array items.  The only exception I've found is returning undefined from the replacer will remove object properties.

Reproducible: Always

Steps to Reproduce:
JSON.stringify({x:1,arr:[1]}, function (k,v) {
    return typeof v === 'number' ? 3 : value;
});
Actual Results:  
'{"x":1,"arr":[3]}'

Expected Results:  
'{"x":3,"arr":["X":3]}'

Comment 1

8 years ago
Confirming with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20090815 Minefield/3.7a1pre

json.org's json2.js implementation gives '{"x":3,"arr":[3]}' as the result. That's what you meant, right?
Status: UNCONFIRMED → NEW
Component: General → DOM
Ever confirmed: true
OS: Mac OS X → All
Product: Firefox → Core
QA Contact: general → general
(Reporter)

Comment 2

8 years ago
Doh!  Yes. IE8, json2.js, WebKit, and Chrome 3 return
{"x":3,"arr":[3]}

This is correct according to the ES5 spec.  FF is catching the array items, but not the object properties.  Sorry for the bogus Expected value!

Comment 3

8 years ago
Oh, this is a regression! It actually worked properly for a brief period of time in May.

regression window:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9703fd69e126&tochange=409416c625bc

Bisecting now...
Keywords: regression, regressionwindow-wanted

Comment 4

8 years ago
Regression from bug 493586.
Assignee: nobody → general
Blocks: 493586
Component: DOM → JavaScript Engine
Keywords: regressionwindow-wanted
QA Contact: general → general
(Assignee)

Comment 5

8 years ago
ugh.
Assignee: general → sayrer
(Assignee)

Updated

8 years ago
Duplicate of this bug: 512443
(Assignee)

Comment 7

8 years ago
Created attachment 398724 [details] [diff] [review]
WIP

stupid, unnecessary code. mea culpa.

I think this tests out, gonna check with test suites from MS / Opera / WebKit, then add one of our own.

Updated

8 years ago
blocking2.0: --- → ?
Blocks: 445494
Flags: wanted1.9.2?

Comment 8

8 years ago
Despite of https://developer.mozilla.org/en/Using_JSON_in_Firefox stating that the issue will be fixed in 1.9.1.5 I can still confirm it on 1.9.1.6 (linux x86). It is blocking me from using native MOZ json entirely. 

Please update the release vision for fixing the issue so people could set the js code workarounds accoringly (i.e. [if < 1.9.1.X] => emulate ).
(Assignee)

Updated

8 years ago
blocking2.0: ? → alpha1
(Assignee)

Updated

8 years ago
blocking2.0: alpha1 → beta1
(Assignee)

Comment 9

7 years ago
Created attachment 433951 [details] [diff] [review]
fix

this fixes one problem, the patch in bug 512447 fixes another.
Attachment #398724 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Attachment #433951 - Flags: review?(jorendorff)
(Assignee)

Updated

7 years ago
Attachment #433951 - Flags: review?(jorendorff) → review?(jwalden+bmo)
(Assignee)

Updated

7 years ago
Attachment #433951 - Flags: review?(jwalden+bmo) → review?(jorendorff)
Comment on attachment 433951 [details] [diff] [review]
fix

Hm, didn't realize you'd shifted this until just now -- looked it over, looks fine to me.

Any particular reason to move Str step 1 into callers, rather than having it in Str directly?  Does call overhead show up on benchmarks or something here?  If that's the case, it would seem preferable to make Str JS_ALWAYS_INLINE and have it tail-call "StrHelper" so as to match the structure of the spec.
Attachment #433951 - Flags: review?(jorendorff) → review+
The StrHelper idea, I think, would also make fixing bug 512447 easier, because then I think you might be able to avoid the not-in-the-spec-method callReplacer argument.
(Assignee)

Comment 12

7 years ago
http://hg.mozilla.org/tracemonkey/rev/b79a14c88df1
Whiteboard: fixed-in-tracemonkey
(Assignee)

Comment 13

7 years ago
http://hg.mozilla.org/mozilla-central/rev/b79a14c88df1
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Updated

7 years ago
Target Milestone: --- → mozilla1.9.3a5
Version: unspecified → Trunk

Updated

7 years ago
Duplicate of this bug: 548462
You need to log in before you can comment on or make changes to this bug.