Created attachment 200813 [details] [review]
util: Make gjs_get_instance_private_dynamic generic

This is a hack for now, that I'll have to find a way around in the future. The
problem is that "Lang.Class" instances don't have a JSClass, but we still need
to be able to set and get private data for them.

Created attachment 200814 [details] [review]
object: Implement gobject inheritance

Created attachment 200815 [details] [review]
function: Make GjsCallbackTrampoline public

Created attachment 200816 [details] [review]
object: Implement vfuncs

Code stolen from pygobject

Created attachment 200817 [details] [review]
Add a test showing what we're now capable of

Yeah, yeah, this is more of an example than a test.

Created attachment 200818 [details] [review]
Add helper "pointer_string" method for debugging

I'm going to be doing most of this development in a branch:

https://github.com/magcius/gjs
https://github.com/magcius/gobject-introspection

(In reply to comment #1)
> Created an attachment (id=200813) [details] [review]
> util: Make gjs_get_instance_private_dynamic generic
> 
> This is a hack for now, that I'll have to find a way around in the future. The
> problem is that "Lang.Class" instances don't have a JSClass, but we still need
> to be able to set and get private data for them.

I think this is the strategy that I'm going to go with. You can't set an object's JSClass after creation, so our only other strategy is adding a public hook to define a stub class. For various reasons, that's extraordinarily hard, so I'm going to remove priv_with_js_with_typecheck and the dynamic priv_from_js, and just make priv_from_js a wrapper for:

    gpointer priv;
    JS_BeginRequest(context);
    priv = JS_GetPrivate(context, obj);
    JS_EndRequest(context);
    return priv;

Looking at your branch, I'm sorry to report that removing priv_from_js_with_typecheck() breaks boxed_get_copy_source(), since that function is called even on non boxed objects (which has undefined results / segfaults).

I'm going to rebase around my work without that removal (which seems gratuitous).

(My plan is to implement meta classes using the class framework, then move GObject inheritance to an explicit GObject.Class meta, then rework the GDBus bindings to take advantage of that and finally be usable. I'll push at github when I have something)

(In reply to comment #9)
> Looking at your branch, I'm sorry to report that removing
> priv_from_js_with_typecheck() breaks boxed_get_copy_source(), since that
> function is called even on non boxed objects (which has undefined results /
> segfaults).

Oh, I pushed that broken patch? I pushed a fixed version now.

Also, you have a problem in 89f8ec16 "arg: Don't copy boxed types" (I don't know where this patch is hosted): you set the NO_COPY flag on all STRUCT types, but then unref the variants (that are not boxed).
Following the advice at http://git.gnome.org/browse/glib/tree/gobject/gobject.c#n3216 (the same advice is also in the GObject docs, though I can't find where), I fixed it to set NO_COPY only for subtypes of G_TYPE_BOXED.

I really don't like that variants are of GI_INFO_TYPE_BOXED, but actually aren't boxed. That's a big gotcha.

(In reply to comment #12)
> I really don't like that variants are of GI_INFO_TYPE_BOXED, but actually
> aren't boxed. That's a big gotcha.

There is a lot of stuff that's GI_INFO_TYPE_STRUCT without being a registered boxed (stack allocate structs), and there are many boxeds that are not GI_INFO_TYPE_STRUCT (unions, opaque types). GVariant is not the first here.
Btw, to handle non boxed structs, I had to revert the check (so variants get the ref, and everything else gets no_copy).

So, what I should I do:

GI_INFO_TYPE_STRUCT gets NONE, GI_INFO_TYPE_BOXED gets NO_COPY, except for variants, which get ref'd and NONE?

(In reply to comment #14)
> So, what I should I do:
> 
> GI_INFO_TYPE_STRUCT gets NONE, GI_INFO_TYPE_BOXED gets NO_COPY, except for
> variants, which get ref'd and NONE?

Locally I made NO_COPY for everything (including G_TYPE_NONE GI_INFO_TYPE_STRUCT, GI_TYPE_BOXED GI_INFO_TYPE_STRUCT and GI_INFO_TYPE_BOXED) but variants. I guess we can also have everything NO_COPY and just remove the extra unref when releasing.

(Even if refcounted, variants are effectively value types, in that you cannot modify the contents after created, so it should not matter much).

(Btw: half week of hacking online at https://github.com/gcampax/gjs/commits/meta-class - including a lot of building upon this bug and your branch)

Isn't it entirely possible that while we're inside the JS function, it calls something that releases its ref to the variant, effectively destroying it, even when we're supposed to have a ref to it in the JS function?

(In reply to comment #16)
> Isn't it entirely possible that while we're inside the JS function, it calls
> something that releases its ref to the variant, effectively destroying it, even
> when we're supposed to have a ref to it in the JS function?

Uhm... Technically, yes - in the sense that the JS function could store the variant somewhere, and the C function outside the callee could unref the variant. The problem is the same as boxed, though: every time the JS function receives something it doesn't own, and stores it, we're potentially in trouble, as the C caller is likely to free that value (or worse, to have it in the stack)


Btw, I found a deeper problem in the way we retrieve C implementations of vfuncs. You implemented the same scheme as used by methods, and I extended it locally to work with interfaces.
The problem is, this retrieves the vfunc info on the prototype of the class that defines it; that class's GTypeClass has the first implementation of the vfunc, but C subclasses could have overridden that, and we wouldn't notice.
On the other hand, if we just used g_type_class_peek_parent from the instance, we could invoke JS subclass methods more than once, if we derive a JS GObject class from another.

Also, I fear there may be another problem with this patch (not sure, cause I didn't test it yet): since new instances don't have a dynamic JSClass, they don't get object_instance_get_prop, therefore you cannot retrieve gobject properties from those (they're defined in the instance, not in the prototype).
There are two approaches to this: either we make the JS object have a JSClass (need to hack around Lang.Class, to ensure that we return the constructor from JS_InitClass, with constructor.__proto__ === GObject.Class.prototype), or we put gobject properties on the prototype (hackish, but technically possible, as I read the documentation...)

(In reply to comment #17)
> (In reply to comment #16)
> > Isn't it entirely possible that while we're inside the JS function, it calls
> > something that releases its ref to the variant, effectively destroying it, even
> > when we're supposed to have a ref to it in the JS function?
> 
> Uhm... Technically, yes - in the sense that the JS function could store the
> variant somewhere, and the C function outside the callee could unref the
> variant. The problem is the same as boxed, though: every time the JS function
> receives something it doesn't own, and stores it, we're potentially in trouble,
> as the C caller is likely to free that value (or worse, to have it in the
> stack)

Right. As I understand it, this is why we copied before. One solution we could do is to have a special kind of boxed copy that copies the data, but keeps around a reference of the original, and the boxed's getters/setters update both the copy and the original. This is the "best of both worlds" approach.

> Btw, I found a deeper problem in the way we retrieve C implementations of
> vfuncs. You implemented the same scheme as used by methods, and I extended it
> locally to work with interfaces.
> The problem is, this retrieves the vfunc info on the prototype of the class
> that defines it; that class's GTypeClass has the first implementation of the
> vfunc, but C subclasses could have overridden that, and we wouldn't notice.
> On the other hand, if we just used g_type_class_peek_parent from the instance,
> we could invoke JS subclass methods more than once, if we derive a JS GObject
> class from another.

I didn't understand this. What's the main problem, here? Have a quick example?

> Also, I fear there may be another problem with this patch (not sure, cause I
> didn't test it yet): since new instances don't have a dynamic JSClass, they
> don't get object_instance_get_prop, therefore you cannot retrieve gobject
> properties from those (they're defined in the instance, not in the prototype).
> There are two approaches to this: either we make the JS object have a JSClass
> (need to hack around Lang.Class, to ensure that we return the constructor from
> JS_InitClass, with constructor.__proto__ === GObject.Class.prototype), or we
> put gobject properties on the prototype (hackish, but technically possible, as
> I read the documentation...)

That's the point of this commit: https://github.com/magcius/gjs/commit/d2f756a84dfedc729e5ee0f8582e944778f3100c

It's extremely hard to make a subclass from C and then return it to JS. I've tried it several times, and I always fail. If you want to take a stab at it...

(In reply to comment #18)
> (In reply to comment #17)
> [...]
> 
> Right. As I understand it, this is why we copied before. One solution we could
> do is to have a special kind of boxed copy that copies the data, but keeps
> around a reference of the original, and the boxed's getters/setters update both
> the copy and the original. This is the "best of both worlds" approach.

No, it has the same problem: you don't know when it's safe to update the "real" object and when it would cause a segfault.
If you really want reference types (updating one changes all the copies of it), I guess there is no alternative to toggle references...

There are 2 cases that we can still address, though:
1) boxed that return the same C object from g_boxed_copy() - no problem, just copy everytime (this includes variants)
2) boxed that we allocate ourselves with g_slice - we can keep the reference count ourselves, and share the underlying structure
Case 2 should address the majority of G_SIGNAL_TYPE_STATIC_SCOPE uses (which I see used for stack allocated types like ClutterActorBox or GtkTreeIter), without memory safety issues.

> > Btw, I found a deeper problem in the way we retrieve C implementations of
> > vfuncs. You implemented the same scheme as used by methods, and I extended it
> > locally to work with interfaces.
> > The problem is, this retrieves the vfunc info on the prototype of the class
> > that defines it; that class's GTypeClass has the first implementation of the
> > vfunc, but C subclasses could have overridden that, and we wouldn't notice.
> > On the other hand, if we just used g_type_class_peek_parent from the instance,
> > we could invoke JS subclass methods more than once, if we derive a JS GObject
> > class from another.
> 
> I didn't understand this. What's the main problem, here? Have a quick example?

Yep: Gtk.Application.
Let's assume I want to override startup.
I write all my boilerplate, then write
do_startup: function() {
    this.parent();
}
Here: this.parent calls this.__caller__.super.prototype.do_startup, that is, Gtk.Application.prototype.do_startup. Since there is no startup vfunc in Gtk.Application, lookup continues at Gtk.Application.prototype.__proto__.do_startup, or Gio.Application.prototype.do_startup, where the function is defined.
At that point, the obj given to object_new_resolve is Gio.Application.prototype, and priv->gtype is GApplication, which results in g_type_class_ref (inside g_vfunc_info_get_address) returning the pristine GApplication class, not that from GtkApplication, so the function pointer is wrong, and gtk_application_real_startup() is never called.
On the other hand, if we used the "this" object given to function_call (instead of resolving the virtual pointer immediately and caching it), and retrieved the pointer upon invocation, we could not now how many levels of g_type_class_peek_parent we need to find the first non-JS implementation of the vfunc.

> [...]
> 
> That's the point of this commit:
> https://github.com/magcius/gjs/commit/d2f756a84dfedc729e5ee0f8582e944778f3100c
> 
> It's extremely hard to make a subclass from C and then return it to JS. I've
> tried it several times, and I always fail. If you want to take a stab at it...

I see... bah, I'll try and see what I come up with.

It seems PyGObject doesn't do it correctly either:

>>> Gtk.Application.do_startup == Gio.Application.do_startup
True

The proper way I'd like to do this is to expose gtk_application_real_startup as Gtk.Application.do_startup. I filed bug #666027 about this. I'm going to hack on a solution for this right now.

I had an idea.

Read the giant comment at the start of: https://github.com/magcius/gjs/blob/master/gi/object.c#L328

I started by defining a vfunc for every sub-type, even it's not overridden. I then take advantage of the fact that the subtype's class struct is copied when subclassing an object, so I can simply test if an object's vfunc pointer is the same as its parent, and then fall back to prototype chaining there.

Created attachment 203662 [details] [review]
function: Make GjsCallbackTrampoline public

Created attachment 203663 [details] [review]
function: Allow out parameters in callbacks and vfuncs

If a callback needs an out parameter or return value to work,
we need a way for vfunc implementation to take advantage of this.
The pattern:

If the callback takes a non-void return value and no out args:
  The callback should return the return value:
    return return_value;

If the callback takes no return value and has one out arg:
  The callback should return the one out arg:
    return out_arg_1;

If the callback takes no return value and has multiple out args:
  The callback should return an array containg the out args in sequence:
    return [out_arg_1, out_arg_2, ..., out_arg_N];

If the callback takes a non-void return value and has multiple out args:
  The callback should return an array containing first the return value,
  then the out args in sequence:
    return [return_value, out_arg_1, out_arg_2, ..., out_arg_N];

Created attachment 203664 [details] [review]
Expose parent vfuncs on the parent prototype, prefixed with "do_"

Created attachment 203665 [details] [review]
util: Make GJS_DEFINE_DYNAMIC_PRIV_FROM_JS generic

Because we expect subclasses of GI objects to be just regular functions,
we can't assert that they have a JSClass. Unfortunately, this means that
we can't retrieve private JS data for a subclass. To fix this, make
the helpers installed by GJS_DEFINE_DYNAMIC_PRIV_FROM_JS simple wrappers
around JS_GetPrivate. In the future, we may want to consider removing
the dynamic class system and replacing it with something a lot simpler.

Created attachment 203666 [details] [review]
object: Implement gobject inheritance

Created attachment 203667 [details] [review]
object: Throw an error if priv is NULL, giving a common reason

If you forget to chain up in your _init override, you can be left
staring at "JS_ExecuteScript() failed but no exception message?",
clueless as to what has gone wrong. Let's provide an exception
message that makes sense for the user.

Created attachment 203668 [details] [review]
object: Allow subclasses to provide their own vfunc overrides

Some code stolen from pygobject

Created attachment 203669 [details] [review]
lang: Add helper "pointer_string" method for debugging

Created attachment 203670 [details] [review]
arg: Don't copy boxed types

In some places, an API passes a vfunc a boxed parameter and expects
the vfunc implementation to modify it. We can't copy boxed objects
in this case, as the modifications that the vfunc implementation did
won't apply to the copy.

We still need to not copy variants, as we ref them at the end of
the function.

Created attachment 203671 [details] [review]
object: Add a new register_property function

It takes a prototype and a ParamSpec and uses g_object_install_property
to install the corresponding property.

    imports._gi.register_property(my_constructor.prototype,
                                  GObject.ParamSpec.int("foo-property",
                                                        "Foo property",
                                                        "Blurb about Foo",
                                                        GObject.ParamSpecFlags.READABLE,
                                                        0, 255, 42))

Created attachment 203672 [details] [review]
object: Implement get/set property

Created attachment 203673 [details] [review]
lang: Hook up properties automatically

If a class has a GProperties instance full of GParamSpec instances, all the
properties in it will be installed on the corresponding GType.

Review of attachment 203672 [details] [review]:

This patch should also block do_set_property/do_get_property from prototype chains, since GType does not copy them (or actually, GObject.base_init clears them), and in any case they have a different signature from what gjs_call_function_value expects.

::: gi/object.c
@@ +1873,3 @@
+    jsval get_property_func;
+
+    runtime = g_object_get_qdata(object, __gjs_runtime);

This is not going to work, if gjs::runtime is not yet set, which can unfortunately happen if you have construct properties.
The way I implemented them locally was with to additional construct-only properties (js object and context), of type G_TYPE_POINTER, and some magic when building the arguments to g_object_newv (which sets them in the right order).

::: modules/lang.js
@@ +236,3 @@
+                prop === 'do_set_property')
+                continue;
+

If we have special support for do_get_property/do_set_property, we should also provide a default implementation of it (I think that routing it to normal JS property access is fine)

Review of attachment 203668 [details] [review]:

::: modules/lang.js
@@ +233,3 @@
+            let value = newClass.prototype[prop];
+            if (typeof value === 'function' && prop.slice(0, 3) == 'do_') {
+                Gi.hook_up_vfunc(newClass.prototype, prop.slice(3), value);

Hooking the exact function means that setting .prototype does not work (which would for example prevent gnome-shell extensions from working, once we start getting rid of ShellGenericContainer), and means that we leak the function and all its enclosing scope (which includes the module and all objects that are defined in it).
Hooking "function() { this[name].apply(this, arguments); }" (and doing it if and only if there is no implementation of it in the JS parent) solves this problem.

Review of attachment 203664 [details] [review]:

::: gi/object.c
@@ +247,3 @@
+    }
+
+    addr2 = g_vfunc_info_get_address(info, ptype, &error);

What if the vfunc first appeared in gtype? Reading its address from ptype's class would give garbage, and possibly segfault, I think.

@@ +361,3 @@
+                g_base_info_unref((GIBaseInfo *)vfunc);
+                ret = JS_TRUE;
+                goto out;

Maybe we should cache this condition for (name, gtype)?
Otherwise, we walk the whole chain every time a JS class calls parent(), and the C class does not override the parent.
(If the C class does override, then we define the property, and JSClass.resolve is not called)

Btw, having this in github as well would have made rebasing my work easier...

(maybe we should reconsider the "bugzilla patches only" policy, and start reviewing entire git branches)

Sorry, my fault, I thought git fetch with no arguments would update all branches.
I rebased my stuff at gcampax/gjs/meta-class, it's almost ready (it passes tests, at least)

OK, I've cleaned up and rebased and worked hard on getting this into a reviewable state. Thanks especially to Giovanni for all your hard work!

It's a lot of random things, so I stuck it in a branch called "wip/gobj-kitchen-sink":

http://git.gnome.org/browse/gjs/log/?h=wip/gobj-kitchen-sink

This seems like it needs some rebasing for "make check" to pass at every point.  For example 6fd48fc376e6a6b19b1c7d7268ecbff9688e64cd contains test cases for "do_" prefixing that are only implemented in the next commit.

Also, I find the "do_" convention really hacky...what about something like:

const VFuncTester = new Lang.Class({
    Name: 'VFuncTester',
    Extends: GIMarshallingTests.Object,

    gobject_vfuncs: {
      return_value_only: function () {
        ... 
    },

    one_out_parameter: function () {
        ...
    },
}

Can you remind me what the point of the metaclass stuff is?  We only have one metaclass being GObjectMeta, right?  How much easier/clearer would things be if we wired up the GObject inheritance more directly?

(In reply to comment #40)
> This seems like it needs some rebasing for "make check" to pass at every point.
>  For example 6fd48fc376e6a6b19b1c7d7268ecbff9688e64cd contains test cases for
> "do_" prefixing that are only implemented in the next commit.
> 
> Also, I find the "do_" convention really hacky...what about something like:
> 
> const VFuncTester = new Lang.Class({
>     Name: 'VFuncTester',
>     Extends: GIMarshallingTests.Object,
> 
>     gobject_vfuncs: {
>       return_value_only: function () {
>         ... 
>     },
> 
>     one_out_parameter: function () {
>         ...
>     },
> }

How do we expose vfunc implementations on other classes?

  Clutter.Actor.prototype.gobject_vfuncs.get_preferred_width ?

That would require modifications to the "parent" function to do something special in the context of a vfunc.

(In reply to comment #41)
> Can you remind me what the point of the metaclass stuff is?  We only have one
> metaclass being GObjectMeta, right?

See the "GDBus: introduce new convenience wrappers" commit. It adds new metaclasses.

See https://github.com/magcius/gnome-shell/commit/6361305d9946aee060542d67cbe2c43d2d2ae727 for an example of gnome-shell ported to the new GDBus convenience wrappers that replace makeProxyMethod/wrapJSObject.

> How much easier/clearer would things be if
> we wired up the GObject inheritance more directly?

My original approach did something like that -- see the patches in this bug, for instance.

gjs_parse_args() for gjs_register_type() is safer/clearer.

We leak "name" if we throw in gjs_register_type() too.

I'm worried the "don't copy boxed types" patch is going to break (out caller-allocates) structures.  Maybe we don't have any of those in the test suite?  It's not clear to me from your commit message why the nasty code and big comment can now be deleted.

The quark stuff in "support glib properties" seems likely to also trigger bug 626682.

And the refcounting changes in here look REALLY suspicious.  What of this requires changing the object refcount logic?  It looks to me like we're losing an _unref() call.

If you have to touch the refcount code please break it out into a separate commit.

"Move GParamSpec registration" seems to duplicate the g_irepository_find_by_name() logic.  Bad rebase?

And the "GParamSpec: set an exception..." is crying out for use of gjs_parse_args()

Okay it's going to take me a while to get my head around the GDBus convenience wrappers.  Skipped for now.  Maybe it can be pushed to the top of the stack?

The gjs_register_type() part of of "object: Introduce support for signals" seems like it could be rebased earlier.  And I still don't like the do_ and on_ prefixing.

Can we push the "boxed: Introduce support for complex constructors" until later?

Ok going to take a break for now...

(In reply to comment #42)

>   Clutter.Actor.prototype.gobject_vfuncs.get_preferred_width ?
> 
> That would require modifications to the "parent" function to do something
> special in the context of a vfunc.

Hm, where is the parent() function?


One other random question - how does this work intersect at all with the ES6 classes?

(In reply to comment #51)
> Hm, where is the parent() function?

http://git.gnome.org/browse/gjs/tree/modules/lang.js#n145

> One other random question - how does this work intersect at all with the ES6
> classes?

It's orthogonal - the proposed class syntax ( http://wiki.ecmascript.org/doku.php?id=harmony:classes ) doesn't have a meta-class system, so if we choose to convert to that system in the future, then we'd need some form of registration. It could be something like:

  class MyClutterActor extends Clutter.Actor {
      do_get_preferred_width() {
          return [500, 500];
      }

      // ...
  }

  MyClutterActor = GObject.register(MyClutterActor);

GObject.register would do everything that GObjectMeta currently does. The main problem here is the JSClass - we can't simply register things with the system, because in SpiderMonkey they need to have the correct JSClass for priv_from_js to work. You can't set a JSClass on an existing object, so that won't work. Of course, the specification doesn't say if the binding that the class statement creates is "const" or not, so I'm not even sure if that would work.

If we replaced the JSClass stuff with something else, or at least the priv_from_js type-checking part of it, then this patch stack would become a lot simpler.

Of course, we could also have:

  class One extends Clutter.Actor {
      do_get_preferred_width() { return [500, 500]; }
  }

  class Two extends Clutter.Actor {
      do_get_preferred_width() { return [500, 500]; }
  }

  GObject.scanAndRegister(this);

which would scan the passed object for GObject subclasses:

  function scan_and_register(obj) {
      for (let o of obj)
          if (o.prototype && o.prototype instanceof GObject.Object)
             GObject.register(o);
  }

(In reply to comment #46)
> I'm worried the "don't copy boxed types" patch is going to break (out
> caller-allocates) structures.  Maybe we don't have any of those in the test
> suite?

Why?

> It's not clear to me from your commit message why the nasty code and
> big comment can now be deleted.

We copied the boxed, and then freed the old one. The big comment and nasty code were about freeing the old one. If we don't get rid of that, we're freeing the boxed that we're now not copying, making the boxed wrapper point to freed memory.

(In reply to comment #47)
> The quark stuff in "support glib properties" seems likely to also trigger bug
> 626682.

Why?

> And the refcounting changes in here look REALLY suspicious.  What of this
> requires changing the object refcount logic?

We're changing the logic for how the JS object is stored on the GObject, moving it to a construct property so that other construct properties can properly reference the JS object. Before we did something like this:

    priv->gobj = g_object_newv(...);
    g_object_set_data(priv->gobj, "gjs-js-object", obj);

The problem is that if a JS implementation installs a GObject property, it can't be used as a construct property because grabbing the JS object from the GObject will fail because the two haven't been associated at the time g_object_newv is called. So instead, we're doing:

    priv->gobj = g_object_new(..., "js-object", obj);

where the set_property implementation for that sets the "gjs-js-object" data.

> It looks to me like we're losing an _unref() call.

I don't see it. We're moving an unref call from manage_js_gobject/associate_js_gobject to the callers because we don't want the _unref() when we're setting the construct property.

> If you have to touch the refcount code please break it out into a separate
> commit.

There's no reason to split it out unless we move to a construct property like we're doing here.

I just want to mention that the auto-hookup of "do_"-prefixed functions and "on_"-prefixed functions is something that I stole from pygobject, but:

http://git.gnome.org/browse/gjs/commit/?h=wip/gobj-kitchen-sink&id=f24087654f894c0527851b15a25ef901b3e095e5
http://git.gnome.org/browse/gjs/commit/?h=wip/gobj-kitchen-sink&id=6a1b1938cd89d7b48cfbdcb3559bb01b3df3c0a5

The implementation is messy in a few ways: I find myself expanding GJS_DEFINE_PROTO because I really don't want to write the same boilerplate for the tenth time. We should probably make a new set of macros and use those all over the place consistently -- the "define_class" method which inits the class has three or four different signatures across all the objects and it's hard to remember.

Any way, this all has testcases, and they all pass.

(In reply to comment #48)
> "Move GParamSpec registration" seems to duplicate the
> g_irepository_find_by_name() logic.  Bad rebase?

Seems to be…

> And the "GParamSpec: set an exception..." is crying out for use of
> gjs_parse_args()

It required feature additions to gjs_parse_args, but this is done. I also reworked most of the other functions to use it as well -- I just didn't know about it!

(In reply to comment #53)

> > It's not clear to me from your commit message why the nasty code and
> > big comment can now be deleted.
> 
> We copied the boxed, and then freed the old one. The big comment and nasty code
> were about freeing the old one. If we don't get rid of that, we're freeing the
> boxed that we're now not copying, making the boxed wrapper point to freed
> memory.

The dangerous thing about this is that boxed return values now can only be referred to by JS as long as the C code that "owns" them runs.  For example if a GObject returned a ClutterColor * that just referred to priv->color object, and later we destroy the object, any accesses to the color.red property are going to refer to freed memory.

In C, this kind of thing happens all of the time - e.g. every time you get a const char * you expect the lifetime of the string to only be as long as the C object.

But in JS there is an expectation of safety, e.g. that if you happen to do myobject.color = rectangle.get_color(), and then later run LookingGlass and inspect myobject, your desktop won't crash.


Now what I was referring to with the change to function.c is that in the (out caller-allocates) case we're allocating the a temporary C structure with g_slice (which we knew we could do because we copied it, then freed it), but now you're assuming that the C code implementing the boxed happens to use g_slice_free, but that may not be true.  And in fact is often not true.

What's the problem with copying boxed types?  What boxed type was being copied that you didn't want to be?

(In reply to comment #57)
> What's the problem with copying boxed types?  What boxed type was being copied
> that you didn't want to be?

Virtual functions. A virtual function may hand you a boxed type and expect you do modify fields on it. If we copy that, the vfunc implementation won't copy the real boxed, but a dummy one which is then freed.

(In reply to comment #54)
> (In reply to comment #47)
> > The quark stuff in "support glib properties" seems likely to also trigger bug
> > 626682.
> 
> Why?
> 
> > And the refcounting changes in here look REALLY suspicious.  What of this
> > requires changing the object refcount logic?
> 
> We're changing the logic for how the JS object is stored on the GObject, moving
> it to a construct property so that other construct properties can properly
> reference the JS object.

I see that part.  What made me pause was the fact that the patch touched the "if (G_IS_INITIALLY_UNOWNED(priv->gobj))" code but upon closer inspection, it looks like it's actually just whitespace changes.

> I don't see it. We're moving an unref call from
> manage_js_gobject/associate_js_gobject to the callers because we don't want the
> _unref() when we're setting the construct property.

OK.

(In reply to comment #58)
> (In reply to comment #57)
> > What's the problem with copying boxed types?  What boxed type was being copied
> > that you didn't want to be?
> 
> Virtual functions. A virtual function may hand you a boxed type and expect you
> do modify fields on it. If we copy that, the vfunc implementation won't copy
> the real boxed, but a dummy one which is then freed.

What function and what boxed type?  It's really helpful for me to have a concrete example to work with.

So as far as modifying fields...that's a messy area.  If that's desired I think the boxed type should probably be changed to be refcounted instead of copied.  Then it's very clear how mutation works - there is only one struct in memory, always.

Another option would be to pass a flag down through the argument conversion code which says to not copy if we're converting an argument to invoke a signal handler or vfunc override.  That'd probably be pretty easy?

But if we don't copy after function returns - that's dangerous.

(In reply to comment #60)
> (In reply to comment #58)
> > (In reply to comment #57)
> > > What's the problem with copying boxed types?  What boxed type was being copied
> > > that you didn't want to be?
> > 
> > Virtual functions. A virtual function may hand you a boxed type and expect you
> > do modify fields on it. If we copy that, the vfunc implementation won't copy
> > the real boxed, but a dummy one which is then freed.
> 
> What function and what boxed type?  It's really helpful for me to have a
> concrete example to work with.

I... I actually don't remember. I just removed the patch, rebuilt gjs and my Clutter example worked fine... tests passed, and gnome-shell is running with it right now, so we can ignore that patch for now. Whoops.

Pretty much everything here has landed except Giovanni's new "GDBus convenience wrappers". I filed bug #669350 for them.