Attachment 340501 Details for Bug 185657 – Fix the issue in GRefPtrGStreamer.cpp directly.

[patch] Fix the issue in GRefPtrGStreamer.cpp directly.

GStreamer-Do-not-steal-GstStreamCollections-refere.patch (text/plain), 5.12 KB, created by Thibault Saunier on 2018-05-16 11:11:10 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Thibault Saunier

Created: 2018-05-16 11:11:10 PDT

Size: 5.12 KB

patch

obsolete

>From 39ed79e9a2b526e7268e477b2c030b69dbf75af3 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                      | 23 +++++++++++++++++++
> .../graphics/gstreamer/GRefPtrGStreamer.cpp   |  6 ++---
> .../gstreamer/MediaPlayerPrivateGStreamer.cpp |  3 ++-
> 3 files changed, 27 insertions(+), 5 deletions(-)
>
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 31c41ac4c5f..c224a49362e 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,26 @@
>+2018-05-15  Thibault Saunier  <tsaunier@igalia.com>
>+
>+        [GStreamer]: Consider GstStream(Collection) as if they were not GInitiallyUnowned
>+        https://bugs.webkit.org/show_bug.cgi?id=185657
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Starting with GStreamer 1.14[1] the reference is sunk at creation making it a normal
>+        GObject in practice, in our GRefPtr template implementation we should just consider GstStream
>+        as if it was a normal GObject and not care about its floating reference state as
>+        the ref is never sunk by anyone before 1.12 though that floating ref is considered
>+        as a hard reference by the GstStreamCollection.
>+
>+        The exact same issue happens with GstStreamCollection, so fix it there too.
>+
>+        Also do not adoptRef() on the result of gst_stream_collection_get_stream() as this function
>+        is transfer-none.
>+
>+        [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::handleMessage):
>+
> 2018-05-10  Thibault Saunier  <tsaunier@igalia.com>
> 
>         [GStreamer] Fix style issue in MediaPlayerPrivateGStreamerBase
>diff --git a/Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.cpp b/Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.cpp
>index f1cbad7e05d..26014d2ec19 100644
>--- a/Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.cpp
>+++ b/Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.cpp
>@@ -356,14 +356,13 @@ template <> void derefGPtr<GstQuery>(GstQuery* ptr)
> #if GST_CHECK_VERSION(1, 10, 0)
> template <> GRefPtr<GstStream> adoptGRef(GstStream* ptr)
> {
>-    ASSERT(!ptr || !g_object_is_floating(ptr));
>     return GRefPtr<GstStream>(ptr, GRefPtrAdopt);
> }
> 
> template <> GstStream* refGPtr<GstStream>(GstStream* ptr)
> {
>     if (ptr)
>-        gst_object_ref_sink(GST_OBJECT_CAST(ptr));
>+        gst_object_ref(GST_OBJECT_CAST(ptr));
> 
>     return ptr;
> }
>@@ -376,14 +375,13 @@ template <> void derefGPtr<GstStream>(GstStream* ptr)
> 
> template <> GRefPtr<GstStreamCollection> adoptGRef(GstStreamCollection* ptr)
> {
>-    ASSERT(!ptr || !g_object_is_floating(ptr));
>     return GRefPtr<GstStreamCollection>(ptr, GRefPtrAdopt);
> }
> 
> template <> GstStreamCollection* refGPtr<GstStreamCollection>(GstStreamCollection* ptr)
> {
>     if (ptr)
>-        gst_object_ref_sink(GST_OBJECT_CAST(ptr));
>+        gst_object_ref(GST_OBJECT_CAST(ptr));
> 
>     return ptr;
> }
>diff --git a/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp b/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp
>index ce0ab819e4c..8822271e78c 100644
>--- a/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp
>+++ b/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp
>@@ -1323,9 +1323,10 @@ 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));
>+            GRefPtr<GstStream> stream = gst_message_streams_selected_get_stream(message, i);
>             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