[mutter/gnome-40] clutter/actor: First clear all stage views before emitting they changed

[Jonas Ådahl <jadahl src gnome org>]

commit 09a078f4c13c9b4fbe8f260606eceedc09f6b08b
Author: Jonas Ådahl <jadahl gmail com>
Date:   Tue Sep 28 11:26:25 2021 +0200

    clutter/actor: First clear all stage views before emitting they changed
    
    If one would end up with an actor attached to mapped actor, where the
    attached actor doesn't itself have an up to date stage view list while
    listening on the stage for updating, when clearing the stage views of
    the list, anything that would query the stage views list at this time
    would end up accessing freed memory.
    
    This could happen if
    
     1) An actor was added to a newly created container actor attached to
        the stage
     2) The actor got a timeline attached to it
     3) The actor was moved to a container that already was mapped
     4) A hotplug happened
    
    After (1) both the container and actor would not have any stage views.
    After (2) the timeline would listen on the stage for stage views
    updates. After (3) the actor would still listen on the stage for stage
    views updates. When (4) happened, the actor would be signalled when the
    stage got its stage view cleared, at which point it would traverse up
    its actor's tree finding an appropriate stage view to base its animation
    on. The problem here would be that it'd query the already mapped
    container and its yet-to-be-cleared stage view list, resulting in
    use-after free, resulting in for example the following backtrace:
    
      0)  g_type_check_instance_cast ()
      1)  CLUTTER_STAGE_VIEW ()
      2)  clutter_actor_pick_frame_clock ()
      3)  clutter_actor_pick_frame_clock ()
      4)  update_frame_clock ()
      5)  on_frame_clock_actor_stage_views_changed ()
      6)  g_closure_invoke ()
      7)  signal_emit_unlocked_R ()
      8)  g_signal_emit_valist ()
      9)  g_signal_emit ()
      10) clear_stage_views_cb ()
      11) _clutter_actor_traverse_depth ()
      12) _clutter_actor_traverse ()
      13) clutter_actor_clear_stage_views_recursive ()
      14) clutter_stage_clear_stage_views ()
      ...
    
    Avoid this issue by making sure that we don't emit 'stage-views-changed'
    signals while the actor tree is in an invalid state. While we now end up
    traversing tree twice, it doesn't change the Big-O notation. It has not
    been measured whether this has any noticible performance impact.
    
    Closes: https://gitlab.gnome.org/GNOME/mutter/-/issues/1950

 clutter/clutter/clutter-actor.c | 24 +++++++++++++++++--
 src/tests/stage-view-tests.c    | 53 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 75 insertions(+), 2 deletions(-)
---
diff --git a/clutter/clutter/clutter-actor.c b/clutter/clutter/clutter-actor.c
index 989e479a82..64cf291f8b 100644
--- a/clutter/clutter/clutter-actor.c
+++ b/clutter/clutter/clutter-actor.c
@@ -844,6 +844,7 @@ struct _ClutterActorPrivate
   guint needs_paint_volume_update   : 1;
   guint had_effects_on_last_paint_volume_update : 1;
   guint needs_update_stage_views    : 1;
+  guint clear_stage_views_needs_stage_views_changed : 1;
 };
 
 enum
@@ -15656,7 +15657,21 @@ clear_stage_views_cb (ClutterActor *actor,
   old_stage_views = g_steal_pointer (&actor->priv->stage_views);
 
   if (old_stage_views)
-    g_signal_emit (actor, actor_signals[STAGE_VIEWS_CHANGED], 0);
+    actor->priv->clear_stage_views_needs_stage_views_changed = TRUE;
+
+  return CLUTTER_ACTOR_TRAVERSE_VISIT_CONTINUE;
+}
+
+static ClutterActorTraverseVisitFlags
+maybe_emit_stage_views_changed_cb (ClutterActor *actor,
+                                   int           depth,
+                                   gpointer      user_data)
+{
+  if (actor->priv->clear_stage_views_needs_stage_views_changed)
+    {
+      actor->priv->clear_stage_views_needs_stage_views_changed = FALSE;
+      g_signal_emit (actor, actor_signals[STAGE_VIEWS_CHANGED], 0);
+    }
 
   return CLUTTER_ACTOR_TRAVERSE_VISIT_CONTINUE;
 }
@@ -15669,6 +15684,11 @@ clutter_actor_clear_stage_views_recursive (ClutterActor *self)
                            clear_stage_views_cb,
                            NULL,
                            NULL);
+  _clutter_actor_traverse (self,
+                           CLUTTER_ACTOR_TRAVERSE_DEPTH_FIRST,
+                           maybe_emit_stage_views_changed_cb,
+                           NULL,
+                           NULL);
 }
 
 float
@@ -15981,7 +16001,7 @@ clutter_actor_pick_frame_clock (ClutterActor  *self,
 
   for (l = stage_views_list; l; l = l->next)
     {
-      ClutterStageView *view = l->data;
+      ClutterStageView *view = CLUTTER_STAGE_VIEW (l->data);
       float refresh_rate;
 
       refresh_rate = clutter_stage_view_get_refresh_rate (view);
diff --git a/src/tests/stage-view-tests.c b/src/tests/stage-view-tests.c
index 70909a2b9a..344423d926 100644
--- a/src/tests/stage-view-tests.c
+++ b/src/tests/stage-view-tests.c
@@ -1157,6 +1157,57 @@ meta_test_timeline_actor_destroyed (void)
   clutter_actor_destroy (persistent_actor);
 }
 
+static void
+meta_test_timeline_actor_tree_clear (void)
+{
+  ClutterActor *stage;
+  ClutterActor *container1;
+  ClutterActor *container2;
+  g_autoptr (ClutterActor) floating = NULL;
+  g_autoptr (ClutterTimeline) timeline = NULL;
+  GList *stage_views;
+
+  stage = meta_backend_get_stage (meta_get_backend ());
+
+  ensure_view_count (1);
+
+  container1 = clutter_actor_new ();
+  clutter_actor_set_size (container1, 100, 100);
+  clutter_actor_add_child (stage, container1);
+
+  wait_for_paint (stage);
+
+  container2 = clutter_actor_new ();
+  clutter_actor_set_size (container2, 100, 100);
+  clutter_actor_add_child (stage, container2);
+
+  floating = g_object_ref_sink (clutter_actor_new ());
+  clutter_actor_set_size (floating, 100, 100);
+
+  clutter_actor_add_child (container2, floating);
+  timeline = clutter_timeline_new_for_actor (floating, 100);
+  clutter_actor_remove_child (container2, floating);
+
+  clutter_actor_add_child (container1, floating);
+
+  ensure_view_count (1);
+
+  is_on_stage_views (container1, 0);
+  is_on_stage_views (container2, 0);
+  is_on_stage_views (floating, 0);
+
+  wait_for_paint (stage);
+
+  stage_views = clutter_stage_peek_stage_views (CLUTTER_STAGE (stage));
+  is_on_stage_views (container1, 1, stage_views->data);
+  is_on_stage_views (container2, 1, stage_views->data);
+  is_on_stage_views (floating, 1, stage_views->data);
+
+  clutter_actor_destroy (floating);
+  clutter_actor_destroy (container1);
+  clutter_actor_destroy (container2);
+}
+
 static void
 init_tests (int argc, char **argv)
 {
@@ -1186,6 +1237,8 @@ init_tests (int argc, char **argv)
                    meta_test_actor_stage_views_and_frame_clocks_freed);
   g_test_add_func ("/stage-views/timeline/actor-destroyed",
                    meta_test_timeline_actor_destroyed);
+  g_test_add_func ("/stage-views/timeline/tree-clear",
+                   meta_test_timeline_actor_tree_clear);
 }
 
 int