462778 - top crash [@ 0x20202020][@ xpsp2res.dll@0x202113][@ js_ConsumeJSONText]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Samuel Sidler (old account; do not CC)]

(fwiw, I wouldn't consider this bug blocking until after beta 2, if the report appears then.)

The number 2 and number 8 topcrashes currently happen [@ 0x20202020] and [@ xpsp2res.dll@0x202113]. The crashing thread is almost identical on all crash reports, with the exception of frame 2 which can vary (mostly OpenObject or PushObject, sometimes HandleData).

Sample report (OpenObject) from bp-0ff9c05c-a67c-11dd-85a5-001a4bd43ef6:

Frame  	Module  	Signature [Expand]  	Source
0 	xpsp2res.dll 	xpsp2res.dll@0x202113 	
1 	js3250.dll 	OpenObject 	js/src/json.cpp:535
2 	js3250.dll 	js_ConsumeJSONText 	js/src/json.cpp:728
3 	js3250.dll 	js_json_parse 	js/src/json.cpp:81
4 	js3250.dll 	js_Interpret 	js/src/jsinterp.cpp:4985
5 	js3250.dll 	js_Invoke 	js/src/jsinterp.cpp:1324
6 	js3250.dll 	js_fun_apply 	js/src/jsfun.cpp:1731
7 	js3250.dll 	js_Interpret 	js/src/jsinterp.cpp:4985
8 	js3250.dll 	js_Invoke 	js/src/jsinterp.cpp:1324
9 	js3250.dll 	JS_CallFunctionValue 	js/src/jsapi.cpp:5136
10 	xul.dll 	nsJSContext::CallEventHandler 	dom/src/base/nsJSEnvironment.cpp:2000
11 	xul.dll 	nsGlobalWindow::RunTimeout 	dom/src/base/nsGlobalWindow.cpp:7721
12 	xul.dll 	nsGlobalWindow::TimerCallback 	dom/src/base/nsGlobalWindow.cpp:8053
13 	xul.dll 	nsTimerImpl::Fire 	xpcom/threads/nsTimerImpl.cpp:420
14 	xul.dll 	nsTimerEvent::Run 	xpcom/threads/nsTimerImpl.cpp:512
15 	xul.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:510
16 	xul.dll 	nsBaseAppShell::Run 	widget/src/xpwidgets/nsBaseAppShell.cpp:170
17 	nspr4.dll 	PR_GetEnv 	
18 	firefox.exe 	wmain 	toolkit/xre/nsWindowsWMain.cpp:87
19 	firefox.exe 	firefox.exe@0x2197 	
20 	kernel32.dll 	BaseProcessStart

Another sample (HandleData instead of OpenObject), from bp-b4e0b9c7-a88a-11dd-8089-001321b13766:

Frame  	Module  	Signature [Expand]  	Source
0 	xpsp2res.dll 	xpsp2res.dll@0x202113 	
1 	js3250.dll 	HandleData 	js/src/json.cpp:661
2 	js3250.dll 	js_ConsumeJSONText 	js/src/json.cpp:865
3 	js3250.dll 	js_json_parse 	js/src/json.cpp:81
4 	js3250.dll 	js_Interpret 	js/src/jsinterp.cpp:4985
5 	js3250.dll 	js_Invoke 	js/src/jsinterp.cpp:1324
6 	js3250.dll 	js_fun_apply 	js/src/jsfun.cpp:1731
7 	js3250.dll 	js_Interpret 	js/src/jsinterp.cpp:4985
8 	js3250.dll 	js_Invoke 	js/src/jsinterp.cpp:1324
9 	js3250.dll 	JS_CallFunctionValue 	js/src/jsapi.cpp:5136
10 	xul.dll 	nsJSContext::CallEventHandler 	dom/src/base/nsJSEnvironment.cpp:2000
11 	xul.dll 	nsGlobalWindow::RunTimeout 	dom/src/base/nsGlobalWindow.cpp:7721
12 	xul.dll 	nsGlobalWindow::TimerCallback 	dom/src/base/nsGlobalWindow.cpp:8053
13 	xul.dll 	nsTimerImpl::Fire 	xpcom/threads/nsTimerImpl.cpp:420
14 	xul.dll 	nsTimerEvent::Run 	xpcom/threads/nsTimerImpl.cpp:512
15 	xul.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:510
16 	xul.dll 	nsBaseAppShell::Run 	widget/src/xpwidgets/nsBaseAppShell.cpp:170
17 	nspr4.dll 	PR_GetEnv 	
18 	firefox.exe 	wmain 	toolkit/xre/nsWindowsWMain.cpp:87
19 	firefox.exe 	firefox.exe@0x2197 	
20 	kernel32.dll 	BaseProcessStart

Sample report (PushObject) from bp-0aed55dc-a856-11dd-b265-001a4bd43ed6:

Frame  	Module  	Signature [Expand]  	Source
0 		@0x20202020 	
1 	libmozjs.dylib 	PushObject 	js/src/json.cpp:499
2 	libmozjs.dylib 	js_ConsumeJSONText 	js/src/json.cpp:535
3 	libmozjs.dylib 	js_json_parse 	js/src/json.cpp:81
4 	libmozjs.dylib 	js_Interpret 	js/src/jsinterp.cpp:4985
5 	libmozjs.dylib 	js_Invoke 	js/src/jsinvoke.cpp:1324
6 	libmozjs.dylib 	js_fun_apply 	js/src/jsfun.cpp:1731
7 	libmozjs.dylib 	js_Interpret 	js/src/jsinterp.cpp:4985
8 	libmozjs.dylib 	js_Invoke 	js/src/jsinvoke.cpp:1324
9 	libmozjs.dylib 	js_InternalInvoke 	js/src/jsinvoke.cpp:1381
10 	libmozjs.dylib 	JS_CallFunctionValue 	js/src/jsapi.cpp:5136
11 	XUL 	nsJSContext::CallEventHandler 	dom/src/base/nsJSEnvironment.cpp:2000
12 	XUL 	nsGlobalWindow::RunTimeout 	dom/src/base/nsGlobalWindow.cpp:7721
13 	XUL 	nsGlobalWindow::TimerCallback 	dom/src/base/nsGlobalWindow.cpp:8053
14 	XUL 	nsTimerImpl::Fire 	xpcom/threads/nsTimerImpl.cpp:420
15 	XUL 	nsTimerEvent::Run 	xpcom/threads/nsTimerImpl.cpp:512
16 	XUL 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:510
17 	XUL 	NS_ProcessPendingEvents_P 	nsThreadUtils.cpp:180
18 	XUL 	nsBaseAppShell::NativeEventCallback 	widget/src/xpwidgets/nsBaseAppShell.cpp:121
19 	XUL 	nsAppShell::ProcessGeckoEvents 	widget/src/cocoa/nsAppShell.mm:302
20 	CoreFoundation 	CoreFoundation@0x72614 	
21 	CoreFoundation 	CoreFoundation@0x72cf7 	
22 	HIToolbox 	HIToolbox@0x2f47f 	
23 	HIToolbox 	HIToolbox@0x2f298 	
24 	HIToolbox 	HIToolbox@0x2f10c 	
25 	AppKit 	AppKit@0x403ec 	
26 	AppKit 	AppKit@0x3fc9f 	
27 	JavaEmbeddingPlugin 	JavaEmbeddingPlugin@0x1341e 	
28 	AppKit 	AppKit@0x38cda 	
29 	XUL 	nsAppShell::Run 	widget/src/cocoa/nsAppShell.mm:591
30 	XUL 	nsAppStartup::Run 	toolkit/components/startup/src/nsAppStartup.cpp:182
31 	XUL 	XRE_main 	toolkit/xre/nsAppRunner.cpp:3265
32 	firefox-bin 	main 	browser/app/nsBrowserApp.cpp:156
33 	firefox-bin 	_start 	
34 	firefox-bin 	start 	
35 		@0x1

I need to do more investigation of these, but they don't appear at all in nightlies after Firefox 3.1b1. That's weird, but not unheard of. I don't think it got fixed along the way, so I need to figure out what caused it to disappear, if anything.

As I said in my first comment, I don't consider this blocking until we see it again in beta 2 (if we do at all). However, I'm nominating to get it on some radars and so drivers can make their own determination.

Comment 1

[timeless]

http://mxr.mozilla.org/mozilla-central/source/js/src/json.cpp?mark=195-196,347l356,362,225,233,235,249,545,549,514,518,646-647

this can fail:
195             JSString *us = JS_NewStringCopyN(cx, ubuf, len);
this would crash:
196             if (!callback(JS_GetStringChars(us), len, data))

these can fail:
347                         outputString = JS_NewStringCopyN(cx, "null", 4);
356                 outputString = JS_NewStringCopyN(cx, "null", 4);
this would crash:
362             ok = callback(JS_GetStringChars(outputString), JS_GetStringLength(outputString), data);

i don't think iterObj is properly rooted:
225     JSObject *iterObj = NULL;
233         if (!js_ValueToIterator(cx, JSITER_ENUMERATE, vp))
235         iterObj = JSVAL_TO_OBJECT(*vp);
239     JSAutoTempValueRooter tvr(cx, 1, &outputValue);
i think it could die here:
249             ok = JS_GetElement(cx, obj, i++, &outputValue);

i don't think obj is properly rooted:
545     JSObject *obj = js_NewObject(cx, &js_ObjectClass, NULL, NULL, 0);
549     return PushObject(cx, jp, obj);
i expect it could die here:
514     if (!JS_GetElement(cx, jp->objectStack, len - 1, &p))
before being needed here:
518     if (!PushValue(cx, jp, parent, OBJECT_TO_JSVAL(obj)))

this could fail:
646       jp->objectKey = js_NewStringCopyN(cx, buf, len);
in which case this should not be executed:
647       ok = JS_TRUE;

Comment 2

[Brendan Eich [:brendan]]

(In reply to comment #1)
> http://mxr.mozilla.org/mozilla-central/source/js/src/json.cpp?mark=195-196,347l356,362,225,233,235,249,545,549,514,518,646-647
> 
> this can fail:
> 195             JSString *us = JS_NewStringCopyN(cx, ubuf, len);
> this would crash:
> 196             if (!callback(JS_GetStringChars(us), len, data))
> 
> these can fail:
> 347                         outputString = JS_NewStringCopyN(cx, "null", 4);
> 356                 outputString = JS_NewStringCopyN(cx, "null", 4);
> this would crash:
> 362             ok = callback(JS_GetStringChars(outputString),
> JS_GetStringLength(outputString), data);
. . .
> this could fail:
> 646       jp->objectKey = js_NewStringCopyN(cx, buf, len);
> in which case this should not be executed:
> 647       ok = JS_TRUE;

Good points.

> i don't think iterObj is properly rooted:
> 225     JSObject *iterObj = NULL;
> 233         if (!js_ValueToIterator(cx, JSITER_ENUMERATE, vp))
> 235         iterObj = JSVAL_TO_OBJECT(*vp);

This code looks similar to dom/src/json code I once reviewed, but you're right that the vp supplied by js_json_stringify is not rooted.

> 239     JSAutoTempValueRooter tvr(cx, 1, &outputValue);

Why pick on this line? It's unrelated and of course it roots outputValue.

> i think it could die here:
> 249             ok = JS_GetElement(cx, obj, i++, &outputValue);

Why?

> i don't think obj is properly rooted:
> 545     JSObject *obj = js_NewObject(cx, &js_ObjectClass, NULL, NULL, 0);
> 549     return PushObject(cx, jp, obj);
> i expect it could die here:
> 514     if (!JS_GetElement(cx, jp->objectStack, len - 1, &p))
> before being needed here:
> 518     if (!PushValue(cx, jp, parent, OBJECT_TO_JSVAL(obj)))

Maybe, but jp->objectStack is a rooted Array instance. It is dense meaning all elements from 0 up to len are direct, so no prototype getter could run under that JS_GetElement call. So where would GC nest here?

That aside, it's dicey to pass an unrooted obj across a number of API out-calls. Rooting as you go is much safer.

I also recalled proposing using OBJ_[GS]ET_PROPERTY directly from json.cpp, cutting out the API middleman, but I didn't follow up on that. Sorry if I missed things during review -- looking at json.cpp now I see code that I do not recall reviewing. We should make a pass over it with IRC review and attach the diff here.

Unrelated nit:

        JS_ReportError(cx, "Error parsing JSON.");

Error messages do not contain full-stop punctuation. Error reporters may do that however, and we don't want .. at the end.

/be

Comment 3

[Robert Sayre]

>
> I also recalled proposing using OBJ_[GS]ET_PROPERTY directly from json.cpp,
> cutting out the API middleman, but I didn't follow up on that.

I couldn't find this comment anywhere, maybe it was in irc? 

> Sorry if I
> missed things during review -- looking at json.cpp now I see code that I do not
> recall reviewing.

Other people reviewed most of the recent changes to this file, see the log:

http://hg.mozilla.org/mozilla-central/log/93813ce86c71/js/src/json.cpp

Comment 4

[Robert Sayre]

Attachment: first cut, fix timeless' comments, change to JSStringBuffer for key string

Comment 5

[Robert Sayre]

Comment on attachment 346090 [details] [diff] [review]
first cut, fix timeless' comments, change to JSStringBuffer for key string

I was able to reproduce a stringify crash with gczeal, and this patch does fix it.

Comment 6

[Robert Sayre]

I think I addressed all but one comment, and that did fix at least one crash.

I also switch objectKey to be a JSStringBuffer, rather than a JSString.

The remaining comment is here:

> > i don't think obj is properly rooted:
> > 545     JSObject *obj = js_NewObject(cx, &js_ObjectClass, NULL, NULL, 0);
> > 549     return PushObject(cx, jp, obj);
> > i expect it could die here:
> > 514     if (!JS_GetElement(cx, jp->objectStack, len - 1, &p))
> > before being needed here:
> > 518     if (!PushValue(cx, jp, parent, OBJECT_TO_JSVAL(obj)))
> 
> Maybe, but jp->objectStack is a rooted Array instance. It is dense meaning all
> elements from 0 up to len are direct, so no prototype getter could run under
> that JS_GetElement call. So where would GC nest here?

If jp->objectStack gets long enough, it will become sparse, right?

> 
> That aside, it's dicey to pass an unrooted obj across a number of API
> out-calls. Rooting as you go is much safer.
> 
> I also recalled proposing using OBJ_[GS]ET_PROPERTY directly from json.cpp,
> cutting out the API middleman, but I didn't follow up on that.

I'm not quite sure what's being recommended here.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #6)
> If jp->objectStack gets long enough, it will become sparse, right?

Yes.

> I'm not quite sure what's being recommended here.

Instead of using JS_GetProperty, use OBJ_GET_PROPERTY (which has a different signature but avoids the relocatable function call penalty).

Comment 8

[Brendan Eich [:brendan]]

(In reply to comment #6)
> > Maybe, but jp->objectStack is a rooted Array instance. It is dense meaning all
> > elements from 0 up to len are direct, so no prototype getter could run under
> > that JS_GetElement call. So where would GC nest here?
> 
> If jp->objectStack gets long enough, it will become sparse, right?

No, not in the sparse as opposite of "dense meaning all elements from 0 up to len are direct" sense.

> I'm not quite sure what's being recommended here.

What mrbkap said -- we talked about this briefly on IRC, IIRC :-P.

/be

Comment 9

[Robert Sayre]

Attachment: switched to property macros and root in PushObject

Comment 10

[Robert Sayre]

Comment on attachment 346154 [details] [diff] [review]
switched to property macros and root in PushObject

I think this addresses all comments. I left two public API calls to JS_[Get/Set]UCProperty. It looks like I would have to duplicate their bodies, and they are not the cause of this bug anyway.

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 346154 [details] [diff] [review]
switched to property macros and root in PushObject

Not in context:

static JSBool
WriteCallback(const jschar *buf, uint32 len, void *data)
{
    StringifyClosure *sc = static_cast<StringifyClosure*>(data);
    JSString *s1 = JSVAL_TO_STRING(*sc->s);
    JSString *s2 = js_NewStringCopyN(sc->cx, buf, len);
    if (!s2)
        return JS_FALSE;

    s1 = js_ConcatStrings(sc->cx, s1, s2);
    if (!s1)
        return JS_FALSE;

    *sc->s = STRING_TO_JSVAL(s1);
    return JS_TRUE;
}

What keeps s2 rooted under js_ConcatStrings? It may be that only the newborn root for string type (see cx->weakRoots) does. This may be enough, but you are hanging by a thread.

Alternative: make StringifyClosure have a jsval s[2]; array member, and derive that class from JSAutoTempValueRooter.

>    jsval v = OBJECT_TO_JSVAL(obj);
>    JSBool ok = js_TryJSON(cx, &v);
>    JSType type;

That &v is a warning sign. You are passing an unrooted jsval by its address. Does js_TryJSON need its vp param to be a root? Maybe not, since obj came from vp[2] and that's a root. It would be better to avoid jsval v here and use vp (&vp[0], I mean, but spell it vp) directly. This is the place the return value goes; on entry it refers to the callee (the js_json_stringify native function object). So long as you no longer need the callee, you can store the r.v. (or an intermediate result) in *vp.

>     if (!(ok && !JSVAL_IS_PRIMITIVE(v) &&
>           (type = JS_TypeOfValue(cx, v)) != JSTYPE_FUNCTION &&
>           type != JSTYPE_XML)) {

I found the De Morgan conversion easier to read:

>     if (!ok ||
>         JSVAL_IS_PRIMITIVE(v) ||
>         (type = JS_TypeOfValue(cx, v)) == JSTYPE_FUNCTION ||
>         type == JSTYPE_XML)) {

YMMV but those extra parens and the mental need to keep holding the outer ! at bay until the end, avoiding double-negative misreadings of the inner != ops, seem strictly more complicated.

>     if (ok) {
>         jsval sv = STRING_TO_JSVAL(s);
>         StringifyClosure sc(cx, &sv);
>-        JSAutoTempValueRooter tvr(cx, 1, sc.s);
>+        JSAutoTempValueRooter resultTvr(cx, 1, sc.s);
>         ok = js_Stringify(cx, &v, NULL, &WriteCallback, &sc, 0);

v is not rooted, but you pass its address here -- again, this is always a warning sign (we had a static analysis under way to find all such). It is used as vp in js_Stringify to hold the result of js_ValueToIterator. Suggest you pass along vp here.

>@@ -188,16 +190,18 @@ write_string(JSContext *cx, JSONWriteCal
>         } else if (buf[i] <= 31 || buf[i] == 127) {
>             if (!callback(&buf[mark], i - mark, data) || !callback(unicodeEscape, 4, data))
>                 return JS_FALSE;
>             char ubuf[10];
>             unsigned int len = JS_snprintf(ubuf, sizeof(ubuf), "%.2x", buf[i]);
>             JS_ASSERT(len == 2);
>             // TODO: don't allocate a JSString just to inflate (js_InflateStringToBuffer on static?)

Should be a FIXME: bug nnnnnn. A blank line before would help readability and match common style.

>+            ok = OBJ_GET_PROPERTY(cx, obj, INT_TO_JSID(i++), &outputValue);

Macrology is currently safe but it could (debug only?) grow hair that evaluates args more than once. Suggest putting the i++ after on its own line.

>+    if (jp->objectKey)
>+        js_FinishStringBuffer(jp->objectKey);
>+    JS_free(cx, jp->objectKey);

We add (void *) casts to JS_free params elsewhere, perhaps for old C compilers. If you get no warning in any tier 1 C++ compiler and build combo, then never mind (or do mind: we should remove all the old-school (void *) casts; need a bug).

>+            ok = OBJ_DEFINE_PROPERTY(cx, parent, INT_TO_JSID(len), value,
>+                                     NULL, NULL, JSPROP_ENUMERATE, NULL);
>+        }
>     } else {
>-        ok = JS_DefineUCProperty(cx, parent, JS_GetStringChars(jp->objectKey),
>-                                 JS_GetStringLength(jp->objectKey), value,
>+        ok = JS_DefineUCProperty(cx, parent, jp->objectKey->base,
>+                                 STRING_BUFFER_OFFSET(jp->objectKey), value,
>                                  NULL, NULL, JSPROP_ENUMERATE);
[snip]
> PushObject(JSContext *cx, JSONParser *jp, JSObject *obj)
> {
>     jsuint len;
>     if (!js_GetLengthProperty(cx, jp->objectStack, &len))
>         return JS_FALSE;
>     if (len >= JSON_MAX_DEPTH)
>         return JS_FALSE; // decoding error
> 
>     jsval v = OBJECT_TO_JSVAL(obj);
>+    JSAutoTempValueRooter tvr(cx, v);
> 
>     // Check if this is the root object
>     if (len == 0) {
>         *jp->rootVal = v;
>-        if (!JS_SetElement(cx, jp->objectStack, 0, jp->rootVal))
>+        if (!OBJ_DEFINE_PROPERTY(cx, jp->objectStack, INT_TO_JSID(0), *jp->rootVal,
>+                                 NULL, NULL, NULL, NULL)) {

Next-to-last NULL should be 0.

>             return JS_FALSE;
>+        }
>         return JS_TRUE;
>     }
> 
>     jsval p;
>-    if (!JS_GetElement(cx, jp->objectStack, len - 1, &p))
>+    if (!OBJ_GET_PROPERTY(cx, jp->objectStack, INT_TO_JSID(len - 1), &p))
>         return JS_FALSE;
>+
>     JS_ASSERT(JSVAL_IS_OBJECT(p));
>     JSObject *parent = JSVAL_TO_OBJECT(p);
>     if (!PushValue(cx, jp, parent, OBJECT_TO_JSVAL(obj)))
>         return JS_FALSE;
> 
>-    if (!JS_SetElement(cx, jp->objectStack, len, &v))
>+    if (!OBJ_DEFINE_PROPERTY(cx, jp->objectStack, INT_TO_JSID(len), v,
>+                             NULL, NULL, NULL, NULL)) {

Ditto.

>+void
>+js_AppendUCString(JSStringBuffer *sb, const jschar *buf, uintN len)
>+{
>+    jschar *bp;
>+
>+    if (!STRING_BUFFER_OK(sb))
>+        return;
>+
>+    if (len == 0 || !ENSURE_STRING_BUFFER(sb, len))
>+        return;
>+
>+    bp = sb->ptr;
>+    js_strncpy(bp, buf, len);
>+    bp += len;
>+    *bp = 0;
>+    sb->ptr = bp;

Blank lines were not in original (js_AppendJSString) code, could lose without too much vertical cuddliness in my book (but it's a fine line). Would match nearby code better, at any rate.

>+}
>+
> #if JS_HAS_XML_SUPPORT
> 
> void
> js_RepeatChar(JSStringBuffer *sb, jschar c, uintN count)
> {
>     jschar *bp;
> 
>     if (!STRING_BUFFER_OK(sb) || count == 0)
>@@ -781,29 +799,17 @@ js_AppendCString(JSStringBuffer *sb, con
>         *bp++ = (jschar) *asciiz++;
>     *bp = 0;
>     sb->ptr = bp;
> }
> 
> void
> js_AppendJSString(JSStringBuffer *sb, JSString *str)
> {
>-    size_t length;
>-    jschar *bp;
>-
>-    if (!STRING_BUFFER_OK(sb))
>-        return;
>-    length = JSSTRING_LENGTH(str);
>-    if (length == 0 || !ENSURE_STRING_BUFFER(sb, length))
>-        return;
>-    bp = sb->ptr;
>-    js_strncpy(bp, JSSTRING_CHARS(str), length);
>-    bp += length;
>-    *bp = 0;
>-    sb->ptr = bp;
>+    js_AppendUCString(sb, JSSTRING_CHARS(str), JSSTRING_LENGTH(str));

(This nearby code.)

Rooting is hard, it's important to use a pattern-y style (jsval *vp in final or otherwise canonical parameter position, always passed a root). Please patch more code, if other places in json.cpp need this too.

/be

Comment 12

[Brendan Eich [:brendan]]

(In reply to comment #11)
> Alternative: make StringifyClosure have a jsval s[2]; array member, and derive
> that class from JSAutoTempValueRooter.

And of course the ctor super-constructs with len=2 and vec=s.

/be

Comment 13

[Robert Sayre]

(In reply to comment #11)
> >    jsval v = OBJECT_TO_JSVAL(obj);
> >    JSBool ok = js_TryJSON(cx, &v);
> >    JSType type;
> 
> That &v is a warning sign. You are passing an unrooted jsval by its address.

For the record, my attachment has this value rooted (the source was pasted from the wrong place). The vp alternative is better, so we'll go with that anyway.

Comment 14

[Brendan Eich [:brendan]]

Yes, sorry for the mis-copied source -- I deleted too much of the patch and then went to vi on tip json.cpp, but using vp is still winning.

/be

Comment 15

[Robert Sayre]

Attachment: address comments, hopefully

Comment 16

[Robert Sayre]

(In reply to comment #11)

> We add (void *) casts to JS_free params elsewhere, perhaps for old C compilers.
> If you get no warning in any tier 1 C++ compiler and build combo, then never
> mind (or do mind: we should remove all the old-school (void *) casts; need a
> bug).

Filed bug 463717 on this issue. MXR says we only cast about half the time, so it's probably no longer needed.

Comment 17

[Brendan Eich [:brendan]]

Comment on attachment 346967 [details] [diff] [review]
address comments, hopefully

>+            // Fix me bug 463715: don't allocate a JSString just to inflate

s/Fix me /FIXME: / for grep-ability by humans as well as computers -- FIXME is the new XXX.

>@@ -475,69 +501,79 @@ PushValue(JSContext *cx, JSONParser *jp,
> PushValue(JSContext *cx, JSONParser *jp, JSObject *parent, jsval value)
> {
>     JSAutoTempValueRooter tvr(cx, 1, &value);
>  
>     JSBool ok;
>     if (OBJ_IS_ARRAY(cx, parent)) {
>         jsuint len;
>         ok = js_GetLengthProperty(cx, parent, &len);
>-        if (ok)
>-            ok = JS_SetElement(cx, parent, len, &value);
>+        if (ok) {
>+            ok = OBJ_DEFINE_PROPERTY(cx, parent, INT_TO_JSID(len), value,
>+                                     NULL, NULL, JSPROP_ENUMERATE, NULL);

So JSON arrays are fully dense (no notation for holes). Any that don't get super-big and fall out of shavarray denseness will have array_defineProperty as the OBJ_DEFINE_PROPERTY impl here. That just devolves to array_setProperty, so it might be better to use OBJ_SET_PROPERTY here.

If you didn't have the JSPROP_ENUMERATE sole attribute flag, array_defineProperty would make the array slow.

There's no prototype setter hazard in-language, because in-language means of defining setters do not make those setters JSPROP_SHARED.

>+        }
>     } else {
>-        ok = JS_DefineUCProperty(cx, parent, JS_GetStringChars(jp->objectKey),
>-                                 JS_GetStringLength(jp->objectKey), value,
>+        ok = JS_DefineUCProperty(cx, parent, jp->objectKey->base,
>+                                 STRING_BUFFER_OFFSET(jp->objectKey), value,
>                                  NULL, NULL, JSPROP_ENUMERATE);

If the OBJ_IS_ARRAY branch uses OBJ_SET_PROPERTY, this could use JS_SetUCProperty. Define is better here, mainly in reducing reader concern about what setters (SHARED, even?) might be lurking.

But sticking with JS_DefineUCProperty here slightly favors OBJ_DEFINE_PROPERTY in the OBJ_IS_ARRAY case. If you do that, please comment on the need for JSPROP_ENUMERATE (and only that attr) in order to preserve shav-density.

> static JSBool
> PushObject(JSContext *cx, JSONParser *jp, JSObject *obj)
> {
>     jsuint len;
>     if (!js_GetLengthProperty(cx, jp->objectStack, &len))
>         return JS_FALSE;
>     if (len >= JSON_MAX_DEPTH)
>         return JS_FALSE; // decoding error
> 
>     jsval v = OBJECT_TO_JSVAL(obj);
>+    JSAutoTempValueRooter tvr(cx, v);
> 
>     // Check if this is the root object
>     if (len == 0) {
>         *jp->rootVal = v;
>-        if (!JS_SetElement(cx, jp->objectStack, 0, jp->rootVal))
>+        if (!OBJ_DEFINE_PROPERTY(cx, jp->objectStack, INT_TO_JSID(0), *jp->rootVal,
>+                                 NULL, NULL, 0, NULL)) {

No JSPROP_ENUMERATE makes objectStack slow/sparse, d'oh. Sorry I didn't catch this. Since non-shared JS-defined setters are no threat, this could go back to OBJ_SET_PROPERTY. Or it could grow JSPROP_ENUMERATE, but in that case it must have a comment about why.

>+    if (!OBJ_DEFINE_PROPERTY(cx, jp->objectStack, INT_TO_JSID(len), v,
>+                             NULL, NULL, 0, NULL)) {

Ditto.

Looks good otherwise!

/be

Comment 18

[Robert Sayre]

Attachment: keep arrays dense, fix nits

Comment 19

[Brendan Eich [:brendan]]

Comment on attachment 346973 [details] [diff] [review]
keep arrays dense, fix nits

r=me, thanks.

/be

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Both mentioned stacks from comment 0 aren't listed anymore as topcrashers. Even no results are listed when querying for the last two weeks on Socorro. I think we can safely mark this bug as verified.