Last Comment Bug 331667 - XPCConvert::NativeArray2JS should root current across call to JS_SetElement
: XPCConvert::NativeArray2JS should root current across call to JS_SetElement
: fixed1.8.0.4, fixed1.8.1
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla1.8.1
Assigned To: David Baron :dbaron: ⌚️UTC-10
: Phil Schwartau
: Andrew Overholt [:overholt]
Depends on:
Blocks: 307312
  Show dependency treegraph
Reported: 2006-03-24 22:27 PST by David Baron :dbaron: ⌚️UTC-10
Modified: 2006-04-17 16:43 PDT (History)
3 users (show)
dveditz: blocking1.8.0.4+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch (9.00 KB, patch)
2006-03-24 23:36 PST, David Baron :dbaron: ⌚️UTC-10
no flags Details | Diff | Splinter Review
patch (9.18 KB, patch)
2006-03-25 10:20 PST, David Baron :dbaron: ⌚️UTC-10
jst: review+
dbradley: review+
brendan: superreview+
Details | Diff | Splinter Review
patch (8.57 KB, patch)
2006-04-05 14:32 PDT, David Baron :dbaron: ⌚️UTC-10
jst: approval‑branch‑1.8.1+
dveditz: approval1.8.0.4+
Details | Diff | Splinter Review

Description David Baron :dbaron: ⌚️UTC-10 2006-03-24 22:27:28 PST
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-10 2006-03-24 23:36:58 PST
Created attachment 216192 [details] [diff] [review]

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-10 2006-03-25 10:20:01 PST
Created attachment 216234 [details] [diff] [review]

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] 2006-03-25 12:12:28 PST
Comment on attachment 216234 [details] [diff] [review]

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

Comment 4 Brendan Eich [:brendan] 2006-03-25 12:13:50 PST
Comment on attachment 216234 [details] [diff] [review]

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

Comment 5 Johnny Stenback (:jst, 2006-03-28 14:49:38 PST
Comment on attachment 216234 [details] [diff] [review]

Comment 6 David Bradley 2006-04-05 14:14:56 PDT
Comment on attachment 216234 [details] [diff] [review]


Looks fine.
Comment 7 David Baron :dbaron: ⌚️UTC-10 2006-04-05 14:32:51 PDT
Created attachment 217339 [details] [diff] [review]

This addresses brendan's review comments.
Comment 8 David Baron :dbaron: ⌚️UTC-10 2006-04-05 14:34:26 PDT
Checked in to trunk.
Comment 9 David Baron :dbaron: ⌚️UTC-10 2006-04-07 18:15:33 PDT
Fix checked in to MOZILLA_1_8_BRANCH.
Comment 10 Daniel Veditz [:dveditz] 2006-04-17 12:14:06 PDT
Comment on attachment 217339 [details] [diff] [review]

approved for 1.8.0 branch, a=dveditz for drivers
Comment 11 David Baron :dbaron: ⌚️UTC-10 2006-04-17 16:43:41 PDT
Fix checked in to MOZILLA_1_8_0_BRANCH.

Note You need to log in before you can comment on or make changes to this bug.