866527 - Fix ctypes rooting hazards

Status: RESOLVED FIXED

(Core :: js-ctypes, defect)

Description

[David Zbarsky (:dzbarsky)]

Attachment: Patch

Comment 1

[Terrence Cole [:terrence]]

Comment on attachment 742840 [details] [diff] [review]
Patch

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

Awesome! r=me with a couple minor style nits fixed. Thanks for the patch!

::: toolkit/components/ctypes/ctypes.cpp
@@ +65,5 @@
>  
>  static JSBool
>  SealObjectAndPrototype(JSContext* cx, JSObject* parent, const char* name)
>  {
> +  JS::RootedValue prop(cx);

Even though this code is closely tied to JS, lets use the browser's style here since it's outside of the js/ tree: JS::Rooted<JS::Value> rather than JS::RootedValue. Likewise for JS::Rooted<JSObject*>

@@ +74,5 @@
>      // Pretend we sealed the object.
>      return true;
>    }
>  
> +  JS::RootedObject obj(cx, JSVAL_TO_OBJECT(prop));

While you are here, please change JSVAL_TO_OBJECT(prop) to prop.toObjectOrNull().

@@ +80,3 @@
>      return false;
>  
> +  JS::RootedObject prototype(cx, JSVAL_TO_OBJECT(prop));

Ditto for both of the above.

@@ +83,5 @@
>    return JS_FreezeObject(cx, obj) && JS_FreezeObject(cx, prototype);
>  }
>  
>  static JSBool
> +InitAndSealCTypesClass(JSContext* cx, JS::HandleObject global)

JS::Handle<JSObject*>

@@ +90,5 @@
>    if (!JS_InitCTypesClass(cx, global))
>      return false;
>  
>    // Set callbacks for charset conversion and such.
> +  JS::RootedValue ctypes(cx);

JS::Rooted<JS::Value>

Comment 2

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/004679f2317b

Comment 3

[David Zbarsky (:dzbarsky)]

Attachment: Part 2

Comment 4

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/004679f2317b

Comment 5

[Terrence Cole [:terrence]]

Comment on attachment 743415 [details] [diff] [review]
Part 2

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

Excellent! r=me with or without the nit addressed, but might as well clean it up while you are touching the line.

::: js/src/ctypes/CTypes.cpp
@@ +7117,5 @@
>      JS_ReportError(cx, "CDataFinalizer.prototype.forget takes no arguments");
>      return JS_FALSE;
>    }
>  
> +  JS::Rooted<JSObject*> obj(cx, JS_THIS_OBJECT(cx, vp));

JS_THIS_OBJECT(cx,vp) can be replaced with args.thisv().toObjectOrNull().

Comment 6

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/98545c1d2810

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/98545c1d2810