581264 - js_NewArrayObjectWithCapacity is unsafe and is being abused in several places

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Andreas Gal :gal]

A couple things.

Don't ever ever ever declare JS functions in your own files. If we change our headers, you are out of sync and horrible things happen. Waldo should have balked at you for this one.

http://hg.mozilla.org/mozilla-central/diff/d15754ab3ff1/content/canvas/src/nsCanvasRenderingContext2D.cpp

Second, you can't use NewArrayObjectWithCapacity if there is any chance that a GC happens. The interior parts of it are not initialized. Best case we crash. Making that function a friend in itself is a mistake (I am deleting the function altogether). 

http://hg.mozilla.org/mozilla-central/diff/c51c255be44d/content/base/src/nsFrameMessageManager.cpp

Comment 1

[Andreas Gal :gal]

CTypes is parenting fields of the struct array to the array. That seems .. mhm? .. bizarre? I changed that so I can use NewArrayObject. Regular objects shouldn't have parents to begin with. I don't think this is used either. dwitte will have to review.

Comment 2

[Andreas Gal :gal]

FrameMessageManager: When you guys use new JSAPI functionality, better get someone from JS to review as well.

Comment 3

[Andreas Gal :gal]

Attachment: patch

Comment 4

[Andreas Gal :gal]

Once this patch is in I will delete JS_NewArrayObjectWithCapacity.

Comment 5

[Andreas Gal :gal]

Filed bug 581271 to get rid of JS_FRIEND_API in general.

Comment 6

[Boris Zbarsky [:bzbarsky]]

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

r=me on the canvas bit, which is what I assume you were looking for from me.  If not, what else did you want me to look at?

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #2)
> FrameMessageManager: When you guys use new JSAPI functionality, better get
> someone from JS to review as well.
I actually copied the code from canvas code.
I probably shouldn't have done that.

Comment 8

[[e-mail]]

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

>diff --git a/js/src/ctypes/CTypes.cpp b/js/src/ctypes/CTypes.cpp

> // For a struct field with 'name' and 'type', add an element to field
> // descriptor array 'arrayObj' of the form { name : type }.
> static JSBool
> AddFieldToArray(JSContext* cx,
>-                JSObject* arrayObj,

Comment is stale.

>   // Prepare a new array for the 'fields' property of the StructType.
>-  jsval* fieldsVec;
>-  JSObject* fieldsProp =
>-    js_NewArrayObjectWithCapacity(cx, len, &fieldsVec);
>-  if (!fieldsProp)
>+  Vector<jsval> fieldsVec(cx);

We don't want ContextAllocPolicy here, and it'd be nice to use an autoarray. CTypes.h declares an Array<T, N> for this purpose (derived from Vector); you should use Array<jsval, 16>.

Don't you need to root the vector elements? (We only scan the C stack, not the heap, right?)

r=dwitte with changes.

Comment 9

[[e-mail]]

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

>+  JSObject* fieldsProp = JS_NewArrayObject(cx, len, fieldsVec.begin());
>+  if (!fieldsProp)
>+    return NULL;

Also, just to make sure -- this doesn't need to be explicitly rooted, right?

Comment 10

[Andreas Gal :gal]

We have a conservative stack scanner now.

Comment 11

[Andreas Gal :gal]

http://hg.mozilla.org/tracemonkey/rev/53f9014f9a23

Comment 12

[Andreas Gal :gal]

Spectacular fail.

http://hg.mozilla.org/tracemonkey/rev/6391c7547ecc
http://hg.mozilla.org/tracemonkey/rev/0a242b150116

Comment 13

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/53f9014f9a23