bug#73752: 29.4; Ligatures are randomly rendered with extra spaces

[Tim Ruffing <dev@real-or-random.org>]

[-- Attachment #1: Type: text/plain, Size: 2648 bytes --]

On Wed, 2024-11-06 at 15:11 +0200, Eli Zaretskii wrote:
> 
> 
> Thanks.  Can you try calling hb_font_destroy in
> ftcrhbfont_end_hb_font
> and setting ftcrfont_info->hb_font to NULL right after that?  If that
> solves the problem, we could at least install this for now, until we
> have a better solution (if one exists).

Yes, that appears to work. And I don't think there's an obvious better
solution. See attached patch.

And I think I understand the root cause now:

What the cairo backend persists is a cairo_font_face_t, from which it
will obtain an FT_Face on demand using cairo_ft_scaled_font_lock_face
(and cairo_ft_scaled_font_unlock_face), e.g., to pass it to
hb_ft_font_create_referenced to create a hb_font_t. The lock/unlock
interface implies that we should not keep a reference to the FT_Face in
hb_font_t after cairo_ft_scaled_font_unlock_face, and so we should not
persist the hb_font_t.

Here's an unconfirmed theory what actually happens under the hood: 

Cairo internally caches an unscaled FT_Face and scales it on demand to
a specific size. That means that cairo_ft_scaled_font_lock_face will
scale the one FT_Face to which we still hold possibly multiple
references (one for each size) inside the hb_font_t object. Since we
call cairo_ft_scaled_font_lock_face every time before accessing the
hb_font_t, we happen to see the scaled version with the correct size
every time. So far we were lucky.

But Cairo's cache is limited to (currently) 10 elements [1]. As a
result, if many fonts are used, Cairo will evict a random cached
FT_Face, and later create a new one if requested via 
cairo_ft_scaled_font_lock_face. In that case, we hold a reference to
the outdated FT_Face, which won't be scaled any longer by
cairo_ft_scaled_font_lock_face, and thus we pass a face with the wrong
size to harfbuzz. 

The caching logic also explains why removing the unlock call makes the
problem disappear: Cairo won't evict the FT_Face of locked
cairo_scaled_font_t objects, even if this means going over 10.

This suggests that removing the cairo_ft_scaled_font_unlock_face call
is another fix, probably slightly more efficient, but also somewhat
frivolous. The docs explicitly say this: "Since the FT_Face object can
be shared between multiple cairo_scaled_font_t objects, you must not
lock any other font objects until you unlock this one." Though, if you
look at the implementation, they release the mutex explicitly, so there
can't be any deadlocks in the end. (Not suggesting we should rely on
this...)

[1] See MAX_OPEN_FACES in src/cairo-ft-font.c in the Cairo source code.


[-- Attachment #2: 0001-Fix-additional-spacing-in-compositions-Cairo-backend.patch --]
[-- Type: text/x-patch, Size: 1225 bytes --]

From b52b47453665fbc088dcd570ffb79ac755dfbe8f Mon Sep 17 00:00:00 2001
From: Tim Ruffing <dev@real-or-random.org>
Date: Thu, 7 Nov 2024 03:09:09 +0100
Subject: [PATCH] Fix additional spacing in compositions (Cairo backend)

* src/ftcrfont.c (ftcrhbfont_end_hb_font): Don't persist the result of
cairo_ft_scaled_font_lock_face in violation of the API contract.
(Bug#73752)
---
 src/ftcrfont.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/src/ftcrfont.c b/src/ftcrfont.c
index 3700154e44a..ee111d18763 100644
--- a/src/ftcrfont.c
+++ b/src/ftcrfont.c
@@ -708,6 +708,13 @@ ftcrhbfont_end_hb_font (struct font *font, hb_font_t *hb_font)
   struct font_info *ftcrfont_info = (struct font_info *) font;
   cairo_scaled_font_t *scaled_font = ftcrfont_info->cr_scaled_font;
 
+  eassert (hb_font == ftcrfont_info->hb_font);
+  /* ftcrfont_info->hb_font holds a reference to the FT_Face returned by
+     cairo_ft_scaled_font_lock_face. Keeping it around after the
+     matching unlock call would violate the API contract (Bug#73752). */
+  hb_font_destroy (ftcrfont_info->hb_font);
+  ftcrfont_info->hb_font = NULL;
+
   cairo_ft_scaled_font_unlock_face (scaled_font);
   ftcrfont_info->ft_size = NULL;
 }
-- 
2.47.0