Merge r228639 - [GStreamer] Crash in WebCore::MediaPlayerRequestInsta…

…llMissingPluginsCallback::complete https://bugs.webkit.org/show_bug.cgi?id=166733 Reviewed by Philippe Normand. There are a couple of issues to tackle here. First is handling getting more than one missing plugin installation request at the same time. For this we add the request to a Vector and handle them there. Second is that if the player is dead and we still get the result, bad things happen. For that we "weaked" the pointer capture by the lambda. * platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp: (WebCore::MediaPlayerPrivateGStreamer::~MediaPlayerPrivateGStreamer): Handle Vector of callbacks. (WebCore::MediaPlayerPrivateGStreamer::handleMessage): Weak private player pointer and put the callback in the Vector. * platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h: Callback becomes Vector. * platform/graphics/gstreamer/MediaPlayerRequestInstallMissingPluginsCallback.h: (WebCore::MediaPlayerRequestInstallMissingPluginsCallback::create): (WebCore::MediaPlayerRequestInstallMissingPluginsCallback::complete): (WebCore::MediaPlayerRequestInstallMissingPluginsCallback::MediaPlayerRequestInstallMissingPluginsCallback): Callback function is refactored into a "using" type and added self as parameter to the function.
WebKit · Feb 20, 2018 · 9abad32 · 9abad32
1 parent a5a8a6a
commit 9abad32
Show file tree

Hide file tree

Showing 4 changed files with 56 additions and 14 deletions.
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,34 @@
+2018-02-19  Xabier Rodriguez Calvar  <calvaris@igalia.com>
+
+        [GStreamer] Crash in WebCore::MediaPlayerRequestInstallMissingPluginsCallback::complete
+        https://bugs.webkit.org/show_bug.cgi?id=166733
+
+        Reviewed by Philippe Normand.
+
+        There are a couple of issues to tackle here.
+
+        First is handling getting more than one missing plugin
+        installation request at the same time. For this we add the request
+        to a Vector and handle them there.
+
+        Second is that if the player is dead and we still get the result,
+        bad things happen. For that we "weaked" the pointer capture by the
+        lambda.
+
+        * platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:
+        (WebCore::MediaPlayerPrivateGStreamer::~MediaPlayerPrivateGStreamer):
+        Handle Vector of callbacks.
+        (WebCore::MediaPlayerPrivateGStreamer::handleMessage): Weak
+        private player pointer and put the callback in the Vector.
+        * platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:
+        Callback becomes Vector.
+        * platform/graphics/gstreamer/MediaPlayerRequestInstallMissingPluginsCallback.h:
+        (WebCore::MediaPlayerRequestInstallMissingPluginsCallback::create):
+        (WebCore::MediaPlayerRequestInstallMissingPluginsCallback::complete):
+        (WebCore::MediaPlayerRequestInstallMissingPluginsCallback::MediaPlayerRequestInstallMissingPluginsCallback):
+        Callback function is refactored into a "using" type and added self
+        as parameter to the function.
+
 2018-02-19  Fujii Hironori  <Hironori.Fujii@sony.com>
 
         REGRESSION(r219298): RELEASE_ASSERT(!m_owningPointerForClose) fails in WebCore::IDBServer::UniqueIDBDatabase::scheduleShutdownForClose

diff --git a/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp b/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp
@@ -191,10 +191,11 @@ MediaPlayerPrivateGStreamer::~MediaPlayerPrivateGStreamer()
             reinterpret_cast<gpointer>(setAudioStreamPropertiesCallback), this);
 
     m_readyTimerHandler.stop();
-    if (m_missingPluginsCallback) {
-        m_missingPluginsCallback->invalidate();
-        m_missingPluginsCallback = nullptr;
+    for (auto& missingPluginCallback : m_missingPluginCallbacks) {
+        if (missingPluginCallback)
+            missingPluginCallback->invalidate();
     }
+    m_missingPluginCallbacks.clear();
 
     if (m_videoSink) {
         GRefPtr<GstPad> videoSinkPad = adoptGRef(gst_element_get_static_pad(m_videoSink.get(), "sink"));
@@ -941,7 +942,7 @@ void MediaPlayerPrivateGStreamer::handleMessage(GstMessage* message)
     GST_LOG("Message %s received from element %s", GST_MESSAGE_TYPE_NAME(message), GST_MESSAGE_SRC_NAME(message));
     switch (GST_MESSAGE_TYPE(message)) {
     case GST_MESSAGE_ERROR:
-        if (m_resetPipeline || m_missingPluginsCallback || m_errorOccured)
+        if (m_resetPipeline || !m_missingPluginCallbacks.isEmpty() || m_errorOccured)
             break;
         gst_message_parse_error(message, &err.outPtr(), &debug.outPtr());
         GST_ERROR("Error %d: %s (url=%s)", err->code, err->message, m_url.string().utf8().data());
@@ -1037,17 +1038,25 @@ void MediaPlayerPrivateGStreamer::handleMessage(GstMessage* message)
     case GST_MESSAGE_ELEMENT:
         if (gst_is_missing_plugin_message(message)) {
             if (gst_install_plugins_supported()) {
-                m_missingPluginsCallback = MediaPlayerRequestInstallMissingPluginsCallback::create([this](uint32_t result) {
-                    m_missingPluginsCallback = nullptr;
+                RefPtr<MediaPlayerRequestInstallMissingPluginsCallback> missingPluginCallback = MediaPlayerRequestInstallMissingPluginsCallback::create([weakThis = createWeakPtr()](uint32_t result, MediaPlayerRequestInstallMissingPluginsCallback& missingPluginCallback) {
+                    if (!weakThis) {
+                        GST_INFO("got missing pluging installation callback in destroyed player with result %u", result);
+                        return;
+                    }
+
+                    GST_DEBUG("got missing plugin installation callback with result %u", result);
+                    RefPtr<MediaPlayerRequestInstallMissingPluginsCallback> protectedMissingPluginCallback = &missingPluginCallback;
+                    weakThis->m_missingPluginCallbacks.removeFirst(protectedMissingPluginCallback);
                     if (result != GST_INSTALL_PLUGINS_SUCCESS)
                         return;
 
-                    changePipelineState(GST_STATE_READY);
-                    changePipelineState(GST_STATE_PAUSED);
+                    weakThis->changePipelineState(GST_STATE_READY);
+                    weakThis->changePipelineState(GST_STATE_PAUSED);
                 });
+                m_missingPluginCallbacks.append(missingPluginCallback);
                 GUniquePtr<char> detail(gst_missing_plugin_message_get_installer_detail(message));
                 GUniquePtr<char> description(gst_missing_plugin_message_get_description(message));
-                m_player->client().requestInstallMissingPlugins(String::fromUTF8(detail.get()), String::fromUTF8(description.get()), *m_missingPluginsCallback);
+                m_player->client().requestInstallMissingPlugins(String::fromUTF8(detail.get()), String::fromUTF8(description.get()), *missingPluginCallback);
             }
         }
 #if ENABLE(VIDEO_TRACK) && USE(GSTREAMER_MPEGTS)

diff --git a/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h b/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h
@@ -252,7 +252,7 @@ class MediaPlayerPrivateGStreamer : public MediaPlayerPrivateGStreamerBase {
 #endif
     GRefPtr<GstElement> m_autoAudioSink;
     GRefPtr<GstElement> m_downloadBuffer;
-    RefPtr<MediaPlayerRequestInstallMissingPluginsCallback> m_missingPluginsCallback;
+    Vector<RefPtr<MediaPlayerRequestInstallMissingPluginsCallback>> m_missingPluginCallbacks;
 #if ENABLE(VIDEO_TRACK)
     Vector<RefPtr<AudioTrackPrivateGStreamer>> m_audioTracks;
     Vector<RefPtr<InbandTextTrackPrivateGStreamer>> m_textTracks;

diff --git a/Source/WebCore/platform/graphics/gstreamer/MediaPlayerRequestInstallMissingPluginsCallback.h b/Source/WebCore/platform/graphics/gstreamer/MediaPlayerRequestInstallMissingPluginsCallback.h
@@ -29,7 +29,9 @@ namespace WebCore {
 class MediaPlayerRequestInstallMissingPluginsCallback : public RefCounted<MediaPlayerRequestInstallMissingPluginsCallback> {
     WTF_MAKE_FAST_ALLOCATED();
 public:
-    static Ref<MediaPlayerRequestInstallMissingPluginsCallback> create(WTF::Function<void (uint32_t)>&& function)
+    using MediaPlayerRequestInstallMissingPluginsCallbackFunction = std::function<void(uint32_t, MediaPlayerRequestInstallMissingPluginsCallback&)>;
+
+    static Ref<MediaPlayerRequestInstallMissingPluginsCallback> create(MediaPlayerRequestInstallMissingPluginsCallbackFunction&& function)
     {
         return adoptRef(*new MediaPlayerRequestInstallMissingPluginsCallback(WTFMove(function)));
     }
@@ -43,17 +45,17 @@ class MediaPlayerRequestInstallMissingPluginsCallback : public RefCounted<MediaP
     {
         if (!m_function)
             return;
-        m_function(result);
+        m_function(result, *this);
         m_function = nullptr;
     }
 
 private:
-    MediaPlayerRequestInstallMissingPluginsCallback(WTF::Function<void (uint32_t)>&& function)
+    MediaPlayerRequestInstallMissingPluginsCallback(MediaPlayerRequestInstallMissingPluginsCallbackFunction&& function)
         : m_function(WTFMove(function))
     {
     }
 
-    WTF::Function<void (uint32_t)> m_function;
+    MediaPlayerRequestInstallMissingPluginsCallbackFunction m_function;
 };
 
 } // namespace WebCore