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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Malte]

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.

Comment 1

[Brendan Eich [:brendan]]

ES3.1 draft still has this optional second function argument -- called "reviver". If IE8 has it we should too.

/be

Comment 2

[Robert Sayre]

yep, patch on my desk has these

Comment 3

[Mike Shaver (:shaver emeritus)]

ETA for that patch?

Comment 4

[Robert Sayre]

Attachment: revive

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.

Comment 5

[Brendan Eich [:brendan]]

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

Comment 6

[Robert Sayre]

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

Comment 7

[Robert Sayre]

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);
}

Comment 8

[Robert Sayre]

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.

Comment 9

[Robert Sayre]

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.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

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

Comment 11

[Robert Sayre]

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

Comment 12

[Brendan Eich [:brendan]]

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

Comment 13

[Robert Sayre]

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

Comment 14

[Robert Sayre]

Attachment: fix for function values

Comment 15

[Jason Orendorff [:jorendorff]]

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

Comment 16

[Robert Sayre]

Attachment: support the optional reviver function

Comment 17

[Jason Orendorff [: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.

Comment 18

[Brendan Eich [:brendan]]

(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

Comment 19

[Robert Sayre]

Attachment: reviver

Comment 20

[Robert Sayre]

Attachment: reviver

best not to be rooting uninitialized jsvals.

Comment 21

[Robert Sayre]

Attachment: now with js_IndexToId where it's required

Comment 22

[Jason Orendorff [: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.

Comment 23

[Robert Sayre]

http://hg.mozilla.org/tracemonkey/rev/0bec239f232e

Comment 24

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/0bec239f232e

Comment 25

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5f88bd7d7899

Comment 26

[Bob Clary [:bc] (inactive)]

covered by the json unit tests.