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 783256 - adaptivedemux: make sure to free all "old streams"
adaptivedemux: make sure to free all "old streams"
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
unspecified
Other All
: Normal normal
: 1.12.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2017-05-31 01:16 UTC by Mathieu Duponchelle
Modified: 2017-06-12 14:16 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
adaptivedemux: make sure to free all "old streams" (1.20 KB, patch)
2017-05-31 01:16 UTC, Mathieu Duponchelle
none Details | Review
adaptivedemux: make sure to free all "old streams" (1.21 KB, patch)
2017-05-31 16:08 UTC, Mathieu Duponchelle
committed Details | Review

Description Mathieu Duponchelle 2017-05-31 01:16:13 UTC
As we release the MANIFEST_LOCK in stop_tasks,
demux->priv->old_streams can be set, we need to free these
otherwise we may end up trying to dispose elements in the
READY state.
Comment 1 Mathieu Duponchelle 2017-05-31 01:16:17 UTC
Created attachment 352912 [details] [review]
adaptivedemux: make sure to free all "old streams"
Comment 2 Mathieu Duponchelle 2017-05-31 01:18:10 UTC
To reproduce this error, first apply https://bugzilla.gnome.org/show_bug.cgi?id=783255 (otherwise you'll deadlock before even getting there), then simply run:

gst-validate-launcher --mute -t 'validate.hls.media_check.bipbopall_m3u8' --forever
Comment 3 Edward Hervey 2017-05-31 09:41:38 UTC
Review of attachment 352912 [details] [review]:

::: gst-libs/gst/adaptivedemux/gstadaptivedemux.c
@@ +809,3 @@
     g_list_free_full (old_streams,
         (GDestroyNotify) gst_adaptive_demux_stream_free);
+  } else if (demux->priv->old_streams) {

why "else if" and not just "if" ?
Comment 4 Mathieu Duponchelle 2017-05-31 16:07:54 UTC
> why "else if" and not just "if" ?

No good reason, I'll change that, in any case we do want to cleanup whatever is in demux->priv->old_streams, it's our last chance to do so afaiu
Comment 5 Mathieu Duponchelle 2017-05-31 16:08:59 UTC
Created attachment 352957 [details] [review]
adaptivedemux: make sure to free all "old streams"

As we release the MANIFEST_LOCK in stop_tasks,
demux->priv->old_streams can be set, we need to free these
otherwise we may end up trying to dispose elements in the
READY state.
Comment 6 Edward Hervey 2017-06-01 09:14:41 UTC
commit c88125045f06a92615a01a9fc2d3c2e816e0cd2f
Author: Mathieu Duponchelle <mathieu.duponchelle@opencreed.com>
Date:   Wed May 31 03:14:04 2017 +0200

    adaptivedemux: make sure to free all "old streams"
    
    As we release the MANIFEST_LOCK in stop_tasks,
    demux->priv->old_streams can be set, we need to free these
    otherwise we may end up trying to dispose elements in the
    READY state.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=783256