Closed Bug 476374 Opened 11 years ago Closed 11 years ago

JSON.parse does not support reviver argument as defined in spec

Categories

(Core :: JavaScript Engine, defect, P1)

x86
macOS
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: malte.ubl, Assigned: sayrer)

References

()

Details

(Keywords: fixed1.9.1, Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 5 obsolete files)

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; de; rv:1.9.1b2) Gecko/20081201 Firefox/3.1b2 Ubiquity/0.1.5
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; de; rv:1.9.1b2) Gecko/20081201 Firefox/3.1b2 Ubiquity/0.1.5

The spec for the JSON.parse function as defined in
http://wiki.ecmascript.org/doku.php?id=es3.1:json_support
as linked from https://developer.mozilla.org/en/JSON#JSON.c2.a0in_Firefox_3.1
provides for a second parameter that allows modifying the result of the parsing.

This feature is also implemented by the immensely popular json2.js file from json.org that is the reference implementation for the spec.

Reproducible: Always

Steps to Reproduce:
Execute this piece of JavaScript in FF 3.1
var f = function (key, value) { if(key == "a") return "b"; else return value };
alert(JSON.parse('{ "a":"c" }', f).a)

Actual Results:  
It alerted "c"

Expected Results:  
It should have alerted "b".


When executing this code in a browser without native JSON support but with loaded json2.js the result is as expected.
Executing the code in IE 8 beta (which also has native JSON support) yields the correct result.
Version: unspecified → 3.1 Branch
Assignee: nobody → general
Component: General → JavaScript Engine
Product: Firefox → Core
QA Contact: general → general
Version: 3.1 Branch → unspecified
ES3.1 draft still has this optional second function argument -- called "reviver". If IE8 has it we should too.

/be
Flags: blocking1.9.1?
yep, patch on my desk has these
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P1
Assignee: general → sayrer
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Summary: JSON.parse does not support filter argument as defined in spec → JSON.parse does not support reviver argument as defined in spec
Attached patch revive (obsolete) — Splinter Review
include patch for bug 459161. This hits what looks like a bogus assert at the bottom of js_Interpret:

JS_ASSERT(!js_IsActiveWithOrBlock(cx, fp->scopeChain, 0));

The xpcshell harness ends up with a null fp->scopeChain (parent), and js_IsActiveWithOrBlock crashes.
We don't support null scopeChains, there's always the global object at least. But if the API lets you create such a condition, there is indeed a bug. Not a bogus assert though.

/be
(In reply to comment #5)
> We don't support null scopeChains, there's always the global object at least.
> But if the API lets you create such a condition, there is indeed a bug. Not a
> bogus assert though.

I should mention, the testcase works in the js shell just fine. It's only when loaded as part of the xpcshell harness that it breaks.
function doubler(k, v) {
  if ((typeof v) == "number")
    return 2 * v;

  return v;
}

function run_test() {
  print("starting parse...\n");
  var x = JSON.parse('{"a":5,"b":6}', doubler);
}
This looks like it could be a bug in JS_ConvertArguments.

The reviver argument, argv[1], has a parent before the call. After being converted to a JSFunction by JS_ConvertArguments, it doesn't.

Here's the code I used to check:


    if ((argc > 1) && JSVAL_IS_OBJECT(argv[1])) {
        printf("arg1 parent: %p\n", OBJ_GET_PARENT(cx, JSVAL_TO_OBJECT(argv[1]));
    }

    if (!JS_ConvertArguments(cx, argc, argv, "S / f", &s, &reviver))
        return JS_FALSE;

    printf("reviver parent: %p\n", OBJ_GET_PARENT(cx, FUN_OBJECT(reviver)));

This code prints:

arg1 parent: 0x846440
reviver parent: 0x0

js_Invoke attempts to get the parent via the same mechanism, so I think this null parent is the source of the problem.
This null parent bug doesn't seem to bite in the browser either, so I'm going to assert it, use mochitests to get test coverage for this feature, and file a follow up.
(In reply to comment #8)
>     printf("reviver parent: %p\n", OBJ_GET_PARENT(cx, FUN_OBJECT(reviver)));

This isn't what you want. You really want to keep the *original* function object around and use JS_CallFunctionValue(OBJECT_TO_JSVAL(that)). The way that SpiderMonkey represents scope chains is via function.__parent__. When a function is evaluated in a new scope, we create a new object whose private data points to the original JSFunction and whose parent is the environment. Going back the the JSObject via FUN_OBJECT loses that environment.
(In reply to comment #10)
> (In reply to comment #8)
> >     printf("reviver parent: %p\n", OBJ_GET_PARENT(cx, FUN_OBJECT(reviver)));
> 
> This isn't what you want. You really want to keep the *original* function
> object around and use JS_CallFunctionValue(OBJECT_TO_JSVAL(that)). 

Oh, that is too bad. That means the typing of my API is going to be insufficient, but thanks! That's livable.
json2.js uses typeof x == "function" IIRC, and that could mean any callable native object (per ES3). In practice it means a function, but I don't see why the API needs to demand a native function object and preclude a callable (native or host) object.

The JS_ConvertArguments f format specifier should be deprecated so fast we go back in time!

/be
(In reply to comment #12)
> json2.js uses typeof x == "function" IIRC, and that could mean any callable
> native object (per ES3). 

The test in the spec is IsCallable, so that works. Will fix this.
Attached patch fix for function values (obsolete) — Splinter Review
Attachment #364724 - Attachment is obsolete: true
(In reply to comment #12)
> The JS_ConvertArguments f format specifier should be deprecated so fast we go
> back in time!

Maybe we can fix it instead?

I put a big red warning on that and on JS_ValueToFunction, which does the same thing.

But note that JS_ConvertValue does the right thing; you can choose JSTYPE_FUNCTION and it will convert the value to a function object.  GET_FUNCTION_PRIVATE is not called, so closures are not stripped of their scope.

I think we could make JS_ConvertArguments and JS_ValueToFunction do that (just cast the JSObject * to JSFunction *).  JS_GetFunctionObject would then work correctly on JSFunctions produced by these APIs.  JSAPI entry points like JS_GetFunctionId and JS_GetFunctionFlags would have to call GET_FUNCTION_PRIVATE (they don't now).

If you think this could work, I can file and write the (straightforward) patch.
Attachment #364906 - Attachment is obsolete: true
Attachment #365024 - Flags: review?(jorendorff)
Comment on attachment 365024 [details] [diff] [review]
support the optional reviver function

Pass JSVAL_NULL, not NULL, to jsval arguments.

Instead of using JS_TypeOfValue to implement ES3.1 IsCallable, use the test in js_ObjectToCallableValue (factor that out into a little inline please).

Walk should JS_CHECK_RECURSION.

For array elements, the key passed to reviver must be a string, not a number, per spec.

Is it possible to overflow INT_TO_JSID?  We would overflow the dense array implementation constraints first, but I guess it is possible.  Maybe if we're converting to string it doesn't matter.
Attachment #365024 - Flags: review?(jorendorff)
(In reply to comment #15)
> (In reply to comment #12)
> > The JS_ConvertArguments f format specifier should be deprecated so fast we go
> > back in time!
> 
> Maybe we can fix it instead?

Sounds like it's worth doing, now that it's doable thanks to Igor's work to fuse JSFunction with its compile-time JSObject allocation. Thanks,

/be
Attached patch reviver (obsolete) — Splinter Review
Attachment #365024 - Attachment is obsolete: true
Attachment #365068 - Flags: review?(jorendorff)
Attached patch reviver (obsolete) — Splinter Review
best not to be rooting uninitialized jsvals.
Attachment #365068 - Attachment is obsolete: true
Attachment #365129 - Flags: review?(jorendorff)
Attachment #365068 - Flags: review?(jorendorff)
Attachment #365129 - Attachment is obsolete: true
Attachment #365210 - Flags: review?(jorendorff)
Attachment #365129 - Flags: review?(jorendorff)
Attachment #365210 - Flags: review?(jorendorff) → review+
Attachment #365210 - Attachment is patch: true
Attachment #365210 - Attachment mime type: application/octet-stream → text/plain
Attachment #365210 - Flags: review+ → review?(jorendorff)
Comment on attachment 365210 [details] [diff] [review]
now with js_IndexToId where it's required

Kill the new blank line in jsobj.h.

Add a comment on js_IsCallable warning that it doesn't do what it looks
like (namely, functions aren't callable). I'll fix that later (as this is P1).

r=me with those changes.
Attachment #365210 - Flags: review?(jorendorff) → review+
http://hg.mozilla.org/tracemonkey/rev/0bec239f232e
Keywords: 4xp
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/0bec239f232e
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Depends on: 482861
covered by the json unit tests.
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.