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 698614 - GObject: prevent installing properties after init
GObject: prevent installing properties after init
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: gobject
unspecified
Other All
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on: 701196 701804
Blocks:
 
 
Reported: 2013-04-22 21:40 UTC by Allison Karlitskaya (desrt)
Modified: 2014-06-10 21:00 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
GObject: prevent installing properties after init (2.74 KB, patch)
2013-04-22 21:40 UTC, Allison Karlitskaya (desrt)
committed Details | Review
Revert "GObject: prevent installing properties after init" (2.36 KB, patch)
2014-06-06 20:44 UTC, Allison Karlitskaya (desrt)
committed Details | Review
docs: warn about installing properties after init (2.04 KB, patch)
2014-06-06 20:44 UTC, Allison Karlitskaya (desrt)
committed Details | Review

Description Allison Karlitskaya (desrt) 2013-04-22 21:40:50 UTC
This will allow us to nuke GParamSpecPool, which will be awesome.
Comment 1 Allison Karlitskaya (desrt) 2013-04-22 21:40:52 UTC
Created attachment 242171 [details] [review]
GObject: prevent installing properties after init

GObject has previously allowed installing properties after class_init
has finished running.  This means that you could install some of your
own properties on G_TYPE_OBJECT, for example, although they wouldn't
have worked properly.

Prevent this from happening.  Require that all properties are installed by
the time class_init has finished.

Complaints go to this bug:
Comment 2 Alexander Larsson 2013-04-22 22:08:29 UTC
Review of attachment 242171 [details] [review]:

otherwise it looks good

::: gobject/gtype.c
@@ +4649,3 @@
+
+gboolean
+g_type_is_in_init (GType type)

This only works for classed types (due to looking at node->data->class), so it would be nice to reflect that in the name.

@@ +4655,3 @@
+  node = lookup_type_node_I (type);
+
+  return node->data->class.init_state != INITIALIZED;

Will crash if node->data == NULL
Comment 3 Allison Karlitskaya (desrt) 2013-04-22 22:11:02 UTC
This isn't a public API and it only gets called from places where the problems that you point out won't matter...

What do you prefer I call it?
Comment 4 Alexander Larsson 2013-04-22 22:11:59 UTC
Review of attachment 242171 [details] [review]:

argued offlines that these are non-issues
Comment 5 Allison Karlitskaya (desrt) 2013-04-22 22:12:28 UTC
Attachment 242171 [details] pushed as ddb0ce1 - GObject: prevent installing properties after init
Comment 6 Allison Karlitskaya (desrt) 2013-04-22 22:33:46 UTC
We had to revert this already -- problems in gjs and gnome-settings-daemon.
Comment 7 Allison Karlitskaya (desrt) 2013-05-29 13:25:51 UTC
I've uploaded a new version of this patch with a critical instead.
Comment 8 Giovanni Campagna 2013-06-03 21:33:47 UTC
With this patch, metacity tells me:

(metacity:8953): GLib-GObject-CRITICAL **: Attempt to add property GtkSettings::gtk-button-images after class was initialised
and
Log level 8: Attempt to add property GtkSettings::gtk-label-select-on-focus after class was initialised

metacity is 2.34.13 from fedora packages, gtk+-2 is from jhbuild
Other gtk2 apps don't seem to have this problem.
Comment 9 Giovanni Campagna 2013-06-03 21:33:47 UTC
With this patch, metacity tells me:

(metacity:8953): GLib-GObject-CRITICAL **: Attempt to add property GtkSettings::gtk-button-images after class was initialised
and
Log level 8: Attempt to add property GtkSettings::gtk-label-select-on-focus after class was initialised

metacity is 2.34.13 from fedora packages, gtk+-2 is from jhbuild
Other gtk2 apps don't seem to have this problem.
Comment 10 Giovanni Campagna 2013-06-04 16:45:38 UTC
You want more?

Here you are:
(gnome-contacts:2271): GLib-GObject-CRITICAL **: Attempt to add property ESourceCamelEws::filter-inbox after class was initialised

(one for each ESourceCamel* property).
Comment 11 Emmanuele Bassi (:ebassi) 2013-06-05 00:00:52 UTC
The gtk2 warning is gtk2 brain damage: it installs setting properties from various class_init instead of using GtkSettings.class_init(); it should actually be fixed regardless of this warning, as it makes settings depend on certain GTypes being initialised. I remember fixing that for gtk3, so it should be a case of back porting the fix and releasing a new 2.24 tarball.
Comment 12 Giovanni Campagna 2013-06-05 16:35:42 UTC
(In reply to comment #11)
> The gtk2 warning is gtk2 brain damage: it installs setting properties from
> various class_init instead of using GtkSettings.class_init(); it should
> actually be fixed regardless of this warning, as it makes settings depend on
> certain GTypes being initialised. I remember fixing that for gtk3, so it should
> be a case of back porting the fix and releasing a new 2.24 tarball.

I don't disagree with that, but it shows there are apps and libraries out there that hard depend on this misfeature, which IMHO means we cannot remove it until glib 4.
Comment 13 Emmanuele Bassi (:ebassi) 2013-06-05 21:28:19 UTC
(In reply to comment #12)
> (In reply to comment #11)
> > The gtk2 warning is gtk2 brain damage: it installs setting properties from
> > various class_init instead of using GtkSettings.class_init(); it should
> > actually be fixed regardless of this warning, as it makes settings depend on
> > certain GTypes being initialised. I remember fixing that for gtk3, so it should
> > be a case of back porting the fix and releasing a new 2.24 tarball.
> 
> I don't disagree with that, but it shows there are apps and libraries out there
> that hard depend on this misfeature, which IMHO means we cannot remove it until
> glib 4.

we may very well *never* have a GLib API major bump (be it 3.0 or 4.0), given the grief implied in such a thing.

I also very much doubt people are actually relying on this dubious "feature" (which as it is lies in the grey area of undocumented behaviour). the code in gtk2 literally introduced bugs and weird behaviour (requiring classes to be created in order to query settings).

I fully appreciate the fact that there may be code out there doing this kind of crazy crack with the type system, but I also think that long term maintainability and performance are much more critical than preserving undocumented/undefined behaviour.
Comment 14 Dan Winship 2013-06-05 22:39:27 UTC
random thought, and I don't know how well this plays with your "nuke GParamSpecPool" plan, but... we can sort of tell if a program is old and crufty and not keeping up to date with modern APIs, based on whether or not it manually calls g_type_init(). So could we make it so that old code gets to keep the old behavior, and only new code is forced into the new behavior?
Comment 15 Colin Walters 2013-06-07 17:22:25 UTC
(In reply to comment #14)
> random thought, and I don't know how well this plays with your "nuke
> GParamSpecPool" plan, but... we can sort of tell if a program is old and crufty
> and not keeping up to date with modern APIs, based on whether or not it
> manually calls g_type_init(). So could we make it so that old code gets to keep
> the old behavior, and only new code is forced into the new behavior?

In that plan then, we keep the old code paths forever?  If that's not too much pain, it seems like a kind of reasonable hack.

We definitely need to try hard to support people using a brand new glib with old versions of gtk and applications, as much as reasonable.

Anyways, let's expand the reports here into proper bugs.
Comment 16 Allison Karlitskaya (desrt) 2013-06-07 17:30:43 UTC
The problem with this approach is that we already told people to stop calling g_type_init().  We can't introduce new restrictions on what that means now...

It also doesn't solve the problem of old libraries being used from new programs.

I'm not sure about keeping the old codepaths around for a long time.... with interfaces, people sort of fixed their stuff and the world moved on.  C++ and C# bindings are the last case there, and they're expected to be done during this cycle.  Meanwhile, distros tend to update these things more or less in sync with each other, so although we don't explicitly say "you can't install a new glib unless you also take the new pygobject", everyone is already doing this anyway...

I think this is not too dissimilar: there are a few cases of problems that this is causing.  We need to fix them and move on...


That said, I'm not about to declare a hard cut-off until I'm sure that it won't be a problem.  This is exactly why I want the critical here -- we know that there are some problems in the wild already... this has convinced me that there are probably a few more (3rd party or closed source apps?) that we don't know about... We need to make sure we keep getting this information.
Comment 17 Colin Walters 2013-06-07 17:52:10 UTC
(In reply to comment #16)
>  Meanwhile, distros tend to update these things more or less in
> sync with each other,

In Red Hat Enterprise Linux, we definitely have cases where we want to run incredibly ancient code against brand new glib.  I don't always expect that to work perfectly, but even small incompatibilities can magnify into a lot of pain fixing up apps that aren't worth much engineering time.

> so although we don't explicitly say "you can't install a
> new glib unless you also take the new pygobject", everyone is already doing
> this anyway...

I'd say bindings are expected to be a bit more special, yes.  gobject-introspection is certainly hard-tied to a specific GLib version.

> That said, I'm not about to declare a hard cut-off until I'm sure that it won't
> be a problem.  This is exactly why I want the critical here -- we know that
> there are some problems in the wild already... this has convinced me that there
> are probably a few more (3rd party or closed source apps?) that we don't know
> about... We need to make sure we keep getting this information.

Yeah, I understand that.  I guess though I'd like to be in a world where we make a coordinated change to "gnome" (glib + bindings + core apps), and then once that works, we work on the rest of the ecosystem.
Comment 18 Paul Davis 2013-10-16 18:54:15 UTC
the ability to define custom Gtkmm widgets with Glib::Properties depends upon being able to register properties after class init.

C++ has no introspection, so it is not possible for the code that calls  g_type_register_static to know what properties need to be registered. By the time Glib::PropertyBase::install_property() is called (as the class member property is created), class init has already been done.

This does not affect Gtkmm itself, because all of its Glib::Properties are actually just wrappers around existing Gtk/Glib-level properties.

But removing this will completely prevent custom widgets with new properties without a significant API and ABI change in gtkmm, I think.
Comment 19 A. Walton 2014-01-30 01:25:52 UTC
I'm registering another "please revert this" on behalf of my coworkers who've already filed two different bugs against our entirely gtkmm product asking how to shut up this spew.

Can we at least change it so that release builds don't have this spew?
Comment 20 Allison Karlitskaya (desrt) 2014-01-30 11:14:22 UTC
(In reply to comment #19)
> Can we at least change it so that release builds don't have this spew?

This is really absolutely missing the point.  That output is there specifically because we want to hear about people who are having problems.

I'm starting to get the impression that this might be one that we won't be able to get away with (and may just not be worth it in the long run).  I'm thinking that a complete revert may be in order.
Comment 21 Liam White 2014-05-03 21:10:39 UTC
A few of the developers for Inkscape, which uses gtkmm, are quite annoyed, since there is no way to fix this with out "C-ifying" everything to be able to control the timing of class_init(). As it is currently, classes derived from Gtk classes cannot control the timing of the constructor initialization lists we give:

ImageToggler::ImageToggler(char const* on, char const* off) :
    Glib::ObjectBase(typeid(ImageToggler)),
    Gtk::CellRendererPixbuf(),
    _pixOnName(on),
    _pixOffName(off),
    _property_active(*this, "active", false),
    _property_activatable(*this, "activatable", true),
    _property_pixbuf_on(*this, "pixbuf_on", Glib::RefPtr<Gdk::Pixbuf>(0)),
    _property_pixbuf_off(*this, "pixbuf_off", Glib::RefPtr<Gdk::Pixbuf>(0))

and class_init() will /always/ finish by the time the properties are registered:

(inkscape:19755): GLib-GObject-WARNING **: Attempt to add property gtkmm__CustomObject_N8Inkscape2UI6Widget12ImageTogglerE::active after class was initialised

... etc.
Also note that according to Google(tm), gtkmm was not the only thing affected by this change:

http://stackoverflow.com/questions/20708657/python-gtk-how-to-suppress-warnings#comment31525797_20920122

$./helloworld.py 
./helloworld.py:52: Warning: Attempt to add property GtkSettings::gtk-button-images after class was initialised
   self.button = gtk.Button("Hello World")
./helloworld.py:52: Warning: Attempt to add property GtkSettings::gtk-label-select-on-focus after class was initialised
   self.button = gtk.Button("Hello World")

Seriously, please revert this. Or change everything else to make it work correctly.
Comment 22 Povilas Kanapickas 2014-06-06 20:17:26 UTC
Another vote for completely reverting this change. The requirement of installing all properties before class_init makes the implementation of custom properties on C++ side essentially impossible. In our proprietary applications we have several important use-cases which depend on the ability to define custom C++ properties and can't be emulated in a reasonable way. After the revelation of the possibility that the properties may be gone in the future, we're seriously considering moving away from Gtkmm (and thus GTK+) for new code. It's a pity, since I like Gtkmm quite a lot.

Some clarity about the fate of the creation of GObject properties after init would certainly help much.

Thanks!
Comment 23 Allison Karlitskaya (desrt) 2014-06-06 20:44:35 UTC
OK.
Comment 24 Allison Karlitskaya (desrt) 2014-06-06 20:44:51 UTC
Created attachment 278055 [details] [review]
Revert "GObject: prevent installing properties after init"

This reverts commit ddb0ce14215cd62c7a2497d6cf9f2ea63c40ebb5.

Conflicts:
	gobject/gobject.c
Comment 25 Allison Karlitskaya (desrt) 2014-06-06 20:44:55 UTC
Created attachment 278056 [details] [review]
docs: warn about installing properties after init

Leave ourselves a little wiggle room: if people install properties after
initialisation then we reserve the right to handle that in a way that
may not be threadsafe.
Comment 26 Allison Karlitskaya (desrt) 2014-06-06 20:45:35 UTC
Attachment 278055 [details] pushed as 85e9455 - Revert "GObject: prevent installing properties after init"
Attachment 278056 [details] pushed as f2f66bf - docs: warn about installing properties after init
Comment 27 Povilas Kanapickas 2014-06-10 21:00:45 UTC
Many thanks, Ryan.