Changeset 200455 in webkit

Timestamp:

May 5, 2016 6:03:46 AM (8 years ago)

Author:

Carlos Garcia Campos

Message:

[GStreamer] Adaptive streaming issues
​https://bugs.webkit.org/show_bug.cgi?id=144040

Reviewed by Philippe Normand.

In the case of adaptive streaming, the GST URI downloader object is creating the source object, in our case
WebKitWebSrc, without taking its ownership. This is breaking the lifetime of the WebKitWebSrc element. We are
using GRefPtr in WebKitWebSrc to ref/unref the object when sending notifications to the main thread, ensuring
that the object is not destroyed before the main thread dispatches the message. But our smart pointers are so
smart that in case of receiving a floating reference, it's converted to a full reference, so that the first time
we try to take a ref of a WebKitWebSrc having a floating reference we are actually taking the ownership
instead. When we try to release the reference, we are actuallty destroying the object, something that the actual
owner is not expecting and causing runtime critical warnings and very often web process crashes.

(WebKitWebProcess:6863): GStreamer-CRITICAL :
Trying to dispose element appsrc1, but it is in READY instead of the NULL state.
You need to explicitly set elements to the NULL state before
dropping the final reference, to allow them to clean up.
This problem may also be caused by a refcounting bug in the
application or some element.

(WebKitWebProcess:6863): GStreamer-CRITICAL : gst_uri_handler_get_uri: assertion 'GST_IS_URI_HANDLER(handler)' failed

(WebKitWebProcess:6863): GStreamer-CRITICAL : gst_uri_get_protocol: assertion 'uri != NULL' failed

This should be fixed in GST, but we can workaround it in WebKit while it's fixed in GST or to prevent this from
happening if other users make the same mistake. The idea is to add a ensureGRef() only available for GRefPtr
when using WebKitWebSrc objects that consumes the floating reference if needed before taking the actual reference.

• platform/graphics/gstreamer/GRefPtrGStreamer.cpp:

(WTF::ensureGRef): Consume the floating ref if needed.

• platform/graphics/gstreamer/GRefPtrGStreamer.h:
• platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:

(webKitWebSrcChangeState): Use ensureGRef().

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• platform/graphics/gstreamer/GRefPtrGStreamer.cpp (modified) (1 diff)
• platform/graphics/gstreamer/GRefPtrGStreamer.h (modified) (1 diff)
• platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp (modified) (5 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2016-05-05  Carlos Garcia Campos  <cgarcia@igalia.com>
+
+        [GStreamer] Adaptive streaming issues
+        https://bugs.webkit.org/show_bug.cgi?id=144040
+
+        Reviewed by Philippe Normand.
+
+        In the case of adaptive streaming, the GST URI downloader object is creating the source object, in our case
+        WebKitWebSrc, without taking its ownership. This is breaking the lifetime of the WebKitWebSrc element. We are
+        using GRefPtr in WebKitWebSrc to ref/unref the object when sending notifications to the main thread, ensuring
+        that the object is not destroyed before the main thread dispatches the message. But our smart pointers are so
+        smart that in case of receiving a floating reference, it's converted to a full reference, so that the first time
+        we try to take a ref of a WebKitWebSrc having a floating reference we are actually taking the ownership
+        instead. When we try to release the reference, we are actuallty destroying the object, something that the actual
+        owner is not expecting and causing runtime critical warnings and very often web process crashes.
+
+            (WebKitWebProcess:6863): GStreamer-CRITICAL **:
+            Trying to dispose element appsrc1, but it is in READY instead of the NULL state.
+            You need to explicitly set elements to the NULL state before
+            dropping the final reference, to allow them to clean up.
+            This problem may also be caused by a refcounting bug in the
+            application or some element.
+
+            (WebKitWebProcess:6863): GStreamer-CRITICAL **: gst_uri_handler_get_uri: assertion 'GST_IS_URI_HANDLER(handler)' failed
+
+            (WebKitWebProcess:6863): GStreamer-CRITICAL **: gst_uri_get_protocol: assertion 'uri != NULL' failed
+
+        This should be fixed in GST, but we can workaround it in WebKit while it's fixed in GST or to prevent this from
+        happening if other users make the same mistake. The idea is to add a ensureGRef() only available for GRefPtr
+        when using WebKitWebSrc objects that consumes the floating reference if needed before taking the actual reference.
+
+        * platform/graphics/gstreamer/GRefPtrGStreamer.cpp:
+        (WTF::ensureGRef): Consume the floating ref if needed.
+        * platform/graphics/gstreamer/GRefPtrGStreamer.h:
+        * platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:
+        (webKitWebSrcChangeState): Use ensureGRef().
+
 2016-05-05  Csaba Osztrogonác  <ossy@webkit.org>
 

trunk/Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.cpp

@@ -343 +343 @@
 }
 
+// This method is only available for WebKitWebSrc and should not be used for any other type.
+// This is only to work around a bug in GST where the URI downloader is not taking the ownership of WebKitWebSrc.
+// See https://bugs.webkit.org/show_bug.cgi?id=144040.
+GRefPtr<WebKitWebSrc> ensureGRef(WebKitWebSrc* ptr)
+{
+    if (ptr && g_object_is_floating(ptr))
+        gst_object_ref_sink(GST_OBJECT(ptr));
+    return GRefPtr<WebKitWebSrc>(ptr);
+}
+
 template <> WebKitWebSrc* refGPtr<WebKitWebSrc>(WebKitWebSrc* ptr)
 {

trunk/Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.h

@@ -109 +109 @@
 
 template<> GRefPtr<WebKitWebSrc> adoptGRef(WebKitWebSrc* ptr);
+GRefPtr<WebKitWebSrc> ensureGRef(WebKitWebSrc* ptr);
 template<> WebKitWebSrc* refGPtr<WebKitWebSrc>(WebKitWebSrc* ptr);
 template<> void derefGPtr<WebKitWebSrc>(WebKitWebSrc* ptr);

trunk/Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp

@@ -189 +189 @@
         }
 
-        GRefPtr<WebKitWebSrc> protector(src);
+        GRefPtr<WebKitWebSrc> protector = WTF::ensureGRef(src);
         priv->notifier.notify(MainThreadSourceNotification::NeedData, [protector] { webKitWebSrcNeedData(protector.get()); });
     },
@@ -203 +203 @@
         }
 
-        GRefPtr<WebKitWebSrc> protector(src);
+        GRefPtr<WebKitWebSrc> protector = WTF::ensureGRef(src);
         priv->notifier.notify(MainThreadSourceNotification::EnoughData, [protector] { webKitWebSrcEnoughData(protector.get()); });
     },
@@ -223 +223 @@
         }
 
-        GRefPtr<WebKitWebSrc> protector(src);
+        GRefPtr<WebKitWebSrc> protector = WTF::ensureGRef(src);
         priv->notifier.notify(MainThreadSourceNotification::Seek, [protector] { webKitWebSrcSeek(protector.get()); });
         return TRUE;
@@ -641 +641 @@
     {
         GST_DEBUG_OBJECT(src, "READY->PAUSED");
-        GRefPtr<WebKitWebSrc> protector(src);
+        GRefPtr<WebKitWebSrc> protector = WTF::ensureGRef(src);
         priv->notifier.notify(MainThreadSourceNotification::Start, [protector] { webKitWebSrcStart(protector.get()); });
         break;
@@ -649 +649 @@
         GST_DEBUG_OBJECT(src, "PAUSED->READY");
         priv->notifier.cancelPendingNotifications();
-        GRefPtr<WebKitWebSrc> protector(src);
+        GRefPtr<WebKitWebSrc> protector = WTF::ensureGRef(src);
         priv->notifier.notify(MainThreadSourceNotification::Stop, [protector] { webKitWebSrcStop(protector.get()); });
         break;