Attachment 369484 Details for Bug 197558 – Patch

[patch] Patch

bug-197558-20190509123108.patch (text/plain), 16.96 KB, created by Philippe Normand on 2019-05-09 04:31:09 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Philippe Normand

Created: 2019-05-09 04:31:09 PDT

Size: 16.96 KB

patch

obsolete

>Subversion Revision: 245126
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 31d3917a0b6ecff2955871d24d4162e119dbb344..70fd38675b05d4ebfd0c5989ba26a9de4d2886d2 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,49 @@
>+2019-05-09  Philippe Normand  <pnormand@igalia.com>
>+
>+        REGRESSION(r243197): [GStreamer] Web process hangs when scrolling twitter timeline which contains HLS videos
>+        https://bugs.webkit.org/show_bug.cgi?id=197558
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        The deadlock was due to the player disposal running and blocking
>+        the main thread while the webkitwebsrc element used to download
>+        fragments requires the main thread as well to close its HTTP
>+        session. The proposed solution is to ask all webkitwebsrc elements
>+        to close their session before disposing of the whole pipeline.
>+
>+        This patch also includes GStreamer logging support for the
>+        MainThreadNotifier. Whenever possible the notifier uses a
>+        reference of the GStreamer object that is most likely associated
>+        with it, to use debugging.
>+
>+        * platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp:
>+        (WebCore::AudioSourceProviderGStreamer::AudioSourceProviderGStreamer):
>+        Associate notifier with the MediaStream pipeline.
>+        * platform/graphics/gstreamer/MainThreadNotifier.h: Add logging
>+        support. Usual GST_TRACE macros can't be used here because we
>+        can't safely assume GST_CAT_DEFAULT is defined, because this is a
>+        header file.
>+        * platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:
>+        (WebCore::MediaPlayerPrivateGStreamerBase::~MediaPlayerPrivateGStreamerBase):
>+        Close HTTP sessions before tearing down the whole pipeline.
>+        (WebCore::MediaPlayerPrivateGStreamerBase::setPipeline): Associate
>+        notifier with the playback pipeline.
>+        * platform/graphics/gstreamer/TrackPrivateBaseGStreamer.cpp:
>+        (WebCore::TrackPrivateBaseGStreamer::TrackPrivateBaseGStreamer):
>+        Associate notifier with pad or GstStream, depending on the runtime
>+        use-case.
>+        * platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:
>+        (webkit_web_src_class_init): Add an event handler.
>+        (webkit_web_src_init): Associate notifier with the src element.
>+        (webKitWebSrcCloseSession): Cancel pending notifications and
>+        return early if the HTTP session was already closed.
>+        (webKitWebSrcEvent): Handle custom upstream webkit-close-http-session events.
>+        (webKitWebSrcStop): Session is now closed from the event handler.
>+        (webKitWebSrcUnLock): Cancel pending notifications, might avoid
>+        potential deadlocks as well.
>+        * platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:
>+        (webkit_media_src_init): Associate notifier with the src element.
>+
> 2019-05-09  Antti Koivisto  <antti@apple.com>
> 
>         Elements with "display: inline-block" don't have a touch-action region
>diff --git a/Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp b/Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp
>index 54b39f45a5ba700ee0e9baa16536d21884ed10f8..e5e2b5a0b84fe71ecdeb96c032580dccd90116de 100644
>--- a/Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp
>+++ b/Source/WebCore/platform/audio/gstreamer/AudioSourceProviderGStreamer.cpp
>@@ -111,6 +111,7 @@ AudioSourceProviderGStreamer::AudioSourceProviderGStreamer(MediaStreamTrackPriva
>     m_frontRightAdapter = gst_adapter_new();
>     auto pipelineName = makeString("WebAudioProvider_MediaStreamTrack_", source.id());
>     m_pipeline = adoptGRef(GST_ELEMENT(g_object_ref_sink(gst_element_factory_make("pipeline", pipelineName.utf8().data()))));
>+    m_notifier->setGstObject(GST_OBJECT_CAST(m_pipeline.get()));
>     auto src = webkitMediaStreamSrcNew();
>     webkitMediaStreamSrcAddTrack(WEBKIT_MEDIA_STREAM_SRC(src), &source, true);
> 
>diff --git a/Source/WebCore/platform/graphics/gstreamer/MainThreadNotifier.h b/Source/WebCore/platform/graphics/gstreamer/MainThreadNotifier.h
>index 3548989216d565d9bbbde0850c190dad0949f115..93a97aa6bba2365db8282e33e54930c1dd2e480f 100644
>--- a/Source/WebCore/platform/graphics/gstreamer/MainThreadNotifier.h
>+++ b/Source/WebCore/platform/graphics/gstreamer/MainThreadNotifier.h
>@@ -19,12 +19,17 @@
> #pragma once
> 
> #include <functional>
>+#include <gst/gst.h>
> #include <wtf/Atomics.h>
> #include <wtf/Lock.h>
> #include <wtf/MainThread.h>
> #include <wtf/RunLoop.h>
> #include <wtf/ThreadSafeRefCounted.h>
> 
>+// This class is used only by components enabled for VIDEO support so far. So
>+// it's safe to assume the media player logging category is available.
>+GST_DEBUG_CATEGORY_EXTERN(webkit_media_player_debug);
>+
> namespace WebCore {
> 
> template <typename T>
>@@ -45,52 +50,70 @@ public:
>     template<typename F>
>     void notify(T notificationType, F&& callbackFunctor)
>     {
>+        GST_CAT_LEVEL_LOG(webkit_media_player_debug, GST_LEVEL_TRACE, m_object.get(), "Notify for type %d", notificationType);
>         ASSERT(m_isValid.load());
>         // Assert that there is only one bit on at a time.
>         ASSERT(!(static_cast<int>(notificationType) & (static_cast<int>(notificationType) - 1)));
>         if (isMainThread()) {
>             removePendingNotification(notificationType);
>             callbackFunctor();
>+            GST_CAT_LEVEL_LOG(webkit_media_player_debug, GST_LEVEL_TRACE, m_object.get(), "Functor called on main thread");
>             return;
>         }
> 
>-        if (!addPendingNotification(notificationType))
>+        if (!addPendingNotification(notificationType)) {
>+            GST_CAT_LEVEL_LOG(webkit_media_player_debug, GST_LEVEL_WARNING, m_object.get(), "Notification already pending");
>             return;
>+        }
> 
>-        RunLoop::main().dispatch([this, protectedThis = makeRef(*this), notificationType, callback = WTF::Function<void()>(WTFMove(callbackFunctor))] {
>-            if (!m_isValid.load())
>+        GST_CAT_LEVEL_LOG(webkit_media_player_debug, GST_LEVEL_TRACE, m_object.get(), "Scheduling dispatch");
>+        RunLoop::main().dispatch([this, notificationType, callback = WTF::Function<void()>(WTFMove(callbackFunctor))] {
>+            GST_CAT_LEVEL_LOG(webkit_media_player_debug, GST_LEVEL_TRACE, m_object.get(), "Dispatch");
>+            if (!m_isValid.load()) {
>+                GST_CAT_LEVEL_LOG(webkit_media_player_debug, GST_LEVEL_WARNING, m_object.get(), "Notifier was invalidated, bailing out");
>                 return;
>-            if (removePendingNotification(notificationType))
>+            }
>+            if (removePendingNotification(notificationType)) {
>                 callback();
>+                GST_CAT_LEVEL_LOG(webkit_media_player_debug, GST_LEVEL_TRACE, m_object.get(), "Functor called");
>+            } else
>+                GST_CAT_LEVEL_LOG(webkit_media_player_debug, GST_LEVEL_WARNING, m_object.get(), "Unable to remove pending notification");
>         });
>     }
> 
>     template<typename F>
>     void notifyAndWait(T notificationType, F&& callbackFunctor)
>     {
>-        Lock mutex;
>-        Condition condition;
>-
>-        notify(notificationType, [functor = WTFMove(callbackFunctor), &condition, &mutex] {
>+        GST_CAT_LEVEL_LOG(webkit_media_player_debug, GST_LEVEL_TRACE, m_object.get(), "Blocking notify for type %d", notificationType);
>+        notify(notificationType, [this, functor = WTFMove(callbackFunctor)] {
>             functor();
>-            LockHolder holder(mutex);
>-            condition.notifyOne();
>+            LockHolder holder(m_synchronousNotificationMutex);
>+            m_synchronousNotificationCondition.notifyOne();
>         });
> 
>         if (!isMainThread()) {
>-            LockHolder holder(mutex);
>-            condition.wait(mutex);
>+            GST_CAT_LEVEL_LOG(webkit_media_player_debug, GST_LEVEL_TRACE, m_object.get(), "Waiting for completion");
>+            LockHolder holder(m_synchronousNotificationMutex);
>+            m_synchronousNotificationCondition.wait(m_synchronousNotificationMutex);
>+            GST_CAT_LEVEL_LOG(webkit_media_player_debug, GST_LEVEL_TRACE, m_object.get(), "Blocking notificationn completed");
>         }
>     }
> 
>     void cancelPendingNotifications(unsigned mask = 0)
>     {
>         ASSERT(m_isValid.load());
>+        GST_CAT_LEVEL_LOG(webkit_media_player_debug, GST_LEVEL_TRACE, m_object.get(), "Cancelling pending notifications");
>+
>         LockHolder locker(m_pendingNotificationsLock);
>         if (mask)
>             m_pendingNotifications &= ~mask;
>         else
>             m_pendingNotifications = 0;
>+
>+        {
>+            LockHolder holder(m_synchronousNotificationMutex);
>+            m_synchronousNotificationCondition.notifyAll();
>+        }
>     }
> 
>     void invalidate()
>@@ -99,6 +122,11 @@ public:
>         m_isValid.store(false);
>     }
> 
>+    void setGstObject(GstObject* object)
>+    {
>+        m_object = object;
>+    }
>+
> private:
>     MainThreadNotifier()
>     {
>@@ -127,6 +155,9 @@ private:
>     Lock m_pendingNotificationsLock;
>     unsigned m_pendingNotifications { 0 };
>     Atomic<bool> m_isValid;
>+    GRefPtr<GstObject> m_object;
>+    Lock m_synchronousNotificationMutex;
>+    Condition m_synchronousNotificationCondition;
> };
> 
> } // namespace WebCore
>diff --git a/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp b/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp
>index 5cb5f7536ac3cefe1e16c98fe967021130a6df74..e2e0f639c8089cb9f9359d8564d3a40a327e812a 100644
>--- a/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp
>+++ b/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp
>@@ -282,10 +282,19 @@ MediaPlayerPrivateGStreamerBase::~MediaPlayerPrivateGStreamerBase()
>     // waiting there, and ensure that any triggerRepaint call reaching the lock won't wait on m_drawCondition.
>     cancelRepaint(true);
> 
>-    // The change to GST_STATE_NULL state is always synchronous. So after this gets executed we don't need to worry
>-    // about handlers running in the GStreamer thread.
>-    if (m_pipeline)
>+    if (m_pipeline) {
>+        // Notify all webkitwebsrc elements of the pipeline that they should
>+        // close their HTTP session ASAP. Because this has to happen in the
>+        // WebProcess main thread it might deadlock HLS pipelines if that was
>+        // requested during a standard PAUSED->READY transition.
>+        gst_element_send_event(m_pipeline.get(), gst_event_new_custom(GST_EVENT_CUSTOM_UPSTREAM,
>+            gst_structure_new_empty("webkit-close-http-session")));
>+
>+        // The change to GST_STATE_NULL state is always synchronous. So after
>+        // this gets executed we don't need to worry about handlers running in
>+        // the GStreamer thread.
>         gst_element_set_state(m_pipeline.get(), GST_STATE_NULL);
>+    }
> 
>     m_player = nullptr;
> }
>@@ -294,6 +303,8 @@ void MediaPlayerPrivateGStreamerBase::setPipeline(GstElement* pipeline)
> {
>     m_pipeline = pipeline;
> 
>+    m_notifier->setGstObject(GST_OBJECT_CAST(pipeline));
>+
>     GRefPtr<GstBus> bus = adoptGRef(gst_pipeline_get_bus(GST_PIPELINE(m_pipeline.get())));
>     gst_bus_set_sync_handler(bus.get(), [](GstBus*, GstMessage* message, gpointer userData) {
>         auto& player = *static_cast<MediaPlayerPrivateGStreamerBase*>(userData);
>diff --git a/Source/WebCore/platform/graphics/gstreamer/TrackPrivateBaseGStreamer.cpp b/Source/WebCore/platform/graphics/gstreamer/TrackPrivateBaseGStreamer.cpp
>index 05ba1e5b12383d127ec0d655377bfaaf6dfd9758..56f4af37e3f5ececf02eabafce27f17c19c4f130 100644
>--- a/Source/WebCore/platform/graphics/gstreamer/TrackPrivateBaseGStreamer.cpp
>+++ b/Source/WebCore/platform/graphics/gstreamer/TrackPrivateBaseGStreamer.cpp
>@@ -50,6 +50,7 @@ TrackPrivateBaseGStreamer::TrackPrivateBaseGStreamer(TrackPrivateBase* owner, gi
>     , m_owner(owner)
> {
>     ASSERT(m_pad);
>+    m_notifier->setGstObject(GST_OBJECT_CAST(m_pad.get()));
> 
>     g_signal_connect_swapped(m_pad.get(), "notify::active", G_CALLBACK(activeChangedCallback), this);
>     g_signal_connect_swapped(m_pad.get(), "notify::tags", G_CALLBACK(tagsChangedCallback), this);
>@@ -66,6 +67,7 @@ TrackPrivateBaseGStreamer::TrackPrivateBaseGStreamer(TrackPrivateBase* owner, gi
>     , m_owner(owner)
> {
>     ASSERT(m_stream);
>+    m_notifier->setGstObject(GST_OBJECT_CAST(m_stream.get()));
> 
>     // We can't call notifyTrackOfTagsChanged() directly, because we need tagsChanged() to setup m_tags.
>     tagsChanged();
>diff --git a/Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp b/Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp
>index 3f6c2f36e1aeaea8fdce69256deabebf650152ff..e3c72777978009123f46324480a923be4627caeb 100644
>--- a/Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp
>+++ b/Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp
>@@ -165,6 +165,7 @@ static gboolean webKitWebSrcQuery(GstBaseSrc*, GstQuery*);
> static gboolean webKitWebSrcUnLock(GstBaseSrc*);
> static gboolean webKitWebSrcUnLockStop(GstBaseSrc*);
> static void webKitWebSrcSetContext(GstElement*, GstContext*);
>+static gboolean webKitWebSrcEvent(GstBaseSrc*, GstEvent*);
> 
> #define webkit_web_src_parent_class parent_class
> // We split this out into another macro to avoid a check-webkit-style error.
>@@ -225,6 +226,7 @@ static void webkit_web_src_class_init(WebKitWebSrcClass* klass)
>     baseSrcClass->is_seekable = GST_DEBUG_FUNCPTR(webKitWebSrcIsSeekable);
>     baseSrcClass->do_seek = GST_DEBUG_FUNCPTR(webKitWebSrcDoSeek);
>     baseSrcClass->query = GST_DEBUG_FUNCPTR(webKitWebSrcQuery);
>+    baseSrcClass->event = GST_DEBUG_FUNCPTR(webKitWebSrcEvent);
> 
>     GstPushSrcClass* pushSrcClass = GST_PUSH_SRC_CLASS(klass);
>     pushSrcClass->create = GST_DEBUG_FUNCPTR(webKitWebSrcCreate);
>@@ -254,6 +256,7 @@ static void webkit_web_src_init(WebKitWebSrc* src)
>     new (priv) WebKitWebSrcPrivate();
> 
>     priv->notifier = MainThreadNotifier<MainThreadSourceNotification>::create();
>+    priv->notifier->setGstObject(GST_OBJECT_CAST(src));
>     priv->adapter = adoptGRef(gst_adapter_new());
>     priv->minimumBlocksize = gst_base_src_get_blocksize(GST_BASE_SRC_CAST(src));
> 
>@@ -606,6 +609,14 @@ static void webKitWebSrcCloseSession(WebKitWebSrc* src)
>     WebKitWebSrcPrivate* priv = src->priv;
>     GRefPtr<WebKitWebSrc> protector = WTF::ensureGRef(src);
> 
>+    priv->notifier->cancelPendingNotifications();
>+
>+    GST_DEBUG_OBJECT(src, "Closing session, loader: %p, resource: %p", priv->loader.get(), priv->resource.get());
>+    if (!priv->resource) {
>+        GST_DEBUG_OBJECT(src, "Session already closed");
>+        return;
>+    }
>+
>     priv->notifier->notifyAndWait(MainThreadSourceNotification::Stop, [protector, keepAlive = priv->keepAlive] {
>         WebKitWebSrcPrivate* priv = protector->priv;
> 
>@@ -624,14 +635,27 @@ static void webKitWebSrcCloseSession(WebKitWebSrc* src)
>     GST_DEBUG_OBJECT(src, "Resource loader stopped");
> }
> 
>+static gboolean webKitWebSrcEvent(GstBaseSrc* baseSrc, GstEvent* event)
>+{
>+    GST_DEBUG_OBJECT(baseSrc, "Handle event %" GST_PTR_FORMAT, event);
>+    const GstStructure* structure = gst_event_get_structure(event);
>+    if (structure && gst_structure_has_name(structure, "webkit-close-http-session")) {
>+        WebKitWebSrc* src = WEBKIT_WEB_SRC(baseSrc);
>+        WebKitWebSrcPrivate* priv = src->priv;
>+
>+        if (priv->resource || (priv->loader && !priv->keepAlive))
>+            webKitWebSrcCloseSession(src);
>+
>+        return TRUE;
>+    }
>+    return GST_BASE_SRC_CLASS(parent_class)->event(baseSrc, event);
>+}
>+
> static gboolean webKitWebSrcStop(GstBaseSrc* baseSrc)
> {
>     WebKitWebSrc* src = WEBKIT_WEB_SRC(baseSrc);
>     WebKitWebSrcPrivate* priv = src->priv;
> 
>-    if (priv->resource || (priv->loader && !priv->keepAlive))
>-        webKitWebSrcCloseSession(src);
>-
>     {
>         LockHolder adapterLocker(priv->adapterLock);
>         gst_adapter_clear(priv->adapter.get());
>@@ -731,6 +755,7 @@ static gboolean webKitWebSrcUnLock(GstBaseSrc* baseSrc)
>     GST_DEBUG_OBJECT(src, "Unlock");
>     src->priv->isFlushing = true;
>     src->priv->responseCondition.notifyOne();
>+    src->priv->notifier->cancelPendingNotifications();
>     return TRUE;
> }
> 
>diff --git a/Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp b/Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp
>index 1ad442490219564d5001cf468dee287c0d412f11..09783e83284f771d58cbe737c6e45940ad409b09 100644
>--- a/Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp
>+++ b/Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp
>@@ -260,6 +260,7 @@ static void webkit_media_src_init(WebKitMediaSrc* source)
>     source->priv->appsrcSeekDataNextAction = Nothing;
>     source->priv->flowCombiner = GUniquePtr<GstFlowCombiner>(gst_flow_combiner_new());
>     source->priv->notifier = WebCore::MainThreadNotifier<WebKitMediaSrcMainThreadNotification>::create();
>+    source->priv->notifier->setGstObject(GST_OBJECT_CAST(source));
> 
>     // No need to reset Stream.appsrcNeedDataFlag because there are no Streams at this point yet.
> }

Flags:

calvaris: commit-queue-

Actions: View | Formatted Diff | Diff

Attachments on bug 197558: 369484 | 369493 | 371048 | 373468 | 373621 | 373622