After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 767879 - Deadlock in WebKit when deleting the media player while HLS media was loaded
Deadlock in WebKit when deleting the media player while HLS media was loaded
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
1.8.2
Other Linux
: Normal normal
: 1.9.2
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-06-20 16:43 UTC by Carlos Garcia Campos
Modified: 2016-07-25 14:35 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gstadaptivedemux: Release manifest lock before calling gst_element_no_more_pads() (1.08 KB, patch)
2016-06-20 16:55 UTC, Carlos Garcia Campos
reviewed Details | Review

Description Carlos Garcia Campos 2016-06-20 16:43:26 UTC
It happens when the media player is destroyed by the garbacge collector and pipeline is set to NULL state. This always happens in the main thread, since the garbage colector frees the objects in the main thread. The situation is the following one:

Main Thread:
  - blocked on GST_PAD_STREAM_LOCK (post_activate() gstpad.c)

Secondary Thread:
  - blocked on GST_MANIFEST_LOCK (gst_adaptive_demux_src_event() reconfigure gstadaptivedemux.c)
  - has EXPOSE_LOCK of gstdecodebin2 taken from source_pad_blocked_cb()

Secondary Thread:
  - blocked on EXPOSE_LOCK (no_more_pads_cb() gstdecodebin2.c)
  - it seems to have GST_PAD_STREAM_LOCK of gstpad taken from gst_pad_send_event_unchecked()
  - has GST_MANIFEST_LOCK of gstadaptivedemux taken from _src_event, got fragment EOS

Secondary Thread:
  - infinite loop waiting for fragment download to finish (gst_adaptive_demux_stream_download_uri()

I haven't been able to fix or workaround this from WebKit code, the only change the seemed to work was releasing the manifest lock in gst_adaptive_demux_expose_streams before calling gst_element_no_more_pads() and taking it again. But I have no idea if the change is correct or just a workaround, the gst code is full of locks and conditions that is very difficult to follow.
Comment 1 Carlos Garcia Campos 2016-06-20 16:50:46 UTC
bts of relevant threads

Thread 18 (Thread 0x7f07433fb700 (LWP 16767))

  • #0 syscall
    at ../sysdeps/unix/sysv/linux/x86_64/syscall.S line 38
  • #1 g_mutex_lock_slowpath
    at gthread-posix.c line 1315
  • #2 g_mutex_lock
    at gthread-posix.c line 1339
  • #3 no_more_pads_cb
    at gstdecodebin2.c line 3085
  • #4 _g_closure_invoke_va
    at gclosure.c line 867
  • #5 g_signal_emit_valist
    at gsignal.c line 3294
  • #6 g_signal_emit
    at gsignal.c line 3441
  • #7 gst_element_no_more_pads
    at gstelement.c line 857
  • #8 gst_adaptive_demux_expose_streams
    at gstadaptivedemux.c line 1003
  • #9 gst_adaptive_demux_stream_advance_fragment_unlocked
    at gstadaptivedemux.c line 3270
  • #10 gst_adaptive_demux_stream_advance_fragment
    at gstadaptivedemux.c line 3192
  • #11 gst_hls_demux_finish_fragment
    at gsthlsdemux.c line 622
  • #12 _src_event
    at gstadaptivedemux.c line 2167
  • #13 gst_pad_send_event_unchecked
    at gstpad.c line 5576
  • #14 gst_pad_push_event_unchecked
    at gstpad.c line 5234
  • #15 push_sticky
    at gstpad.c line 3779
  • #16 events_foreach
    at gstpad.c line 601
  • #17 check_sticky
    at gstpad.c line 3836
  • #18 gst_pad_push_event
    at gstpad.c line 5365
  • #19 event_forward_func
    at gstpad.c line 2982
  • #20 gst_pad_forward
    at gstpad.c line 2936
  • #21 gst_pad_event_default
    at gstpad.c line 3033
  • #22 gst_pad_send_event_unchecked
    at gstpad.c line 5576
  • #23 gst_pad_push_event_unchecked
    at gstpad.c line 5234
  • #24 push_sticky
    at gstpad.c line 3779
  • #25 events_foreach
    at gstpad.c line 601
  • #26 check_sticky
    at gstpad.c line 3836
  • #27 gst_pad_push_event
    at gstpad.c line 5365
  • #28 gst_queue2_push_one
    at gstqueue2.c line 2860
  • #29 gst_queue2_loop
    at gstqueue2.c line 2949
  • #30 gst_task_func
    at gsttask.c line 332
  • #31 g_thread_pool_thread_proxy
    at gthreadpool.c line 307
  • #32 g_thread_proxy
    at gthread.c line 780
  • #33 start_thread
    at pthread_create.c line 334
  • #34 clone
    at ../sysdeps/unix/sysv/linux/x86_64/clone.S line 109

Thread 11 (Thread 0x7f073c9fd700 (LWP 16774))

  • #0 __lll_lock_wait
    at ../sysdeps/unix/sysv/linux/x86_64/lowlevellock.S line 135
  • #1 __GI___pthread_mutex_lock
    at ../nptl/pthread_mutex_lock.c line 116
  • #2 gst_adaptive_demux_src_event
    at gstadaptivedemux.c line 1444
  • #3 gst_pad_send_event_unchecked
    at gstpad.c line 5576
  • #4 gst_pad_push_event_unchecked
    at gstpad.c line 5234
  • #5 gst_pad_push_event
    at gstpad.c line 5371
  • #6 gst_pad_send_event_unchecked
    at gstpad.c line 5576
  • #7 gst_pad_push_event_unchecked
    at gstpad.c line 5234
  • #8 gst_pad_push_event
    at gstpad.c line 5371
  • #9 event_forward_func
    at gstpad.c line 2982
  • #10 gst_pad_forward
    at gstpad.c line 2936
  • #11 gst_pad_event_default
    at gstpad.c line 3033
  • #12 gst_pad_send_event_unchecked
    at gstpad.c line 5576
  • #13 gst_pad_push_event_unchecked
    at gstpad.c line 5234
  • #14 gst_pad_push_event
    at gstpad.c line 5371
  • #15 gst_pad_send_event_unchecked
    at gstpad.c line 5576
  • #16 gst_pad_push_event_unchecked
    at gstpad.c line 5234
  • #17 gst_pad_push_event
    at gstpad.c line 5371
  • #18 event_forward_func
    at gstpad.c line 2982
  • #19 gst_pad_forward
    at gstpad.c line 2936
  • #20 gst_pad_event_default
    at gstpad.c line 3033
  • #21 gst_base_parse_src_event_default
    at gstbaseparse.c line 1650
  • #22 gst_pad_send_event_unchecked
    at gstpad.c line 5576
  • #23 gst_pad_push_event_unchecked
    at gstpad.c line 5234
  • #24 gst_pad_push_event
    at gstpad.c line 5371
  • #25 event_forward_func
    at gstpad.c line 2982
  • #26 gst_pad_forward
    at gstpad.c line 2936
  • #27 gst_pad_event_default
    at gstpad.c line 3033
  • #28 gst_audio_decoder_src_eventfunc
    at gstaudiodecoder.c line 2447
  • #29 gst_pad_send_event_unchecked
    at gstpad.c line 5576
  • #30 gst_pad_push_event_unchecked
    at gstpad.c line 5234
  • #31 gst_pad_push_event
    at gstpad.c line 5371
  • #32 event_forward_func
    at gstpad.c line 2982
  • #33 gst_pad_forward
    at gstpad.c line 2936
  • #34 gst_pad_event_default
    at gstpad.c line 3033
  • #35 gst_pad_send_event_unchecked
    at gstpad.c line 5576
  • #36 gst_pad_push_event_unchecked
    at gstpad.c line 5234
  • #37 gst_pad_push_event
    at gstpad.c line 5371
  • #38 event_forward_func
    at gstpad.c line 2982
  • #39 gst_pad_forward
    at gstpad.c line 2936
  • #40 gst_pad_event_default
    at gstpad.c line 3033
  • #41 gst_pad_send_event_unchecked
    at gstpad.c line 5576
  • #42 gst_pad_send_event
    at gstpad.c line 5746
  • #43 gst_pad_link_full
    at gstpad.c line 2521
  • #44 gst_pad_link
    at gstpad.c line 2579
  • #45 pad_added_cb
    at gstplaybin2.c line 3454
  • #46 g_cclosure_marshal_VOID__OBJECTv
    at gmarshal.c line 2102
  • #47 _g_closure_invoke_va
    at gclosure.c line 867
  • #48 g_signal_emit_valist
    at gsignal.c line 3294
  • #49 g_signal_emit
    at gsignal.c line 3441
  • #50 gst_element_add_pad
    at gstelement.c line 704
  • #51 g_cclosure_marshal_VOID__OBJECTv
  • #52 _g_closure_invoke_va
    at gclosure.c line 867
  • #53 g_signal_emit_valist
    at gsignal.c line 3294
  • #54 g_signal_emit
    at gsignal.c line 3441
  • #55 gst_element_add_pad
    at gstelement.c line 704
  • #56 gst_decode_bin_expose
    at gstdecodebin2.c line 4655
  • #57 source_pad_blocked_cb
    at gstdecodebin2.c line 4851
  • #58 probe_hook_marshal
    at gstpad.c line 3442
  • #59 g_hook_list_marshal
    at ghook.c line 672
  • #60 do_probe_callbacks
    at gstpad.c line 3594
  • #61 gst_pad_push_event_unchecked
    at gstpad.c line 5201
  • #62 push_sticky
    at gstpad.c line 3779
  • #63 events_foreach
    at gstpad.c line 601
  • #64 check_sticky
    at gstpad.c line 3836
  • #65 gst_pad_push_event
    at gstpad.c line 5365
  • #66 gst_pad_set_caps
    at /home/cgarcia/gnome/include/gstreamer-1.0/gst/gstcompat.h line 58
  • #67 gst_video_decoder_negotiate_default
    at gstvideodecoder.c line 3835
  • #68 gst_video_decoder_negotiate
    at gstvideodecoder.c line 3889
  • #69 gst_ffmpegviddec_negotiate
    at /home/cgarcia/src/git/gnome/gst-libav/ext/libav/gstavviddec.c line 1094
  • #70 gst_ffmpegviddec_video_frame
    at /home/cgarcia/src/git/gnome/gst-libav/ext/libav/gstavviddec.c line 1376
  • #71 gst_ffmpegviddec_frame
    at /home/cgarcia/src/git/gnome/gst-libav/ext/libav/gstavviddec.c line 1521
  • #72 gst_ffmpegviddec_handle_frame
    at /home/cgarcia/src/git/gnome/gst-libav/ext/libav/gstavviddec.c line 1634
  • #73 gst_video_decoder_decode_frame
    at gstvideodecoder.c line 3417
  • #74 gst_video_decoder_chain_forward
    at gstvideodecoder.c line 2201
  • #75 gst_video_decoder_chain
    at gstvideodecoder.c line 2503
  • #76 gst_pad_chain_data_unchecked
    at gstpad.c line 4177
  • #77 gst_pad_push_data
    at gstpad.c line 4429
  • #78 gst_pad_push
    at gstpad.c line 4548
  • #79 gst_base_transform_chain
    at gstbasetransform.c line 2369
  • #80 gst_pad_chain_data_unchecked
    at gstpad.c line 4177
  • #81 gst_pad_push_data
    at gstpad.c line 4429
  • #82 gst_pad_push
    at gstpad.c line 4548
  • #83 gst_base_parse_push_frame
    at gstbaseparse.c line 2512
  • #84 gst_base_parse_handle_and_push_frame
    at gstbaseparse.c line 2331
  • #85 gst_base_parse_finish_frame
    at gstbaseparse.c line 2670
  • #86 gst_h264_parse_handle_frame
    at gsth264parse.c line 1228
  • #87 gst_base_parse_handle_buffer
    at gstbaseparse.c line 2145
  • #88 gst_base_parse_chain
    at gstbaseparse.c line 3209
  • #89 gst_pad_chain_data_unchecked
    at gstpad.c line 4177
  • #90 gst_pad_push_data
    at gstpad.c line 4429
  • #91 gst_pad_push
    at gstpad.c line 4548
  • #92 gst_single_queue_push_one
    at gstmultiqueue.c line 1417
  • #93 gst_multi_queue_loop
    at gstmultiqueue.c line 1701
  • #94 gst_task_func
    at gsttask.c line 332
  • #95 g_thread_pool_thread_proxy
    at gthreadpool.c line 307
  • #96 g_thread_proxy
    at gthread.c line 780
  • #97 start_thread
    at pthread_create.c line 334
  • #98 clone
    at ../sysdeps/unix/sysv/linux/x86_64/clone.S line 109

Comment 2 Carlos Garcia Campos 2016-06-20 16:55:08 UTC
Created attachment 330093 [details] [review]
gstadaptivedemux: Release manifest lock before calling gst_element_no_more_pads()
Comment 3 Sebastian Dröge (slomo) 2016-06-21 08:24:24 UTC
Comment on attachment 330093 [details] [review]
gstadaptivedemux: Release manifest lock before calling gst_element_no_more_pads()

This needs to be carefully checked if we can just release the lock there, and if we might have to check again after taking the lock if things have changed. There is also the problem that subclasses might expect it to be not released when advancing fragments, etc.
Comment 4 Carlos Garcia Campos 2016-06-21 10:32:30 UTC
(In reply to Sebastian Dröge (slomo) from comment #3)
> Comment on attachment 330093 [details] [review] [review]
> gstadaptivedemux: Release manifest lock before calling
> gst_element_no_more_pads()
> 
> This needs to be carefully checked if we can just release the lock there,
> and if we might have to check again after taking the lock if things have
> changed. There is also the problem that subclasses might expect it to be not
> released when advancing fragments, etc.

Yes, I really have no idea what I'm doing, TBH. So, I was just expecting you to help me to properly fix these deadlocks, because they are making popular sites like facebook or twitter unusable with WebKit.
Comment 5 Sebastian Dröge (slomo) 2016-06-21 11:16:35 UTC
It might be better or at least safer to defer the signal until a later point in one of the callers of the function. Also the same problem probably applies to pad-added.

OTOH you can't really set the state of any element to NULL from no-more-pads. Is that what you're doing? I'm surprised that doesn't deadlock from other elements too :)
Comment 6 Carlos Garcia Campos 2016-06-21 17:10:14 UTC
(In reply to Sebastian Dröge (slomo) from comment #5)
> It might be better or at least safer to defer the signal until a later point
> in one of the callers of the function. Also the same problem probably
> applies to pad-added.

What signal and function are you talking about exactly?

> OTOH you can't really set the state of any element to NULL from
> no-more-pads. Is that what you're doing? I'm surprised that doesn't deadlock
> from other elements too :)

No, we are setting the pipeline to NULL right before deleting it, no more pads happens in a different thread:

Secondary Thread:
  - blocked on EXPOSE_LOCK (no_more_pads_cb() gstdecodebin2.c)
  - it seems to have GST_PAD_STREAM_LOCK of gstpad taken from gst_pad_send_event_unchecked()
  - has GST_MANIFEST_LOCK of gstadaptivedemux taken from _src_event, got fragment EOS

See Thread 18 in the bt I submitted. Setting the pipeline to NULL is blocking on GST_PAD_STREAM_LOCK in post_activate() gstpad.c
Comment 7 Sebastian Dröge (slomo) 2016-06-21 17:21:43 UTC
Ok, so the problem is that there is:

Thread 11: streaming thread of video decoder, pad probe in decodebin, having decodebin locks, linking things, causing reconfigure event, reconfigure event in adaptivedemux causes manifest lock

Thread 18: streaming thread of one of the internal souphttpsrc, manifest lock, no-more-pads signal, decodebin no-more-pads handler, decodebin locks


So in any case we need to find a way of moving the no-more-pads signal emission outside of the lock in a safe way.
Comment 8 Carlos Garcia Campos 2016-06-22 06:11:31 UTC
(In reply to Sebastian Dröge (slomo) from comment #7)
> Ok, so the problem is that there is:
> 
> Thread 11: streaming thread of video decoder, pad probe in decodebin, having
> decodebin locks, linking things, causing reconfigure event, reconfigure
> event in adaptivedemux causes manifest lock
> 
> Thread 18: streaming thread of one of the internal souphttpsrc, manifest
> lock, no-more-pads signal, decodebin no-more-pads handler, decodebin locks

Yes, and then main thread blocked on post_activate because the pad streams lock is never released.

> 
> So in any case we need to find a way of moving the no-more-pads signal
> emission outside of the lock in a safe way.

My first attempt to fix this was moving the no more pads emission after the cancellation of the threads, but that didn't work and caused even more blocks. I want to help here, but I don't know gst enough to know how to properly fix this.
Comment 9 Sebastian Dröge (slomo) 2016-06-22 06:19:43 UTC
(In reply to Carlos Garcia Campos from comment #8)
> (In reply to Sebastian Dröge (slomo) from comment #7)
> > Ok, so the problem is that there is:
> > 
> > Thread 11: streaming thread of video decoder, pad probe in decodebin, having
> > decodebin locks, linking things, causing reconfigure event, reconfigure
> > event in adaptivedemux causes manifest lock
> > 
> > Thread 18: streaming thread of one of the internal souphttpsrc, manifest
> > lock, no-more-pads signal, decodebin no-more-pads handler, decodebin locks
> 
> Yes, and then main thread blocked on post_activate because the pad streams
> lock is never released.

Yes, that's expected if there is a deadlock in the streaming threads elsewhere. The main thread can be ignored here, fortunately the problem is only between two threads and not three :)

> > 
> > So in any case we need to find a way of moving the no-more-pads signal
> > emission outside of the lock in a safe way.
> 
> My first attempt to fix this was moving the no more pads emission after the
> cancellation of the threads, but that didn't work and caused even more
> blocks. I want to help here, but I don't know gst enough to know how to
> properly fix this.

This will need some more thorough analysis of the locking in adaptivedemux unfortunately. Not sure when I'll have time to go through that...

Maybe your patch is good, maybe not :) The problem with moving no-more-pads up to the callers is also that there are a few callers, and the caller is already holding the manifest lock.
Comment 10 Sebastian Dröge (slomo) 2016-07-21 10:48:43 UTC
This is probably fixed by https://cgit.freedesktop.org/gstreamer/gst-plugins-bad/commit/?id=03f96dd7319da1cb6e190f05cd37464404b09b1c

Can you test and confirm? Please reopen if it's not fixed, thanks :)
Comment 11 Carlos Garcia Campos 2016-07-21 16:40:22 UTC
(In reply to Sebastian Dröge (slomo) from comment #10)
> This is probably fixed by
> https://cgit.freedesktop.org/gstreamer/gst-plugins-bad/commit/
> ?id=03f96dd7319da1cb6e190f05cd37464404b09b1c
> 
> Can you test and confirm? Please reopen if it's not fixed, thanks :)

Cool, thanks! I'll do it.
Comment 12 Michael Catanzaro 2016-07-25 14:35:02 UTC
(In reply to Carlos Garcia Campos from comment #11)
> (In reply to Sebastian Dröge (slomo) from comment #10)
> > This is probably fixed by
> > https://cgit.freedesktop.org/gstreamer/gst-plugins-bad/commit/
> > ?id=03f96dd7319da1cb6e190f05cd37464404b09b1c
> > 
> > Can you test and confirm? Please reopen if it's not fixed, thanks :)
> 
> Cool, thanks! I'll do it.

If it works, then it'd be great for WebKit if this one could be backported too.