OK I guess this happens because view-type has been made a construct property in [1].
Would it be enough to override constructed() in GdMainView and manually set the adjustments to NULL before chaining up to the parent class?

[1] https://git.gnome.org/browse/libgd/commit/?id=d93382bf33ccefbd34f0140445c535c09881d5f7

Honestly, the construct parameters should not be set in an essentially random order...

I don't think constructed can affect anything, but you could have a custom constructors that reorders the parameters. Seems a bit fragile though.

Another approach is to have a constructed implementation that calls gd_main_view_rebuild() and avoid doing that during construction.

(In reply to comment #2)
> Honestly, the construct parameters should not be set in an essentially random
> order...
> 
> I don't think constructed can affect anything, but you could have a custom
> constructors that reorders the parameters. Seems a bit fragile though.

Yeah I meant constructor() in the previous comment, sorry...

> Another approach is to have a constructed implementation that calls
> gd_main_view_rebuild() and avoid doing that during construction.

This also works, as long as we ensure that a call to g_object_new (GD_TYPE_MAIN_VIEW, NULL); gets you a view which is properly initialized to the default (icon) type.

It may be that this will be fixed by: 
https://git.gnome.org/browse/glib/commit/?h=wip/gobjectnew&id=a1b0101887b4ecad90d742912ead2c81836b0fb1

Created attachment 242101 [details] [review]
GParamSpec: add g_param_spec_get_default_value()

The way of getting the default value out of a GParamSpec is to allocate
a GValue, initialise it, then call g_param_spec_set_default() to set the
default value into that GValue.

This is exactly how we handle setting the default value for all of the
construct properties that were not explicitly passed to g_object_new().

Instead of doing the alloc/init/store on all construct properties on
every call to g_object_new(), we can cache those GValue structs as qdata
on the GParamSpec itself and reuse them.

This patch does not actually make that change to g_object_new() yet, but
it adds the API to GParamSpec so that a future patch to GObject can make
the change.

Created attachment 242102 [details] [review]
GObject: substantially rework g_object_new()

Make a number of improvements to g_object_new():

 - instead of looking up the GParamSpec for the named property once in
   g_object_new() (in order to collect) and then again in g_object_newv
   (when actually setting the property), g_object_new_internal() is a
   new function that takes the GParamSpec on the interface to avoid the
   second lookup

 - in the case that ->constructor() is not set, we need not waste time
   creating an array of GObjectConstructParam to pass in.  Just directly
   iterate the list of parameters, calling set_property() on each.

 - instead of playing with linked lists to keep track of the construct
   properties, realise that the number of construct properties that we
   will set is exactly equal to the length of the construct_properties
   list on GObjectClass and the only thing that may change is where the
   value comes from (in the case that it was passed in)

   This assumption was already implicit in the existing code and can be
   seen from the sizing of the array used to hold the construct
   properties, but it wasn't taken advantage of to make things simpler.

 - instead of allocating and filling a separate array of the
   non-construct properties just re-iterate the passed-in list and set
   all properties that were not marked G_PARAM_CONSTRUCT (since the ones
   that were construct params were already used during construction)

 - use the new g_param_spec_get_default_value() API instead of
   allocating and setting the GValue for each construct property that
   wasn't passed from the user

These changes show a very small improvement on the simple-construction
performance testcase (probably just noise) and they improve the
complex-construction case by ~30%.

Created attachment 242103 [details] [review]
paramspec changes

Created attachment 242104 [details] [review]
params

Review of attachment 242101 [details] [review]:

::: gobject/gparam.c
@@ +1536,3 @@
+
+  if (g_once_init_enter (&default_value_quark))
+    g_once_init_leave (&default_value_quark, g_quark_from_static_string ("GParamSpec default value qdata"));

It just feels wrong to do something core like default values using qdata. Can we please make this using g_type_class_add_private.

Review of attachment 242102 [details] [review]:

This looks ok to me, although we also want to get rid of the g_param_spec_pool_lookup calls, because that shit is insane.

Created attachment 242228 [details] [review]
Fix construct_only properties

Created attachment 242229 [details] [review]
gobject: Fix constructor order

We append rather than prepend to the constructor list, which means
that properties of base classes are set before that of child classes.

Comment on attachment 242102 [details] [review]
GObject: substantially rework g_object_new()

> - in the case that ->constructor() is not set

->constructor is always set; GObject itself sets it.

Comment on attachment 242102 [details] [review]
GObject: substantially rework g_object_new()

oh, i see. you mean "in the case that the class does not override ->constructor"

Created attachment 242236 [details] [review]
GParamSpec: add g_param_spec_get_default_value()

The way of getting the default value out of a GParamSpec is to allocate
a GValue, initialise it, then call g_param_spec_set_default() to set the
default value into that GValue.

This is exactly how we handle setting the default value for all of the
construct properties that were not explicitly passed to g_object_new().

Instead of doing the alloc/init/store on all construct properties on
every call to g_object_new(), we can cache those GValues in the private
data of the GParamSpec itself and reuse them.

This patch does not actually make that change to g_object_new() yet, but
it adds the API to GParamSpec so that a future patch to GObject can make
the change.

Created attachment 242237 [details] [review]
GType: add accessor for instance private offset

Since instance private data is now always at a constant offset to the
instance pointer, we can add an accessor for it that doesn't also
require an instance.

The idea is that classes can call this from their class_init and store
it in a file-scoped static variable and use that to find their private
data on instances very quickly, without a priv pointer.

Created attachment 242256 [details] [review]
GType: add accessor for instance private offset

Since instance private data is now always at a constant offset to the
instance pointer, we can add an accessor for it that doesn't also
require an instance.

The idea is that classes can call this from their class_init and store
it in a file-scoped static variable and use that to find their private
data on instances very quickly, without a priv pointer.

Created attachment 242257 [details] [review]
GParamSpec: add g_param_spec_get_default_value()

The way of getting the default value out of a GParamSpec is to allocate
a GValue, initialise it, then call g_param_spec_set_default() to set the
default value into that GValue.

This is exactly how we handle setting the default value for all of the
construct properties that were not explicitly passed to g_object_new().

Instead of doing the alloc/init/store on all construct properties on
every call to g_object_new(), we can cache those GValues in the private
data of the GParamSpec itself and reuse them.

This patch does not actually make that change to g_object_new() yet, but
it adds the API to GParamSpec so that a future patch to GObject can make
the change.

Created attachment 242258 [details] [review]
GObject: substantially rework g_object_new()

Make a number of improvements to g_object_new():

 - instead of looking up the GParamSpec for the named property once in
   g_object_new() (in order to collect) and then again in g_object_newv
   (when actually setting the property), g_object_new_internal() is a
   new function that takes the GParamSpec on the interface to avoid the
   second lookup

 - in the case that ->constructor() is not set, we need not waste time
   creating an array of GObjectConstructParam to pass in.  Just directly
   iterate the list of parameters, calling set_property() on each.

 - instead of playing with linked lists to keep track of the construct
   properties, realise that the number of construct properties that we
   will set is exactly equal to the length of the construct_properties
   list on GObjectClass and the only thing that may change is where the
   value comes from (in the case that it was passed in)

   This assumption was already implicit in the existing code and can be
   seen from the sizing of the array used to hold the construct
   properties, but it wasn't taken advantage of to make things simpler.

 - instead of allocating and filling a separate array of the
   non-construct properties just re-iterate the passed-in list and set
   all properties that were not marked G_PARAM_CONSTRUCT (since the ones
   that were construct params were already used during construction)

 - use the new g_param_spec_get_default_value() API instead of
   allocating and setting the GValue for each construct property that
   wasn't passed from the user

Because we are now iterating the linked list of properties in-order we
need to append to that list during class initialising instead of
prepending.

These changes show a very small improvement on the simple-construction
performance testcase (probably just noise) and they improve the
complex-construction case by ~30%.

Thanks to Alex Larsson for reviews and fixes.

Review of attachment 242256 [details] [review]:

ack

Review of attachment 242257 [details] [review]:

ack

Review of attachment 242258 [details] [review]:

looks good to me

if it already hits breakage just within glib, it seems pretty much doomed in gnome as a whole... can't you just tweak the code to set the properties in the same order the old code did?

This bug was opened because the current order is ridiculous: you can set properties of subclasses before the construct properties on the parent just by changing the order of the arguments passed to g_object_new()....

Not only is it ridiculous, but it actually broke libgd, as it set the GdMainView construct properties before the construct properties in the parent class (GtkScrolledWindow) which causes it to error out due to non-created GtkAdjustments. So, this is really a necessary fix for sane object construction.

I just kicked off a smoketest with the three patches in this bug and it passed.  I agree that there is a high chance that this will break something else, but we will never find out what it is until we merge this anyway, so let's do that and try to fix issues as they come up.

Attachment 242256 [details] pushed as c30c0bb - GType: add accessor for instance private offset
Attachment 242257 [details] pushed as c18462b - GParamSpec: add g_param_spec_get_default_value()
Attachment 242258 [details] pushed as bfa8bef - GObject: substantially rework g_object_new()

These changes cause to gnome-shell does not rise.

(In reply to comment #28)
> These changes cause to gnome-shell does not rise.

Can you be more specific about the problem?  Do you have an error message or log file?  How are you sure that it's this change that caused the problem?

Before the changes:
Sign into gnome-shell through the GDM works.

After the changes:
Sign into gnome-shell through the GDM doesn't works. GDM restarts as soon as I try to sing into gnome-shell.

Using startx gives the same result.

We smoketested against the shell to ensure that this change would not cause this problem.  What is your setup?  Are you on jhbuild?  Did you try bisecting glib?  A log really would be useful...

I am using git master.
OS: Arch Linux

Without JHbuild.

The commit fixed the bug:
https://git.gnome.org/browse/gnome-session/commit/?id=6e14faf9138dbbffd1e63e177e53d88963c379e5

(In reply to comment #34)
> The commit fixed the bug:
> https://git.gnome.org/browse/gnome-session/commit/?id=6e14faf9138dbbffd1e63e177e53d88963c379e5

> GsmAutostartApp: Don't return NULL from constructor, use GInitable
> 
> Recent GObject changes break this; it was never a supported operation.
> GInitable is the way to return errors to callers.

Although it was always *supposed* to be illegal to return NULL from constructor(), it worked as long as you didn't pass any non-CONSTRUCT properties to g_object_new(). Unfortunately, there are places where people accidentally depended on this behavior in public APIs. (libsoup and libnm-glib both do this.) We can add GInitable support there. (I already have in some places.) But getting rid of the old constructor-returns-NULL-on-error behavior is essentially an ABI break.

I don't see anything obvious in the new code that would make it crash in this case, but it's probably a simple matter of a missing "if (newly_constructed)". (And if there is a missing "if (newly_constructed)" then that means the code is doing the wrong thing in the singleton case too.)

It's worth noting that returning NULL from your constructor after having chained up is a good way to leak memory.  That's because g_object_init:

  if (CLASS_HAS_CUSTOM_CONSTRUCTOR (class))
    {
      /* enter construction list for notify_queue_thaw() and to allow construct-only properties */
      G_LOCK (construction_mutex);
      construction_objects = g_slist_prepend (construction_objects, object);
      G_UNLOCK (construction_mutex);
    }


and then later, back in g_object_newv()

  if (CLASS_HAS_CUSTOM_CONSTRUCTOR (class))
    {
      /* adjust freeze_count according to g_object_init() and remaining properties */
      G_LOCK (construction_mutex);
      newly_constructed = slist_maybe_remove (&construction_objects, object);
      G_UNLOCK (construction_mutex);


but of course, the object can't get removed from the list if you passed NULL back...



In any case, I can see making this code return-NULL-safe and throw a critical instead of crashing.