Bug 159552 - [GStreamer][GL] crash within triggerRepaint
Summary: [GStreamer][GL] crash within triggerRepaint
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 159626 154066
  Show dependency treegraph
 
Reported: 2016-07-08 04:00 PDT by Philippe Normand
Modified: 2016-07-13 05:05 PDT (History)
4 users (show)

See Also:


Attachments
patch (1.85 KB, patch)
2016-07-08 04:04 PDT, Philippe Normand
cgarcia: review-
Details | Formatted Diff | Diff
patch (6.89 KB, patch)
2016-07-08 05:40 PDT, Philippe Normand
calvaris: review+
Details | Formatted Diff | Diff
patch (6.57 KB, patch)
2016-07-13 00:33 PDT, Philippe Normand
calvaris: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2016-07-08 04:00:34 PDT
../Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp(787) : virtual void WebCore::MediaPlayerPrivateGStreamerBase::setSize(const WebCore::IntSize&)
1   0x73989ebe WTFCrash
2   0x7281a976 WebCore::TaskDispatcher<WebCore::Timer>::pendingDispatchers()
3   0x7281a6f6 WebCore::TaskDispatcher<WebCore::Timer>::postTask(WTF::NoncopyableFunction<void ()>&&)
4   0x724ebba8 WebCore::GenericTaskQueue<WebCore::Timer>::enqueueTask(WTF::NoncopyableFunction<void ()>&&)
5   0x72f482f0 WebCore::GenericEventQueue::enqueueEvent(WTF::RefPtr<WebCore::Event>&&)
6   0x724d6102 WebCore::HTMLMediaElement::scheduleEvent(WTF::AtomicString const&)
7   0x7304ac00 WebCore::HTMLVideoElement::scheduleResizeEvent()
8   0x7304ac46 WebCore::HTMLVideoElement::scheduleResizeEventIfSizeChanged()
9   0x724e1932 WebCore::HTMLMediaElement::mediaPlayerSizeChanged(WebCore::MediaPlayer*)
10  0x728a5a5c WebCore::MediaPlayer::sizeChanged()
11  0x72ce1eee WebCore::MediaPlayerPrivateGStreamerBase::triggerRepaint(_GstSample*)
mediaControlsScript:22:14: CONSOLE ERROR TypeError: null is not an object (evaluating 'panel.parentElement.querySelector')
12  0x72ce1fc2 WebCore::MediaPlayerPrivateGStreamerBase::drawCallback(WebCore::MediaPlayerPrivateGStreamerBase*, _GstBuffer*, _GstPad*, _GstBaseSink*)
13  0x6a250adc ffi_call_VFP
14  0x6a2510ea ffi_call

The sizeChanged() call needs to be scheduled to run on the main loop.
Comment 1 Philippe Normand 2016-07-08 04:04:03 PDT
Created attachment 283134 [details]
patch
Comment 2 WebKit Commit Bot 2016-07-08 04:04:46 PDT
Attachment 283134 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:553:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Carlos Garcia Campos 2016-07-08 04:13:15 PDT
Comment on attachment 283134 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=283134&action=review

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:553
> -        m_player->sizeChanged();
> +        RunLoop::main().dispatch([this] { m_player->sizeChanged(); });

Are you sure this is because of the threaded compositor and not because of GStreamerGL. Because I think in case of not using GSTGL, triggerRepaint is called in the main thread. In case of being in the main thread we would be scheduling this unnecessarily. Also, if the instance is destroyed before the source is dispatches in the main thread, this will crash, so you would need to protect this. It would be better if we could use the main thread notifier for this, but it's int he derived class not here in the base class.
Comment 4 Carlos Garcia Campos 2016-07-08 04:18:04 PDT
(In reply to comment #3)
> Comment on attachment 283134 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=283134&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:553
> > -        m_player->sizeChanged();
> > +        RunLoop::main().dispatch([this] { m_player->sizeChanged(); });
> 
> Are you sure this is because of the threaded compositor and not because of
> GStreamerGL. Because I think in case of not using GSTGL, triggerRepaint is
> called in the main thread. In case of being in the main thread we would be
> scheduling this unnecessarily. Also, if the instance is destroyed before the
> source is dispatches in the main thread, this will crash, so you would need
> to protect this. It would be better if we could use the main thread notifier
> for this, but it's int he derived class not here in the base class.

We already have a weak factory for this, so could you do something like this:

if (isMainThread())
    m_player->sizeChanged();
else {
    auto weakThis = player.createWeakPtr();
    RunLoop::main().dispatch([weakThis]) { 
        if (weakThis)
            weakThis->m_player->sizeChanged();
    });
}
Comment 5 Philippe Normand 2016-07-08 05:40:06 PDT
Created attachment 283139 [details]
patch
Comment 6 Xabier Rodríguez Calvar 2016-07-11 06:38:44 PDT
(In reply to comment #4)
> if (isMainThread())
>     m_player->sizeChanged();
> else {
>     auto weakThis = player.createWeakPtr();
>     RunLoop::main().dispatch([weakThis]) { 
>         if (weakThis)
>             weakThis->m_player->sizeChanged();
>     });
> }

I understand the concerns about reducing performance but if there isn't we should create a function to do something like:

{
     auto weakThis = player.createWeakPtr();
     RunLoop::main().runNowIfMainThreadOrdispatch([weakThis]) { 
         if (weakThis)
             weakThis->m_player->sizeChanged();
     });
}

Otherwise I think we are creating these code structures and IMHO it makes code hard to read.
Comment 7 Xabier Rodríguez Calvar 2016-07-11 07:21:03 PDT
Comment on attachment 283139 [details]
patch

> I understand the concerns about reducing performance but if there isn't we
> should create a function to do something like:
> 
> {
>      auto weakThis = player.createWeakPtr();
>      RunLoop::main().runNowIfMainThreadOrdispatch([weakThis]) { 
>          if (weakThis)
>              weakThis->m_player->sizeChanged();
>      });
> }
> 
> Otherwise I think we are creating these code structures and IMHO it makes
> code hard to read.

Let's not block landing this because of this nit, but please, open a new bug about this.
Comment 8 Philippe Normand 2016-07-11 07:37:18 PDT
Committed r203056: <http://trac.webkit.org/changeset/203056>
Comment 9 Carlos Garcia Campos 2016-07-12 23:45:52 PDT
(In reply to comment #6)
> (In reply to comment #4)
> > if (isMainThread())
> >     m_player->sizeChanged();
> > else {
> >     auto weakThis = player.createWeakPtr();
> >     RunLoop::main().dispatch([weakThis]) { 
> >         if (weakThis)
> >             weakThis->m_player->sizeChanged();
> >     });
> > }
> 
> I understand the concerns about reducing performance but if there isn't we
> should create a function to do something like:

This is not about performance at all, this is to a void a use after free.

> {
>      auto weakThis = player.createWeakPtr();
>      RunLoop::main().runNowIfMainThreadOrdispatch([weakThis]) { 
>          if (weakThis)
>              weakThis->m_player->sizeChanged();
>      });
> }
> 
> Otherwise I think we are creating these code structures and IMHO it makes
> code hard to read.

That's what the MainThreadNotifier is for, I already agreed with philn to use it.
Comment 10 Carlos Garcia Campos 2016-07-12 23:46:59 PDT
(In reply to comment #7)
> Comment on attachment 283139 [details]
> patch
> 
> > I understand the concerns about reducing performance but if there isn't we
> > should create a function to do something like:
> > 
> > {
> >      auto weakThis = player.createWeakPtr();
> >      RunLoop::main().runNowIfMainThreadOrdispatch([weakThis]) { 
> >          if (weakThis)
> >              weakThis->m_player->sizeChanged();
> >      });
> > }
> > 
> > Otherwise I think we are creating these code structures and IMHO it makes
> > code hard to read.
> 
> Let's not block landing this because of this nit, but please, open a new bug
> about this.

A nit? This is needed to avoid a crash because of a user after free, I wouldn't say it's a nit.
Comment 11 Carlos Garcia Campos 2016-07-12 23:56:37 PDT
(In reply to comment #8)
> Committed r203056: <http://trac.webkit.org/changeset/203056>

Landed patch is not correct, the advantage of using the MainThreadNotifier is that you don't need to care about creating a weak ptr, nor checking the caller thread. MainThreadNotifier has its own weak ptr and it's destroyed by the media player private class destructor, so you don't need to move the weak ptr factory either. You just need to add the new notification to the enum and then 

- m_player->sizeChanged();
+ m_notifier.notify(MainThreadNotification::SizeChanged, [this] { m_player->sizeChanged() });

That's all
Comment 12 Philippe Normand 2016-07-13 00:11:30 PDT
Ok then I'll prepare a follow-up patch, reverting the un-needed changes. Thanks Carlos.
Comment 13 Philippe Normand 2016-07-13 00:33:41 PDT
Created attachment 283493 [details]
patch
Comment 14 Philippe Normand 2016-07-13 00:34:25 PDT
Reopening for follow-up patch.
Comment 15 Xabier Rodríguez Calvar 2016-07-13 01:17:58 PDT
(In reply to comment #10)
> > Let's not block landing this because of this nit, but please, open a new bug
> > about this.
> 
> A nit? This is needed to avoid a crash because of a user after free, I
> wouldn't say it's a nit.

The nit I referred was not to avoid what is needed to avoid the crash, but the implementation of a function like runNowIfMainThreadOrDispatch.

Good catch about the weak ptr.
Comment 16 Philippe Normand 2016-07-13 05:05:16 PDT
Committed r203159: <http://trac.webkit.org/changeset/203159>