Closed Bug 866432 Opened 7 years ago Closed 7 years ago

Remove AutoValueRooter and replace it with RootedValue

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: sfink, Assigned: sfink)

Details

Attachments

(2 files)

AutoValueRooter is dangerous in a moving GC world. A Value protected by an autoValueRooter could still end up on the stack and not get updated. Also, all uses of AutoValueRooter can trivially be replaced with RootedValue right now.
Attachment #742730 - Flags: review?(benjamin) → review?(bugs)
Comment on attachment 742730 [details] [diff] [review]
Replace AutoValueRooter with JS::Rooted<JS::Value> in gecko

You're using both RootedValue and Rooted<Value>. We really must get rid of one of
those, or at least not expose them both in public JS APIs.
Please use Rooted<Value>
Attachment #742730 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #3)
> Comment on attachment 742730 [details] [diff] [review]
> Replace AutoValueRooter with JS::Rooted<JS::Value> in gecko
> 
> You're using both RootedValue and Rooted<Value>. We really must get rid of
> one of
> those, or at least not expose them both in public JS APIs.
> Please use Rooted<Value>

I agree, I think we should be able to remove RootedValue and the other typedefs from js:: with a bit of work.
Comment on attachment 742729 [details] [diff] [review]
Remove AutoValueRooter and replace it with RootedValue

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

This is full of win!

::: js/src/ctypes/CTypes.cpp
@@ +4649,4 @@
>      return NULL;
>  
> +  if (propVal.isPrimitive() ||
> +      !CType::IsCType(JSVAL_TO_OBJECT(propVal))) {

While you are here, propVal.toObjectOrNull().

@@ +4656,5 @@
>  
>    // Undefined size or zero size struct members are illegal.
>    // (Zero-size arrays are legal as struct members in C++, but libffi will
>    // choke on a zero-size struct, so we disallow them.)
> +  *typeObj = JSVAL_TO_OBJECT(propVal);

While you are here, propVal.toObjectOrNull().

::: js/src/jsapi.h
@@ -111,5 @@
>       */
>      ptrdiff_t tag_;
>  
>      enum {
> -        JSVAL =        -1, /* js::AutoValueRooter */

\o/

@@ -117,5 @@
>          PARSER =       -3, /* js::frontend::Parser */
>          SHAPEVECTOR =  -4, /* js::AutoShapeVector */
>          IDARRAY =      -6, /* js::AutoIdArray */
>          DESCRIPTORS =  -7, /* js::AutoPropDescArrayRooter */
> -        OBJECT =       -8, /* js::AutoObjectRooter */

\o/ \o/

::: js/xpconnect/src/XPCConvert.cpp
@@ +1135,5 @@
>  }
>  
>  /********************************/
>  
>  class AutoExceptionRestorer

We should probably mark this as a stack class.
Attachment #742729 - Flags: review?(terrence) → review+
(In reply to Olli Pettay [:smaug] from comment #3)
> Comment on attachment 742730 [details] [diff] [review]
> Replace AutoValueRooter with JS::Rooted<JS::Value> in gecko
> 
> You're using both RootedValue and Rooted<Value>. We really must get rid of
> one of
> those, or at least not expose them both in public JS APIs.
> Please use Rooted<Value>

Argh! I can't believe I did that. You are right, of course.
(In reply to Terrence Cole [:terrence] from comment #5)
> @@ -117,5 @@
> >          PARSER =       -3, /* js::frontend::Parser */
> >          SHAPEVECTOR =  -4, /* js::AutoShapeVector */
> >          IDARRAY =      -6, /* js::AutoIdArray */
> >          DESCRIPTORS =  -7, /* js::AutoPropDescArrayRooter */
> > -        OBJECT =       -8, /* js::AutoObjectRooter */
> 
> \o/ \o/

Sadly, this was just a patch splitting screwup. I have a separate set of patches that I'm working on for this.
Attachment #742729 - Flags: checkin+
Attachment #742730 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/9fd825f383a6
https://hg.mozilla.org/mozilla-central/rev/354d7c54a270
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
(In reply to Terrence Cole [:terrence] from comment #5)
> Comment on attachment 742729 [details] [diff] [review]
> Remove AutoValueRooter and replace it with RootedValue
> 
> Review of attachment 742729 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is full of win!
> 
> ::: js/src/ctypes/CTypes.cpp
> @@ +4649,4 @@
> >      return NULL;
> >  
> > +  if (propVal.isPrimitive() ||
> > +      !CType::IsCType(JSVAL_TO_OBJECT(propVal))) {
> 
> While you are here, propVal.toObjectOrNull().
> 
> @@ +4656,5 @@
> >  
> >    // Undefined size or zero size struct members are illegal.
> >    // (Zero-size arrays are legal as struct members in C++, but libffi will
> >    // choke on a zero-size struct, so we disallow them.)
> > +  *typeObj = JSVAL_TO_OBJECT(propVal);
> 
> While you are here, propVal.toObjectOrNull().

Why toObjectOrNull()? It can't be null.
You need to log in before you can comment on or make changes to this bug.