The default bug view has changed. See this FAQ.

Fix typed array rooting issues with destructor ordering

RESOLVED FIXED in mozilla16

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: sfink, Assigned: sfink)

Tracking

unspecified
mozilla16
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

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

Comment 1

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

Comment 2

5 years ago
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.
Attachment #628469 - Flags: feedback?(luke)

Comment 3

5 years ago
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.
Attachment #628469 - Flags: feedback?(luke) → feedback+
(Assignee)

Comment 4

5 years ago
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.
Attachment #630224 - Flags: review?(terrence)
(Assignee)

Updated

5 years ago
Attachment #628469 - Attachment is obsolete: true
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.
Attachment #630224 - Flags: review?(terrence) → review+
(Assignee)

Comment 6

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/5940aa453f9f
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/5940aa453f9f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Blocks: 753203
You need to log in before you can comment on or make changes to this bug.