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.

Implements the global lock nastiness. Tries to be a bit pretty about it with an RAII AutoLockFreeType mechanism.

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.

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.

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.

Alternative approach that uses wrappers and does the locking internally, as suggested.

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.

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

https://hg.mozilla.org/mozilla-central/rev/f5c1bf433c6a https://hg.mozilla.org/mozilla-central/rev/cb7db33245b9