Bug 18947 - cairo.SurfacePattern should INCREF the used surface
Summary: cairo.SurfacePattern should INCREF the used surface
Status: RESOLVED FIXED
Alias: None
Product: pycairo
Classification: Unclassified
Component: general (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Steve Chaplin
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-12-08 04:15 UTC by Gustavo J. A. M. Carneiro
Modified: 2009-06-08 20:27 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
demonstration (1.61 KB, text/plain)
2008-12-09 03:08 UTC, Gustavo J. A. M. Carneiro
Details
simpler test case (703 bytes, text/plain)
2008-12-09 03:30 UTC, Gustavo J. A. M. Carneiro
Details

Description Gustavo J. A. M. Carneiro 2008-12-08 04:15:45 UTC
Consider an innocent looking function to create a stipple pattern:

def create_stipple():
    color = gtk.gdk.color_parse ("mediumseagreen")
    
    stipple_data = array.array('B', [0, 0, 0, 255,   0, 0, 0, 0,
                                     0, 0, 0, 0,   0, 0, 0, 255])
    
    stipple_data[2] = stipple_data[14] = color.red >> 8
    stipple_data[1] = stipple_data[13] = color.green >> 8
    stipple_data[0] = stipple_data[12] = color.blue >> 8

    surface = cairo.ImageSurface.create_for_data (stipple_data,
                                                  cairo.FORMAT_ARGB32,
                                                  2, 2, 8)
    pattern = cairo.SurfacePattern(surface)
    pattern.set_extend (cairo.EXTEND_REPEAT)
    
    return pattern

This function is buggy because stipple_data will go out of scope and leave the ImageSurface pointing at garbage.  Worrying about reference counting is not Pythonic, therefore cairo.ImageSurface.create_for_data should INCREF the data parameter and schedule it to be DECREFed when the surface is destroyed.
Comment 1 Steve Chaplin 2008-12-08 19:59:50 UTC
I'm aware of the problems of data used by pycairo going out of scope and causing problems. Pycairo already deals correctly with these situations, AFAIK.

Have a look at pycairo-surface.c
image_surface_create_for_data() reads stipple_data into 'obj'
PycairoSurface_FromSurface(surface, obj) is called and 'obj' is received as the 'base' argument
Py_XINCREF(base);  is called to keep the object alive.

At the end of the ImageSurface's life, in surface_dealloc(), Py_CLEAR(o->base); is called to release the object.

Have a read through the code to see if you agree that its already working OK. Or see if you can write a test case that demonstrates pycairo reading corrupted data.
Comment 2 Gustavo J. A. M. Carneiro 2008-12-09 03:08:11 UTC
Created attachment 20951 [details]
demonstration

With this program I get:

gjc@dark-tower:gnome$ python /tmp/test_goocairo.py 
Creating pattern
[113, 179, 60, 255, 0, 0, 0, 0, 0, 0, 0, 0, 113, 179, 60, 255]
data is no more <weakref at 0x2a49720; dead>
Pattern created
Deleting pattern
Pattern deleted
[58712 refs]

Which clearly demonstrates that data is being freed too soon.

I'm using pycairo 1.4.12.
Comment 3 Gustavo J. A. M. Carneiro 2008-12-09 03:30:30 UTC
Created attachment 20954 [details]
simpler test case
Comment 4 Gustavo J. A. M. Carneiro 2008-12-09 03:33:37 UTC
OK, it turns out that the problem is that, although the surface references the data, the pattern does not reference the surface, at least in pycairo 1.4.12.
Comment 5 Steve Chaplin 2008-12-10 00:39:50 UTC
That is a problem.
I've updated PycairoPattern to be like PycairoContext and PycairoSurface - it has a 'base' field which can be used to keep the base object alive. This fixes the problem shown in your test case, and changes the pycairo C API.
Comment 6 Daniel Macks 2009-06-08 20:27:20 UTC
Changing the API of an existing public function is causing several types of breakage and upgrade problems. See http://bugzilla.gnome.org/show_bug.cgi?id=581895


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.