Fix ctypes rooting hazards

RESOLVED FIXED in mozilla23

Status

()

Core
js-ctypes
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dzbarsky, Assigned: dzbarsky)

Tracking

unspecified
mozilla23
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

5 years ago
Created attachment 742840 [details] [diff] [review]
Patch
Attachment #742840 - Flags: review?(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>
Attachment #742840 - Flags: review?(terrence) → review+
(Assignee)

Comment 3

5 years ago
Created attachment 743415 [details] [diff] [review]
Part 2
Attachment #743415 - Flags: review?(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().
Attachment #743415 - Flags: review?(terrence) → review+
https://hg.mozilla.org/mozilla-central/rev/98545c1d2810
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.