Caching an hb_face_t in gfxFontEntry is not thread-safe
Categories
(Core :: Graphics: Text, defect)
Tracking
()
People
(Reporter: jfkthame, Assigned: jfkthame)
Details
Crash Data
Attachments
(1 file)
With font objects potentially being created and destroyed by canvas worker threads, it is no longer safe to keep a weak reference to an hb_face_t in gfxFontEntry and hand out references via GetHBFace().
In the past, we relied on the font entry's mHBFace being cleared in response to the delete callback from harfbuzz, but in the multi-threaded world that's not good enough: there's a risk that another thread may have called GetHBFace() and been handed a reference to the currently-being-deleted face before the HBFaceDeletedCallback has a chance to clear it.
Two reasonably straightforward ways to address this would be
(1) Make gfxFontEntry hold a strong reference to mHBFace. This means that once the face is created, it'd never be deleted (unless the gfxFontEntry itself is deleted, which happens for webfonts, but for installed fonts they persist until the process shuts down). So there'd probably be a fairly significant memory impact from this.
(2) Don't cache an hb_face_t in the gfxFontEntry at all. This means that when the same font is used at multiple sizes, we'll create a separate hb_face_t as well as hb_font_t for each size, instead of sharing the face. So again, some memory impact, but it'll be more transient as these faces will go away once the fonts are no longer being used and the gfxFont instances expire from cache.
I'm inclined to try (2), and see whether any performance/memory impact is noticeable.
Assignee | ||
Comment 1•2 years ago
|
||
Updated•2 years ago
|
Pushed by jkew@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6f0f72915ef9 Don't cache an hb_face_t in gfxFontEntry to share between font instances, as it is not thread-safe. r=aosmond
Updated•2 years ago
|
Updated•2 years ago
|
Updated•2 years ago
|
Comment 3•2 years ago
|
||
Backed out for causing assertion failures at hb-object.hh.
Backout link: https://hg.mozilla.org/integration/autoland/rev/ddca220ec171c86db850309b71310bc9c4d50876
Failure log: https://treeherder.mozilla.org/logviewer?job_id=386527524&repo=autoland&lineNumber=12338
Comment 4•2 years ago
|
||
There is also this failure: https://treeherder.mozilla.org/logviewer?job_id=386527443&repo=autoland&lineNumber=6370
Pushed by jkew@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/67975e339c73 Don't cache an hb_face_t in gfxFontEntry to share between font instances, as it is not thread-safe. r=aosmond
Comment 6•2 years ago
|
||
bugherder |
Assignee | ||
Updated•2 years ago
|
Description
•