Closed
Bug 1364628
Opened 8 years ago
Closed 8 years ago
Crash in FTUserFontData::Release when running with WebRender/BlobImage
Categories
(Core :: Graphics: WebRender, enhancement)
Core
Graphics: WebRender
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: jrmuizel, Assigned: lsalzman)
References
Details
Attachments
(2 files, 1 obsolete file)
12.04 KB,
patch
|
jrmuizel
:
review+
|
Details | Diff | Splinter Review |
25.51 KB,
patch
|
jrmuizel
:
review+
|
Details | Diff | Splinter Review |
Assignee | ||
Comment 1•8 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•8 years ago
|
||
Implements the global lock nastiness. Tries to be a bit pretty about it with an RAII AutoLockFreeType mechanism.
Assignee | ||
Comment 3•8 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•8 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•8 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•8 years ago
|
||
Alternative approach that uses wrappers and does the locking internally, as suggested.
Attachment #8868712 -
Flags: review?(jmuizelaar)
Reporter | ||
Comment 7•8 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•8 years ago
|
Attachment #8868624 -
Flags: review?(jmuizelaar) → review-
Assignee | ||
Updated•8 years ago
|
Attachment #8868624 -
Attachment is obsolete: true
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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f5c1bf433c6a
https://hg.mozilla.org/mozilla-central/rev/cb7db33245b9
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•