833396 - Fix some rooting issues found by static analysis

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Jon Coppeard (:jonco)]

Attachment: Fix

Comment 2

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

Comment on attachment 704923 [details] [diff] [review]
Fix

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

Looks good, but I'd like to skim it one more time after the Handlification changes.

::: js/src/jsdbgapi.cpp
@@ +931,5 @@
> +            if (!js_AddRoot(cx, &pd[i].id, NULL))
> +                goto bad;
> +            if (!js_AddRoot(cx, &pd[i].value, NULL))
> +                goto bad;
> +            RootedShape shape(cx, const_cast<Shape *>(&r.front()));

Hoist this declaration out of the loop, to save a push/pop on every iteration.

::: js/src/jsobj.cpp
@@ +1639,3 @@
>  {
> +    RootedObject from(cx, fromArg);
> +    RootedObject to(cx, toArg);

Why root here? CopySlots should take Handles. In fact, you're already rooting the only unrooted thing that gets passed into it, a little later.

@@ +1730,4 @@
>                                TradeGutsReserved &reserved)
>  {
> +    RootedObject a(cx, aArg);
> +    RootedObject b(cx, bArg);

These are rooted again a little later. You just need to pull these roots up above the AutoCompartment (it's what can gc.)

::: js/src/json.cpp
@@ +328,5 @@
>  
>      /* Step 4. */
>      if (vp.get().isObject()) {
> +        RootedObject obj(cx, &vp.get().toObject());
> +        if (ObjectClassIs(*obj, ESClass_Number, cx)) {

I could look it up, but I'm lazy: what's the rooting hazard here?

::: js/src/jsopcode.cpp
@@ +4764,5 @@
>                  sn = js_GetSrcNote(cx, jp->script, pc);
>                  if (sn && SN_TYPE(sn) == SRC_GENEXP) {
>                      BindingVector *outerLocalNames;
> +                    JSScript *inner;
> +                    RootedScript outer(cx);

This file is a very good demonstration of why the static analysis is good. :-)

::: js/src/jsscope.cpp
@@ +734,5 @@
>      return shape;
>  }
>  
>  /* static */ UnrootedShape
> +JSObject::changeProperty(JSContext *cx, HandleObject obj, RawShape shapeArg, unsigned attrs, unsigned mask,

I did a quick scan, and it looks like using a HandleShape for ::changeProperty and ::changePropertyAttributes would be straightforward.

Comment 3

[Jon Coppeard (:jonco)]

Attachment: Fix with review comments addressed

Comment 4

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

Comment on attachment 705360 [details] [diff] [review]
Fix with review comments addressed

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

::: js/src/jsscope.cpp
@@ +740,2 @@
>  {
> +    RootedShape shape(cx, shapeArg);

Seems like an unnecessary root. AFAICT, shape is never assigned, so why not use the HandleShape param directly?

Comment 5

[Jon Coppeard (:jonco)]

(In reply to Steve Fink [:sfink] from comment #4)
> Seems like an unnecessary root. AFAICT, shape is never assigned, so why not
> use the HandleShape param directly?

Yes, that's better, I must have missed that.

Cheers!

Comment 6

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7227a6cb2e2e

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7227a6cb2e2e