Crash in FTUserFontData::Release when running with WebRender/BlobImage

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jrmuizel, Assigned: lsalzman)

Tracking

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Updated

2 years ago
Blocks: 1362115
(Assignee)

Comment 1

2 years ago
An outline of the motivating problem for this bug:

Skia uses one shared global glyph cache among all threads that may be using Skia. This glyph cache by itself is thread-safe, since it does all the right locking.

The problem arises with SkFontHost_cairo and its interaction with FreeType. We have, with the blob image renderer, potentially multiple cairo-ft-fonts created on different threads living in the same shared glyph cache. When a given thread wants to render text, even if it uses its own FT_Library per thread to try to be safe, it still needs to create new glyph cache entries, which will push out old glyph cache entries. When these old glyph cache entries are destroyed, they release the cairo-ft-font, which in turn calls FT_Done_Face, which in turn modifies the owning FT_Library. In short, rendering text on one thread may trigger destruction of a FreeType resource owned by a different thread, on the thread that was attempting to render text, causing a race like here.

So a rough solution is to use a global lock, or a lock per FT_Library, for any operations that may modify the FT_Library or an FT_Face owned by that library. Since rasterization and metrics is somewhat more contentious/performance sensitive, we've decided to just give each thread its own FT_Library and ensure that rasterization and metrics are done on that given thread with that FT_Library. However, that still leaves FT_New_Face/FT_Done_Face, which need to be synchronized via the lock, requiring us to annotate all such places we make those calls, and notably cairo-ft-font.c itself.
(Assignee)

Comment 2

2 years ago
Implements the global lock nastiness. Tries to be a bit pretty about it with an RAII AutoLockFreeType mechanism.
Assignee: nobody → lsalzman
Status: NEW → ASSIGNED
Attachment #8868624 - Flags: review?(jmuizelaar)
(Assignee)

Comment 3

2 years ago
Handles the other detail of giving the WR threads their own FT_Library so they can rasterize safely. The other ugly part of this is the plumbing necessary to pass it down to NativeFontResourceFontconfig where we actually create those FT_Faces on playback, but I've tried to do that as sanely and minimally as possible.
Attachment #8868627 - Flags: review?(jmuizelaar)
(Reporter)

Comment 4

2 years ago
Comment on attachment 8868624 [details] [diff] [review]
use a global lock around thread-unsafe FreeType FT_New_Face/FT_Done_Face calls

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

I feel like it might be better to have wrapper functions around FT_New_Face/FT_Done_Face that do the locking instead of having to the logging everywhere manually.
(Reporter)

Comment 5

2 years ago
Comment on attachment 8868627 [details] [diff] [review]
give each webrender blob image renderer thread its own thread-local FT_Library to work around unsafe FT_Face rasterization/metrics calls

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

Well this will win no prizes at a beauty contest but it gets the job done.
Attachment #8868627 - Flags: review?(jmuizelaar) → review+
(Assignee)

Comment 6

2 years ago
Alternative approach that uses wrappers and does the locking internally, as suggested.
Attachment #8868712 - Flags: review?(jmuizelaar)
(Reporter)

Comment 7

2 years ago
Comment on attachment 8868712 [details] [diff] [review]
implement thread-safe variants of FT_New_Face/FT_Done_Face

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

I like this better.
Attachment #8868712 - Flags: review?(jmuizelaar) → review+
(Reporter)

Updated

2 years ago
Attachment #8868624 - Flags: review?(jmuizelaar) → review-
(Assignee)

Updated

2 years ago
Attachment #8868624 - Attachment is obsolete: true

Comment 8

2 years ago
Pushed by lsalzman@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5c1bf433c6a
implement thread-safe variants of FT_New_Face/FT_Done_Face. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb7db33245b9
give each webrender blob image renderer thread its own thread-local FT_Library to work around unsafe FT_Face rasterization/metrics calls. r=jrmuizel

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f5c1bf433c6a
https://hg.mozilla.org/mozilla-central/rev/cb7db33245b9
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Updated

2 years ago
Depends on: 1365935
You need to log in before you can comment on or make changes to this bug.