Bug 44757

Summary: [GStreamer] ImageGStreamer doesn't need to hold a Cairo surface
Product: WebKit Reporter: Philippe Normand <pnormand>
Component: MediaAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: alex
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
proposed patch xan.lopez: review+

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:

http://webkit-bots.igalia.com/amd64/svn_66395.core-when_1283190163-_-who_DumpRenderTree-_-why_6.11551.trace.html
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/