Changeset 247215 in webkit


Ignore:
Timestamp:
Jul 8, 2019 11:16:08 AM (5 years ago)
Author:
cturner@igalia.com
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.
Location:
trunk
Files:
1 added
4 edited

Legend:

Unmodified
Added
Removed
  • trunk/Source/WebCore/ChangeLog

    r247213 r247215  
     12019-07-08  Charlie Turner  <cturner@igalia.com>
     2
     3        REGRESSION(r243197): [GStreamer] Web process hangs when scrolling twitter timeline which contains HLS videos
     4        https://bugs.webkit.org/show_bug.cgi?id=197558
     5
     6        Reviewed by Xabier Rodriguez-Calvar.
     7
     8        Not covered, I have a test locally that would probably trigger the
     9        deadlock if the network requests took a realistic amount of time,
     10        but from a local webserver the window of time to hit this deadlock
     11        is too narrow.
     12
     13        * platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:
     14        (webkit_web_src_init): Make the websrc start asynchronously, this
     15        allows the main thread to be free to complete resource loader
     16        setup.
     17        (webKitWebSrcCreate): Calling start() from the create() vfunc is a
     18        recipe for deadlock, since BaseSrc holds the streaming lock during
     19        seeks, and then calls create(). In these cases, we do not want to
     20        notify async-completion, since we've already completed from the
     21        necessarily preceeding start() vfunc, and calling it again would
     22        require the stream-lock and deadlock us.
     23        (webKitWebSrcStart): Refactor to use webKitWebSrcMakeRequest, but
     24        ensuring that we do perform an async-complete notification.
     25        (webKitWebSrcMakeRequest): What Start() used to be, but now can be
     26        toggled when to notify of async-completion. Start() no longer
     27        blocks, since the return value of initiating a resource loader is
     28        of no interest to the callers.
     29        (webKitWebSrcCloseSession): Similarly to Start(), we do not need
     30        to wait for the completion of cancelled net requests.
     31
    1322019-07-08  Chris Dumez  <cdumez@apple.com>
    233
  • trunk/Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp

    r247010 r247215  
    157157static GstStateChangeReturn webKitWebSrcChangeState(GstElement*, GstStateChange);
    158158static GstFlowReturn webKitWebSrcCreate(GstPushSrc*, GstBuffer**);
     159static gboolean webKitWebSrcMakeRequest(GstBaseSrc*, bool);
    159160static gboolean webKitWebSrcStart(GstBaseSrc*);
    160161static gboolean webKitWebSrcStop(GstBaseSrc*);
     
    261262    webkitWebSrcReset(src);
    262263    gst_base_src_set_automatic_eos(GST_BASE_SRC_CAST(src), FALSE);
     264    gst_base_src_set_async(GST_BASE_SRC_CAST(src), TRUE);
    263265}
    264266
     
    362364        webKitWebSrcStop(baseSrc);
    363365        priv->requestedPosition = requestedPosition;
    364         webKitWebSrcStart(baseSrc);
     366        // Do not notify async-completion, in seeking flows, we will
     367        // be called from GstBaseSrc's perform_seek vfunc, which holds
     368        // a streaming lock in our frame. Hence, we would deadlock
     369        // trying to notify async completion, since that also requires
     370        // the streaming lock.
     371        webKitWebSrcMakeRequest(baseSrc, false);
    365372    }
    366373
     
    498505static gboolean webKitWebSrcStart(GstBaseSrc* baseSrc)
    499506{
     507    // This method should only be called by BaseSrc, do not call it
     508    // from ourselves unless you ensure the streaming lock is not
     509    // held. If it is, you will deadlock the WebProcess.
     510    return webKitWebSrcMakeRequest(baseSrc, true);
     511}
     512
     513static gboolean webKitWebSrcMakeRequest(GstBaseSrc* baseSrc, bool notifyAsyncCompletion)
     514{
    500515    WebKitWebSrc* src = WEBKIT_WEB_SRC(baseSrc);
    501516    WebKitWebSrcPrivate* priv = src->priv;
     
    583598
    584599    GRefPtr<WebKitWebSrc> protector = WTF::ensureGRef(src);
    585     priv->notifier->notifyAndWait(MainThreadSourceNotification::Start, [protector, request = WTFMove(request)] {
     600    priv->notifier->notify(MainThreadSourceNotification::Start, [protector, request = WTFMove(request), src, notifyAsyncCompletion] {
    586601        WebKitWebSrcPrivate* priv = protector->priv;
    587602        if (!priv->loader)
     
    595610            priv->resource->setClient(std::make_unique<CachedResourceStreamingClient>(protector.get(), ResourceRequest(request)));
    596611            GST_DEBUG_OBJECT(protector.get(), "Started request");
     612            if (notifyAsyncCompletion)
     613                gst_base_src_start_complete(GST_BASE_SRC(src), GST_FLOW_OK);
    597614        } else {
    598615            GST_ERROR_OBJECT(protector.get(), "Failed to setup streaming client");
     616            if (notifyAsyncCompletion)
     617                gst_base_src_start_complete(GST_BASE_SRC(src), GST_FLOW_ERROR);
    599618            priv->loader = nullptr;
    600619        }
    601620    });
    602621
    603     GST_DEBUG_OBJECT(src, "Resource loader started");
    604622    return TRUE;
    605623}
     
    610628    GRefPtr<WebKitWebSrc> protector = WTF::ensureGRef(src);
    611629
    612     priv->notifier->notifyAndWait(MainThreadSourceNotification::Stop, [protector, keepAlive = priv->keepAlive] {
     630    priv->notifier->notify(MainThreadSourceNotification::Stop, [protector, keepAlive = priv->keepAlive] {
    613631        WebKitWebSrcPrivate* priv = protector->priv;
    614632
  • trunk/Tools/ChangeLog

    r247205 r247215  
     12019-07-08  Charlie Turner  <cturner@igalia.com>
     2
     3        REGRESSION(r243197): [GStreamer] Web process hangs when scrolling twitter timeline which contains HLS videos
     4        https://bugs.webkit.org/show_bug.cgi?id=197558
     5
     6        Reviewed by Xabier Rodriguez-Calvar.
     7
     8        On shutdown we can easily deadlock the web process if we don't
     9        ensure all network operations are completed before comitting state
     10        changes. In HLS, make sure the network operations are cancelled,
     11        and also prevent hlsdemux's retry logic from scuppering our
     12        efforts.
     13
     14        * gstreamer/jhbuild.modules: Include the patch.
     15        * gstreamer/patches/gst-plugins-bad-do-not-retry-downloads-during-shutdown.patch: Added.
     16
    1172019-07-08  Antoine Quint  <graouts@apple.com>
    218
  • trunk/Tools/gstreamer/jhbuild.modules

    r245677 r247215  
    9393    </dependencies>
    9494    <branch hash="sha256:22139de35626ada6090bdfa3423b27b7fc15a0198331d25c95e6b12cb1072b05" module="gst-plugins-bad/gst-plugins-bad-${version}.tar.xz" repo="gstreamer" version="1.16.0">
     95      <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 -->
    9596    </branch>
    9697  </meson>
Note: See TracChangeset for help on using the changeset viewer.