Jeff Walden [:Waldo] Reporter
	Description • 16 years ago

Look six lines up.

	Robert Sayre Assignee
	Comment 1 • 16 years ago

lame, sorry. OS X debug didn't catch this. Valgrind does.

	Jeff Walden [:Waldo] Reporter
	Comment 3 • 16 years ago

Comment on attachment 366984 [details] [diff] [review]
fix

>diff --git a/js/src/json.cpp b/js/src/json.cpp
>--- a/js/src/json.cpp
>+++ b/js/src/json.cpp
>@@ -586,23 +586,24 @@ js_FinishJSONParse(JSContext *cx, JSONPa
> 
>     if (jp->buffer)
>         js_FinishStringBuffer(jp->buffer);
>     JS_free(cx, jp->buffer);
> 
>     if (!js_RemoveRoot(cx->runtime, &jp->objectStack))
>         return JS_FALSE;
>     JSBool ok = *jp->statep == JSON_PARSE_STATE_FINISHED;
>+    jsval *v = jp->rootVal;
>     JS_free(cx, jp);
> 
>     if (!ok)
>         JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_JSON_BAD_PARSE);
> 
>     if (!JSVAL_IS_PRIMITIVE(reviver) && js_IsCallable(JSVAL_TO_OBJECT(reviver), cx))
>-        ok = Revive(cx, reviver, jp->rootVal);
>+        ok = Revive(cx, reviver, v);

jp->rootVal isn't a rooted value, is it?  In the case it's called from js_json_parse jp->rootVal is the address of the return location for the function, but in the JSAPI case it could be anything.  It looks to me like the rootVal field needs to be a root, and the value you grab here has to be rooted while revive does its work.

	Robert Sayre Assignee
	Comment 4 • 16 years ago

(In reply to comment #3)
> 
> jp->rootVal isn't a rooted value, is it?  

It's designed to be rooted by the caller.

	Jeff Walden [:Waldo] Reporter
	Comment 5 • 16 years ago

Comment on attachment 366984 [details] [diff] [review]
fix

The current way things are set up smells funny, but yeah, as discussed on IRC this is a separate bug -- please file a followup.

	Robert Sayre Assignee
	Comment 6 • 16 years ago

(In reply to comment #5)
> (From update of attachment 366984 [details] [diff] [review])
> The current way things are set up smells funny, but yeah, as discussed on IRC
> this is a separate bug -- please file a followup.

482895

	Robert Sayre Assignee
	Comment 7 • 16 years ago

http://hg.mozilla.org/tracemonkey/rev/cf269afe3d21

	Robert Sayre Assignee
	Comment 8 • 16 years ago

http://hg.mozilla.org/mozilla-central/rev/cf269afe3d21

	Brendan Eich [:brendan]
	Comment 9 • 16 years ago

Tardy nit: v is canonical name for jsval; vp for jsval*. Next time...

/be

	Robert Sayre Assignee
	Comment 10 • 16 years ago

(In reply to comment #9)
> Tardy nit: v is canonical name for jsval; vp for jsval*. Next time...

fixed in bug 482899

	Bob Clary [:bc] (inactive)
	Comment 11 • 16 years ago

hint for a test?

	Jeff Walden [:Waldo] Reporter
	Comment 12 • 16 years ago

Every call to JSON.parse that doesn't hit a rare error condition like OOM will hit this; I found this by running xpcshell tests, in particular <http://mxr.mozilla.org/mozilla-central/source/dom/src/json/test/unit/test_reviver.js>, in a Windows debug build.

	Jeff Walden [:Waldo] Reporter
	Comment 13 • 16 years ago

If this problem wasn't introduced in time for 3.1b3 and hadn't yet made its way into 191 (or as soon as this fix does make it in there), I think we should open it, otherwise wait for b4 to open it.  I don't know the checkin/release timelines well enough to confidently and deliberately open or not open it, tho.

	Robert Sayre Assignee
	Comment 14 • 16 years ago

(In reply to comment #12)
> Every call to JSON.parse that doesn't hit a rare error condition like OOM will
> hit this; 

Only calls to JSON.parse with the optional reviver argument will hit this.

>I found this by running xpcshell tests, in particular
> <http://mxr.mozilla.org/mozilla-central/source/dom/src/json/test/unit/test_reviver.js>,
> in a Windows debug build.

Doesn't seem to bite in opt somehow, but we should wait till b4 to open this bug, imho.

	neil@parkwaycc.co.uk
	Comment 16 • 16 years ago

(In reply to comment #12)
> Every call to JSON.parse that doesn't hit a rare error condition like OOM will
> hit this; I found this by running xpcshell tests, in particular
> <http://mxr.mozilla.org/mozilla-central/source/dom/src/json/test/unit/test_reviver.js>,
> in a Windows debug build.
Can we get this on 1.9.1? Our unit test box is red :-(
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1238422048.1238425884.10078.gz

	Robert Sayre Assignee
	Comment 17 • 15 years ago

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/7b5a3f6ade94