From 0ba15df57c4211db8fd90048f40cc96ff78ad3e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonas=20Dre=C3=9Fler?= Date: Sat, 23 May 2020 10:31:29 +0200 Subject: [PATCH] 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 --- clutter/clutter/clutter-stage.c | 40 ++++++++++++--------------------- 1 file 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);