Closed
Bug 759895
Opened 12 years ago
Closed 12 years ago
Fix typed array rooting issues with destructor ordering
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla16
People
(Reporter: sfink, Assigned: sfink)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
Before requesting review, I'm going to check with a resident language lawyer to see if this is really valid behavior.
Assignee | ||
Comment 2•12 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•12 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•12 years ago
|
||
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•12 years ago
|
Attachment #628469 -
Attachment is obsolete: true
Comment 5•12 years ago
|
||
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•12 years ago
|
||
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla16
Comment 7•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
Blocks: ExactRooting
You need to log in
before you can comment on or make changes to this bug.
Description
•