Changeset 228639 in webkit

Timestamp:

Feb 19, 2018 2:47:49 AM (6 years ago)

Author:

calvaris@igalia.com

Message:

[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.

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp (modified) (3 diffs)
• platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h (modified) (1 diff)
• platform/graphics/gstreamer/MediaPlayerRequestInstallMissingPluginsCallback.h (modified) (2 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+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  Philippe Normand  <pnormand@igalia.com>
 

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

@@ -192 +192 @@
 
     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) {
@@ -1108 +1109 @@
     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());
@@ -1204 +1205 @@
         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);
             }
         }

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

@@ -269 +269 @@
     GRefPtr<GstElement> m_autoAudioSink;
     GRefPtr<GstElement> m_downloadBuffer;
-    RefPtr<MediaPlayerRequestInstallMissingPluginsCallback> m_missingPluginsCallback;
+    Vector<RefPtr<MediaPlayerRequestInstallMissingPluginsCallback>> m_missingPluginCallbacks;
 #if ENABLE(VIDEO_TRACK)
     HashMap<AtomicString, RefPtr<AudioTrackPrivateGStreamer>> m_audioTracks;

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

@@ -30 +30 @@
     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)));
@@ -44 +46 @@
         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;
 };