558675 - Reliable crash on motion tracker demo caused by buggy AutoIDArray

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Just got his crash on mac at the following url:

http://people.mozilla.com/~prouget/demos/tracker/tracker.xhtml

Crash: bp-72fc0add-5a0a-4738-83bc-067c02100411

Looks like it crashes pretty reliably, but only if you set the "Zoom Factor" to 1, for some reason.

I'm marking this one as s-g sensitive until we know otherwise.

Comment 1

[Brendan Eich [:brendan]]

This looks like a failure to root something in time, probably not in the JS engine. Ben, the structured clone code is on the stack, triggering a last ditch GC trying to allocate a double, and an autorooter is being scanned to mark its thing, which seems to be a bogus pointer. Inspecting the autoroot usage in the structured clone code is in order.

/be

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1

Actually, I think this is fallout from bug 548702. Looks like the IDARRAY GCRooter type isn't being used - in fact it's telling the trace code to cast to the wrong type of autoroot object!

While investigating this I also found some additional problems in the typed array constructor code where the autorooted jsvals were uninitialized. Thanks to gczeal or I never would have figured this out.

Comment 3

[Brendan Eich [:brendan]]

(In reply to comment #2)
> Created an attachment (id=438389) [details]
> Patch, v1
> 
> Actually, I think this is fallout from bug 548702. Looks like the IDARRAY
> GCRooter type isn't being used - in fact it's telling the trace code to cast to
> the wrong type of autoroot object!

Waldo should review.

> While investigating this I also found some additional problems in the typed
> array constructor code where the autorooted jsvals were uninitialized. Thanks
> to gczeal or I never would have figured this out.

This must be bug 551507 -- fixed in tm, not merged to m-c yet :-(. It should be hand-merged pronto, I think.

/be

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 438389 [details] [diff] [review]
Patch, v1

Don't the initializers need more JSVAL_NULL elements to match the length of the arrays? Anyway, let's just get the patch for bug 551507 merged into m-c. Please take jstypedarray.cpp out of the patch here.

/be

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 438389 [details] [diff] [review]
Patch, v1

Eugh, how'd that happen...

Does { 0 } actually initialize trailing members to 0 as well?  If not, that seems a problem.  If so, that's still a problem, because it's presuming 0 is a safe jsval value.  Please use however many JSVAL_NULLs should actually be used, rather than just one.

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, only AutoIDArray fix

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 438389 [details] [diff] [review]
Patch, v1

>diff --git a/js/src/jscntxt.h b/js/src/jscntxt.h
>--- a/js/src/jscntxt.h
>+++ b/js/src/jscntxt.h
>@@ -1852,17 +1852,17 @@ class AutoIdRooter : private AutoGCRoote
>     jsid idval;
>     JS_DECL_USE_GUARD_OBJECT_NOTIFIER
> };
> 
> class AutoIdArray : private AutoGCRooter {
>   public:
>     AutoIdArray(JSContext *cx, JSIdArray *ida
>                   JS_GUARD_OBJECT_NOTIFIER_PARAM)
>-      : AutoGCRooter(cx, ida ? ida->length : 0), idArray(ida)
>+      : AutoGCRooter(cx, IDARRAY), idArray(ida)

Again, Waldo should review definitively, but this looks like a fix to me.

BTW the JS_GUARD_OBJECT_NOTIFIER_PARAM can go on the previous line, drive-by-nit.

/be

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #5)
> Does { 0 } actually initialize trailing members to 0 as well?  If not, that
> seems a problem.  If so, that's still a problem, because it's presuming 0 is a
> safe jsval value.  Please use however many JSVAL_NULLs should actually be used,
> rather than just one.

The JS engine has a JS_STATIC_ASSERT that JSVAL_NULL is 0, is that too much of an assumption for clients to assert too?

For what it's worth, if you don't supply enough values in an array initializer, the compiler will supply 0 for you.

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

http://hg.mozilla.org/mozilla-central/rev/369599ed72c0

Comment 10

[Reed Loden [:reed]]

(In reply to comment #9)
> http://hg.mozilla.org/mozilla-central/rev/369599ed72c0

Understand you need to be discreet with the check-in comment due to this being a security bug, but please always include the name of the reviewer in your commits.

Comment 11

[Jeff Walden [:Waldo]]

(In reply to comment #8)
> The JS engine has a JS_STATIC_ASSERT that JSVAL_NULL is 0, is that too much of
> an assumption for clients to assert too?

Clients certainly may do so, but I don't think it's good form for any client to rely on the internal representation of a jsval, as this would do.  We promise source compatibility, and this decision is an explicit precludes a promise of such.  How can we support that?