759895 - Fix typed array rooting issues with destructor ordering

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Steve Fink [:sfink] [:s:]]

This appears to be invalid:

  RootedObject robj(cx, func(RootedObject(cx, obj)));

because Rooted<T> uses RAII to maintain a stack of rooted cells, which means it depends on constructors and destructors to properly nest. But in the above code, you can get:

 1. inner constructor
 2. outer constructor
 3. inner destructor
 4. outer destructor

Apparently the compiler chooses to delay the inner destructor until after the outer constructor has run.

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Fix typed array rooting issues with destructor ordering

Before requesting review, I'm going to check with a resident language lawyer to see if this is really valid behavior.

Comment 2

[Steve Fink [:sfink] [:s:]]

Comment on attachment 628469 [details] [diff] [review]
Fix typed array rooting issues with destructor ordering

Luke -- is this a legitimate ordering? This is a fairly subtle gotcha in our rooting API, though it seems unavoidable.

Comment 3

[Luke Wagner [:luke]]

Comment on attachment 628469 [details] [diff] [review]
Fix typed array rooting issues with destructor ordering

Not just legal but mandatory: the temporary must be constructed before calling the variable's constructor and must be destroyed at the end of the statement, so the non-LIFO-ness is mandatory.  You're right, though, this is a big footgun.  No good solutions come to mind.  On the bright side, it should show up on the first time the code is run.

Comment 4

[Steve Fink [:sfink] [:s:]]

Attachment: Fix typed array rooting issues with destructor ordering

This is invalid:

  RootedObject robj(cx, func(RootedObject(cx, obj)));

because Rooted<T> uses RAII to maintain a stack of rooted cells, which means it depends on constructors and destructors to properly nest. But the above code will call:

 1. inner constructor
 2. outer constructor
 3. inner destructor
 4. outer destructor

According to Luke, this is per spec; the temporary must last to the end of the statement.

Comment 5

[Terrence Cole [:terrence]]

Comment on attachment 630224 [details] [diff] [review]
Fix typed array rooting issues with destructor ordering

Review of attachment 630224 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with one nit fixed.  There are other places that are going to need this cleanup: it should be as easy as a double-grepping for Rooted.  I'll open a bug.

::: js/src/jstypedarray.cpp
@@ +1457,5 @@
>  
> +    static JSObject *
> +    makeInstance(JSContext *cx, HandleObject bufobj, uint32_t byteOffset, uint32_t len)
> +    {
> +        RootedObject nullproto(cx);

RootedObject nullproto(cx, NULL);

I realize that the default Rooted is NULL, but lets pass it explicitly here to make it clearer for readers that haven't read Root.h.

Comment 6

[Steve Fink [:sfink] [:s:]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/5940aa453f9f

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5940aa453f9f