Bug 159552 – [GStreamer][GL] crash within triggerRepaint

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Philippe Normand]

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

[Philippe Normand]

Created attachment 283134 [details]
patch

[WebKit Commit Bot]

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.

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

(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();
    });
}

[Philippe Normand]

Created attachment 283139 [details]
patch

[Xabier Rodríguez Calvar]

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

[Xabier Rodríguez Calvar]

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.

[Philippe Normand]

Committed r203056: <http://trac.webkit.org/changeset/203056>

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

(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

[Philippe Normand]

Ok then I'll prepare a follow-up patch, reverting the un-needed changes. Thanks Carlos.

[Philippe Normand]

Created attachment 283493 [details]
patch

[Philippe Normand]

Reopening for follow-up patch.

[Xabier Rodríguez Calvar]

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

[Philippe Normand]

Committed r203159: <http://trac.webkit.org/changeset/203159>