After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 695636 - _gdk_windowing_create_cairo_surface() may return already finished surfaces
_gdk_windowing_create_cairo_surface() may return already finished surfaces
Product: gtk+
Classification: Platform
Component: Backend: Win32
Other Windows
: Normal normal
: ---
Assigned To: gtk-win32 maintainers
Depends on:
Reported: 2013-03-11 15:31 UTC by Aleksander Morgado
Modified: 2015-03-08 20:05 UTC
See Also:
GNOME target: ---
GNOME version: ---

Patch for the issue. (2.87 KB, patch)
2013-03-11 15:32 UTC, Aleksander Morgado
none Details | Review
Same patch, without g_message() :) (2.76 KB, patch)
2013-03-11 15:38 UTC, Aleksander Morgado
none Details | Review
Updated patch. (3.81 KB, patch)
2013-03-15 10:13 UTC, Aleksander Morgado
reviewed Details | Review
monodevelop logs when triggering the issue (5.95 KB, text/plain)
2013-03-15 17:20 UTC, Aleksander Morgado
Tester to reproduce the issue (4.25 KB, patch)
2013-03-19 16:58 UTC, Aleksander Morgado
none Details | Review
Screenshot with and without this patch on windows xp (25.97 KB, image/png)
2014-02-05 06:50 UTC, Andrei Fadeev

Description Aleksander Morgado 2013-03-11 15:31:45 UTC
In the win32 implementation, _gdk_windowing_create_cairo_surface() will return a new reference to the already available cairo surface in the GdkDrawable, but this surface may already be finished (i.e. in CAIRO_STATUS_SURFACE_FINISHED), and therefore is quite unusable.

Instead, _gdk_windowing_create_cairo_surface() should always create a fresh cairo surface.
Comment 1 Aleksander Morgado 2013-03-11 15:32:39 UTC
Created attachment 238586 [details] [review]
Patch for the issue.
Comment 2 Aleksander Morgado 2013-03-11 15:38:11 UTC
Created attachment 238590 [details] [review]
Same patch, without g_message() :)
Comment 3 Michael Natterer 2013-03-12 10:18:59 UTC
I don't think it should clear the previous surface, no other backend
is doing that. And we should probably avoid assertions in libraries.
Comment 4 Aleksander Morgado 2013-03-12 12:19:17 UTC
My first approach was actually to avoid clearing the previous surface, but I ended up doing it in order to properly balance out the HDC count (impl->hdc_count), which is win32-specific.

Basically, when you create a new surface we need to increase the HDC count, and with cairo_surface_set_user_data() we add a callback to get the count decreased when the GdkDrawing no longer makes use of the surface. If we just created a new surface and returned it without clearing the one in the GdkDrawing, we end up with the HDC count acquired for the new surface never released, which ends up assert-ing in _gdk_win32_drawable_finish(). That's why I added the logic to clear up the previous surface and keeping the new one internally instead of just returning it.

If we cannot clear the previous surface, we may end up needing to keep track of all created surfaces in the GdkDrawing, so that we balance the HDC count properly. I'll think about the issue a bit further, anyway.
Comment 5 Michael Natterer 2013-03-12 14:30:05 UTC
I see, but I think we definitely can't just kill the drawable's surface.
Comment 6 Aleksander Morgado 2013-03-15 10:13:33 UTC
Created attachment 238966 [details] [review]
Updated patch.

So, this new patch avoids to clear up the internal surface. Instead, we just create a new surface and return it.

In order to balance out the HDC count, I added a new user_data key which will just release the acquired DC. Still, for the case where the cairo surface is kept in impl->cairo_surface, we keep the previous user_data key to clear that internal pointer.

I also removed the assert() checking that impl->hdc_count is zero when we call _gdk_win32_drawable_finish(), as there may still be surfaces created with _gdk_windowing_create_cairo_surface() out there which will release the DC count themselves when they're destroyed.
Comment 7 Aleksander Morgado 2013-03-15 17:20:06 UTC
Created attachment 238993 [details]
monodevelop logs when triggering the issue

Some additional info regarding the original place where this issue was found...

The bug is found in XamarinStudio/Monodevelop under win32 (so GTK#). To reproduce it, just open a project in monodevelop, click on the 'Solution' dock in the left side of the window to open it, hide it, and when back in the main window the tabs in the custom notebook widget stop working.

On mouse move events, these tabs will create a cairo context just to see if the pointer is within the area of each tab, and the root problem is that when these contexts are created, the underlying cairo surface of the window is already finished. Showing and hiding the 'Solution' dock triggers the surface finish (see attached logs), mainly because _gdk_window_destroy_hierarchy() calls gdk_window_drop_cairo_surface() and this last one calls cairo_surface_finish().
Comment 8 Aleksander Morgado 2013-03-19 16:58:12 UTC
Created attachment 239273 [details] [review]
Tester to reproduce the issue

Owen asked me a tester to reproduce the issue and I came up with this one. The tester, attached, basically does this:

 * Create a GtkEventBox
 * Get a cairo context (ctx#1) from the GtkEventBox
 * Destroy the GtkEventBox
 * Create a new GtkEventBox
 * Get a cairo context (ctx#2) from the GtkEventBox
 * This context is already in SURFACE_FINISHED state
 * Destroy both cairo contexts ctx#2 and ctx#1

From now on any the context that is retrieved from the GtkEventBox (e.g. through clicking on the label in the app window) will also be in SURFACE_FINISHED state.

So it seems an issue with destroying the widget from where we got the first cairo context while a reference to the context is still valid, even if we destroy that context afterwards.

The previous patch I suggested seems to fix the issue, btw.
Comment 9 Aleksander Morgado 2013-03-19 17:26:33 UTC
I run the same reproducer under gtk+3/win32 and there is no issue. So the problem really seems only to be in gtk-2-24 *and* the win32 backend.
Comment 10 Carlos Garnacho 2013-07-12 11:00:13 UTC
Comment on attachment 238966 [details] [review]
Updated patch.

I've reviewed and tested this patch extensively, and it does look completely right to me, it does bring the same surface lifetime semantics on win32 than on x11.

Before the patch:
on X11:
- _gdk_windowing_create_cairo_surface() returns a new, Xlib surface, attaches no data to it, so destroying it doesn't trigger any GDK code.
- ref_cairo_surface() returns a ref to impl->cairo_surface, if that doesn't exist, it is created through the former and surface data is set so impl->cairo_surface is reset back to NULL when the surface is destroyed

on Win32:
- _gdk_windowing_create_cairo_surface() merely calls ref_cairo_surface(), which,
- ref_cairo_surface() creates impl->cairo_surface, in a similar way to X11

So the bug really is that both calls return impl->cairo_surface, even on callers that expect a surface of their own, and destroying those surfaces will then finish/erase the drawable surface at odd times.

With aleksander's patch the call order is inverted, just like in X11, and HDC refcount handling is separated from other surface data. It looks like the current code attempts to unify HDC refcount on both calls, or tries really hard to keep HDC refcount on control of GDK, the patch may help a bit less on the second (eg. leaking windowing_create_cairo_surface() surfaces), but returning impl->cairo_surface anytime sounds like a misbehavior
Comment 11 Carlos Garnacho 2013-07-12 11:32:58 UTC
(In reply to comment #10)
> second (eg. leaking windowing_create_cairo_surface() surfaces), but returning
              ^^ "if you leak", to avoid confusions, this patch itself doesn't leak anything
Comment 12 Matthias Clasen 2013-07-12 11:43:39 UTC
Thanks for the testing, Carlos. I'm not really in a good position to ack win32 patches, but if this works, is tested and fixes a real issue, we should get it in.
Comment 13 Andrei Fadeev 2014-02-05 06:50:54 UTC
Created attachment 268127 [details]
Screenshot with and without this patch on windows xp

The use of this patch leads to various visual artifacts on win32 platform. GTK version 2.24.22 with removed patch (left side of screenshot) does not have this problem, while the original version 2.24.22 (right side) has.

Builded with MinGW GCC 4.7.2 and
Pixman lib - 0.32.4,
Cairo lib - 1.12.16,
Gdk-pixbuf lib - 2.26.5,
Pango lib - 1.30.1,
Atk lib - 2.4.0.
Comment 14 Patrick Griffis (tingping) 2014-05-29 14:51:48 UTC
This patch causes rendering issues for me as well.
Comment 15 Jacob Nevins 2015-02-08 15:40:54 UTC
Apparent knock-on problems from this fix raised separately as bug #744169.
Comment 16 Ignacio Casal Quinteiro (nacho) 2015-02-22 09:21:37 UTC
I am reopening this since it is reverted in msys2 and reverted on the msvc binaries distribution. TingPing can you please give some more info about the rendering problems here?
Comment 17 Patrick Griffis (tingping) 2015-02-22 09:24:35 UTC
(In reply to Ignacio Casal Quinteiro (nacho) from comment #16)
> TingPing can you please give some more info about the rendering problems here?

It has been a while but if you build HexChat with this commit the treeview
doesn't full render or something like that.
Comment 18 Patrick Griffis (tingping) 2015-02-22 09:27:18 UTC
Ah the picture in comment #13 is what I remember, the text in the treeview did not render or had glitching rendering.
Comment 19 Jacob Nevins 2015-02-22 09:35:20 UTC
Bug #744169 links to the problems we had in Freeciv, including screenshots. We've been stuck on Gtk+ 2.24.20 due to this, but now it's reverted we can consider using the latest version (2.24.26).

I believe bug #741060 is where this fix was reverted, and gives rationale.
Comment 20 Ignacio Casal Quinteiro (nacho) 2015-02-22 09:35:33 UTC
Just for reference this seems to be gtk2 specific since in gtk3 there is no such patch: and I haven't experienced those glitches.
Comment 21 Ignacio Casal Quinteiro (nacho) 2015-02-22 09:37:11 UTC
TingPing, can you double check 2.24.26 fixes this problem for you too?
Comment 22 Aleksander Morgado 2015-02-23 11:45:03 UTC
Hey! Sorry my original patch made so much noise :/ Looks like alexl didn't revert it, but pushed a fix on top of it?
Comment 23 Ignacio Casal Quinteiro (nacho) 2015-02-23 11:46:58 UTC
Hey, yeah it seems alex made a fix. I asked the hexchat guys to test the new tarball so let's see what they reply. I've also asked msys2 maintainer to update it so once it is updated I can also update it. I'll leave this bug opened until we get a confirmation.
Comment 24 Ignacio Casal Quinteiro (nacho) 2015-03-08 20:05:56 UTC
Closing this since it was indeed fixed by alex.