Bug 107544 – [gstreamer] GstBus signal watch should be removed on clean up

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Chris Dumez]

Our gstreamer backend code currently calls gst_bus_add_signal_watch() on GstBus to add a signal watch. As per the documentation [1], "To clean up, the caller is responsible for calling gst_bus_remove_signal_watch() as many times as this function is called". This is because gst_bus_add_signal_watch() causes the GstBus object to be ref'd and gst_bus_remove_signal_watch() needs to be called to properly unref it.

I also noticed that adding those calls to gst_bus_remove_signal_watch() where needed appears to get rid of the following crashes we sometimes see when registering a bus signal watch:

crash log for WebProcess (pid <unknown>):
STDOUT: <empty>
STDERR:
STDERR: (WebProcess:12640): GStreamer-CRITICAL **: gst_poll_get_read_gpollfd: assertion `set != NULL' failed
STDERR:
STDERR: (WebProcess:12640): GStreamer-CRITICAL **: gst_bus_create_watch: assertion `bus->priv->poll != NULL' failed
STDERR:
STDERR: (WebProcess:12640): GLib-CRITICAL **: g_source_set_callback: assertion `source != NULL' failed
STDERR: 1   0x7fe74675cf7b
STDERR: 2   0x7fe7490824a0
STDERR: 3   0x7fe741d93a64 g_source_attach
STDERR: 4   0x7fe73f0bf6b8
STDERR: 5   0x7fe73f0c01df gst_bus_add_signal_watch_full
STDERR: 6   0x7fe73f0c0278 gst_bus_add_signal_watch
STDERR: 7   0x7fe746663cef WebCore::MediaPlayerPrivateGStreamer::createGSTPlayBin()
STDERR: 8   0x7fe74665d9a0 WebCore::MediaPlayerPrivateGStreamer::load(WTF::String const&)
STDERR: 9   0x7fe745be68f2 WebCore::MediaPlayer::loadWithNextMediaEngine(WebCore::MediaPlayerFactory*)
STDERR: 10  0x7fe745be6601 WebCore::MediaPlayer::load(WebCore::KURL const&, WebCore::ContentType const&, WTF::String const&)
STDERR: 11  0x7fe745739ad3 WebCore::HTMLMediaElement::loadResource(WebCore::KURL const&, WebCore::ContentType&, WTF::String const&)
STDERR: 12  0x7fe745739409 WebCore::HTMLMediaElement::loadNextSourceChild()
STDERR: 13  0x7fe7457386f1 WebCore::HTMLMediaElement::loadTimerFired(WebCore::Timer<WebCore::HTMLMediaElement>*)
STDERR: 14  0x7fe7457576b8 WebCore::Timer<WebCore::HTMLMediaElement>::fired()
STDERR: 15  0x7fe745b743e6 WebCore::ThreadTimers::sharedTimerFiredInternal()
STDERR: 16  0x7fe745b74307 WebCore::ThreadTimers::sharedTimerFired()
STDERR: 17  0x7fe746647365
STDERR: 18  0x7fe74a33cc70
STDERR: 19  0x7fe74a33dc44 _ecore_timer_expired_call
STDERR: 20  0x7fe74a33dac4 _ecore_timer_expired_timers_call
STDERR: 21  0x7fe74a33af4a
STDERR: 22  0x7fe74a339407 ecore_main_loop_begin
STDERR: 23  0x7fe746645c25 WebCore::RunLoop::run()
STDERR: 24  0x7fe749e57156 WebProcessMainEfl
STDERR: 25  0x4007c4 main
STDERR: 26  0x7fe74906d76d __libc_start_main
STDERR: 27  0x4006e9
STDERR: LEAK: 1 WebPage
STDERR: LEAK: 1 WebFrame
STDERR: LEAK: 24 RenderObject
STDERR: LEAK: 1 Page
STDERR: LEAK: 1 Frame
STDERR: LEAK: 3 CachedResource
STDERR: LEAK: 54 WebCoreNode

I could easily reproduce this crash by running media/video-source-moved.html in a loop (before my fix).

[1] http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gstreamer/html/GstBus.html#gst-bus-add-signal-watch

[Philippe Normand]

Good catch!

[Chris Dumez]

Created attachment 183955 [details]
Patch

[Early Warning System Bot]

Comment on attachment 183955 [details]
Patch

Attachment 183955 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16037541

[Early Warning System Bot]

Comment on attachment 183955 [details]
Patch

Attachment 183955 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16043499

[EFL EWS Bot]

Comment on attachment 183955 [details]
Patch

Attachment 183955 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16033586

[Chris Dumez]

Created attachment 183959 [details]
Patch

Should fix build (conflict with another local patch, sorry).

[Philippe Normand]

Comment on attachment 183959 [details]
Patch

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

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:280
> +        GstBus* bus = gst_pipeline_get_bus(GST_PIPELINE(m_playBin));
> +        ASSERT(bus);

You see me coming with a GRefPtr<GstBus>? :) I'm not sure anymore why we added support for that and not using it though

[Chris Dumez]

(In reply to comment #7)
> (From update of attachment 183959 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=183959&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:280
> > +        GstBus* bus = gst_pipeline_get_bus(GST_PIPELINE(m_playBin));
> > +        ASSERT(bus);
> 
> You see me coming with a GRefPtr<GstBus>? :) I'm not sure anymore why we added support for that and not using it though

Ok :)

[Chris Dumez]

(In reply to comment #7)
> (From update of attachment 183959 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=183959&action=review
> 
> > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:280
> > +        GstBus* bus = gst_pipeline_get_bus(GST_PIPELINE(m_playBin));
> > +        ASSERT(bus);
> 
> You see me coming with a GRefPtr<GstBus>? :) I'm not sure anymore why we added support for that and not using it though

To confirm, should I use a GRefPtr<GstBus> in all 3 places where the GstBus is used for both registration and unregistration? Or would you rather I keep this patch small and only use GRefPtr<GstBus> in the new code that it adds?

[Philippe Normand]

I'd be fine with a single patch fixing all this at once, but it's your call :)

[Chris Dumez]

Ok, I'll use GRefPtr<GstBus> where needed and reupload.

[Chris Dumez]

Created attachment 183969 [details]
Patch

Rebased on master and use smart pointers for GstBus.

[Philippe Normand]

Comment on attachment 183969 [details]
Patch

Looks good, have you tested with a Debug build?

[Chris Dumez]

(In reply to comment #13)
> (From update of attachment 183969 [details])
> Looks good, have you tested with a Debug build?

Yes, I'm using a debug build and this has been well tested.

[Chris Dumez]

Based on Bug 105869, it seems adopting the GstBus returned by gst_pipeline_get_bus() was causing assertions before on EFL port... I cannot reproduce these assertions anymore so it may be specific to gstreamer 0.10. Probably this needs some testing with gstreamer 0.10 as well...

[Philippe Normand]

(In reply to comment #15)
> Based on Bug 105869, it seems adopting the GstBus returned by gst_pipeline_get_bus() was causing assertions before on EFL port... I cannot reproduce these assertions anymore so it may be specific to gstreamer 0.10. Probably this needs some testing with gstreamer 0.10 as well...

Ah!
Could it be that in 0.10 we get a floating ref and not in 1.0?

[Chris Dumez]

(In reply to comment #16)
> (In reply to comment #15)
> > Based on Bug 105869, it seems adopting the GstBus returned by gst_pipeline_get_bus() was causing assertions before on EFL port... I cannot reproduce these assertions anymore so it may be specific to gstreamer 0.10. Probably this needs some testing with gstreamer 0.10 as well...
> 
> Ah!
> Could it be that in 0.10 we get a floating ref and not in 1.0?

Well, if that's the case then the doc would be wrong :/

http://gstreamer.freedesktop.org/data/doc/gstreamer/0.10.36/gstreamer/html/GstPipeline.html#gst-pipeline-get-bus

[Chris Dumez]

Anyway, I am going to rebuild with gstreamer 0.10 and make sure it does not hit assertions. It seems that the gstreamer 0.10 documentation can be inaccurate (c.f. Bug 107554).

[Chris Dumez]

(In reply to comment #16)
> (In reply to comment #15)
> > Based on Bug 105869, it seems adopting the GstBus returned by gst_pipeline_get_bus() was causing assertions before on EFL port... I cannot reproduce these assertions anymore so it may be specific to gstreamer 0.10. Probably this needs some testing with gstreamer 0.10 as well...
> 
> Ah!
> Could it be that in 0.10 we get a floating ref and not in 1.0?

Ok, it is confirmed:

STDERR: ASSERTION FAILED: !gstObjectIsFloating(GST_OBJECT(ptr))
STDERR: /home/chris/Devel/WebKit/Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.cpp(130) : WTF::GRefPtr<T> WTF::adoptGRef(T*) [with T = _GstBus]
STDERR: 1   0x7f08f2c9f422 WTF::GRefPtr<_GstBus> WTF::adoptGRef<_GstBus>(_GstBus*)
STDERR: 2   0x7f08f2c9f89f WebCore::GStreamerGWorld::GStreamerGWorld(_GstElement*)
STDERR: 3   0x7f08f2c9f801 WebCore::GStreamerGWorld::createGWorld(_GstElement*)
STDERR: 4   0x7f08f2ca7ebe WebCore::MediaPlayerPrivateGStreamer::createGSTPlayBin()
STDERR: 5   0x7f08f2ca1afe WebCore::MediaPlayerPrivateGStreamer::load(WTF::String const&)
STDERR: 6   0x7f08f2229566 WebCore::MediaPlayer::loadWithNextMediaEngine(WebCore::MediaPlayerFactory*)
STDERR: 7   0x7f08f2229275 WebCore::MediaPlayer::load(WebCore::KURL const&, WebCore::ContentType const&, WTF::String const&)
STDERR: 8   0x7f08f1d7c217 WebCore::HTMLMediaElement::loadResource(WebCore::KURL const&, WebCore::ContentType&, WTF::String const&)
STDERR: 9   0x7f08f1d7ba2b WebCore::HTMLMediaElement::selectMediaResource()
STDERR: 10  0x7f08f1d7b759 WebCore::HTMLMediaElement::loadInternal()
STDERR: 11  0x7f08f1d7ae43 WebCore::HTMLMediaElement::loadTimerFired(WebCore::Timer<WebCore::HTMLMediaElement>*)
STDERR: 12  0x7f08f1d99dfc WebCore::Timer<WebCore::HTMLMediaElement>::fired()
STDERR: 13  0x7f08f21b6c3a WebCore::ThreadTimers::sharedTimerFiredInternal()
STDERR: 14  0x7f08f21b6b5b WebCore::ThreadTimers::sharedTimerFired()
STDERR: 15  0x7f08f2c8a485
STDERR: 16  0x7f08f69d551e _ecore_timer_expired_call
STDERR: 17  0x7f08f69d56eb _ecore_timer_expired_timers_call
STDERR: 18  0x7f08f69d25f1
STDERR: 19  0x7f08f69d2c87 ecore_main_loop_begin
STDERR: 20  0x7f08f2c88d45 WebCore::RunLoop::run()
STDERR: 21  0x7f08f649d2ca WebProcessMainEfl
STDERR: 22  0x4007c4 main
STDERR: 23  0x7f08f56b376d __libc_start_main

gstreamer 0.10 does return a floating reference so I guess I'll need #ifdefs...

[Chris Dumez]

I think the cleanest would be to add a "GRefPtr<GstBus> webkitGstPipelineGetBus(GstPipeline*)" function to GstreamerVersioning.h to limit the number of #ifdefs. I'll reupload a fixed patch soon.

[Chris Dumez]

Created attachment 184002 [details]
Patch

Make patch work with gstreamer 0.10 as well.

[Philippe Normand]

Comment on attachment 184002 [details]
Patch

Nice!

[WebKit Review Bot]

Comment on attachment 184002 [details]
Patch

Clearing flags on attachment: 184002

Committed r140443: <http://trac.webkit.org/changeset/140443>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Philippe Normand]

I wonder if this patch introduced bug 110015

[Chris Dumez]

(In reply to comment #25)
> I wonder if this patch introduced bug 110015

If you look at the bug description, you will see that the backtrace this patch is fixing is the same as the one from Bug 110015. Therefore, I doubt that my patch introduced this issue, however, I guess it is possible it did not (fully?) fix it?

Do we know for sure that the WebKit version used in Bug 110015 has this fix already?

[Philippe Normand]

(In reply to comment #26)
> (In reply to comment #25)
> > I wonder if this patch introduced bug 110015
> 
> If you look at the bug description, you will see that the backtrace this patch is fixing is the same as the one from Bug 110015. Therefore, I doubt that my patch introduced this issue, however, I guess it is possible it did not (fully?) fix it?
> 
> Do we know for sure that the WebKit version used in Bug 110015 has this fix already?

1.11.5 is r144035. So yes, it has this patch in.

I think many media elements are created and kept around in memory, so they're not freed, the bus watches are not removed, and we reach the limit of too many open fds :(

So yeah, now I don't think this patch introduced a regression, sorry for the noise.

[Philippe Normand]

*** Bug 110015 has been marked as a duplicate of this bug. ***