- Revision
- 247215
- Author
- ctur...@igalia.com
- Date
- 2019-07-08 11:16:08 -0700 (Mon, 08 Jul 2019)
Log Message
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 Xabier Rodriguez-Calvar.
Source/WebCore:
Not covered, I have a test locally that would probably trigger the
deadlock if the network requests took a realistic amount of time,
but from a local webserver the window of time to hit this deadlock
is too narrow.
* platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:
(webkit_web_src_init): Make the websrc start asynchronously, this
allows the main thread to be free to complete resource loader
setup.
(webKitWebSrcCreate): Calling start() from the create() vfunc is a
recipe for deadlock, since BaseSrc holds the streaming lock during
seeks, and then calls create(). In these cases, we do not want to
notify async-completion, since we've already completed from the
necessarily preceeding start() vfunc, and calling it again would
require the stream-lock and deadlock us.
(webKitWebSrcStart): Refactor to use webKitWebSrcMakeRequest, but
ensuring that we do perform an async-complete notification.
(webKitWebSrcMakeRequest): What Start() used to be, but now can be
toggled when to notify of async-completion. Start() no longer
blocks, since the return value of initiating a resource loader is
of no interest to the callers.
(webKitWebSrcCloseSession): Similarly to Start(), we do not need
to wait for the completion of cancelled net requests.
Tools:
On shutdown we can easily deadlock the web process if we don't
ensure all network operations are completed before comitting state
changes. In HLS, make sure the network operations are cancelled,
and also prevent hlsdemux's retry logic from scuppering our
efforts.
* gstreamer/jhbuild.modules: Include the patch.
* gstreamer/patches/gst-plugins-bad-do-not-retry-downloads-during-shutdown.patch: Added.
Modified Paths
Added Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (247214 => 247215)
--- trunk/Source/WebCore/ChangeLog 2019-07-08 18:14:23 UTC (rev 247214)
+++ trunk/Source/WebCore/ChangeLog 2019-07-08 18:16:08 UTC (rev 247215)
@@ -1,3 +1,34 @@
+2019-07-08 Charlie Turner <ctur...@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 Xabier Rodriguez-Calvar.
+
+ Not covered, I have a test locally that would probably trigger the
+ deadlock if the network requests took a realistic amount of time,
+ but from a local webserver the window of time to hit this deadlock
+ is too narrow.
+
+ * platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:
+ (webkit_web_src_init): Make the websrc start asynchronously, this
+ allows the main thread to be free to complete resource loader
+ setup.
+ (webKitWebSrcCreate): Calling start() from the create() vfunc is a
+ recipe for deadlock, since BaseSrc holds the streaming lock during
+ seeks, and then calls create(). In these cases, we do not want to
+ notify async-completion, since we've already completed from the
+ necessarily preceeding start() vfunc, and calling it again would
+ require the stream-lock and deadlock us.
+ (webKitWebSrcStart): Refactor to use webKitWebSrcMakeRequest, but
+ ensuring that we do perform an async-complete notification.
+ (webKitWebSrcMakeRequest): What Start() used to be, but now can be
+ toggled when to notify of async-completion. Start() no longer
+ blocks, since the return value of initiating a resource loader is
+ of no interest to the callers.
+ (webKitWebSrcCloseSession): Similarly to Start(), we do not need
+ to wait for the completion of cancelled net requests.
+
2019-07-08 Chris Dumez <cdu...@apple.com>
Unable to play videos on xfinity.com/stream on macOS Catalina
Modified: trunk/Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp (247214 => 247215)
--- trunk/Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp 2019-07-08 18:14:23 UTC (rev 247214)
+++ trunk/Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp 2019-07-08 18:16:08 UTC (rev 247215)
@@ -156,6 +156,7 @@
static void webKitWebSrcGetProperty(GObject*, guint propertyID, GValue*, GParamSpec*);
static GstStateChangeReturn webKitWebSrcChangeState(GstElement*, GstStateChange);
static GstFlowReturn webKitWebSrcCreate(GstPushSrc*, GstBuffer**);
+static gboolean webKitWebSrcMakeRequest(GstBaseSrc*, bool);
static gboolean webKitWebSrcStart(GstBaseSrc*);
static gboolean webKitWebSrcStop(GstBaseSrc*);
static gboolean webKitWebSrcGetSize(GstBaseSrc*, guint64* size);
@@ -260,6 +261,7 @@
webkitWebSrcReset(src);
gst_base_src_set_automatic_eos(GST_BASE_SRC_CAST(src), FALSE);
+ gst_base_src_set_async(GST_BASE_SRC_CAST(src), TRUE);
}
static void webKitWebSrcDispose(GObject* object)
@@ -361,7 +363,12 @@
uint64_t requestedPosition = priv->requestedPosition;
webKitWebSrcStop(baseSrc);
priv->requestedPosition = requestedPosition;
- webKitWebSrcStart(baseSrc);
+ // Do not notify async-completion, in seeking flows, we will
+ // be called from GstBaseSrc's perform_seek vfunc, which holds
+ // a streaming lock in our frame. Hence, we would deadlock
+ // trying to notify async completion, since that also requires
+ // the streaming lock.
+ webKitWebSrcMakeRequest(baseSrc, false);
}
{
@@ -497,6 +504,14 @@
static gboolean webKitWebSrcStart(GstBaseSrc* baseSrc)
{
+ // This method should only be called by BaseSrc, do not call it
+ // from ourselves unless you ensure the streaming lock is not
+ // held. If it is, you will deadlock the WebProcess.
+ return webKitWebSrcMakeRequest(baseSrc, true);
+}
+
+static gboolean webKitWebSrcMakeRequest(GstBaseSrc* baseSrc, bool notifyAsyncCompletion)
+{
WebKitWebSrc* src = ""
WebKitWebSrcPrivate* priv = src->priv;
@@ -582,7 +597,7 @@
request.setHTTPHeaderField(HTTPHeaderName::IcyMetadata, "1");
GRefPtr<WebKitWebSrc> protector = WTF::ensureGRef(src);
- priv->notifier->notifyAndWait(MainThreadSourceNotification::Start, [protector, request = WTFMove(request)] {
+ priv->notifier->notify(MainThreadSourceNotification::Start, [protector, request = WTFMove(request), src, notifyAsyncCompletion] {
WebKitWebSrcPrivate* priv = protector->priv;
if (!priv->loader)
priv->loader = priv->player->createResourceLoader();
@@ -594,13 +609,16 @@
if (priv->resource) {
priv->resource->setClient(std::make_unique<CachedResourceStreamingClient>(protector.get(), ResourceRequest(request)));
GST_DEBUG_OBJECT(protector.get(), "Started request");
+ if (notifyAsyncCompletion)
+ gst_base_src_start_complete(GST_BASE_SRC(src), GST_FLOW_OK);
} else {
GST_ERROR_OBJECT(protector.get(), "Failed to setup streaming client");
+ if (notifyAsyncCompletion)
+ gst_base_src_start_complete(GST_BASE_SRC(src), GST_FLOW_ERROR);
priv->loader = nullptr;
}
});
- GST_DEBUG_OBJECT(src, "Resource loader started");
return TRUE;
}
@@ -609,7 +627,7 @@
WebKitWebSrcPrivate* priv = src->priv;
GRefPtr<WebKitWebSrc> protector = WTF::ensureGRef(src);
- priv->notifier->notifyAndWait(MainThreadSourceNotification::Stop, [protector, keepAlive = priv->keepAlive] {
+ priv->notifier->notify(MainThreadSourceNotification::Stop, [protector, keepAlive = priv->keepAlive] {
WebKitWebSrcPrivate* priv = protector->priv;
GST_DEBUG_OBJECT(protector.get(), "Stopping resource loader");
Modified: trunk/Tools/ChangeLog (247214 => 247215)
--- trunk/Tools/ChangeLog 2019-07-08 18:14:23 UTC (rev 247214)
+++ trunk/Tools/ChangeLog 2019-07-08 18:16:08 UTC (rev 247215)
@@ -1,3 +1,19 @@
+2019-07-08 Charlie Turner <ctur...@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 Xabier Rodriguez-Calvar.
+
+ On shutdown we can easily deadlock the web process if we don't
+ ensure all network operations are completed before comitting state
+ changes. In HLS, make sure the network operations are cancelled,
+ and also prevent hlsdemux's retry logic from scuppering our
+ efforts.
+
+ * gstreamer/jhbuild.modules: Include the patch.
+ * gstreamer/patches/gst-plugins-bad-do-not-retry-downloads-during-shutdown.patch: Added.
+
2019-07-08 Antoine Quint <grao...@apple.com>
[Pointer Events] Enable only on the most recent version of the supported iOS family
Modified: trunk/Tools/gstreamer/jhbuild.modules (247214 => 247215)
--- trunk/Tools/gstreamer/jhbuild.modules 2019-07-08 18:14:23 UTC (rev 247214)
+++ trunk/Tools/gstreamer/jhbuild.modules 2019-07-08 18:16:08 UTC (rev 247215)
@@ -92,6 +92,7 @@
<dep package="libsrtp"/>
</dependencies>
<branch hash="sha256:22139de35626ada6090bdfa3423b27b7fc15a0198331d25c95e6b12cb1072b05" module="gst-plugins-bad/gst-plugins-bad-${version}.tar.xz" repo="gstreamer" version="1.16.0">
+ <patch file="gst-plugins-bad-do-not-retry-downloads-during-shutdown.patch" strip="1"/> <!-- In review: https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/merge_requests/427 -->
</branch>
</meson>
Added: trunk/Tools/gstreamer/patches/gst-plugins-bad-do-not-retry-downloads-during-shutdown.patch (0 => 247215)
--- trunk/Tools/gstreamer/patches/gst-plugins-bad-do-not-retry-downloads-during-shutdown.patch (rev 0)
+++ trunk/Tools/gstreamer/patches/gst-plugins-bad-do-not-retry-downloads-during-shutdown.patch 2019-07-08 18:16:08 UTC (rev 247215)
@@ -0,0 +1,89 @@
+From 4c21593e5fcd1337b433119b8c7800dc5565f514 Mon Sep 17 00:00:00 2001
+From: Charlie Turner <ctur...@igalia.com>
+Date: Tue, 2 Jul 2019 12:27:40 +0100
+Subject: [PATCH] WIP: adaptivedemux: do not retry downloads during shutdown.
+
+---
+ ext/hls/gsthlsdemux.c | 15 +++++++++++++--
+ ext/hls/gsthlsdemux.h | 4 ++++
+ 2 files changed, 17 insertions(+), 2 deletions(-)
+
+diff --git a/ext/hls/gsthlsdemux.c b/ext/hls/gsthlsdemux.c
+index 4317d65c3..f9583ad1a 100644
+--- a/ext/hls/gsthlsdemux.c
++++ b/ext/hls/gsthlsdemux.c
+@@ -73,6 +73,7 @@ static gboolean gst_hls_demux_update_playlist (GstHLSDemux * demux,
+ gboolean update, GError ** err);
+ static gchar *gst_hls_src_buf_to_utf8_playlist (GstBuffer * buf);
+
++/* FIXME: the return value is never used? */
+ static gboolean gst_hls_demux_change_playlist (GstHLSDemux * demux,
+ guint max_bitrate, gboolean * changed);
+ static GstBuffer *gst_hls_demux_decrypt_fragment (GstHLSDemux * demux,
+@@ -193,6 +194,8 @@ gst_hls_demux_init (GstHLSDemux * demux)
+
+ demux->keys = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, g_free);
+ g_mutex_init (&demux->keys_lock);
++
++ demux->cancelling_downloads = FALSE;
+ }
+
+ static GstStateChangeReturn
+@@ -205,6 +208,11 @@ gst_hls_demux_change_state (GstElement * element, GstStateChange transition)
+ case GST_STATE_CHANGE_READY_TO_PAUSED:
+ gst_hls_demux_reset (GST_ADAPTIVE_DEMUX_CAST (demux));
+ break;
++ case GST_STATE_CHANGE_PAUSED_TO_READY:
++ GST_DEBUG_OBJECT (demux, "PAUSED->READY cancelling downloads");
++ demux->cancelling_downloads = TRUE;
++ gst_uri_downloader_cancel (GST_ADAPTIVE_DEMUX (demux)->downloader);
++ break;
+ default:
+ break;
+ }
+@@ -1158,6 +1166,8 @@ gst_hls_demux_reset (GstAdaptiveDemux * ademux)
+ {
+ GstHLSDemux *demux = GST_HLS_DEMUX_CAST (ademux);
+
++ GST_DEBUG_OBJECT (demux, "resetting");
++
+ GST_M3U8_CLIENT_LOCK (hlsdemux->client);
+ if (demux->master) {
+ gst_hls_master_playlist_unref (demux->master);
+@@ -1379,7 +1389,8 @@ retry:
+ if (download == NULL) {
+ gchar *base_uri;
+
+- if (!update || main_checked || demux->master->is_simple) {
++ if (!update || main_checked || demux->master->is_simple
++ || demux->cancelling_downloads) {
+ g_free (uri);
+ return FALSE;
+ }
+@@ -1612,7 +1623,7 @@ retry_failover_protection:
+ if (changed)
+ *changed = TRUE;
+ stream->discont = TRUE;
+- } else {
++ } else if (!demux->cancelling_downloads) {
+ GstHLSVariantStream *failover_variant = NULL;
+ GList *failover;
+
+diff --git a/ext/hls/gsthlsdemux.h b/ext/hls/gsthlsdemux.h
+index 0cab19627..9c0decabf 100644
+--- a/ext/hls/gsthlsdemux.h
++++ b/ext/hls/gsthlsdemux.h
+@@ -147,6 +147,10 @@ struct _GstHLSDemux
+ GstHLSMasterPlaylist *master;
+
+ GstHLSVariantStream *current_variant;
++
++ /* Set when the parent is state-changing down from PAUSED to avoid
++ making further network requests. */
++ gboolean cancelling_downloads;
+ };
+
+ struct _GstHLSDemuxClass
+--
+2.17.1
+
