XPCConvert::NativeArray2JS should root current across call to JS_SetElement

RESOLVED FIXED in mozilla1.8.1

Status

()

Core
XPConnect
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

({fixed1.8.0.4, fixed1.8.1})

Trunk
mozilla1.8.1
fixed1.8.0.4, fixed1.8.1
Points:
---
Bug Flags:
blocking1.8.0.4 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [patch])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

12 years ago
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.
(Assignee)

Updated

12 years ago
Status: NEW → ASSIGNED
Flags: blocking1.8.1?
Flags: blocking1.8.0.3?
Whiteboard: [patch]
Target Milestone: --- → mozilla1.8.1
(Assignee)

Comment 1

12 years ago
Created attachment 216192 [details] [diff] [review]
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.
Attachment #216192 - Flags: superreview?(brendan)
Attachment #216192 - Flags: review?(jst)
(Assignee)

Comment 2

12 years ago
Created attachment 216234 [details] [diff] [review]
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).
Attachment #216192 - Attachment is obsolete: true
Attachment #216234 - Flags: superreview?(brendan)
Attachment #216234 - Flags: review?(jst)
Attachment #216192 - Flags: superreview?(brendan)
Attachment #216192 - Flags: review?(jst)
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
Attachment #216234 - Flags: superreview?(brendan) → superreview+
Comment on attachment 216234 [details] [diff] [review]
patch

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

/be
Attachment #216234 - Flags: review?(dbradley)
Flags: blocking1.8.0.3? → blocking1.8.0.3+
Comment on attachment 216234 [details] [diff] [review]
patch

r=jst
Attachment #216234 - Flags: review?(jst) → review+

Comment 6

12 years ago
Comment on attachment 216234 [details] [diff] [review]
patch

r=dbradley

Looks fine.
Attachment #216234 - Flags: review?(dbradley) → review+
(Assignee)

Comment 7

12 years ago
Created attachment 217339 [details] [diff] [review]
patch

This addresses brendan's review comments.
Attachment #216234 - Attachment is obsolete: true
(Assignee)

Comment 8

12 years ago
Checked in to trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Assignee)

Updated

12 years ago
Attachment #217339 - Flags: approval1.8.0.3?
Attachment #217339 - Flags: approval-branch-1.8.1?(jst)

Updated

12 years ago
Attachment #217339 - Flags: approval-branch-1.8.1?(jst) → approval-branch-1.8.1+
(Assignee)

Comment 9

12 years ago
Fix checked in to MOZILLA_1_8_BRANCH.
Keywords: fixed1.8.1
(Assignee)

Updated

12 years ago
Flags: blocking1.8.1?
Comment on attachment 217339 [details] [diff] [review]
patch

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #217339 - Flags: approval1.8.0.3? → approval1.8.0.3+
(Assignee)

Comment 11

12 years ago
Fix checked in to MOZILLA_1_8_0_BRANCH.
Keywords: fixed1.8.0.3
You need to log in before you can comment on or make changes to this bug.