Closed Bug 833396 Opened 11 years ago Closed 11 years ago

Fix some rooting issues found by static analysis

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: jonco, Assigned: jonco)

References

Details

Attachments

(1 file, 1 obsolete file)

      No description provided.
Attached patch Fix (obsolete) — Splinter Review
Attachment #704923 - Flags: review?(sphink)
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.
Attachment #704923 - Flags: review?(sphink)
Attachment #704923 - Attachment is obsolete: true
Attachment #705360 - Flags: review?(sphink)
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?
Attachment #705360 - Flags: review?(sphink) → review+
(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!
Blocks: 831409
https://hg.mozilla.org/mozilla-central/rev/7227a6cb2e2e
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.