654574 - JSON.stringify omitted replacer array prepass

Status: RESOLVED FIXED

(Tamarin Graveyard :: Library, defect)

Description

[Felix S. Klock II [:pnkfelix, :fklock]]

On #jsapi there was some discussion of corner cases involving -0.

The Mozilla folks mentioned tests here:

http://hg.mozilla.org/tracemonkey/file/a15c6194d2f0/js/src/tests/ecma_5/JSON/stringify-replacer-array-edgecase-jsid-elements.js

One of them I tried on AVM:


$ ~/Dev/tamarin-redux/objdir-ged32/shell/avmsll -Dversion
shell 1.4 debug-debugger build 6258:0ee91619d908
features AVMSYSTEM_32BIT;AVMSYSTEM_UNALIGNED_INT_ACCESS;AVMSYSTEM_UNALIGNED_FP_ACCESS;AVMSYSTEM_LITTLE_ENDIAN;AVMSYSTEM_IA32;AVMSYSTEM_MAC;AVMFEATURE_DEBUGGER;AVMFEATURE_ALLOCATION_SAMPLER;AVMFEATURE_JIT;AVMFEATURE_ABC_INTERP;AVMFEATURE_SELFTEST;AVMFEATURE_EVAL;AVMFEATURE_PROTECT_JITMEM;AVMFEATURE_SHARED_GCHEAP;AVMFEATURE_MEMORY_PROFILER;AVMFEATURE_CACHE_GQCN;AVMFEATURE_SAFEPOINTS;AVMFEATURE_SWF12;AVMFEATURE_SWF13;AVMTWEAK_EXACT_TRACING;
$ ~/Dev/tamarin-redux/objdir-ged32/shell/avmshell -repl
avmplus interactive shell
Type '?' for help

> JSON.stringify({ "-0": 17, 0: 42 }, [-0])
{}
> 

No matter what your thoughts are on how -0 should convert to a string, the above seems wrong to me.

Comment 1

[Tamarin Bot]

changeset: 6276:b4a71849a889
user:      Felix S Klock II <[e-mail]>
summary:   Bug 654574: JSON left out replacer Array prepass (original=lhansen, adapted by fklockii, r=fklockii).

http://hg.mozilla.org/tamarin-redux/rev/b4a71849a889

Comment 2

[Felix S. Klock II [:pnkfelix, :fklock]]

The problem was far more broad than my original bug description implied.  The negative zero stuff was a symptom of a deeper issue: I largely left out Step 4.b of stringify; I had thought I could get away with just doing the type checks on demand, but that doesn't cover:

* Proper number-to-string conversion

* Repeated entries (which are supposed to be skipped after the first entry)

(The above two bullets are not an exhaustive list of problems; e.g. there are some details in the spec that I believe yielded Lars's inclusion of a hasOwnProperty check in computePropertyList.)

At this point the simplest thing was to just adapt Lars's code directly into the builtin JSON.as.  The only thing left is to expand our test suite to add the uncovered cases.

Comment 3

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: new replacer array tests

Prior to the fix (already pushed as changeset 6276:b4a71849a889) four of the tests added in this proposed patch fail.

After the fix, all tests pass.

Comment 4

[Tamarin Bot]

changeset: 6278:6dc6dfdeb72f
user:      Felix S Klock II <[e-mail]>
summary:   Bug 654574: new replacer array tests (r=dschaffe).

http://hg.mozilla.org/tamarin-redux/rev/6dc6dfdeb72f

Comment 5

[Felix S. Klock II [:pnkfelix, :fklock]]

should be ported to Serrano.

Comment 6

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: fix to JSON.as

This matches the non-generated/ portions of changeset 6276:b4a71849a889
(the push of which was logged in comment 1).

(I pushed it because I felt that since it was authored by Lars, I could, and did, serve as an appropriate reviewer for it.)

Comment 7

[Felix S. Klock II [:pnkfelix, :fklock]]

Pushed:
tamarin-redux-serrano - changeset - 6289:147fd4e37237
http://asteam.corp.adobe.com/hg/tamarin-redux-serrano/rev/147fd4e37237