bin: fix refcount when removing elements during state change

When an element is removed from a bin because it caused a state change error,
don't unref the child twice.
Add some more debug info.
Add a unit test for this error.

Fixes #615756

2 files changed, 63 insertions, 3 deletions

diff --git a/gst/gstbin.c b/gst/gstbin.c
index 3942d69e32..4d7d392218 100644
--- a/gst/gstbin.c
+++ b/gst/gstbin.c
@@ -2446,17 +2446,22 @@ restart:
             /* Only fail if the child is still inside
              * this bin. It might've been removed already
              * because of the error by the bin subclass
-             * to ignore the error.
-             */
+             * to ignore the error.  */
             parent = gst_object_get_parent (GST_OBJECT_CAST (child));
             if (parent == GST_OBJECT_CAST (element)) {
+              /* element is still in bin, really error now */
               gst_object_unref (child);
               gst_object_unref (parent);
               goto done;
             }
+            /* child removed from bin, let the resync code redo the state
+             * change */
+            GST_CAT_INFO_OBJECT (GST_CAT_STATES, element,
+                "child '%s' was removed from the bin",
+                GST_ELEMENT_NAME (child));
+
             if (parent)
               gst_object_unref (parent);
-            gst_object_unref (child);
 
             break;
           }
diff --git a/tests/check/gst/gstbin.c b/tests/check/gst/gstbin.c
index d466a28ae0..badb0491a2 100644
--- a/tests/check/gst/gstbin.c
+++ b/tests/check/gst/gstbin.c
@@ -971,6 +971,60 @@ GST_START_TEST (test_link_structure_change)
 
 GST_END_TEST;
 
+static GstBusSyncReply
+sync_handler_remove_sink (GstBus * bus, GstMessage * message, gpointer data)
+{
+  if (GST_MESSAGE_TYPE (message) == GST_MESSAGE_ERROR) {
+    GstElement *child;
+
+    child = gst_bin_get_by_name (GST_BIN (data), "fakesink");
+    fail_unless (child != NULL, "Could not find fakesink");
+
+    gst_bin_remove (GST_BIN (data), child);
+    gst_object_unref (child);
+  }
+  return GST_BUS_PASS;
+}
+
+GST_START_TEST (test_state_failure_remove)
+{
+  GstElement *src, *sink, *pipeline;
+  GstBus *bus;
+  GstStateChangeReturn ret;
+
+  pipeline = gst_pipeline_new (NULL);
+  fail_unless (pipeline != NULL, "Could not create pipeline");
+
+  src = gst_element_factory_make ("fakesrc", "fakesrc");
+  fail_unless (src != NULL, "Could not create fakesrc");
+
+  sink = gst_element_factory_make ("fakesink", "fakesink");
+  fail_unless (sink != NULL, "Could not create fakesink");
+
+  g_object_set (sink, "state-error", 1, NULL);
+
+  gst_bin_add (GST_BIN (pipeline), src);
+  gst_bin_add (GST_BIN (pipeline), sink);
+
+  gst_element_link (src, sink);
+
+  bus = gst_pipeline_get_bus (GST_PIPELINE (pipeline));
+  fail_unless (bus != NULL, "Could not get bus");
+
+  gst_bus_set_sync_handler (bus, sync_handler_remove_sink, pipeline);
+
+  ret = gst_element_set_state (pipeline, GST_STATE_READY);
+  fail_unless (ret == GST_STATE_CHANGE_SUCCESS,
+      "did not get state change success");
+
+  gst_element_set_state (pipeline, GST_STATE_NULL);
+
+  gst_object_unref (bus);
+  gst_object_unref (pipeline);
+}
+
+GST_END_TEST;
+
 static Suite *
 gst_bin_suite (void)
 {
@@ -992,6 +1046,7 @@ gst_bin_suite (void)
   tcase_add_test (tc_chain, test_add_self);
   tcase_add_test (tc_chain, test_iterate_sorted);
   tcase_add_test (tc_chain, test_link_structure_change);
+  tcase_add_test (tc_chain, test_state_failure_remove);
 
   return s;
 }