GNOME Bugzilla – Bug 684809
proxypad don't hold a ref to their internal pad while streaming through it
Last modified: 2012-09-27 09:13:50 UTC
Proxypad don't hold a ref to their internal pad, so if one removes a src ghostpad while something is streaming through it, it will crash. Test and patch forthcoming.
Created attachment 225159 [details] [review] tests: Test the case where ghost pads are removed while streaming
Created attachment 225160 [details] [review] proxypad: Hold a reference to the internal pad while pushing through it
Comment on attachment 225160 [details] [review] proxypad: Hold a reference to the internal pad while pushing through it Looks good to me, please push
I would rather like to avoid the cost of the lock and refcount somehow. The internal pad is supposed to exist during the whole lifetime of the object so it must be that the ghostpad is finalized while doing datapassing. It should probably avoid doing that by first flushing and then taking the stream lock or so.
This is actually a bug in the probes, I think. The pad is removed from the probe handler and the probe handler doesn't notice. It would suggest to add more refs somewhere.
This should be the rule I think: 1) callers of gst_pad_push() must make sure the pad never disappears while inside the push function. They must, for example, take a ref or make sure the lifecycle of the pad is tied to that of the element. 2) The same rule goes for gst_pad_chain(). Normally, the peer pad calls the chain function with an extra ref to the pad so there is no problem. 3) same rules for query and event functions. Elements with request srcpads, like tee, ref the pad before pushing. Some problem cases: - Ghostpads don't ref the internal pad before gst_pad_psuh(), this is what the patch adds among other cases. - Most elements with static pads want to stop the dataflow before disposing and destroying the pads. I think there can still be a problem if you unref the last ref on an element from a padprobe. - If an app calls gst_pad_chain() without extra ref, a probe could destroy the sinkpad and crash. One option would be to add ref/unref pairs when entering/leaving the push/chain/event/query functions. I don't like the overhead of this. Another option would be to only ref when doing a probe. doing the unref is trickier as it needs to be dome after the lock was released.
It's not probe specific, I can reproduce it without a probe.. just add some another element/pad after and remove the ghostpad while the thread is blocked there. For some reason, I couldn't reproduce the problem with a regular element (like identity), I only see it with ghostpads. GstPad takes a ref before calling gst_pad_chain from a connected pad. So we take one ref for ever pair of connected pads. My usecase is that when rtpbin receives a BYE, it removes its src pad, then in Farstream I remove the decoder and associated ghostpad, then it crashes. This happens on two separate threads, but I can reproduce it relatively often as the rtpbin->sink thread spends almost all of it's time blocked in the sink in the ideal case. Also, the Farstream shutdown code uses the same codepath. I'm not a big fan of taking a ref inside gst_pad_push(), I think everyone who calls a method on a GObject should make sure it holds a ref to it until the function returns.
(In reply to comment #7) > It's not probe specific, I can reproduce it without a probe.. just add some > another element/pad after and remove the ghostpad while the thread is blocked > there. It should also happen when you just remove a ghostpad (at the right time) while things are streaming. > I'm not a big fan of taking a ref inside gst_pad_push(), I think everyone who > calls a method on a GObject should make sure it holds a ref to it until the > function returns. Let's do this for now in the ghostpads as well, then. All the event and query forward functions already take a ref on the pad so I think the ghostpads is the only place left where things can go wrong.
commit e60ee132ce3115391d83d1370e809a0531f3db57 Author: Olivier Crête <olivier.crete@collabora.com> Date: Mon Sep 24 18:26:16 2012 -0400 proxypad: Hold a reference to the internal pad while pushing through it https://bugzilla.gnome.org/show_bug.cgi?id=684809 commit fddf7dcb01595baf678a41661e061811d96edd90 Author: Olivier Crête <olivier.crete@collabora.com> Date: Tue Sep 25 14:44:54 2012 -0400 tests: Test the case where ghost pads are removed while streaming https://bugzilla.gnome.org/show_bug.cgi?id=684809