After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 681254 - segfault when json-glib constructs a gobject by deserializing json
segfault when json-glib constructs a gobject by deserializing json
Status: RESOLVED FIXED
Product: gjs
Classification: Bindings
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gjs-maint
gjs-maint
: 739678 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2012-08-05 19:46 UTC by Jamie Nicol
Modified: 2015-12-10 21:42 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
backrace (13.83 KB, text/plain)
2012-08-05 19:46 UTC, Jamie Nicol
  Details
object: Support external construction of gjs-defined GObjects (5.12 KB, patch)
2015-12-10 13:32 UTC, Florian Müllner
none Details | Review
installed-tests: Add test case for externally constructed gjs objects (1.68 KB, patch)
2015-12-10 13:32 UTC, Florian Müllner
committed Details | Review
object: Support external construction of gjs-defined GObjects (5.28 KB, patch)
2015-12-10 21:38 UTC, Florian Müllner
committed Details | Review

Description Jamie Nicol 2012-08-05 19:46:10 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
Comment 1 Giovanni Campagna 2012-08-05 21:25:39 UTC
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.
Comment 2 Giovanni Campagna 2014-11-08 02:10:32 UTC
*** Bug 739678 has been marked as a duplicate of this bug. ***
Comment 3 Giovanni Campagna 2014-11-08 02:12:36 UTC
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.
Comment 4 Bastien Nocera 2015-10-22 14:02:44 UTC
(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.
Comment 5 Jasper St. Pierre (not reading bugmail) 2015-10-22 19:03:34 UTC
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.
Comment 6 Jasper St. Pierre (not reading bugmail) 2015-10-22 19:10:21 UTC
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.
Comment 7 Giovanni Campagna 2015-10-22 22:29:21 UTC
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)
Comment 8 Florian Müllner 2015-12-10 13:32:43 UTC
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.
Comment 9 Florian Müllner 2015-12-10 13:32:51 UTC
Created attachment 317113 [details] [review]
installed-tests: Add test case for externally constructed gjs objects
Comment 10 Bastien Nocera 2015-12-10 14:28:49 UTC
This fixes the test case in bug 739678 (Gom). Thanks!
Comment 11 Cosimo Cecchi 2015-12-10 19:42:06 UTC
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.
Comment 12 Cosimo Cecchi 2015-12-10 19:42:33 UTC
Review of attachment 317113 [details] [review]:

Looks good once the other patch is ready.
Comment 13 Florian Müllner 2015-12-10 20:11:27 UTC
(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?)
Comment 14 Cosimo Cecchi 2015-12-10 20:16:48 UTC
(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.
Comment 15 Florian Müllner 2015-12-10 20:33:55 UTC
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.
Comment 16 Cosimo Cecchi 2015-12-10 21:25:06 UTC
(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.
Comment 17 Florian Müllner 2015-12-10 21:38:54 UTC
Created attachment 317172 [details] [review]
object: Support external construction of gjs-defined GObjects

Fixed space issues (hopefully) and left-over variable declarations.
Comment 18 Cosimo Cecchi 2015-12-10 21:41:23 UTC
Review of attachment 317172 [details] [review]:

Looks great now. Thanks!
Comment 19 Florian Müllner 2015-12-10 21:42:38 UTC
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