Bug 63787

Someone should paint a picture about this, the reference magic makes my head dizzy:

At the cairo_restore() at the end of gimp_display_shell_render(), the current source of the gc is a surface pattern. In the rotated case, this pattern holds the only reference to the surface "xfer" which is a similar image to cairo_get_target(cr) (a win32 surface). Behind the covers, this image surface has a reference to a win32_display_surface (its parent).

Now when the pattern is destroyed, the reference count to the similar image drops to zero (This seems correct, although the behind-the-scenes reference loop with the win32_display_surface still confuses me). While finishing this image surface, it drops the reference to its parent. Thus, now the win32_display_surface gets finished, too. In _cairo_win32_display_surface_finish(), this one now tries to get rid of the similar image again, although this one is already being finished some stack frames above. And *boom*, assert() notices that something went foobar.

Whoever maintains cairo-win32 can come up with a good solution. Since these reference loops make my head hurt, I can only suggest to make _cairo_image_surface_finish() set ->parent to NULL before destroying its parent and then in _cairo_win32_display_surface_finish() check if the image surface's parent is still set before destroying the image. However, this hack smells quite fishy to me.

Created attachment 82797 [details] [review]
[PATCH] win32: Fix image surface freeing loop

Well, this is another way to fix the bug. This patch simply checks to see if the reference count is zero on the parent surface, and if it is, it won't try to free it.

I've tested the patch, and can confirm that it works.

Hi,

I think I can also confirm that Michael's patch also fixes the use of GTK+ master on Windows [1].

[1]: https://bugzilla.gnome.org/show_bug.cgi?id=704540

With blessings, thank you!

I'm not keen on using the ref-count here, and so used a more explicit check instead. Hopefully, this should keep the reference checking logic intact.


commit fb8881e84bb24b2a54ee5aa449b6f5638de36404
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Fri Aug 23 12:48:08 2013 +0100

    win32: Prevent double-free of similar images
    
    Based on a patch and analysis by Michael Henning.
    
    When we create a similar-image surface for win32, we set up a couple of
    back references from the image to the win32 surface, and vice versa. We
    need to be careful when decoupling the reference cycle to avoid chasing
    around the loop upon destruction. Currently we handled destroying the
    similar-image via the parent win32 surface, but similar precaution is
    required when destroying the surface via the similar-image.
    
    Reported-by: Michael Henning <drawoc@darkrefraction.com>
    Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=63787
    Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

I can confirm that the fix works. Thanks!

Thank you!!!  This has finally fixed #49755, which has made Cairo unusable on Windows all through 1.12.x.