866432 - Remove AutoValueRooter and replace it with RootedValue

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

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.

Comment 1

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

Attachment: Remove AutoValueRooter and replace it with RootedValue

Comment 2

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

Attachment: Replace AutoValueRooter with JS::Rooted<JS::Value> in gecko

Basic rooting guide is at https://developer.mozilla.org/en-US/docs/SpiderMonkey/GC_Rooting_Guide

More detailed documentation is in the source, see http://mxr.mozilla.org/mozilla-central/source/js/public/RootingAPI.h

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

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>

Comment 4

[Terrence Cole [:terrence]]

(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 5

[Terrence Cole [:terrence]]

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.

Comment 6

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

(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.

Comment 7

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

(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.

Comment 8

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

http://hg.mozilla.org/integration/mozilla-inbound/rev/354d7c54a270
http://hg.mozilla.org/integration/mozilla-inbound/rev/9fd825f383a6

Comment 9

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

Try push at https://tbpl.mozilla.org/?tree=Try&rev=ddd7fa2ebadd

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9fd825f383a6
https://hg.mozilla.org/mozilla-central/rev/354d7c54a270

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(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.