GNOME Bugzilla – Bug 681254
segfault when json-glib constructs a gobject by deserializing json
Last modified: 2015-12-10 21:42:50 UTC
Created attachment 220396 [details] backrace Hope I'm right to file a bug here. I think it is a bug and not my fault. This code will produce a segfault (pasting here because it's not too long): const Lang = imports.lang; const GObject = imports.gi.GObject; const Json = imports.gi.Json; const Person = new Lang.Class ({ Name: "Person", Extends: GObject.Object, Properties: { "name": GObject.ParamSpec.string ("name", "", "", GObject.ParamFlags.READABLE | GObject.ParamFlags.WRITABLE, "") }, get name () { return this._name; }, set name (name) { this._name = name; } }); let person = Json.gobject_from_data (Person, '{"name":"John"}', -1); And attached is a backtrace. Using gjs 1.32.0, gobject-introspection 1.32.1, json-glib 0.14.2, glib 2.32.4
Ouch! You're letting the C API create the GObject. This means that it won't pass the necessary properties to set up the JS context and associated JS object. In other words, this is not supported, at the moment. Derived JS classes must be instantiated from JS constructors. However, I'm not saying it's impossible to do, it's just more work. I'll try to see if I can come up with a decent patch to implement this.
*** Bug 739678 has been marked as a duplicate of this bug. ***
In addition to comment 1, the real reason nobody implemented this yet is that there is no good spot to run the JS constructor code, which currently is a bare JS function that creates the object by chaining up through _init() and then does stuff on it.
(In reply to Giovanni Campagna from comment #3) > In addition to comment 1, the real reason nobody implemented this yet is > that there is no good spot to run the JS constructor code, which currently > is a bare JS function that creates the object by chaining up through _init() > and then does stuff on it. I'm poking again, because this makes it impossible to use gom/SQLite for storage in gjs applications.
No matter how much you poke, it still won't solve the hard problems, which you really need to figure out. If you come up with a complete model, I will be happy to implement it. Bonus points if it also works for GtkBuilder construction as well.
One simple option, which is what we were going to do for GtkBuilder: Offer a registration function. Gom.set_constructor_for_gtype(MyType, function() { return new MyType(); }); Now, any time you try to call g_object_new, instead call that constructor function pointer.
One possibility is to go recursive. We override ->constructor(), and check if have our js-context/js-object properites. If yes, we continue to g_object_constructor(), all is well, current code path. If not, we figure out the JS class from the private namespace, then we call it with the demarshalled property values into a raw Object that acts a bag. Eventually the JS constructor will chain up to .parent() / GObject._init, which will call g_object_newv() again, and we'll get the new object we're after. Flash-forward back to the first ->constructor() call, we get the GObject from the JS object, take a ref, root the object, we're happy, we're done, return it. (Might fail for GInitiallyUnowned, but should work for GObject)
Created attachment 317112 [details] [review] object: Support external construction of gjs-defined GObjects Currently GObject subclasses defined in JS must be instantiated from JS constructors, as that's where the association of the newly created native object with the JS context and wrapper object happens. Allow this code to run as well for GObjects that are instantiated externally via g_object_new() (e.g. by GtkBuilder, gom, json-glib, ...) by creating the wrapper object from the ->constructor() vfunc in that case.
Created attachment 317113 [details] [review] installed-tests: Add test case for externally constructed gjs objects
This fixes the test case in bug 739678 (Gom). Thanks!
Review of attachment 317112 [details] [review]: Looks pretty good, just some minor comments. ::: gi/object.cpp @@ +2445,3 @@ + + /* The object is being constructed from JS: + gjs_log_exception(context); Why can't you just chain up to the constructor of the parent class here? @@ +2478,3 @@ + + for (i = 0; i < n_construct_properties; i++) + while (G_OBJECT_CLASS (g_type_class_peek (parent_type))->constructor == gjs_object_constructor) Remove space before paren @@ +2483,3 @@ + + argv = OBJECT_TO_JSVAL(args); + Ditto @@ +2485,3 @@ + object = JS_New (context, constructor, 1, &argv); + } else { + gobj = G_OBJECT_CLASS (g_type_class_peek (parent_type))->constructor (type, n_construct_properties, construct_properties); Ditto @@ +2495,3 @@ + * native code can own. + */ + ObjectInstance *priv; Ditto @@ +2521,2 @@ js_obj = peek_js_obj(object); + jsobj_set_gproperty (context, js_obj, value, pspec); I think you should be able to remove the underscore_name and jsvalue variables from this function here.
Review of attachment 317113 [details] [review]: Looks good once the other patch is ready.
(In reply to Cosimo Cecchi from comment #11) > Why can't you just chain up to the constructor of the parent class here? Those vfuncs are shared between all GObject classes defined in JS, so we need to figure out the parent from the passed-in gtype. And if the direct parent class is itself defined from JS, we end up chaining to the same function indefinitely. (Or maybe I'm overlooking some gtype magic?)
(In reply to Florian Müllner from comment #13) > Those vfuncs are shared between all GObject classes defined in JS, so we > need to figure out the parent from the passed-in gtype. And if the direct > parent class is itself defined from JS, we end up chaining to the same > function indefinitely. > (Or maybe I'm overlooking some gtype magic?) But normally the default implementation would just use the parent function, so chaining to the parent class should have the exact usual behavior, no? Maybe I'm missing something.
Consider this hierarchy: Gjs_Bar : Gjs_Foo : GObject gjs_object_constructor is called with Gjs_Bar's gtype and we can get a pointer to the parent class with g_type_class_peek(g_type_parent(gtype)). So far, so good, however if we then call the constructor() vfunc on that class, we end up in gjs_object_constructor again and g_type_class_peek(g_type_parent(gtype)) will now resolve to the same parent class (i.e. Gjs_Foo's class, not GObjectClass). Usually classes either provide their own constructor() vfunc or they don't, so eventually we end up chaining up to GObject's constructor. But here we have *all* gjs-defined classes sharing a common constructor() vfunc, which has the above result when trying to chain up normally.
(In reply to Florian Müllner from comment #15) > Consider this hierarchy: > Gjs_Bar : Gjs_Foo : GObject > > gjs_object_constructor is called with Gjs_Bar's gtype and we can get a > pointer to the parent class with g_type_class_peek(g_type_parent(gtype)). So > far, so good, however if we then call the constructor() vfunc on that class, > we end up in gjs_object_constructor again and > g_type_class_peek(g_type_parent(gtype)) will now resolve to the same parent > class (i.e. Gjs_Foo's class, not GObjectClass). > > Usually classes either provide their own constructor() vfunc or they don't, > so eventually we end up chaining up to GObject's constructor. But here we > have *all* gjs-defined classes sharing a common constructor() vfunc, which > has the above result when trying to chain up normally. Understood - thanks for the explanation. Only the other minor comments still apply then.
Created attachment 317172 [details] [review] object: Support external construction of gjs-defined GObjects Fixed space issues (hopefully) and left-over variable declarations.
Review of attachment 317172 [details] [review]: Looks great now. Thanks!
Attachment 317113 [details] pushed as 1d7bb71 - installed-tests: Add test case for externally constructed gjs objects Attachment 317172 [details] pushed as a7296c1 - object: Support external construction of gjs-defined GObjects