Bug 44757 - [GStreamer] ImageGStreamer doesn't need to hold a Cairo surface
Summary: [GStreamer] ImageGStreamer doesn't need to hold a Cairo surface
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
Depends on:
Reported: 2010-08-27 02:24 PDT by Philippe Normand
Modified: 2010-09-01 07:13 PDT (History)
1 user (show)

See Also:

proposed patch (3.09 KB, patch)
2010-08-27 02:43 PDT, Philippe Normand
xan.lopez: 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 2010-08-27 02:24:54 PDT
The BitmapImage created already holds it and destroy it when needed. So there could be cases where ImageGStreamerCairo tries to free the already freed surface in its destructor. It happened once on the 64-bits bot:

Thread 1 (Thread 1100):
#0  0x00007f476eb71f45 in raise () from /lib/libc.so.6
#1  0x00007f476eb74d80 in abort () from /lib/libc.so.6
#2  0x00007f476eb6b08a in __assert_fail () from /lib/libc.so.6
#3  0x00007f476fb2b046 in cairo_surface_destroy () from /usr/lib/libcairo.so.2
#4  0x00007f4774e9ed4d in ~ImageGStreamer (this=0xc64750, 
    __in_chrg=<value optimized out>)
    at ../../WebCore/platform/graphics/gstreamer/ImageGStreamerCairo.cpp:69
#5  0x00007f4774e9c197 in WTF::RefCounted<WebCore::ImageGStreamer>::deref (
    this=0xc64750) at ../../JavaScriptCore/wtf/RefCounted.h:139
#6  0x00007f4774e9c006 in WTF::derefIfNotNull<WebCore::ImageGStreamer> (
    ptr=0xc64750) at ../../JavaScriptCore/wtf/PassRefPtr.h:58
#7  0x00007f4774e9be85 in ~RefPtr (this=0x7fffffff9260, 
    __in_chrg=<value optimized out>) at ../../JavaScriptCore/wtf/RefPtr.h:58
#8  0x00007f4774e9a857 in WebCore::MediaPlayerPrivateGStreamer::paint (
    this=0xbf2c30, context=0x7fffffffb350, rect=...)
    at ../../WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1218
#9  0x00007f4774cabc02 in WebCore::MediaPlayer::paint (this=0xa2f0c0, 
    p=0x7fffffffb350, r=...)
    at ../../WebCore/platform/graphics/MediaPlayer.cpp:549
#10 0x00007f4774cb8e36 in WebCore::RenderVideo::paintReplaced (this=0xbab678, 
    paintInfo=..., tx=0, ty=-53)
    at ../../WebCore/rendering/RenderVideo.cpp:207
#11 0x00007f4774b9d33e in WebCore::RenderReplaced::paint (this=0xbab678, 
    paintInfo=..., tx=0, ty=-53)
Comment 1 Philippe Normand 2010-08-27 02:43:57 PDT
Created attachment 65694 [details]
proposed patch
Comment 2 Philippe Normand 2010-08-31 03:37:45 PDT
It happened again with media/video-seek-past-end-paused.html on the 30th of august:

Comment 3 Xan Lopez 2010-09-01 03:25:27 PDT
Comment on attachment 65694 [details]
proposed patch

I don't quite see in the code who will be handling the lifecycle of the surface. Can you point me to it?
Comment 4 Xan Lopez 2010-09-01 03:33:08 PDT
Comment on attachment 65694 [details]
proposed patch

OK, I think it's in ImageCairo.cpp, the BitmapImage constructor steals the surface (I suppose this was desired, seems a bit obscure).
Comment 5 Philippe Normand 2010-09-01 07:13:24 PDT
Thanks, see http://trac.webkit.org/changeset/66600 \m/ \m/