From 21eb49098a7e90007b9043917a8dcf816db765b7 Mon Sep 17 00:00:00 2001 From: Emmanuele Bassi Date: Mon, 13 Sep 2010 23:29:52 +0300 Subject: [PATCH] script: Fix the memory management Currently, the memory management in ClutterScript is overly complicated. The basic design tenet should be: - ClutterScript owns a reference on every object it creates This allows the Script instance to reliably handle the lifetime of the instances from creation to disposal. In case of unmerge, the Script instance should destroy any Actor instance, except for the Stage, and release the reference it owns. The Stage is special because it's really owned by Clutter itself, and it should be destroyed explicitly. When disposing the Script itself, it should just release the reference; any parented actor, or any InitiallyUnowned instance, will then be managed by the parent object, as they should, while every GObject instance will go away, as documented. This commit is based on a patch by: Henrik Hedberg http://bugzilla.clutter-project.org/show_bug.cgi?id=2316 --- clutter/clutter-script-parser.c | 30 ++++++++++++++++++++++-------- clutter/clutter-script-private.h | 3 ++- clutter/clutter-script.c | 20 +++++--------------- 3 files changed, 29 insertions(+), 24 deletions(-) diff --git a/clutter/clutter-script-parser.c b/clutter/clutter-script-parser.c index 8fbcb0f40..959d25a75 100644 --- a/clutter/clutter-script-parser.c +++ b/clutter/clutter-script-parser.c @@ -686,8 +686,10 @@ construct_timeline (ClutterScript *script, _clutter_script_apply_properties (script, oinfo); retval = CLUTTER_TIMELINE (oinfo->object); - /* we transfer ownership to the alpha function later */ - oinfo->is_toplevel = FALSE; + /* we transfer ownership to the alpha function, so we ref before + * destroying the ObjectInfo to avoid the timeline going away + */ + g_object_ref (retval); object_info_free (oinfo); return retval; @@ -864,6 +866,10 @@ _clutter_script_parse_alpha (ClutterScript *script, clutter_alpha_set_func (CLUTTER_ALPHA (retval), alpha_func, NULL, NULL); clutter_alpha_set_timeline (CLUTTER_ALPHA (retval), timeline); + + /* if we created an implicit timeline, the Alpha has full ownership + * of it now, since it won't be accessible from ClutterScript + */ if (unref_timeline) g_object_unref (timeline); @@ -961,9 +967,13 @@ clutter_script_parser_object_end (JsonParser *json_parser, json_object_remove_member (object, "signals"); } + oinfo->is_actor = FALSE; + if (strcmp (oinfo->class_name, "ClutterStage") == 0 && json_object_has_member (object, "is-default")) { + oinfo->is_actor = TRUE; + oinfo->is_stage = TRUE; oinfo->is_stage_default = json_object_get_boolean_member (object, "is-default"); @@ -972,7 +982,6 @@ clutter_script_parser_object_end (JsonParser *json_parser, else oinfo->is_stage_default = FALSE; - oinfo->is_toplevel = FALSE; oinfo->is_unmerged = FALSE; oinfo->has_unresolved = TRUE; @@ -1899,13 +1908,12 @@ _clutter_script_construct_object (ClutterScript *script, if (G_UNLIKELY (oinfo->gtype == G_TYPE_INVALID)) return; - oinfo->is_toplevel = - g_type_is_a (oinfo->gtype, G_TYPE_INITIALLY_UNOWNED) - ? FALSE - : TRUE; + oinfo->is_actor = g_type_is_a (oinfo->gtype, CLUTTER_TYPE_ACTOR); + if (oinfo->is_actor) + oinfo->is_stage = g_type_is_a (oinfo->gtype, CLUTTER_TYPE_STAGE); } - if (oinfo->gtype == CLUTTER_TYPE_STAGE && oinfo->is_stage_default) + if (oinfo->is_stage && oinfo->is_stage_default) { GList *properties = oinfo->properties; @@ -1950,6 +1958,12 @@ _clutter_script_construct_object (ClutterScript *script, params->len, (GParameter *) params->data); + /* by sinking the floating reference, we make sure that the reference + * count is correct whether the object is referenced from somewhere + * else too or only by this ClutterScript object. + */ + g_object_ref_sink (oinfo->object); + for (i = 0; i < params->len; i++) { GParameter *param = &g_array_index (params, GParameter, i); diff --git a/clutter/clutter-script-private.h b/clutter/clutter-script-private.h index b86a000e1..87249bcd7 100644 --- a/clutter/clutter-script-private.h +++ b/clutter/clutter-script-private.h @@ -66,8 +66,9 @@ typedef struct { guint merge_id; + guint is_actor : 1; + guint is_stage : 1; guint is_stage_default : 1; - guint is_toplevel : 1; guint has_unresolved : 1; guint is_unmerged : 1; } ObjectInfo; diff --git a/clutter/clutter-script.c b/clutter/clutter-script.c index 84c4e35d2..dc2b1883d 100644 --- a/clutter/clutter-script.c +++ b/clutter/clutter-script.c @@ -314,26 +314,16 @@ object_info_free (gpointer data) * unless we are unmerging in which case we have to destroy * the actor to unparent them */ - if (oinfo->object) + if (oinfo->object != NULL) { if (oinfo->is_unmerged) { - if (oinfo->is_toplevel) - g_object_unref (oinfo->object); - else - { - /* destroy every actor, unless it's the default stage */ - if (oinfo->is_stage_default == FALSE && - CLUTTER_IS_ACTOR (oinfo->object)) - clutter_actor_destroy (CLUTTER_ACTOR (oinfo->object)); - } - } - else - { - if (oinfo->is_toplevel) - g_object_unref (oinfo->object); + if (oinfo->is_actor && !oinfo->is_stage) + clutter_actor_destroy (CLUTTER_ACTOR (oinfo->object)); } + g_object_unref (oinfo->object); + oinfo->object = NULL; }