Closed Bug 1383767 Opened 7 years ago Closed 7 years ago

FreeType rasterization is not thread-safe

Categories

(Core :: Graphics, enhancement, P3)

Unspecified
Linux
enhancement

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: lsalzman, Assigned: lsalzman)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(2 files)

When trying to rasterize glyphs with FreeType, the LCD filter state is set globally in the FT library. With OMTP, this is not safe with multiple threads potentially vying for access to one global FT library.

The other issue is that Cairo holds a per-face lock internally when doing many operations that might mutate the face. However, cairo_ft_scaled_font_lock_face has this annoying behavior that it releases its mutex on return, so that another thread could jump in and change the face while we're still using it. Cairo expects us to add additional locking to cope with this, but this would then require having to lock down any Cairo usage that may cause it to internally lock/modify the face.

Overall, it's far simpler to just change tree Cairo's behavior to not release the mutex in between the call to cairo_ft_scaled_font_lock_face and unlock_face, so Cairo can't step on Gecko, and Gecko can't on Cairo. The downside is that Cairo's per-face mutex is not recursive, and since now we'd actually be holding this mutex, we would need to resolve cases within thebes where we use gfxFT2LockedFace in a recursive manner to avoid deadlocking.
This implements the discussed changes in FT locking. So now Cairo's cairo_ft_scaled_font_lock_face will leave the mutex held on return, until the corresponding unlock_face is called. The issue of recursive locking is dealt with in a subsequent patch inside thebes.

The other aspect is that we need to lock down glyph rasterization using the LCD filter, so there are now some C hooks for acquiring/releasing the global FT lock inside Cairo and Skia. Therein, the places that set the LCD filter and rasterize using it need to be guarded by the global FT lock. Somewhat luckily, the LCD filter function inside FreeType only actually does anything if using FT_RENDER_MODE_LCD(_V), so the LCD filter has no effect in other render modes, and thus only those modes need to be locked.
Attachment #8889469 - Flags: review?(jmuizelaar)
Now that gfxFT2LockedFace can no longer be used recursively, we need to be more careful about where and how we hold this lock.

The biggest offender is gfxFT2LockedFace::GetMetrics, which held down the face lock and then inside GetCharExtents, by calling into cairo-ft, would as a side-effect recursively lock it again deep inside Cairo. This is no longer safe. This is resolved by moving the actual metrics setup outside the scope of the gfxFT2LockedFace and only holding the lock just long enough to get a reference to the face.

The metrics code is only accessing fields/tables within the face that are invariant with respect to the font and never change so long as the font is alive. The face's lifetime is also bound to the font, so this change is safe in that respect too. The only slight issue is the size metrics within the face, but that is resolved by just copying the structure inside the scope of the lock.

In the process I sanitized the metrics calculation to just always happen in the gfxFT2FontBase constructor, so that we don't have metrics locking the face and set up in random weird places anymore.

One more issue regarding gfxFT2Font::addRange recursively locking inside GetGlyphDataForChar which was resolved by passing the face down, rather than re-locking.
Attachment #8889475 - Flags: review?(jfkthame)
Attachment #8889469 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8889475 [details] [diff] [review]
remove cases of recursive locking of FreeType inside thebes

Review of attachment 8889475 [details] [diff] [review]:
-----------------------------------------------------------------

OK, looks reasonable enough, and I think you know more about freetype and locking requirements than I do. :)

::: gfx/thebes/gfxFT2FontBase.cpp
@@ +172,5 @@
> +
> +    FT_Face face;
> +    FT_Size_Metrics ftMetrics;
> +    {
> +      // Size metrics need to be copied within the scope of the lock to ensure

nit: thebes generally has 4-space indentation, so please make this block match the local style.
Attachment #8889475 - Flags: review?(jfkthame) → review+
Karl, ni? just to bring this to your attention, as I think this is an area where you have (had?) a good understanding of things, so any thoughts you have would be appreciated. If you see issues that make you want to override my review, feel free!
Flags: needinfo?(karlt)
Pushed by lsalzman@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/22947e9aa292
guarantee FreeType thread-safety by holding Cairo per-face lock and locking down rasterization. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/cecd14ecca85
remove cases of recursive locking of FreeType inside thebes. r=jfkthame
I agree that keeping the lock across lock/unlock sounds simplest, and should
work so long as cairo is not used on the same thread between lock and unlock,
which should suit most of Gecko's needs.
Thanks Lee for doing this, and Jeff and Jonathan for reviewing.
It's probably worth documenting the non-standard requirement to avoid cairo
usage between lock and unlock, somewhere that people are likely to find it.
gfxFT2LockedFace and CairoLockedFTFace declarations may be good candidates for
this.  Adjusting the documentation of cairo_ft_scaled_font_lock_face() would
also be helpful.

As to why metrics were initialized separately from the constructor, I suspect
gfxFontGroup may have instantiated all gfxFonts in the group, even if they
were not used.  Reading metrics required reading the font file.  I don't know
whether or not gfxFontGroup still does that.

(In reply to Lee Salzman [:lsalzman] from comment #2)
> The face's lifetime is also bound to the font, so this change is safe
> in that respect too.

Are you sure the FT_Face always remains in existence after unlock?
When the gfxFont was accessing the face through a cairo scaled font created
from a pattern, I recall cairo would keep only a fixed number of faces open.
Does something else keep the face's lifetime bound to the font now?

gfxFT2Fonts used to create the fonts from faces, and so the faces were always
alive (and files open).  I don't recall gfxFontconfigFont always doing this.
Flags: needinfo?(karlt) → needinfo?(lsalzman)
Pushed by lsalzman@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b95ff4c0ba0a
guarantee FreeType thread-safety by holding Cairo per-face lock and locking down rasterization. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/312e9e393fd3
remove cases of recursive locking of FreeType inside thebes. r=jfkthame
After consultation with Karl, restructured the metrics code so that it accesses the face/size metrics/tables within the scope of the lock, then releases the lock before doing the GetCharExtents checks without thereafter doing any more accesses to the face or its tables.

The opening of the face in gfxFT2FontBase's constructor was already occurring as well, so initializing the metrics there was not determined to incur any extra opening of faces that was not already happening.

Fixed the typo causing Android bustage too.
Flags: needinfo?(lsalzman)
Pushed by lsalzman@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce44b4cc02e7
add warning about unsupported recursive usage of gfxFT2LockedFace. r=me
https://hg.mozilla.org/mozilla-central/rev/b95ff4c0ba0a
https://hg.mozilla.org/mozilla-central/rev/312e9e393fd3
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Blocks: 1432531
You need to log in before you can comment on or make changes to this bug.