Merge branch 'freeze-shenanigans' into 'main'

Fix problems with notify-in-instance-init and add unit tests

Closes #2665

See merge request GNOME/glib!2737

gobject/gobject.c

@@ -1176,26 +1176,53 @@ object_get_optional_flags (GObject *object)
 #endif
 }
 
+/* Variant of object_get_optional_flags for when
+ * we know that we have exclusive access (during
+ * construction)
+ */
+static inline guint
+object_get_optional_flags_X (GObject *object)
+{
+#ifdef HAVE_OPTIONAL_FLAGS
+  GObjectReal *real = (GObjectReal *)object;
+  return real->optional_flags;
+#else
+  return 0;
+#endif
+}
+
+#ifdef HAVE_OPTIONAL_FLAGS
 static inline void
 object_set_optional_flags (GObject *object,
                           guint flags)
 {
-#ifdef HAVE_OPTIONAL_FLAGS
   GObjectReal *real = (GObjectReal *)object;
   g_atomic_int_or (&real->optional_flags, flags);
-#endif
 }
 
+/* Variant for when we have exclusive access
+ * (during construction)
+ */
 static inline void
-object_unset_optional_flags (GObject *object,
+object_set_optional_flags_X (GObject *object,
                              guint flags)
 {
-#ifdef HAVE_OPTIONAL_FLAGS
   GObjectReal *real = (GObjectReal *)object;
-  g_atomic_int_and (&real->optional_flags, ~flags);
+  real->optional_flags |= flags;
-#endif
 }
 
+/* Variant for when we have exclusive access
+ * (during construction)
+ */
+static inline void
+object_unset_optional_flags_X (GObject *object,
+                               guint flags)
+{
+  GObjectReal *real = (GObjectReal *)object;
+  real->optional_flags &= ~flags;
+}
+#endif
+
 gboolean
 _g_object_has_signal_handler (GObject *object)
 {
@@ -1217,6 +1244,17 @@ _g_object_has_notify_handler (GObject *object)
 #endif
 }
 
+static inline gboolean
+_g_object_has_notify_handler_X (GObject *object)
+{
+#ifdef HAVE_OPTIONAL_FLAGS
+  return CLASS_HAS_NOTIFY (G_OBJECT_GET_CLASS (object)) ||
+         (object_get_optional_flags_X (object) & OPTIONAL_FLAG_HAS_NOTIFY_HANDLER) != 0;
+#else
+  return TRUE;
+#endif
+}
+
 void
 _g_object_set_has_signal_handler (GObject *object,
                                   guint    signal_id)
@@ -1243,7 +1281,7 @@ static inline void
 set_object_in_construction (GObject *object)
 {
 #ifdef HAVE_OPTIONAL_FLAGS
-  object_set_optional_flags (object, OPTIONAL_FLAG_IN_CONSTRUCTION);
+  object_set_optional_flags_X (object, OPTIONAL_FLAG_IN_CONSTRUCTION);
 #else
   g_datalist_id_set_data (&object->qdata, quark_in_construction, object);
 #endif
@@ -1253,7 +1291,7 @@ static inline void
 unset_object_in_construction (GObject *object)
 {
 #ifdef HAVE_OPTIONAL_FLAGS
-  object_unset_optional_flags (object, OPTIONAL_FLAG_IN_CONSTRUCTION);
+  object_unset_optional_flags_X (object, OPTIONAL_FLAG_IN_CONSTRUCTION);
 #else
   g_datalist_id_set_data (&object->qdata, quark_in_construction, NULL);
 #endif
@@ -1272,11 +1310,8 @@ g_object_init (GObject		*object,
       g_object_notify_queue_freeze (object, FALSE);
     }
 
-  if (CLASS_HAS_CUSTOM_CONSTRUCTOR (class))
-    {
   /* mark object in-construction for notify_queue_thaw() and to allow construct-only properties */
   set_object_in_construction (object);
-    }
 
   GOBJECT_IF_DEBUG (OBJECTS,
     {
@@ -1444,23 +1479,50 @@ static inline void
 g_object_notify_by_spec_internal (GObject    *object,
                                   GParamSpec *pspec)
 {
+#ifdef HAVE_OPTIONAL_FLAGS
+  guint object_flags;
+#endif
+  gboolean has_notify;
+  gboolean in_init;
+
   if (G_UNLIKELY (~pspec->flags & G_PARAM_READABLE))
     return;
 
   param_spec_follow_override (&pspec);
 
-  if (pspec != NULL &&
+#ifdef HAVE_OPTIONAL_FLAGS
-      _g_object_has_notify_handler (object))
+  /* get all flags we need with a single atomic read */
+  object_flags = object_get_optional_flags (object);
+  has_notify = ((object_flags & OPTIONAL_FLAG_HAS_NOTIFY_HANDLER) != 0) ||
+               CLASS_HAS_NOTIFY (G_OBJECT_GET_CLASS (object));
+  in_init = (object_flags & OPTIONAL_FLAG_IN_CONSTRUCTION) != 0;
+#else
+  has_notify = TRUE;
+  in_init = object_in_construction (object);
+#endif
+
+  if (pspec != NULL && has_notify)
     {
       GObjectNotifyQueue *nqueue;
+      gboolean need_thaw = TRUE;
 
       /* conditional freeze: only increase freeze count if already frozen */
       nqueue = g_object_notify_queue_freeze (object, TRUE);
+      if (in_init && !nqueue)
+        {
+          /* We did not freeze the queue in g_object_init, but
+           * we gained a notify handler in instance init, so
+           * now we need to freeze just-in-time
+           */
+          nqueue = g_object_notify_queue_freeze (object, FALSE);
+          need_thaw = FALSE;
+        }
 
       if (nqueue != NULL)
         {
           /* we're frozen, so add to the queue and release our freeze */
           g_object_notify_queue_add (object, nqueue, pspec);
+          if (need_thaw)
             g_object_notify_queue_thaw (object, nqueue);
         }
       else
@@ -2108,28 +2170,16 @@ g_object_new_with_custom_constructor (GObjectClass          *class,
 
   if (CLASS_HAS_PROPS (class))
     {
-      /* If this object was newly_constructed then g_object_init()
+      if ((newly_constructed && _g_object_has_notify_handler_X (object)) ||
-       * froze the queue.  We need to freeze it here in order to get
+          _g_object_has_notify_handler (object))
-       * the handle so that we can thaw it below (otherwise it will
+        {
-       * be frozen forever).
+          /* This may or may not have been setup in g_object_init().
-       *
+           * If it hasn't, we do it now.
-       * We also want to do a freeze if we have any params to set,
-       * even on a non-newly_constructed object.
-       *
-       * It's possible that we have the case of non-newly created
-       * singleton and all of the passed-in params were construct
-       * properties so n_params > 0 but we will actually set no
-       * properties.  This is a pretty lame case to optimise, so
-       * just ignore it and freeze anyway.
            */
-      if (newly_constructed || n_params)
+          nqueue = g_datalist_id_get_data (&object->qdata, quark_notify_queue);
+          if (!nqueue)
             nqueue = g_object_notify_queue_freeze (object, FALSE);
-
+        }
-      /* Remember: if it was newly_constructed then g_object_init()
-       * already did a freeze, so we now have two.  Release one.
-       */
-      if (newly_constructed && CLASS_HAS_NOTIFY (class))
-        g_object_notify_queue_thaw (object, nqueue);
     }
 
   /* run 'constructed' handler if there is a custom one */
@@ -2164,15 +2214,20 @@ g_object_new_internal (GObjectClass          *class,
 
   g_assert (g_object_is_aligned (object));
 
+  unset_object_in_construction (object);
+
   if (CLASS_HAS_PROPS (class))
     {
       GSList *node;
 
-      if (CLASS_HAS_NOTIFY (class))
+      if (_g_object_has_notify_handler_X (object))
         {
-          /* This will have been setup in g_object_init() */
+          /* This may or may not have been setup in g_object_init().
+           * If it hasn't, we do it now.
+           */
           nqueue = g_datalist_id_get_data (&object->qdata, quark_notify_queue);
-          g_assert (nqueue != NULL);
+          if (!nqueue)
+            nqueue = g_object_notify_queue_freeze (object, FALSE);
         }
 
       /* We will set exactly n_construct_properties construct

gobject/tests/meson.build

@@ -29,6 +29,8 @@ marshalers_c = custom_target('marshalers_c',
 )
 
 gobject_tests = {
+  'notify-init' : {},
+  'notify-init2' : {},
   'qdata' : {},
   'accumulator' : {
     'source' : ['accumulator.c', marshalers_h, marshalers_c],

gobject/tests/notify-init.c

@@ -0,0 +1,268 @@
+/* GLib testing framework examples and tests
+ * Copyright (C) 2022 Red Hat, Inc.
+ *
+ * This work is provided "as is"; redistribution and modification
+ * in whole or in part, in any medium, physical or electronic is
+ * permitted without restriction.
+ *
+ * This work is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
+ *
+ * In no event shall the authors or contributors be liable for any
+ * direct, indirect, incidental, special, exemplary, or consequential
+ * damages (including, but not limited to, procurement of substitute
+ * goods or services; loss of use, data, or profits; or business
+ * interruption) however caused and on any theory of liability, whether
+ * in contract, strict liability, or tort (including negligence or
+ * otherwise) arising in any way out of the use of this software, even
+ * if advised of the possibility of such damage.
+ */
+
+#include <stdlib.h>
+#include <gstdio.h>
+#include <glib-object.h>
+
+typedef struct {
+  GObject parent_instance;
+  gint foo;
+  gboolean bar;
+  gchar *baz;
+  gchar *quux;
+} TestObject;
+
+typedef struct {
+  GObjectClass parent_class;
+} TestObjectClass;
+
+typedef enum {
+  PROP_FOO = 1,
+  PROP_BAR,
+  PROP_BAZ,
+  PROP_QUUX,
+  N_PROPERTIES
+} TestObjectProperty;
+
+static GParamSpec *properties[N_PROPERTIES] = { NULL, };
+
+static GType test_object_get_type (void);
+G_DEFINE_TYPE (TestObject, test_object, G_TYPE_OBJECT)
+
+static void
+test_object_set_foo (TestObject *obj,
+                     gint        foo)
+{
+  if (obj->foo != foo)
+    {
+      obj->foo = foo;
+
+      g_assert (properties[PROP_FOO] != NULL);
+      g_object_notify_by_pspec (G_OBJECT (obj), properties[PROP_FOO]);
+    }
+}
+
+static void
+test_object_set_bar (TestObject *obj,
+                     gboolean    bar)
+{
+  bar = !!bar;
+
+  if (obj->bar != bar)
+    {
+      obj->bar = bar;
+
+      g_assert (properties[PROP_BAR] != NULL);
+      g_object_notify_by_pspec (G_OBJECT (obj), properties[PROP_BAR]);
+    }
+}
+
+static void
+test_object_set_baz (TestObject  *obj,
+                     const gchar *baz)
+{
+  if (g_strcmp0 (obj->baz, baz) != 0)
+    {
+      g_free (obj->baz);
+      obj->baz = g_strdup (baz);
+
+      g_assert (properties[PROP_BAZ] != NULL);
+      g_object_notify_by_pspec (G_OBJECT (obj), properties[PROP_BAZ]);
+    }
+}
+
+static void
+test_object_set_quux (TestObject  *obj,
+                      const gchar *quux)
+{
+  if (g_strcmp0 (obj->quux, quux) != 0)
+    {
+      g_free (obj->quux);
+      obj->quux = g_strdup (quux);
+
+      g_assert (properties[PROP_QUUX] != NULL);
+      g_object_notify_by_pspec (G_OBJECT (obj), properties[PROP_QUUX]);
+    }
+}
+
+static void
+test_object_finalize (GObject *gobject)
+{
+  TestObject *self = (TestObject *) gobject;
+
+  g_free (self->baz);
+  g_free (self->quux);
+
+  G_OBJECT_CLASS (test_object_parent_class)->finalize (gobject);
+}
+
+static void
+test_object_set_property (GObject *gobject,
+                          guint prop_id,
+                          const GValue *value,
+                          GParamSpec *pspec)
+{
+  TestObject *tobj = (TestObject *) gobject;
+
+  g_assert_cmpint (prop_id, !=, 0);
+  g_assert_cmpint (prop_id, !=, N_PROPERTIES);
+  g_assert (pspec == properties[prop_id]);
+
+  switch ((TestObjectProperty)prop_id)
+    {
+    case PROP_FOO:
+      test_object_set_foo (tobj, g_value_get_int (value));
+      break;
+
+    case PROP_BAR:
+      test_object_set_bar (tobj, g_value_get_boolean (value));
+      break;
+
+    case PROP_BAZ:
+      test_object_set_baz (tobj, g_value_get_string (value));
+      break;
+
+    case PROP_QUUX:
+      test_object_set_quux (tobj, g_value_get_string (value));
+      break;
+
+    default:
+      g_assert_not_reached ();
+    }
+}
+
+static void
+test_object_get_property (GObject *gobject,
+                          guint prop_id,
+                          GValue *value,
+                          GParamSpec *pspec)
+{
+  TestObject *tobj = (TestObject *) gobject;
+
+  g_assert_cmpint (prop_id, !=, 0);
+  g_assert_cmpint (prop_id, !=, N_PROPERTIES);
+  g_assert (pspec == properties[prop_id]);
+
+  switch ((TestObjectProperty)prop_id)
+    {
+    case PROP_FOO:
+      g_value_set_int (value, tobj->foo);
+      break;
+
+    case PROP_BAR:
+      g_value_set_boolean (value, tobj->bar);
+      break;
+
+    case PROP_BAZ:
+      g_value_set_string (value, tobj->baz);
+      break;
+
+    case PROP_QUUX:
+      g_value_set_string (value, tobj->quux);
+      break;
+
+    default:
+      g_assert_not_reached ();
+    }
+}
+
+static void
+test_object_class_init (TestObjectClass *klass)
+{
+  GObjectClass *gobject_class = G_OBJECT_CLASS (klass);
+
+  properties[PROP_FOO] = g_param_spec_int ("foo", "Foo", "Foo",
+                                           -1, G_MAXINT,
+                                           0,
+                                           G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS);
+  properties[PROP_BAR] = g_param_spec_boolean ("bar", "Bar", "Bar",
+                                               FALSE,
+                                               G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS);
+  properties[PROP_BAZ] = g_param_spec_string ("baz", "Baz", "Baz",
+                                              NULL,
+                                              G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS);
+
+  properties[PROP_QUUX] = g_param_spec_string ("quux", "quux", "quux",
+                                               NULL,
+                                               G_PARAM_READWRITE | G_PARAM_EXPLICIT_NOTIFY | G_PARAM_STATIC_STRINGS);
+
+  gobject_class->set_property = test_object_set_property;
+  gobject_class->get_property = test_object_get_property;
+  gobject_class->finalize = test_object_finalize;
+
+  g_object_class_install_properties (gobject_class, N_PROPERTIES, properties);
+}
+
+static void
+quux_changed (TestObject *self,
+              GParamSpec *pspec,
+              gpointer    data)
+{
+  g_assert (self->baz != NULL);
+}
+
+static void
+test_object_init (TestObject *self)
+{
+  /* This instance init behavior is the thing we are testing:
+   *
+   * 1. Connect to notify::quux
+   * 2. Change the the quux property
+   * 3. Continue to set up things that the quux_changed handler
+   *   relies on
+   *
+   * The expected behavior is that:
+   *
+   * - The quux_changed handler *is* called
+   * - It is only called after the object is fully constructed
+   */
+  g_signal_connect (self, "notify::quux", G_CALLBACK (quux_changed), NULL);
+
+  test_object_set_quux (self, "quux");
+
+  self->foo = 42;
+  self->bar = TRUE;
+  self->baz = g_strdup ("Hello");
+}
+
+static void
+test_notify_in_init (void)
+{
+  TestObject *obj;
+
+  g_test_summary ("Test that emitting notify with a handler already connected in test_object_init() works");
+  g_test_bug ("https://gitlab.gnome.org/GNOME/glib/-/issues/2665");
+
+  obj = g_object_new (test_object_get_type (), NULL);
+
+  g_object_unref (obj);
+}
+
+int
+main (int argc, char *argv[])
+{
+  g_test_init (&argc, &argv, NULL);
+
+  g_test_add_func ("/properties/notify-in-init", test_notify_in_init);
+
+  return g_test_run ();
+}

gobject/tests/notify-init2.c

@@ -0,0 +1,253 @@
+/* GLib testing framework examples and tests
+ * Copyright (C) 2022 Red Hat, Inc.
+ *
+ * This work is provided "as is"; redistribution and modification
+ * in whole or in part, in any medium, physical or electronic is
+ * permitted without restriction.
+ *
+ * This work is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
+ *
+ * In no event shall the authors or contributors be liable for any
+ * direct, indirect, incidental, special, exemplary, or consequential
+ * damages (including, but not limited to, procurement of substitute
+ * goods or services; loss of use, data, or profits; or business
+ * interruption) however caused and on any theory of liability, whether
+ * in contract, strict liability, or tort (including negligence or
+ * otherwise) arising in any way out of the use of this software, even
+ * if advised of the possibility of such damage.
+ */
+
+#include <stdlib.h>
+#include <gstdio.h>
+#include <glib-object.h>
+
+typedef struct {
+  GObject parent_instance;
+  gint foo;
+  gboolean bar;
+  gchar *baz;
+  gchar *quux;
+} TestObject;
+
+typedef struct {
+  GObjectClass parent_class;
+} TestObjectClass;
+
+typedef enum {
+  PROP_FOO = 1,
+  PROP_BAR,
+  PROP_BAZ,
+  PROP_QUUX,
+  N_PROPERTIES
+} TestObjectProperty;
+
+static GParamSpec *properties[N_PROPERTIES] = { NULL, };
+
+static GType test_object_get_type (void);
+G_DEFINE_TYPE (TestObject, test_object, G_TYPE_OBJECT)
+
+static void
+test_object_set_foo (TestObject *obj,
+                     gint        foo)
+{
+  if (obj->foo != foo)
+    {
+      obj->foo = foo;
+
+      g_assert (properties[PROP_FOO] != NULL);
+      g_object_notify_by_pspec (G_OBJECT (obj), properties[PROP_FOO]);
+    }
+}
+
+static void
+test_object_set_bar (TestObject *obj,
+                     gboolean    bar)
+{
+  bar = !!bar;
+
+  if (obj->bar != bar)
+    {
+      obj->bar = bar;
+
+      g_assert (properties[PROP_BAR] != NULL);
+      g_object_notify_by_pspec (G_OBJECT (obj), properties[PROP_BAR]);
+    }
+}
+
+static void
+test_object_set_baz (TestObject  *obj,
+                     const gchar *baz)
+{
+  if (g_strcmp0 (obj->baz, baz) != 0)
+    {
+      g_free (obj->baz);
+      obj->baz = g_strdup (baz);
+
+      g_assert (properties[PROP_BAZ] != NULL);
+      g_object_notify_by_pspec (G_OBJECT (obj), properties[PROP_BAZ]);
+    }
+}
+
+static void
+test_object_set_quux (TestObject  *obj,
+                      const gchar *quux)
+{
+  if (g_strcmp0 (obj->quux, quux) != 0)
+    {
+      g_free (obj->quux);
+      obj->quux = g_strdup (quux);
+
+      g_assert (properties[PROP_QUUX] != NULL);
+      g_object_notify_by_pspec (G_OBJECT (obj), properties[PROP_QUUX]);
+    }
+}
+
+static void
+test_object_finalize (GObject *gobject)
+{
+  TestObject *self = (TestObject *) gobject;
+
+  g_free (self->baz);
+  g_free (self->quux);
+
+  G_OBJECT_CLASS (test_object_parent_class)->finalize (gobject);
+}
+
+static GObject *
+test_object_constructor (GType                  type,
+                         guint                  n_construct_properties,
+                         GObjectConstructParam *construct_properties)
+{
+  return G_OBJECT_CLASS (test_object_parent_class)->constructor (type, n_construct_properties, construct_properties);
+}
+
+static void
+test_object_set_property (GObject *gobject,
+                          guint prop_id,
+                          const GValue *value,
+                          GParamSpec *pspec)
+{
+  TestObject *tobj = (TestObject *) gobject;
+
+  g_assert_cmpint (prop_id, !=, 0);
+  g_assert_cmpint (prop_id, !=, N_PROPERTIES);
+  g_assert (pspec == properties[prop_id]);
+
+  switch ((TestObjectProperty)prop_id)
+    {
+    case PROP_FOO:
+      test_object_set_foo (tobj, g_value_get_int (value));
+      break;
+
+    case PROP_BAR:
+      test_object_set_bar (tobj, g_value_get_boolean (value));
+      break;
+
+    case PROP_BAZ:
+      test_object_set_baz (tobj, g_value_get_string (value));
+      break;
+
+    case PROP_QUUX:
+      test_object_set_quux (tobj, g_value_get_string (value));
+      break;
+
+    default:
+      g_assert_not_reached ();
+    }
+}
+
+static void
+test_object_get_property (GObject *gobject,
+                          guint prop_id,
+                          GValue *value,
+                          GParamSpec *pspec)
+{
+  TestObject *tobj = (TestObject *) gobject;
+
+  g_assert_cmpint (prop_id, !=, 0);
+  g_assert_cmpint (prop_id, !=, N_PROPERTIES);
+  g_assert (pspec == properties[prop_id]);
+
+  switch ((TestObjectProperty)prop_id)
+    {
+    case PROP_FOO:
+      g_value_set_int (value, tobj->foo);
+      break;
+
+    case PROP_BAR:
+      g_value_set_boolean (value, tobj->bar);
+      break;
+
+    case PROP_BAZ:
+      g_value_set_string (value, tobj->baz);
+      break;
+
+    case PROP_QUUX:
+      g_value_set_string (value, tobj->quux);
+      break;
+
+    default:
+      g_assert_not_reached ();
+    }
+}
+
+static void
+test_object_class_init (TestObjectClass *klass)
+{
+  GObjectClass *gobject_class = G_OBJECT_CLASS (klass);
+
+  properties[PROP_FOO] = g_param_spec_int ("foo", "Foo", "Foo",
+                                           -1, G_MAXINT,
+                                           0,
+                                           G_PARAM_READWRITE | G_PARAM_CONSTRUCT | G_PARAM_STATIC_STRINGS);
+  properties[PROP_BAR] = g_param_spec_boolean ("bar", "Bar", "Bar",
+                                               FALSE,
+                                               G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS);
+  properties[PROP_BAZ] = g_param_spec_string ("baz", "Baz", "Baz",
+                                              NULL,
+                                              G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS);
+
+  properties[PROP_QUUX] = g_param_spec_string ("quux", "quux", "quux",
+                                               NULL,
+                                               G_PARAM_READWRITE | G_PARAM_EXPLICIT_NOTIFY | G_PARAM_STATIC_STRINGS);
+
+  gobject_class->constructor = test_object_constructor;
+  gobject_class->set_property = test_object_set_property;
+  gobject_class->get_property = test_object_get_property;
+  gobject_class->finalize = test_object_finalize;
+
+  g_object_class_install_properties (gobject_class, N_PROPERTIES, properties);
+}
+
+static void
+test_object_init (TestObject *self)
+{
+  self->foo = 42;
+  self->bar = TRUE;
+  self->baz = g_strdup ("Hello");
+}
+
+static void
+test_notify_in_init (void)
+{
+  TestObject *obj;
+
+  g_test_summary ("Test that notify freezing during construction of objects with custom constructor works");
+  g_test_bug ("https://gitlab.gnome.org/GNOME/glib/-/issues/2665");
+
+  obj = g_object_new (test_object_get_type (), "bar", FALSE, NULL);
+
+  g_object_unref (obj);
+}
+
+int
+main (int argc, char *argv[])
+{
+  g_test_init (&argc, &argv, NULL);
+
+  g_test_add_func ("/properties/notify-in-init2", test_notify_in_init);
+
+  return g_test_run ();
+}