[GStreamer] Crash after 10 seconds on watchdog thread do to loop when…

… destroying ~ImageDecoderGStreamerSample

https://bugs.webkit.org/show_bug.cgi?id=264824

Reviewed by Xabier Rodriguez-Calvar.

The WebProcess was deadlocking at shutdown when the disposal of the element harness of the image
decoder was triggering disposal of chained harnesses, within the same thread, through the
pad-removed signal emission. The proposed solution is to use a recursive mutex for active pipelines
storage protection.

This patch also fixes a few more leaks in ImageDecoder and the element harness.

* Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp:
(WebCore::deinitializeGStreamer):
* Source/WebCore/platform/graphics/gstreamer/ImageDecoderGStreamer.cpp:
(WebCore::teardownGStreamerImageDecoders):
(WebCore::ImageDecoderGStreamer::create):
(WebCore::ImageDecoderGStreamer::ImageDecoderGStreamer):
(WebCore::ImageDecoderGStreamer::tearDown):
(WebCore::ImageDecoderGStreamer::pushEncodedData):
* Source/WebCore/platform/graphics/gstreamer/ImageDecoderGStreamer.h:
* Source/WebCore/platform/gstreamer/GStreamerElementHarness.cpp:
(WebCore::GStreamerElementHarness::GStreamerElementHarness):
(WebCore::GStreamerElementHarness::~GStreamerElementHarness):
(WebCore::GStreamerElementHarness::Stream::Stream):
(WebCore::GStreamerElementHarness::Stream::~Stream):
(WebCore::GStreamerElementHarness::Stream::sinkEvent):
(WebCore::GStreamerElementHarness::srcEvent):
* Source/WebCore/platform/gstreamer/GStreamerElementHarness.h:

Canonical link: https://commits.webkit.org/275032@main

Source/WebCore/platform/graphics/gstreamer/GStreamerCommon.cpp

@@ -43,6 +43,7 @@
 #include <wtf/HashMap.h>
 #include <wtf/NeverDestroyed.h>
 #include <wtf/PrintStream.h>
+#include <wtf/RecursiveLockAdapter.h>
 #include <wtf/Scope.h>
 #include <wtf/glib/GThreadSafeWeakPtr.h>
 #include <wtf/glib/GUniquePtr.h>
@@ -76,6 +77,7 @@
 #endif
 
 #if ENABLE(VIDEO)
+#include "ImageDecoderGStreamer.h"
 #include "VideoEncoderPrivateGStreamer.h"
 #include "WebKitWebSourceGStreamer.h"
 #endif
@@ -461,7 +463,9 @@ void registerWebKitGStreamerVideoEncoder()
         GStreamerRegistryScanner::singleton().refresh();
 }
 
-static Lock s_activePipelinesMapLock;
+// We use a recursive lock because the removal of a pipeline can trigger the removal of another one,
+// from the same thread, specially when using chained element harnesses.
+static RecursiveLock s_activePipelinesMapLock;
 static HashMap<String, GRefPtr<GstElement>>& activePipelinesMap()
 {
     static NeverDestroyed<HashMap<String, GRefPtr<GstElement>>> activePipelines;
@@ -497,6 +501,7 @@ void deinitializeGStreamer()
     teardownGStreamerRegistryScanner();
 #if ENABLE(VIDEO)
     teardownVideoEncoderSingleton();
+    teardownGStreamerImageDecoders();
 #endif
 
     bool isLeaksTracerActive = false;

Source/WebCore/platform/graphics/gstreamer/ImageDecoderGStreamer.cpp

@@ -38,6 +38,17 @@ namespace WebCore {
 GST_DEBUG_CATEGORY(webkit_image_decoder_debug);
 #define GST_CAT_DEFAULT webkit_image_decoder_debug
 
+static Lock s_decoderLock;
+static Vector<RefPtr<ImageDecoderGStreamer>> s_imageDecoders;
+
+void teardownGStreamerImageDecoders()
+{
+    Locker lock { s_decoderLock };
+    for (auto& decoder : s_imageDecoders)
+        decoder->tearDown();
+    s_imageDecoders.clear();
+}
+
 class ImageDecoderGStreamerSample final : public MediaSampleGStreamer {
 public:
     static Ref<ImageDecoderGStreamerSample> create(GRefPtr<GstSample>&& sample, const FloatSize& presentationSize)
@@ -51,7 +62,11 @@ class ImageDecoderGStreamerSample final : public MediaSampleGStreamer {
             return nullptr;
         return m_image->image().nativeImage()->platformImage();
     }
-    void dropImage() { m_image = nullptr; }
+    void dropImage()
+    {
+        m_image = nullptr;
+        m_frame = nullptr;
+    }
 
     SampleFlags flags() const override
     {
@@ -62,7 +77,7 @@ class ImageDecoderGStreamerSample final : public MediaSampleGStreamer {
     ImageDecoderGStreamerSample(GRefPtr<GstSample>&& sample, const FloatSize& presentationSize)
         : MediaSampleGStreamer(WTFMove(sample), presentationSize, { })
     {
-        m_frame = VideoFrameGStreamer::createWrappedSample(platformSample().sample.gstSample);
+        m_frame = VideoFrameGStreamer::create(GRefPtr(platformSample().sample.gstSample), presentationSize);
         m_image = m_frame->convertToImage();
     }
 
@@ -83,7 +98,12 @@ ImageDecoderGStreamerSample* toSample(Iterator iter)
 
 RefPtr<ImageDecoderGStreamer> ImageDecoderGStreamer::create(FragmentedSharedBuffer& data, const String& mimeType, AlphaOption alphaOption, GammaAndColorProfileOption gammaAndColorProfileOption)
 {
-    return adoptRef(*new ImageDecoderGStreamer(data, mimeType, alphaOption, gammaAndColorProfileOption));
+    RefPtr decoder = adoptRef(*new ImageDecoderGStreamer(data, mimeType, alphaOption, gammaAndColorProfileOption));
+    {
+        Locker lock { s_decoderLock };
+        s_imageDecoders.append(decoder);
+    }
+    return decoder;
 }
 
 ImageDecoderGStreamer::ImageDecoderGStreamer(FragmentedSharedBuffer& data, const String& mimeType, AlphaOption, GammaAndColorProfileOption)
@@ -121,13 +141,25 @@ ImageDecoderGStreamer::ImageDecoderGStreamer(FragmentedSharedBuffer& data, const
         configureVideoDecoderForHarnessing(element);
         m_decoderHarness = GStreamerElementHarness::create(WTFMove(element), [this](auto&, auto&& outputSample) {
             storeDecodedSample(WTFMove(outputSample));
-        }, { });
+        });
         return m_decoderHarness;
     });
 
     pushEncodedData(data);
 }
 
+ImageDecoderGStreamer::~ImageDecoderGStreamer()
+{
+    tearDown();
+}
+
+void ImageDecoderGStreamer::tearDown()
+{
+    m_sampleData.clear();
+    m_decoderHarness = nullptr;
+    m_parserHarness = nullptr;
+}
+
 bool ImageDecoderGStreamer::supportsContainerType(const String& type)
 {
     // Ideally this decoder should operate only from the WebProcess (or from the GPUProcess) which
@@ -316,7 +348,7 @@ void ImageDecoderGStreamer::pushEncodedData(const FragmentedSharedBuffer& shared
         }
     }
 
-    m_decoderHarness->flush();
+    m_decoderHarness->reset();
 }
 
 #undef GST_CAT_DEFAULT

Source/WebCore/platform/graphics/gstreamer/ImageDecoderGStreamer.h

@@ -35,13 +35,15 @@ namespace WebCore {
 class ContentType;
 class ImageDecoderGStreamerSample;
 
+void teardownGStreamerImageDecoders();
+
 class ImageDecoderGStreamer final : public ImageDecoder {
     WTF_MAKE_FAST_ALLOCATED;
     WTF_MAKE_NONCOPYABLE(ImageDecoderGStreamer);
 public:
     static RefPtr<ImageDecoderGStreamer> create(FragmentedSharedBuffer&, const String& mimeType, AlphaOption, GammaAndColorProfileOption);
     ImageDecoderGStreamer(FragmentedSharedBuffer&, const String& mimeType, AlphaOption, GammaAndColorProfileOption);
-    virtual ~ImageDecoderGStreamer() = default;
+    ~ImageDecoderGStreamer();
 
     static bool supportsMediaType(MediaType type) { return type == MediaType::Video; }
     static bool supportsContainerType(const String&);
@@ -74,6 +76,8 @@ class ImageDecoderGStreamer final : public ImageDecoder {
     bool isAllDataReceived() const final { return m_eos; }
     void clearFrameBufferCache(size_t) final;
 
+    void tearDown();
+
 private:
     void pushEncodedData(const FragmentedSharedBuffer&);
     void storeDecodedSample(GRefPtr<GstSample>&&);

Source/WebCore/platform/gstreamer/GStreamerElementHarness.cpp

@@ -151,7 +151,7 @@ GStreamerElementHarness::GStreamerElementHarness(GRefPtr<GstElement>&& element,
     }), this, nullptr);
     gst_pad_set_event_function_full(m_srcPad.get(), reinterpret_cast<GstPadEventFunction>(+[](GstPad* pad, GstObject*, GstEvent* event) -> gboolean {
         auto& harness = *reinterpret_cast<GStreamerElementHarness*>(pad->eventdata);
-        return harness.srcEvent(event);
+        return harness.srcEvent(adoptGRef(event));
     }), this, nullptr);
 
     gst_pad_set_active(m_srcPad.get(), TRUE);
@@ -162,6 +162,7 @@ GStreamerElementHarness::GStreamerElementHarness(GRefPtr<GstElement>&& element,
 GStreamerElementHarness::~GStreamerElementHarness()
 {
     GST_DEBUG_OBJECT(m_element.get(), "Stopping harness");
+    g_signal_handlers_disconnect_by_data(m_element.get(), this);
     pushEvent(adoptGRef(gst_event_new_eos()));
     unregisterPipeline(m_element);
     gst_pad_set_active(m_srcPad.get(), FALSE);
@@ -302,7 +303,7 @@ GStreamerElementHarness::Stream::Stream(GRefPtr<GstPad>&& pad, RefPtr<GStreamerE
     }),  this, nullptr);
     gst_pad_set_event_function_full(m_targetPad.get(), reinterpret_cast<GstPadEventFunction>(+[](GstPad* pad, GstObject*, GstEvent* event) -> gboolean {
         auto& stream = *reinterpret_cast<GStreamerElementHarness::Stream*>(pad->eventdata);
-        return stream.sinkEvent(event);
+        return stream.sinkEvent(adoptGRef(event));
     }), this, nullptr);
 
     gst_pad_set_active(m_targetPad.get(), TRUE);
@@ -316,6 +317,16 @@ GStreamerElementHarness::Stream::~Stream()
     gst_pad_set_chain_function(m_targetPad.get(), nullptr);
     gst_pad_set_event_function(m_targetPad.get(), nullptr);
     gst_pad_set_query_function(m_targetPad.get(), nullptr);
+
+    {
+        Locker locker { m_sampleQueueLock };
+        m_sampleQueue.clear();
+    }
+    {
+        Locker locker { m_sinkEventQueueLock };
+        m_sinkEventQueue.clear();
+    }
+    m_downstreamHarness = nullptr;
 }
 
 GRefPtr<GstSample> GStreamerElementHarness::Stream::pullSample()
@@ -365,16 +376,16 @@ GstFlowReturn GStreamerElementHarness::Stream::chainSample(GRefPtr<GstSample>&&
     return GST_FLOW_OK;
 }
 
-bool GStreamerElementHarness::Stream::sinkEvent(GstEvent* event)
+bool GStreamerElementHarness::Stream::sinkEvent(GRefPtr<GstEvent>&& event)
 {
     Locker locker { m_sinkEventQueueLock };
 
     // Clear cached output caps when the pad receives a new caps event or stream-start (potentially
     // storing a GstStream).
-    if (GST_EVENT_TYPE(event) == GST_EVENT_CAPS || GST_EVENT_TYPE(event) == GST_EVENT_STREAM_START)
+    if (GST_EVENT_TYPE(event.get()) == GST_EVENT_CAPS || GST_EVENT_TYPE(event.get()) == GST_EVENT_STREAM_START)
         m_outputCaps = nullptr;
 
-    m_sinkEventQueue.prepend(GRefPtr<GstEvent>(event));
+    m_sinkEventQueue.prepend(WTFMove(event));
     return true;
 }
 
@@ -405,11 +416,11 @@ bool GStreamerElementHarness::srcQuery(GstPad* pad, GstObject* parent, GstQuery*
     return result;
 }
 
-bool GStreamerElementHarness::srcEvent(GstEvent* event)
+bool GStreamerElementHarness::srcEvent(GRefPtr<GstEvent>&& event)
 {
-    GST_TRACE_OBJECT(m_element.get(), "Got event on src pad: %" GST_PTR_FORMAT, event);
+    GST_TRACE_OBJECT(m_element.get(), "Got event on src pad: %" GST_PTR_FORMAT, event.get());
     Locker locker { m_srcEventQueueLock };
-    m_srcEventQueue.prepend(GRefPtr<GstEvent>(event));
+    m_srcEventQueue.prepend(WTFMove(event));
     return true;
 }
 

Source/WebCore/platform/gstreamer/GStreamerElementHarness.h

@@ -61,7 +61,7 @@ class GStreamerElementHarness : public ThreadSafeRefCounted<GStreamerElementHarn
         Stream(GRefPtr<GstPad>&&, RefPtr<GStreamerElementHarness>&&);
 
         GstFlowReturn chainSample(GRefPtr<GstSample>&&);
-        bool sinkEvent(GstEvent*);
+        bool sinkEvent(GRefPtr<GstEvent>&&);
 
         GRefPtr<GstPad> m_pad;
         RefPtr<GStreamerElementHarness> m_downstreamHarness;
@@ -111,7 +111,7 @@ class GStreamerElementHarness : public ThreadSafeRefCounted<GStreamerElementHarn
     GstFlowReturn pushBufferFull(GRefPtr<GstBuffer>&&);
 
     bool srcQuery(GstPad*, GstObject*, GstQuery*);
-    bool srcEvent(GstEvent*);
+    bool srcEvent(GRefPtr<GstEvent>&&);
 
     void pushStickyEvents(GRefPtr<GstCaps>&&, std::optional<const GstSegment*>&& = { });
     void pushSegmentEvent(std::optional<const GstSegment*>&& = { });