[cairo-bugs] [Bug 33178] Suggestions for cleaning HFONT handling

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=33178

--- Comment #2 from Andrea Canciani <ranma42 at gmail.com> 2011-01-16 01:14:00 PST ---
(In reply to comment #0)
> Created an attachment (id=42096)
 --> (https://bugs.freedesktop.org/attachment.cgi?id=42096)
> A suggestion for cleaning up HFONT lifetime and handling
> 
> I encountered a number of error messages from BoundsChecker under the test
> suite.
> 
> _cairo_win32_scaled_font_fini (void *abstract_font) attempts to delete the
> scaled_hfont object, but this HFONT is still selected into an active device
> context.
> 
> The cause of this specific error is that the
> cairo_win32_scaled_font_select_font function calls SelectObject to bring the
> passed font into active use for the device context.  SelectObject returns the
> previously selected font (often a stock font).  While this may be handled in
> many cases by performing a SaveDC/RestoreDC around these operations, it doesn't
> seem like this is generally done in the test suite.
> 
> Later, when the program is finished with the scaled font, a call to
> cairo_win32_scaled_font_done_font.  However, this operation does not remove the
> font from any active device contexts into which it had been selected. 
> Consequently, the later destruction of the font (when the font cache goes out
> of scope) can generate errors.

The testsuite never calls cairo_win32_scaled_font_select_font or
cairo_win32_scaled_font_done_font. The issue seems to be caused by missing
SaveDC/RestoreDC in cairo-win32-font.c.

I had started writing a patch adding SaveDC/RestoreDC "brackets" around
uses of select_font/done_font, when I noticed that I should also be checking
for errors in SaveDC/RestoreDC (see _draw_glyphs_on_surface as a reference
of one of the few functions using select_font/done_font correctly).

Alos it looks like in cairo-win32-surface.c cairo_win32_scaled_font_done_font
is missing even if cairo_win32_scaled_font_select_font is called.

> 
> I would propose the following:
> 
> 1. Have the cairo_win32_scaled_font_select_font incorporate a new out parameter
> to hold the previously-selected font for the passed device context.
> 2. Have the cairo_win32_scaled_font_done_font take two new parameters: (a) the
> device context that the scaled_font is currently selected into, and the
> original font (now passed back as an out parameter from (1).
> 
> The attached patch implements these changes.
> 
> The various test cases were easy to modify to handle these changes.  However,

What test cases did you have to modify? I can't find any change related to that
in your patch

> I'm not sure how external programs using the win32 Cairo routines would be
> impacted by this change.  From a pragmatic standpoint, the additional terms
> could be allowed to be 'null', and revert to original behavior in those cases.

This is an API change and we should try to avoid it.
Is there anything that would not be fixed by using select_font/done_font
correctly
(i.e. as specified in the documentation) in cairo itself?

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.