Last Comment Bug 759895 - Fix typed array rooting issues with destructor ordering
: Fix typed array rooting issues with destructor ordering
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Steve Fink [:sfink] [:s:]
:
:
Mentors:
Depends on:
Blocks: ExactRooting
  Show dependency treegraph
 
Reported: 2012-05-30 14:08 PDT by Steve Fink [:sfink] [:s:]
Modified: 2012-06-13 09:13 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fix typed array rooting issues with destructor ordering (5.43 KB, patch)
2012-05-30 14:08 PDT, Steve Fink [:sfink] [:s:]
luke: feedback+
Details | Diff | Splinter Review
Fix typed array rooting issues with destructor ordering (5.41 KB, patch)
2012-06-05 10:46 PDT, Steve Fink [:sfink] [:s:]
terrence.d.cole: review+
Details | Diff | Splinter Review

Description Steve Fink [:sfink] [:s:] 2012-05-30 14:08:46 PDT
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:] 2012-05-30 14:08:53 PDT
Created attachment 628469 [details] [diff] [review]
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:] 2012-05-30 14:10:36 PDT
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] 2012-05-30 16:07:39 PDT
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:] 2012-06-05 10:46:53 PDT
Created attachment 630224 [details] [diff] [review]
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] 2012-06-05 11:35:33 PDT
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:] 2012-06-05 12:02:10 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/5940aa453f9f
Comment 7 Ed Morley [:emorley] 2012-06-06 08:42:33 PDT
https://hg.mozilla.org/mozilla-central/rev/5940aa453f9f

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