Created attachment 171875 [details]
patch to add a sleep() to acceptcaps

Created attachment 171876 [details]
test program

I guess the solution in the "easy" solution is to verify the return value of gst_pad_get_parent() everywhere in the tree...

Another solution would be change GstPad to only hold a ref to the parent of the pad while calling the functions that don't require the pad to be active, and not call them if there is no parent and one was ever set.

*** Bug 638844 has been marked as a duplicate of this bug. ***

Copying my comment from bug #646566 here, which essentially seems to be what Olivier and Wim want to have too:

IMHO the best solution for this would be to
a) add a GstObject *parent parameter to all pad callbacks and
b) add a flag to GstPad that you don't want to have your callback called if
there's no parent (and by default that flag is disabled)

For this we have to wait until 0.11 though because it's an API change.

b) might be possible to do in 0.10 by taking a reference of the parent before
calling the callbacks

Created attachment 185702 [details] [review]
pad: Add PARENT_REQUIRED flag and do not call functions if it is set and not parent is set

This is the first step to prevent a possible race where a function is called after it has
been removed from its parent element.

Created attachment 185703 [details] [review]
element: Make regular element pads require a parent for callbacks

Created attachment 185704 [details] [review]
ghostpad: Make sure the ghostpad don't call their proxypad if the parent is no longer set

Created attachment 185705 [details] [review]
pad: Pass the parent to callback functions

This patch is 0.11 material, although there are different solutions that we could use in 0.10:

1. Rename the function prototypes to ...Full and add a set of gst_pad_set_xfunc_full methods to set it and make the current ones just wrappers that typecast the parent-less methods into the parented ones. Then we just need to update all the plugins to take the parent from the parameter instead of fetching it.

2. We could add a GPrivate (thread local storage) and store the current parent inside it at the beginning of a callback (and keep a ref until we return from the callback). Then modify gst_object_get_parent() to return the value from the TLS if it's set.. That said, this wouldnt work with people using GST_{PAD,OBJECT}_PARENT .. and about half of the current plugins do

3. We could add some kind of queue for gst_object_set_parent() and "lock" the parent into place until all callbacks returns and only then actually set it. This would require zero changes to the current plugins, but I'm afraid of the side-effects (and it's also quite an ugly solution).

Review of attachment 185702 [details] [review]:

::: gst/gstpad.c
@@ +4275,3 @@
+      (parent = gst_pad_get_parent (pad)) == NULL) {
+    GST_PAD_STREAM_UNLOCK (pad);
+    return GST_FLOW_NOT_LINKED;

You should probably unref the buffer/event here and return WRONG_STATE instead of NOT_LINKED

@@ +4758,3 @@
+  if (GST_PAD_IS_PARENT_REQUIRED (pad) &&
+      (parent = gst_pad_get_parent (pad)) == NULL) {
+    return GST_FLOW_NOT_LINKED;

Same here

@@ +4887,3 @@
+  if (GST_PAD_IS_PARENT_REQUIRED (pad) &&
+      (parent = gst_pad_get_parent (pad)) == NULL) {
+    return GST_FLOW_NOT_LINKED;

And here

Except the last patch this could all be done in 0.10 already, the last patch requires changes *everywhere* though.

Created attachment 185723 [details] [review]
pad: Add PARENT_REQUIRED flag and do not call functions if it is set and not parent is set

This is the first step to prevent a possible race where a function is called after it has
been removed from its parent element.

(In reply to comment #10)
> Except the last patch this could all be done in 0.10 already, the last patch
> requires changes *everywhere* though.

But having the first patches without the last patch is not so useful because the pad parent could be NULLed between checking in gstpad.c and the element actually trying to read it.

(In reply to comment #12)
> (In reply to comment #10)
> > Except the last patch this could all be done in 0.10 already, the last patch
> > requires changes *everywhere* though.
> 
> But having the first patches without the last patch is not so useful because
> the pad parent could be NULLed between checking in gstpad.c and the element
> actually trying to read it.

Oh, right. The parent could still be unset although the pad holds a reference to it. Nevermind...

I'd say that this is fine for 0.11 but let's wait for Wim to comment first :) Also it will be a lot of work to change all existing elements

While we're changing all the pad functions we could as well add a user_data and GDestroyNotify parameter to them. g-i likes to have this :)

add a different one for each callback ?

Yes, e.g.

gst_pad_set_chain_function (pad, func, user_data, destroy_notify)

Implemented in 0.11.