867341 - Replace AutoObjectRooter with RootedObject

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

AutoObjectRooter is a bit of a footgun in the moving GC world, since it doesn't provide Handles, and all uses are replaceable with RootedObject now.

Comment 1

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

Attachment: Replace AutoObjectRooter with RootedObject

Another boring review for smaug.

Comment 2

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

Attachment: Replace AutoObjectRooter with RootedObject

This should be pretty straightforward, but:

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

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

Attachment: Remove AutoObjectRooter, replace with RootedObject where needed

Comment 4

[Terrence Cole [:terrence]]

Comment on attachment 743806 [details] [diff] [review]
Remove AutoObjectRooter, replace with RootedObject where needed

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

Looks good.

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

\o/

Comment 5

[Vladan Djeric (:vladan)]

Comment on attachment 743805 [details] [diff] [review]
Replace AutoObjectRooter with RootedObject

>   const size_t count = h->bucket_count();
>-  JSObject *rarray = JS_NewArrayObject(cx, count, nullptr);
>+  JS::Rooted<JSObject*> rarray(cx, JS_NewArrayObject(cx, count, nullptr));
>   if (!rarray) {
>     return REFLECT_FAILURE;
>   }

Nitpicks:
- Let's use NULL and nullptr consistently in the changed lines
- Why not RootedObject instead of JS::Rooted<JSObject*>?

Comment 6

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

(In reply to Vladan Djeric (:vladan) from comment #5)
> Comment on attachment 743805 [details] [diff] [review]
> Replace AutoObjectRooter with RootedObject
> 
> >   const size_t count = h->bucket_count();
> >-  JSObject *rarray = JS_NewArrayObject(cx, count, nullptr);
> >+  JS::Rooted<JSObject*> rarray(cx, JS_NewArrayObject(cx, count, nullptr));
> >   if (!rarray) {
> >     return REFLECT_FAILURE;
> >   }
> 
> Nitpicks:
> - Let's use NULL and nullptr consistently in the changed lines

Consistently? I'm not sure what you mean. The file currently uses a mixture; I (or rather, my emacs macro) just preserved whatever was there.

Are you ok with me switching all changed lines to nullptr? Or for that matter, I could change everything in this file to nullptr if you'd like.

> - Why not RootedObject instead of JS::Rooted<JSObject*>?

Current policy is to use the template form everywhere outside of the JS engine and XPConnect. And we may eliminate the typedefs (eg RootedObject) entirely at some point.

Comment 7

[Vladan Djeric (:vladan)]

(In reply to Steve Fink [:sfink] from comment #6)
> (In reply to Vladan Djeric (:vladan) from comment #5)
> > Nitpicks:
> > - Let's use NULL and nullptr consistently in the changed lines
> 
> Consistently? I'm not sure what you mean. The file currently uses a mixture;
> I (or rather, my emacs macro) just preserved whatever was there.
>
> Are you ok with me switching all changed lines to nullptr? Or for that
> matter, I could change everything in this file to nullptr if you'd like.

I was suggesting using nullptr in the changed lines, but you can change the whole file too -- the whole-file change should be a separate commit.

Comment 8

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

http://hg.mozilla.org/integration/mozilla-inbound/rev/e2dc6cdc1c02
http://hg.mozilla.org/integration/mozilla-inbound/rev/e728bef882df
http://hg.mozilla.org/integration/mozilla-inbound/rev/0875726579b2

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0875726579b2
https://hg.mozilla.org/mozilla-central/rev/e728bef882df
https://hg.mozilla.org/mozilla-central/rev/e2dc6cdc1c02