From 3ed0ee95f2d87c34d93aa445b3bed81cf20e023b Mon Sep 17 00:00:00 2001
From: Matthew Waters <matthew@centricular.com>
Date: Fri, 27 Nov 2020 16:18:29 +1100
Subject: [PATCH] wpesrc: fix possible small deadlock on shutdown

Problem is that unreffing the EGLImage/SHM Buffer while holding the
images_mutex lock may deadlock when a new buffer is advertised and
an attempt is made to lock the images_mutex there.

The advertisement of the new image/buffer is performed in the
WPEContextThread and the blocking dispatch when unreffing wants to run
something on the WPEContextThread however images_mutex has already been
locked by the destructor.

Delay unreffing images/buffers outside of images_mutex and instead just
clear the relevant fields within the lock.

Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/-/merge_requests/1843>
---
 ext/wpe/WPEThreadedView.cpp | 37 +++++++++++++++++++++++++++----------
 1 file changed, 27 insertions(+), 10 deletions(-)

diff --git a/ext/wpe/WPEThreadedView.cpp b/ext/wpe/WPEThreadedView.cpp
index 9af1e2db4f..255fac6369 100644
--- a/ext/wpe/WPEThreadedView.cpp
+++ b/ext/wpe/WPEThreadedView.cpp
@@ -292,6 +292,10 @@ WPEView::WPEView(WebKitWebContext* web_context, GstWpeSrc* src, GstGLContext* co
 
 WPEView::~WPEView()
 {
+    GstEGLImage *egl_pending = NULL;
+    GstEGLImage *egl_committed = NULL;
+    GstBuffer *shm_pending = NULL;
+    GstBuffer *shm_committed = NULL;
     GST_TRACE ("%p destroying", this);
 
     g_mutex_clear(&threading.ready_mutex);
@@ -301,25 +305,34 @@ WPEView::~WPEView()
         GMutexHolder lock(images_mutex);
 
         if (egl.pending) {
-            gst_egl_image_unref(egl.pending);
+            egl_pending = egl.pending;
             egl.pending = nullptr;
         }
         if (egl.committed) {
-            gst_egl_image_unref(egl.committed);
+            egl_committed = egl.committed;
             egl.committed = nullptr;
         }
         if (shm.pending) {
             GST_TRACE ("%p freeing shm pending %" GST_PTR_FORMAT, this, shm.pending);
-            gst_buffer_unref(shm.pending);
+            shm_pending = shm.pending;
             shm.pending = nullptr;
         }
         if (shm.committed) {
             GST_TRACE ("%p freeing shm commited %" GST_PTR_FORMAT, this, shm.committed);
-            gst_buffer_unref(shm.committed);
+            shm_committed = shm.committed;
             shm.committed = nullptr;
         }
     }
 
+    if (egl_pending)
+        gst_egl_image_unref (egl_pending);
+    if (egl_committed)
+        gst_egl_image_unref (egl_committed);
+    if (shm_pending)
+        gst_buffer_unref (shm_pending);
+    if (shm_committed)
+        gst_buffer_unref (shm_committed);
+
     WPEContextThread::singleton().dispatch([&]() {
         if (webkit.view) {
             g_object_unref(webkit.view);
@@ -370,6 +383,7 @@ GstEGLImage* WPEView::image()
 {
     GstEGLImage* ret = nullptr;
     bool dispatchFrameComplete = false;
+    GstEGLImage *prev_image = NULL;
 
     {
         GMutexHolder lock(images_mutex);
@@ -380,12 +394,10 @@ GstEGLImage* WPEView::image()
                   GST_IS_EGL_IMAGE(egl.committed) ? GST_MINI_OBJECT_REFCOUNT_VALUE(GST_MINI_OBJECT_CAST(egl.committed)) : 0);
 
         if (egl.pending) {
-            auto* previousImage = egl.committed;
+            prev_image = egl.committed;
             egl.committed = egl.pending;
             egl.pending = nullptr;
 
-            if (previousImage)
-                gst_egl_image_unref(previousImage);
             dispatchFrameComplete = true;
         }
 
@@ -393,6 +405,9 @@ GstEGLImage* WPEView::image()
             ret = egl.committed;
     }
 
+    if (prev_image)
+        gst_egl_image_unref(prev_image);
+
     if (dispatchFrameComplete)
         frameComplete();
 
@@ -403,6 +418,7 @@ GstBuffer* WPEView::buffer()
 {
     GstBuffer* ret = nullptr;
     bool dispatchFrameComplete = false;
+    GstBuffer *prev_image = NULL;
 
     {
         GMutexHolder lock(images_mutex);
@@ -413,12 +429,10 @@ GstBuffer* WPEView::buffer()
                   GST_IS_BUFFER(shm.committed) ? GST_MINI_OBJECT_REFCOUNT_VALUE(GST_MINI_OBJECT_CAST(shm.committed)) : 0);
 
         if (shm.pending) {
-            auto* previousImage = shm.committed;
+            prev_image = shm.committed;
             shm.committed = shm.pending;
             shm.pending = nullptr;
 
-            if (previousImage)
-                gst_buffer_unref(previousImage);
             dispatchFrameComplete = true;
         }
 
@@ -426,6 +440,9 @@ GstBuffer* WPEView::buffer()
             ret = shm.committed;
     }
 
+    if (prev_image)
+        gst_buffer_unref(prev_image);
+
     if (dispatchFrameComplete)
         frameComplete();