Created attachment 104824 [details] [review]
Fix

2008-02-10  Alessandro Decina  <alessandro@nnva.org>

	reviewed by: Edward Hervey  <edward.hervey@collabora.co.uk>
	* gst/gstpad.override:
	* testsuite/test_pad.py:
	Fix memleak in gst.Pad.set_blocked_async()
	Fixes #514717
	

I am not sure if this introduced other problems, but I now (0.10.11) get segfault whose backtrace is very close to what has been changed here (see below; gdb may be a bit off given the O2 optimized code).

I am also not clear on why a Py_DECREF is needed here on py_user_data.
AFAIK, a pad's block callback can be called several times, so pad_block_callback_marshal can be called more than once with the same user_data.  In each case, py_user_data refcount is decreased by Py_DECREF, but I don't know where the refcount of py_user_data is correspondingly increased with each call [PyTuple_GetItem returns a borrowed reference, so it can't be there].
If there is indeed no increase of refcount, this decreasing eventually leads to a premature release of py_user_data.

On the other hand, indeed, if Py_DECREF is not used here, the callback function does not get released if one does not explicitly call Pad.set_blocked_async() with an empty handler.  But that seems to be due to the core not employing some sort of destroy_notify to inform when it is removing refs to the callback.

In any case, presence of Py_DECREF(py_user_data) seems to have a pretty good correspondence with the occurrence of segfaults (with only minute non-determinism).

(gdb) frame

+ Trace 199219

I am re-opening this is blocker for the upcoming release
(rather than a new bug, as it keeps the history).
If the comments above are about right, then the patch is probably to undo the previous patch.
If, however, I am quite simply going daft and missing a point somewhere, then this is easily closed again, and above report should probably go into another bug, but then I have no idea about the cause (or patch), at least for now.

Ping? Is anyone working on this?

I think Mark is right and this needs to  be reverted. The idea was that a pad_block callback could only be called once. This doesn't seem the case though, as the OBJECT_LOCK is released before calling the callback.

2008-06-12  Edward Hervey  <edward.hervey@collabora.co.uk>

	* gst/gstpad.override:
	* testsuite/test_pad.py:
	Revert 2008-02-10  Alessandro Decina  <alessandro@nnva.org>
	Re-opens #514717

I've just realized that the actual trace that Mark posted fails on the line just before the one that has been removed while reverting. With the new version after the revert, I still experience similar crashes as Mark, on:
  1345        Py_DECREF(args);

Created attachment 122363 [details] [review]
Add gst_pad_set_blocked_async_full

Created attachment 122364 [details] [review]
Wrap gst_pad_set_blocked_async_full if available

In the first patch I added gst_pad_set_blocked_async_full
so that it's possible to pass a GDestroyNotify to avoid the leak.
In the gst.Pad.set_blocked_async wrapper then I use _full if
available otherwise there's a fallback on the old implementation.

Created attachment 122673 [details] [review]
gst_pad_set_blocked_async_full + tests

We discussed this a bit with wtay on irc and he gave me some suggestions
(destroy user_data in gst_pad_dispose if necessary and don't call destroy
when resetting user_data to the same value). I also added some tests.

We also agreed in that the current implementation is broken when 
gst_pad_set_blocked_async (pad, state, ...) is called and GST_PAD_IS_BLOCKED (pad) == state. The only problem is that the behaviour is sort of documented in the API. Anyway, I added a test (disabled for the moment) which shows what I think the correct behaviour should be. If people think it can be changed, i'm going to enable the test and change the implementation.

Shouldn't we move this bug to core (or at least create one that would block this bug) ?

Hum, basically I was going to commit this but I forgot.
If you think it needs more discussion then yeah, we should probably
create a core bug that blocks this one.

commit d3940f520b140f62dd616baf3b50832eac8fe648
Author: Alessandro Decina <alessandro.d@gmail.com>
Date:   Sun Feb 22 20:01:05 2009 +0100

    GstPad: Add gst_pad_set_blocked_async_full
    
    This allows connecting a GDestroyNotify for when the callback is removed/replaced.
    Partially fixes #514717

commit 4fadf0700ea010d82073526d2eb91650c9d1be1c
Author: Edward Hervey <bilboed@bilboed.com>
Date:   Sun Feb 22 20:08:54 2009 +0100

    gstbus.override: Allow using set_sync_handler with None as a parameter