GNOME Bugzilla – Bug 669848
Improve generic conversion from JS Object to GValue
Last modified: 2012-02-13 17:14:17 UTC
See patch
Created attachment 207303 [details] [review] Improve detecting GType for objects When using API like GtkListStore, it's common to deal with GValues that have no external type information. Try harder to extract the GType from the JS object in that case.
Review of attachment 207303 [details] [review]: We introduced the magic "$gtype" property on objects. Maybe you could install that on boxeds and use that instead?
Created attachment 207343 [details] [review] Store GType in private struct for Boxed Instead of retrieving it every time from the introspection info, store it in the private data when the class is first defined. Also, ensure that the object has the right JSClass before accessing private data. === Still an useful cleanup, I think. Plus it should prevent invalid memory accesses if the object is not of the right type.
Created attachment 207344 [details] [review] Add tests for GType of GValue Some language bindings (like gjs) attempt to automatically infer the GType from a native object when creating a GValue. This checks that indeed the GValue has the right GType. ---- I want to add tests for this, so this is the gobject-introspection side.
Created attachment 207346 [details] [review] boxed, param, union: add $gtype to constructors Various places assume that they can grab the GType for a class by retrieving the $gtype property. This extends this convention to boxed types, unions and GParamSpec.
Created attachment 207347 [details] [review] Improve GType detection for unconstrained GValues Objects created by GJS are guaranteed to have a "constructor" property, holding an object that can be converted to a GType. Use that to obtain the object type, instead of forcing G_TYPE_OBJECT. This can be used with APIs like GtkListStore, that force the user to create a GValue and expect the right type. Includes tests.
(In reply to comment #3) > Still an useful cleanup, I think. Not convinced. > Plus it should prevent invalid > memory accesses if the object is not of the right type. When could this happen?
Review of attachment 207346 [details] [review]: Yep, sure.
Review of attachment 207347 [details] [review]: Not a fan of the commit message. Let's try: In ebc84492, we added a way to get a GType for a constructor. Let's expand this to objects (by way of the JavaScript 'constructor' property), and then use this mechanism to improve GType detection in GValues. This can be used with APIs like GtkListStore that force the user to create a GValue and expect the right type.
Fine by me. I'm assuming that other than that, patch was fine (including the introspection bit).
No wait, the GType in private data touches some dangerous code, it's better if you look at it first.
(In reply to comment #11) > No wait, the GType in private data touches some dangerous code, it's better if > you look at it first. You mean "Store GType in private struct for Boxed"? I'm not sure it's a useful patch. If everything works without it, let's drop it. In the future, I'd like to do less type checking and more duck typing.
(In reply to comment #12) > (In reply to comment #11) > > No wait, the GType in private data touches some dangerous code, it's better if > > you look at it first. > > You mean "Store GType in private struct for Boxed"? I'm not sure it's a useful > patch. If everything works without it, let's drop it. Ok, I'll rebase the "add $gtype to constructors" without it. > In the future, I'd like to do less type checking and more duck typing. Duck typing at C structure level? That's a recipe for disaster (especially since private structures don't follow a common layout). Really, everything should type checked twice, before JS_GetPrivate and before passing anything to C code. There should be no way to make a JS program segfault or corrupt memory.
(In reply to comment #13) > Duck typing at C structure level? No, not at the C structure level. Think about $gtype. > That's a recipe for disaster (especially > since private structures don't follow a common layout). Really, everything > should type checked twice, before JS_GetPrivate and before passing anything to > C code. > There should be no way to make a JS program segfault or corrupt memory. These are two different tasks. We shouldn't segfault, we shouldn't segbus, we shouldn't corrupt memory. If we accidentally pass the wrong thing to the wrong function, we don't just silently ignore things and then mask what could be a huge error in the error. We fix the error by figuring out why we passed the wrong thing to the wrong function, and then *fix that*. If it turns out that the JS code didn't pass the correct type to a function, we certainly add a type check and then throw a JS error telling them, but that type checking should be done as early on as we can get. By the time we're calling gjs_boxed_do_thing with the wrong type, we've already failed.
Comment on attachment 207344 [details] [review] Add tests for GType of GValue Attachment 207344 [details] pushed as 3042d49 - Add tests for GType of GValue
Attachment 207346 [details] pushed as 1fc375c - boxed, param, union: add $gtype to constructors Attachment 207347 [details] pushed as d2e7143 - Improve GType detection for unconstrained GValues As for the typechecks, arg.c and function.c do very little to ensure that structures are of the right type when you invoke a function. We check GTypes for GObjects, but that's after retrieving the object from the private structure (and thus already late). Stuff for another bug, though.
Right. And I'm fine with fixing that.