Bug 63787 - Assertion failure in cairo under windows when using GIMP's new canvas rotation
Summary: Assertion failure in cairo under windows when using GIMP's new canvas rotation
Alias: None
Product: cairo
Classification: Unclassified
Component: win32 backend (show other bugs)
Version: 1.12.14
Hardware: Other Windows (All)
: medium normal
Assignee: cairo-bugs mailing list
QA Contact: cairo-bugs mailing list
Depends on:
Blocks: cairo-1.14
  Show dependency treegraph
Reported: 2013-04-21 20:37 UTC by Michael Henning
Modified: 2013-09-15 07:04 UTC (History)
4 users (show)

See Also:
i915 platform:
i915 features:

Stack trace of bug (16.63 KB, text/plain)
2013-04-21 20:37 UTC, Michael Henning
[PATCH] win32: Fix image surface freeing loop (1.24 KB, patch)
2013-07-22 03:24 UTC, Michael Henning
Details | Splinter Review

Description Michael Henning 2013-04-21 20:37:28 UTC
Created attachment 78313 [details]
Stack trace of bug

I get an assertion failure in cairo-surface.c, line 390 "CAIRO_REFERENCE_COUNT_HAS_REFERENCE (&surface->ref_count)", when using the Gimp's new (in development) canvas rotation feature on windows.

The issue seems to be in the way the windows backend handles references when an image is destroyed.

The relevant code in the gimp is here:

I have attached a stacktrace.
Comment 1 Uli Schlachter 2013-04-22 06:34:37 UTC
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.
Comment 2 Michael Henning 2013-07-22 03:24:03 UTC
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.
Comment 3 Fan, Chun-wei 2013-07-22 07:04:47 UTC

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!
Comment 4 Chris Wilson 2013-08-23 11:57:38 UTC
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>
Comment 5 Michael Henning 2013-08-23 16:31:04 UTC
I can confirm that the fix works. Thanks!
Comment 6 John Lindgren 2013-09-15 07:04:15 UTC
Thank you!!!  This has finally fixed #49755, which has made Cairo unusable on Windows all through 1.12.x.

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.