2008-10-16 Emmanuele Bassi <ebassi@linux.intel.com>

* clutter/clutter-container.[ch]: Add checks to the Container
	interface invocation methods, to avoid crashing or corrupting
	the stack when an actor does not implement every virtual
	function of the Container interface vtable. GObject allows this
	to happen so we must handle the case gracefully. This also means
	that we can classify some virtual function as mandatory (as is
	the case for ::add, ::remove and ::foreach) and some other
	optional.

ChangeLog

@@ -1,3 +1,14 @@
+2008-10-16  Emmanuele Bassi  <ebassi@linux.intel.com>
+
+	* clutter/clutter-container.[ch]: Add checks to the Container
+	interface invocation methods, to avoid crashing or corrupting
+	the stack when an actor does not implement every virtual
+	function of the Container interface vtable. GObject allows this
+	to happen so we must handle the case gracefully. This also means
+	that we can classify some virtual function as mandatory (as is
+	the case for ::add, ::remove and ::foreach) and some other
+	optional.
+
 2008-10-08  Emmanuele Bassi  <ebassi@linux.intel.com>
 
 	* tests/test-depth.c (raise_top), (main): Fix a shadowing

clutter/clutter-container.c

@@ -43,6 +43,23 @@
 #include "clutter-private.h"
 #include "clutter-enum-types.h"
 
+#define CLUTTER_CONTAINER_WARN_NOT_IMPLEMENTED(container,vfunc) \
+        G_STMT_START { \
+          g_warning ("Container of type `%s' does not implement " \
+                     "the required ClutterContainer::%s virtual " \
+                     "function.",                                 \
+                     G_OBJECT_TYPE_NAME ((container)),            \
+                     (vfunc));                                    \
+        } G_STMT_END
+
+#define CLUTTER_CONTAINER_NOTE_NOT_IMPLEMENTED(container,vfunc) \
+        G_STMT_START { \
+          CLUTTER_NOTE (ACTOR, "Container of type `%s' does not "    \
+                               "implement the ClutterContainer::%s " \
+                               "virtual function.",                  \
+                        G_OBJECT_TYPE_NAME ((container)),            \
+                        (vfunc));                                    \
+        } G_STMT_END
 
 /**
  * SECTION:clutter-container
@@ -69,24 +86,17 @@ enum
 static guint container_signals[LAST_SIGNAL] = { 0, };
 static GQuark quark_child_meta = 0;
 
-static ClutterChildMeta *
+static ClutterChildMeta *get_child_meta     (ClutterContainer *container,
-get_child_meta (ClutterContainer *container,
+                                             ClutterActor     *actor);
-                ClutterActor     *actor);
+static void              create_child_meta  (ClutterContainer *container,
+                                             ClutterActor     *actor);
+static void              destroy_child_meta (ClutterContainer *container,
+                                             ClutterActor     *actor);
 
-static void
+static void clutter_container_create_child_meta  (ClutterContainer *container,
-create_child_meta (ClutterContainer *container,
+                                                  ClutterActor     *actor);
-                   ClutterActor     *actor);
+static void clutter_container_destroy_child_meta (ClutterContainer *container,
-
+                                                  ClutterActor     *actor);
-static void
-destroy_child_meta (ClutterContainer *container,
-                    ClutterActor     *actor);
-
-static void
-clutter_container_create_child_meta  (ClutterContainer *container,
-                                      ClutterActor     *actor);
-static void
-clutter_container_destroy_child_meta (ClutterContainer *container,
-                                      ClutterActor     *actor);
 
 
 static void
@@ -178,7 +188,7 @@ clutter_container_get_type (void)
 
   if (G_UNLIKELY (!container_type))
     {
-      GTypeInfo container_info =
+      const GTypeInfo container_info =
       {
         sizeof (ClutterContainerIface),
         clutter_container_base_init,
@@ -239,17 +249,25 @@ void
 clutter_container_add_actor (ClutterContainer *container,
                              ClutterActor     *actor)
 {
+  ClutterContainerIface *iface;
   ClutterActor *parent;
 
   g_return_if_fail (CLUTTER_IS_CONTAINER (container));
   g_return_if_fail (CLUTTER_IS_ACTOR (actor));
 
+  iface = CLUTTER_CONTAINER_GET_IFACE (container);
+  if (!iface->add)
+    {
+      CLUTTER_CONTAINER_WARN_NOT_IMPLEMENTED (container, "add");
+      return;
+    }
+
   parent = clutter_actor_get_parent (actor);
   if (parent)
     {
       g_warning ("Attempting to add actor of type `%s' to a "
-		 "group of type `%s', but the actor has already "
+		 "container of type `%s', but the actor has "
-		 "a parent of type `%s'.",
+                 "already a parent of type `%s'.",
 		 g_type_name (G_OBJECT_TYPE (actor)),
 		 g_type_name (G_OBJECT_TYPE (container)),
 		 g_type_name (G_OBJECT_TYPE (parent)));
@@ -257,7 +275,8 @@ clutter_container_add_actor (ClutterContainer *container,
     }
 
   clutter_container_create_child_meta (container, actor);
-  CLUTTER_CONTAINER_GET_IFACE (container)->add (container, actor);
+
+  iface->add (container, actor);
 }
 
 /**
@@ -333,24 +352,33 @@ void
 clutter_container_remove_actor (ClutterContainer *container,
                                 ClutterActor     *actor)
 {
+  ClutterContainerIface *iface;
   ClutterActor *parent;
 
   g_return_if_fail (CLUTTER_IS_CONTAINER (container));
   g_return_if_fail (CLUTTER_IS_ACTOR (actor));
 
+  iface = CLUTTER_CONTAINER_GET_IFACE (container);
+  if (!iface->remove)
+    {
+      CLUTTER_CONTAINER_WARN_NOT_IMPLEMENTED (container, "remove");
+      return;
+    }
+
   parent = clutter_actor_get_parent (actor);
   if (parent != CLUTTER_ACTOR (container))
     {
       g_warning ("Attempting to remove actor of type `%s' from "
-		 "group of class `%s', but the group is not the "
+		 "group of class `%s', but the container is not "
-		 "actor's parent.",
+                 "the actor's parent.",
 		 g_type_name (G_OBJECT_TYPE (actor)),
 		 g_type_name (G_OBJECT_TYPE (container)));
       return;
     }
 
   clutter_container_destroy_child_meta (container, actor);
-  CLUTTER_CONTAINER_GET_IFACE (container)->remove (container, actor);
+
+  iface->remove (container, actor);
 }
 
 /**
@@ -429,12 +457,19 @@ clutter_container_foreach (ClutterContainer *container,
                            ClutterCallback   callback,
                            gpointer          user_data)
 {
+  ClutterContainerIface *iface;
+
   g_return_if_fail (CLUTTER_IS_CONTAINER (container));
   g_return_if_fail (callback != NULL);
 
-  CLUTTER_CONTAINER_GET_IFACE (container)->foreach (container,
+  iface = CLUTTER_CONTAINER_GET_IFACE (container);
-                                                    callback,
+  if (!iface->foreach)
-                                                    user_data);
+    {
+      CLUTTER_CONTAINER_WARN_NOT_IMPLEMENTED (container, "foreach");
+      return;
+    }
+
+  iface->foreach (container, callback, user_data);
 }
 
 /**
@@ -452,10 +487,19 @@ clutter_container_raise_child (ClutterContainer *container,
                                ClutterActor     *actor,
                                ClutterActor     *sibling)
 {
+  ClutterContainerIface *iface;
+
   g_return_if_fail (CLUTTER_IS_CONTAINER (container));
   g_return_if_fail (CLUTTER_IS_ACTOR (actor));
   g_return_if_fail (sibling == NULL || CLUTTER_IS_ACTOR (sibling));
 
+  iface = CLUTTER_CONTAINER_GET_IFACE (container);
+  if (!iface->raise)
+    {
+      CLUTTER_CONTAINER_NOTE_NOT_IMPLEMENTED (container, "raise");
+      return;
+    };
+
   if (actor == sibling)
     return;
 
@@ -478,7 +522,7 @@ clutter_container_raise_child (ClutterContainer *container,
       return;
     }
 
-  CLUTTER_CONTAINER_GET_IFACE (container)->raise (container, actor, sibling);
+  iface->raise (container, actor, sibling);
 }
 
 /**
@@ -496,10 +540,19 @@ clutter_container_lower_child (ClutterContainer *container,
                                ClutterActor     *actor,
                                ClutterActor     *sibling)
 {
+  ClutterContainerIface *iface;
+
   g_return_if_fail (CLUTTER_IS_CONTAINER (container));
   g_return_if_fail (CLUTTER_IS_ACTOR (actor));
   g_return_if_fail (sibling == NULL || CLUTTER_IS_ACTOR (sibling));
 
+  iface = CLUTTER_CONTAINER_GET_IFACE (container);
+  if (!iface->lower)
+    {
+      CLUTTER_CONTAINER_NOTE_NOT_IMPLEMENTED (container, "lower");
+      return;
+    }
+
   if (actor == sibling)
     return;
 
@@ -522,7 +575,7 @@ clutter_container_lower_child (ClutterContainer *container,
       return;
     }
 
-  CLUTTER_CONTAINER_GET_IFACE (container)->lower (container, actor, sibling);
+  iface->lower (container, actor, sibling);
 }
 
 /**
@@ -537,9 +590,15 @@ clutter_container_lower_child (ClutterContainer *container,
 void
 clutter_container_sort_depth_order (ClutterContainer *container)
 {
+  ClutterContainerIface *iface;
+
   g_return_if_fail (CLUTTER_IS_CONTAINER (container));
 
-  CLUTTER_CONTAINER_GET_IFACE (container)->sort_depth_order (container);
+  iface = CLUTTER_CONTAINER_GET_IFACE (container);
+  if (iface->sort_depth_order)
+    iface->sort_depth_order (container);
+  else
+    CLUTTER_CONTAINER_NOTE_NOT_IMPLEMENTED (container, "sort_depth_order");
 }
 
 /**

clutter/clutter-container.h

@@ -44,9 +44,12 @@ typedef struct _ClutterContainerIface   ClutterContainerIface;
 
 /**
  * ClutterContainerIface:
- * @add: virtual function for adding an actor to the container
+ * @add: virtual function for adding an actor to the container. The
- * @remove: virtual function for removing an actor from the container
+ *   implementation of this virtual function is required.
- * @foreach: virtual function for iterating over the container's children
+ * @remove: virtual function for removing an actor from the container. The
+ *   implementation of this virtual function is required.
+ * @foreach: virtual function for iterating over the container's children.
+ *   The implementation of this virtual function is required.
  * @raise: virtual function for raising a child
  * @lower: virtual function for lowering a child
  * @sort_depth_order: virtual function for sorting the children of a
@@ -65,7 +68,9 @@ typedef struct _ClutterContainerIface   ClutterContainerIface;
  * @actor_removed: class handler for #ClutterContainer::actor_removed
  * @child_notify: class handler for #ClutterContainer::child-notify
  *
- * Base interface for container actors.
+ * Base interface for container actors. The @add, @remove and @foreach
+ * virtual functions must be provided by any implementation; the other
+ * virtual functions are optional.
  *
  * Since: 0.4
  */
@@ -82,6 +87,8 @@ struct _ClutterContainerIface
   void (* foreach)          (ClutterContainer *container,
                              ClutterCallback   callback,
                              gpointer          user_data);
+
+  /* child stacking */
   void (* raise)            (ClutterContainer *container,
                              ClutterActor     *actor,
                              ClutterActor     *sibling);
@@ -91,7 +98,6 @@ struct _ClutterContainerIface
   void (* sort_depth_order) (ClutterContainer *container);
 
   /* ClutterChildMeta management */
-
   GType                child_meta_type;
   void              (* create_child_meta)  (ClutterContainer *container,
                                             ClutterActor     *actor);