Bug 33178

We can't change the existing API/ABI as this will break applications using cairo.

We can improve the documentation to make it clear that the application needs to unselect the font (either by selecting another font or with RestoreDC) before calling cairo_win32_scaled_font_done_font() and then destroying the font.

I'm not sure why cairo_win32_scaled_font_select_font() doesn't keep a reference to the scaled font until cairo_win32_scaled_font_done_font() is called.

(In reply to comment #0)
> Created an attachment (id=42096) [details]
> 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?

(In reply to comment #2)
> > 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 sorry -- I meant that the changes were easy to implement in the cairo-win32-font.c, not that any test code was modified.
 
> 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?

I don't think so.  I believe that if the select_font/done_font pairs are applied properly (and assuming the context is properly restored) everything would be fine.

My only (small) concern is the fact that "*_done_font" doesn't really seem to be doing anything; the API user is expected to restore the DC externally.  This seems a little inconvenient for the user, but I agree that it should work fine.

(In reply to comment #1)
> We can improve the documentation to make it clear that the application needs to
> unselect the font (either by selecting another font or with RestoreDC) before
> calling cairo_win32_scaled_font_done_font() and then destroying the font.
> 
> I'm not sure why cairo_win32_scaled_font_select_font() doesn't keep a reference
> to the scaled font until cairo_win32_scaled_font_done_font() is called.

I was thinking about this, too.  If we assume that the DC can only be interacted with from a single thread, then retaining the previously-selected-font internally would allow us to properly select the old font back into the context when done_font was called.

I think this would be clearer than SaveDC/RestoreDC around the calls, just in case some kind of drawing/scaling or other transformations need to be interleaved with font handling.

(In reply to comment #4)
> (In reply to comment #1)
> > We can improve the documentation to make it clear that the application needs to
> > unselect the font (either by selecting another font or with RestoreDC) before
> > calling cairo_win32_scaled_font_done_font() and then destroying the font.
> > 
> > I'm not sure why cairo_win32_scaled_font_select_font() doesn't keep a reference
> > to the scaled font until cairo_win32_scaled_font_done_font() is called.
> 
> I was thinking about this, too.  If we assume that the DC can only be
> interacted with from a single thread, then retaining the
> previously-selected-font internally would allow us to properly select the old
> font back into the context when done_font was called.

These function allow you to:
cairo_scaled_font_t *scaled_font;
HDC hdc1, hdc2;

/* init everything */

/* I shoud really SaveDC here */
cairo_win32_scaled_font_select_font (scaled_font, hdc1);
cairo_win32_scaled_font_select_font (scaled_font, hdc2);

draw_on (hdc1);
cairo_win32_scaled_font_done_font (scaled_font);
/*
I can restore hdc1 here, but not hdc2.
How does done_font know about this?
*/
/* Restore *MUST* happen before destroying hdc1 */
destroy(hdc1);

draw_on (hdc2);
cairo_win32_scaled_font_done_font (scaled_font);
/* Restoring here would be safe in this case, but we would never restore  */

How would you handle this case?

> 
> I think this would be clearer than SaveDC/RestoreDC around the calls, just in
> case some kind of drawing/scaling or other transformations need to be
> interleaved with font handling.

It would be clearer, but it would require an API change (it would only be
possible if done_font accepted the HDC as input). I guess we can just fix
the incorrect select_font/done_font pairs in cairo and improve documentation
tom make the need to SaveDC/RestoreDC explicit.

(In reply to comment #5)

> draw_on (hdc1);
> cairo_win32_scaled_font_done_font (scaled_font);
> /*
> I can restore hdc1 here, but not hdc2.
> How does done_font know about this?
> */
> /* Restore *MUST* happen before destroying hdc1 */
> destroy(hdc1);
> 
> draw_on (hdc2);
> cairo_win32_scaled_font_done_font (scaled_font);
> /* Restoring here would be safe in this case, but we would never restore  */
> 
> How would you handle this case?

Ah!  You are totally right -- I didn't consider this case at all.

I agree with your points.
(1) Documentation update
(2) Correct the Cairo code to save/restore.

We can close this bug, unless you want to use it to track any of the other changes you suggested.

Thanks for the quick review!

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/cairo/cairo/issues/173.