GNOME Bugzilla – Bug 663441
gjs: Remove some cruft
Last modified: 2011-11-28 19:45:48 UTC
This should hopefully fix bugs like the crashers in bug #661323
Created attachment 200726 [details] [review] Remove usage of JSCLASS_CONSTRUCT_PROTOTYPE JSCLASS_CONSTRUCT_PROTOTYPE is an old hack that upstream wants to remove. We're only using it to set the private instance, something we can do ourselves after initting the class.
Created attachment 200727 [details] [review] Remove the unthreadsafe_template_for_constructor Rather than setting a magic global which can get un-stuck at times, use the JSAPI equivalent of ES5's Object.create to create an object without running the constructor, so we can set a gobject ourselves
Removal of JSCLASS_CONSRTUCT_PROPERTY upstream: https://bugzilla.mozilla.org/show_bug.cgi?id=702507
Created attachment 201568 [details] [review] Remove usage of JSCLASS_CONSTRUCT_PROTOTYPE JSCLASS_CONSTRUCT_PROTOTYPE is an old hack that upstream wants to remove. We're only using it to set the private instance, something we can do ourselves after initting the class. New patch attached that removes *all* uses of JSCLASS_CONSTRUCT_PROTOTYPE. I doubt anybody is going to review this, so if/when upstream removes this, I will push this.
Created attachment 201619 [details] [review] Remove the unthreadsafe_template_for_constructor Rather than setting a magic global which can get un-stuck at times, use the JSAPI equivalent of ES5's Object.create to create an object without running the constructor, so we can set a gobject ourselves $ git grep unthreadsafe $
Review of attachment 201568 [details] [review]: This makes sense to me. There appears to be a lot of noise in the diff due the un-indentation of the if (!is_proto) case, but if you didn't change any of that, then I'd say it's fine.
Review of attachment 201619 [details] [review]: This is a huge cleanup. The changes to object.c make me a little nervous because it's such tricky code, but I'm not seeing anything obviously wrong. ::: gi/boxed.c @@ +1236,3 @@ + } else if (priv->can_allocate_directly) { + if (!boxed_new_direct(context, obj, priv)) + return JS_FALSE; Hm, so in the error paths like this, we're relying on the GC to eventually notice that our "obj" isn't in use, and it will collect it and the priv data? ::: gi/object.c @@ +1502,3 @@ + if (!obj) + goto out; So won't the above code crash is obj is NULL? Seems like we should be bailing earlier. ::: gi/union.c @@ +237,3 @@ + * gjs_invoke_c_function(), which returns a jsval. + * The returned "gboxed" here is owned by that jsval, + * not by us.2 Stray 2 @@ +246,3 @@ + /* Because "gboxed" is owned by a jsval and will + * be garbage colleced, we make a copy here to be collected
Review of attachment 201619 [details] [review]: If it would make you more comfortable, I could add a new_priv() function to GJS_DEFINE_DYNAMIC_PRIV_FROM_JS that would allocate and return a new private instance. ::: gi/boxed.c @@ +1236,3 @@ + } else if (priv->can_allocate_directly) { + if (!boxed_new_direct(context, obj, priv)) + return JS_FALSE; I don't think there's anything else we can do. I looked for an explicit "JS_DestroyObject" function, but it doesn't exist. ::: gi/union.c @@ +237,3 @@ + * gjs_invoke_c_function(), which returns a jsval. + * The returned "gboxed" here is owned by that jsval, + * not by us.2 Fixed locally. @@ +246,3 @@ + /* Because "gboxed" is owned by a jsval and will + * be garbage colleced, we make a copy here to be An existing error, but fixed.
Created attachment 201631 [details] [review] Remove usage of JS_IsConstructing_PossiblyWithGivenThisObject
Created attachment 201632 [details] [review] Remove the unthreadsafe_template_for_constructor Rather than setting a magic global which can get un-stuck at times, use the JSAPI equivalent of ES5's Object.create to create an object without running the constructor, so we can set a gobject ourselves
Comment on attachment 201568 [details] [review] Remove usage of JSCLASS_CONSTRUCT_PROTOTYPE Attachment 201568 [details] pushed as f9276d7 - Remove usage of JSCLASS_CONSTRUCT_PROTOTYPE
Review of attachment 201631 [details] [review]: This one needs a justification. I'm guessing we'll be breaking compatibility with older spidermonkey versions? Hmm. We'll no longer be assigning to object now, and just call JS_NewObjectForConstructor() always. Is that really intended?
Review of attachment 201632 [details] [review]: ::: gi/object.c @@ +1491,3 @@ + + if (obj == NULL) + goto out; This will lose the JS_EndRequest()
Review of attachment 201631 [details] [review]: The justification is https://hg.mozilla.org/mozilla-central/rev/a7b658e309b9. The only place where constructors were called with a non-standard this object was when using JSCLASS_CONSTRUCT_PROTOTYPE. Constructors also must create their own new object. See the description for the constructor parameter in: https://developer.mozilla.org/en/SpiderMonkey/JSAPI_Reference/JS_InitClass
Created attachment 201695 [details] [review] Remove the unthreadsafe_template_for_constructor Rather than setting a magic global which can get un-stuck at times, use the JSAPI equivalent of ES5's Object.create to create an object without running the constructor, so we can set a gobject ourselves Whoops, sorry about being a moron there.
Review of attachment 201695 [details] [review]: This one looks right to me.
Attachment 201631 [details] pushed as 2e37d24 - Remove usage of JS_IsConstructing_PossiblyWithGivenThisObject Attachment 201695 [details] pushed as 740cf9d - Remove the unthreadsafe_template_for_constructor