Attachment 340434 Details for Bug 185657 – [GStreamer]: Do not steal GstStreamCollection's reference to GstStream

[patch] [GStreamer]: Do not steal GstStreamCollection's reference to GstStream

GStreamer-Do-not-steal-GstStreamCollections-refere.patch (text/plain), 5.13 KB, created by Thibault Saunier on 2018-05-15 14:29:43 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Thibault Saunier

Created: 2018-05-15 14:29:43 PDT

Size: 5.13 KB

patch

obsolete

>From 99dc10f59e64ea263affe390965e0cd07272e52f Mon Sep 17 00:00:00 2001
>From: Thibault Saunier <tsaunier@igalia.com>
>Date: Tue, 15 May 2018 17:12:42 -0400
>Subject: [PATCH xserver] [GStreamer]: Do not steal GstStreamCollection's
> reference to GstStream
>
>Before GStreamer 1.14[1] GstStream's reference was not sunk when creating
>it, meaning that when we were using
>
>    `GRefPtr<GstStream> stream = gst_stream_collection_get(collection, stream_index);`
>
>we were actually calling `g_object_sink_ref(some_stream);` leading to taking
>ownership of that (floating) reference, while we were not actually owning it as it was
>owned by the GstStreamCollection itself. In the end we were ending up
>unrefing an already destroyed object randomly later on (see bug #184581)
>
>This patchs makes sure the floating reference is sunk (giving real ownership
>to the GstStreamCollection in the end) for Gst < 1.14.
>
>Starting from 1.14, the ref is sunk in gst_stream_new and we will not do anything
>as we do not need to anymore.
>
>[1] commit f119e93b47efb06ffc68c01d3e094d5346c30041 `gst: Clear floating flag in constructor of all GstObject subclasses that are not owned by any parent`
>
>https://bugs.webkit.org/show_bug.cgi?id=185657
>---
> Source/WebCore/ChangeLog                      | 29 +++++++++++++++++++
> .../gstreamer/MediaPlayerPrivateGStreamer.cpp | 16 ++++++++--
> 2 files changed, 43 insertions(+), 2 deletions(-)
>
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 31c41ac4c5f..8966387a02d 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,32 @@
>+2018-05-15  Thibault Saunier  <tsaunier@igalia.com>
>+
>+        [GStreamer]: Do not steal GstStreamCollection's reference to GstStream
>+        https://bugs.webkit.org/show_bug.cgi?id=185657
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Before GStreamer 1.14[1] GstStream's reference was not sunk when creating
>+        it, meaning that when we were using
>+
>+            `GRefPtr<GstStream> stream = gst_stream_collection_get(collection, stream_index);`
>+
>+        we were actually calling `g_object_sink_ref(some_stream);` leading to taking
>+        ownership of that (floating) reference, while we were not actually owning it as it was
>+        owned by the GstStreamCollection itself. In the end we were ending up
>+        unrefing an already destroyed object randomly later on (see bug #184581)
>+
>+        This patchs makes sure the floating reference is sunk (giving real ownership
>+        to the GstStreamCollection in the end) for Gst < 1.14.
>+
>+        Starting from 1.14, the ref is sunk in gst_stream_new and we will not do anything
>+        as we do not need to anymore.
>+
>+        [1] commit f119e93b47efb06ffc68c01d3e094d5346c30041 `gst: Clear floating flag in constructor of all GstObject subclasses that are not owned by any parent`
>+
>+        * platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:
>+        (WebCore::MediaPlayerPrivateGStreamer::updateTracks):
>+        (WebCore::MediaPlayerPrivateGStreamer::handleMessage):
>+
> 2018-05-10  Thibault Saunier  <tsaunier@igalia.com>
> 
>         [GStreamer] Fix style issue in MediaPlayerPrivateGStreamerBase
>diff --git a/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp b/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp
>index ce0ab819e4c..5843dc0c1ba 100644
>--- a/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp
>+++ b/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp
>@@ -632,7 +632,14 @@ void MediaPlayerPrivateGStreamer::updateTracks()
>     Vector<String> validVideoStreams;
>     Vector<String> validTextStreams;
>     for (unsigned i = 0; i < length; i++) {
>-        GRefPtr<GstStream> stream = gst_stream_collection_get_stream(m_streamCollection.get(), i);
>+        GstStream * tmpstream = gst_stream_collection_get_stream(m_streamCollection.get(), i);
>+
>+        // Before Gst 1.14 the stream ref wasn't sunk on creation, meaning that we would
>+        // sink it and consider our own, while it is m_streamCollection' property.
>+        if (g_object_is_floating (tmpstream))
>+            gst_object_ref_sink(tmpstream);
>+
>+        GRefPtr<GstStream> stream = tmpstream;
>         String streamId(gst_stream_get_stream_id(stream.get()));
>         GstStreamType type = gst_stream_get_stream_type(stream.get());
>         GST_DEBUG("Inspecting %s track with ID %s", gst_stream_type_get_name(type), streamId.utf8().data());
>@@ -1323,9 +1330,14 @@ void MediaPlayerPrivateGStreamer::handleMessage(GstMessage* message)
> 
>         unsigned length = gst_message_streams_selected_get_size(message);
>         for (unsigned i = 0; i < length; i++) {
>-            GRefPtr<GstStream> stream = adoptGRef(gst_message_streams_selected_get_stream(message, i));
>+            auto tmpstream = gst_message_streams_selected_get_stream(message, i);
>+            if (g_object_is_floating (tmpstream))
>+                 gst_object_ref_sink(tmpstream);
>+
>+            GRefPtr<GstStream> stream = tmpstream;
>             if (!stream)
>                 continue;
>+
>             GstStreamType type = gst_stream_get_stream_type(stream.get());
>             String streamId(gst_stream_get_stream_id(stream.get()));
> 
>-- 
>2.17.0

Actions: View | Formatted Diff | Diff

Attachments on bug 185657: 340434 | 340435 | 340501 | 340505