clutter/stage: Don't do newly queued relayouts after allocation cycle

While it's strongly discouraged, it is possible to queue a new relayout
of an actor in the middle of an allocation cycle, we warn about it but
don't forbid it.

With the introduction of the "shallow relayout" API, our handling of
those relayouts silently changed: Before introducing "shallow
relayouts", we'd handle them on the next stage update, but with the
priv->pending_relayouts hashtable and the
priv->pending_relayouts_version counter, we now do them immediately
during the same allocation cycle (the counter is increased by 1 when
queuing the relayout and we switch to a new GHashTableIter after
finishing the current relayout, which means we'll now do the newly
queued relayout).

This change in behavior was probably not intended and wasn't mentioned
in the commit message of 5257c6ecc2d2c843ecc1ad5558e6a471eff5c7ee, so
switch back to the old behavior, which is more robust in preventing
allocation-loops. To do this, use a GSList instead of GHashTable for the
pending_relayouts list, and simply steal that list before doing the
relayouts in _clutter_stage_maybe_relayout().

https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/1267

1 files changed, 14 insertions, 26 deletions

diff --git a/clutter/clutter/clutter-stage.c b/clutter/clutter/clutter-stage.c
index 4af445484..fd40393e2 100644
--- a/clutter/clutter/clutter-stage.c
+++ b/clutter/clutter/clutter-stage.c
@@ -119,8 +119,7 @@ struct _ClutterStagePrivate
 
   ClutterPlane current_clip_planes[4];
 
-  GHashTable *pending_relayouts;
-  unsigned int pending_relayouts_version;
+  GSList *pending_relayouts;
   GList *pending_queue_redraws;
 
   gint sync_delay;
@@ -1295,7 +1294,7 @@ _clutter_stage_needs_update (ClutterStage *stage)
 
   return (priv->redraw_pending ||
           priv->needs_update ||
-          g_hash_table_size (priv->pending_relayouts) > 0);
+          priv->pending_relayouts != NULL);
 }
 
 void
@@ -1304,11 +1303,11 @@ clutter_stage_queue_actor_relayout (ClutterStage *stage,
 {
   ClutterStagePrivate *priv = stage->priv;
 
-  if (g_hash_table_size (priv->pending_relayouts) == 0)
+  if (priv->pending_relayouts == NULL)
     clutter_stage_schedule_update (stage);
 
-  g_hash_table_add (priv->pending_relayouts, g_object_ref (actor));
-  priv->pending_relayouts_version++;
+  priv->pending_relayouts = g_slist_prepend (priv->pending_relayouts,
+                                             g_object_ref (actor));
 }
 
 void
@@ -1316,24 +1315,20 @@ _clutter_stage_maybe_relayout (ClutterActor *actor)
 {
   ClutterStage *stage = CLUTTER_STAGE (actor);
   ClutterStagePrivate *priv = stage->priv;
-  GHashTableIter iter;
-  gpointer key;
+  g_autoptr (GSList) stolen_list = NULL;
+  GSList *l;
   int count = 0;
 
   /* No work to do? Avoid the extraneous debug log messages too. */
-  if (g_hash_table_size (priv->pending_relayouts) == 0)
+  if (priv->pending_relayouts == NULL)
     return;
 
   CLUTTER_NOTE (ACTOR, ">>> Recomputing layout");
 
-  g_hash_table_iter_init (&iter, priv->pending_relayouts);
-  while (g_hash_table_iter_next (&iter, &key, NULL))
+  stolen_list = g_steal_pointer (&priv->pending_relayouts);
+  for (l = stolen_list; l; l = l->next)
     {
-      g_autoptr (ClutterActor) queued_actor = key;
-      unsigned int old_version;
-
-      g_hash_table_iter_steal (&iter);
-      priv->pending_relayouts_version++;
+      g_autoptr (ClutterActor) queued_actor = l->data;
 
       if (CLUTTER_ACTOR_IN_RELAYOUT (queued_actor))  /* avoid reentrancy */
         continue;
@@ -1351,16 +1346,11 @@ _clutter_stage_maybe_relayout (ClutterActor *actor)
 
       CLUTTER_SET_PRIVATE_FLAGS (queued_actor, CLUTTER_IN_RELAYOUT);
 
-      old_version = priv->pending_relayouts_version;
       clutter_actor_allocate_preferred_size (queued_actor);
 
       CLUTTER_UNSET_PRIVATE_FLAGS (queued_actor, CLUTTER_IN_RELAYOUT);
 
       count++;
-
-      /* Prevent using an iterator that's been invalidated */
-      if (old_version != priv->pending_relayouts_version)
-        g_hash_table_iter_init (&iter, priv->pending_relayouts);
     }
 
   CLUTTER_NOTE (ACTOR, "<<< Completed recomputing layout of %d subtrees", count);
@@ -1923,7 +1913,9 @@ clutter_stage_dispose (GObject *object)
                     (GDestroyNotify) free_queue_redraw_entry);
   priv->pending_queue_redraws = NULL;
 
-  g_clear_pointer (&priv->pending_relayouts, g_hash_table_destroy);
+  g_slist_free_full (priv->pending_relayouts,
+                     (GDestroyNotify) g_object_unref);
+  priv->pending_relayouts = NULL;
 
   /* this will release the reference on the stage */
   stage_manager = clutter_stage_manager_get_default ();
@@ -2237,10 +2229,6 @@ clutter_stage_init (ClutterStage *self)
   clutter_actor_set_background_color (CLUTTER_ACTOR (self),
                                       &default_stage_color);
 
-  priv->pending_relayouts = g_hash_table_new_full (NULL,
-                                                   NULL,
-                                                   g_object_unref,
-                                                   NULL);
   clutter_stage_queue_actor_relayout (self, CLUTTER_ACTOR (self));
 
   clutter_actor_set_reactive (CLUTTER_ACTOR (self), TRUE);