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.

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.

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 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 on attachment 216234 [details] [diff] [review] patch dbradley knows the IDispatch code, it would be great to get his review all around. /be

Comment on attachment 216234 [details] [diff] [review] patch r=jst

Comment on attachment 216234 [details] [diff] [review] patch r=dbradley Looks fine.

This addresses brendan's review comments.

Checked in to trunk.

Fix checked in to MOZILLA_1_8_BRANCH.

Comment on attachment 217339 [details] [diff] [review] patch approved for 1.8.0 branch, a=dveditz for drivers

Fix checked in to MOZILLA_1_8_0_BRANCH.