331667 - XPCConvert::NativeArray2JS should root current across call to JS_SetElement

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

XPCConvert::NativeArray2JS should root current across call to JS_SetElement -- brendan says it's the responsibility of the caller to root vp.  If it doesn't it can be destroyed here:

js_GC (/home/dbaron/builds/trunk/mozilla/js/src/jsgc.c:2147)
js_NewGCThing (/home/dbaron/builds/trunk/mozilla/js/src/jsgc.c:668)
AllocSlots (/home/dbaron/builds/trunk/mozilla/js/src/jsobj.c:1936)
js_AllocSlot (/home/dbaron/builds/trunk/mozilla/js/src/jsobj.c:2290)
js_AddScopeProperty (/home/dbaron/builds/trunk/mozilla/js/src/jsscope.c:1127)
js_SetProperty (/home/dbaron/builds/trunk/mozilla/js/src/jsobj.c:3199)
XPCConvert::NativeArray2JS(XPCCallContext&, long*, void const**, nsXPTType const&, nsID const*, unsigned int, JSObject*, unsigned int*) (/home/dbaron/builds/trunk/mozilla/js/src/xpconnect/src/xpcconvert.cpp:1661)

This causes a crash on startup with WAY_TOO_MUCH_GC.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

Here's a patch that fixes this crash (and lets me start up with WAY_TOO_MUCH_GC again).  All of the changes except the one at the site described in comment 0 were made on the basis of code inspection only (I did a bit of code inspection in js/src/xpconnect/src/ for similar problems, although not all that thorough).  I don't have a whole lot of confidence that they're really needed, so please review this carefully.  I haven't even compiled the XPCDisp* stuff, and I wouldn't know how to test it if I were on Windows anyway.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

Thanks to timeless for reminding me how to do the macro trick that I was trying to do yesterday (with one level too few macros, since I was forgetting that I needed the double macros *inside* my macro, for three in total).

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 216234 [details] [diff] [review]
patch

>Index: XPCDispConvert.cpp
>===================================================================
>RCS file: /cvs/mozilla/js/src/xpconnect/src/XPCDispConvert.cpp,v
>retrieving revision 1.13
>diff -p -u -1 -2 -r1.13 XPCDispConvert.cpp
>--- XPCDispConvert.cpp	31 Jan 2006 01:47:01 -0000	1.13
>+++ XPCDispConvert.cpp	25 Mar 2006 18:14:52 -0000
>@@ -393,59 +393,62 @@ JSBool XPCDispConvert::COMArrayToJSArray
>     if(FAILED(SafeArrayGetLBound(src.parray, 1, &lbound)))
>     {
>         err = NS_ERROR_FAILURE;
>         return JS_FALSE;
>     }
>     // Create the JS Array
>     JSObject * array = JS_NewArrayObject(ccx, ubound - lbound + 1, nsnull);
>     if(!array)
>     {
>         err = NS_ERROR_OUT_OF_MEMORY;
>         return JS_FALSE;
>     }
>+    jsval d = OBJECT_TO_JSVAL(array);
>+    AUTO_MARK_JSVAL(ccx, d);

Why not pass &d here?  AUTO_MARK_JSVAL can work with value or pointer (if value, it tracks its own jsval storage by address).

>     // Devine the type of our array

Please feel free to fix the misspelled word here.  Also, a blank line before this comment would be easier on the eyes -- I know the prior revision ran the closing brace line up against this comment, but that was visually less dense than with the inserted AUTO_MARK_JSVAL code.

>+            AUTO_MARK_JSVAL(ccx, val);

Another lack of &val that seems inconsistent.  When in doubt, and when you already need a jsval local outside of the macro, use &.  Otherwise, avoid the explicit local and just pass OBJECT_TO_JSVAL(foo) into the macro.

The other uses I saw in the patch used &, AFAICT in a hurry.

Thanks for patching this, sr=me in advance with nits picked.

/be

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 216234 [details] [diff] [review]
patch

dbradley knows the IDispatch code, it would be great to get his review all around.

/be

Comment 5

[Johnny Stenback (:jst)]

Comment on attachment 216234 [details] [diff] [review]
patch

r=jst

Comment 6

[David Bradley]

Comment on attachment 216234 [details] [diff] [review]
patch

r=dbradley

Looks fine.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

This addresses brendan's review comments.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Checked in to trunk.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fix checked in to MOZILLA_1_8_BRANCH.

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 217339 [details] [diff] [review]
patch

approved for 1.8.0 branch, a=dveditz for drivers

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fix checked in to MOZILLA_1_8_0_BRANCH.