Closed Bug 1783421 Opened 2 years ago Closed 2 years ago

Caching an hb_face_t in gfxFontEntry is not thread-safe

Categories

(Core :: Graphics: Text, defect)

defect

Tracking

()

RESOLVED FIXED
105 Branch
Tracking Status
firefox-esr102 --- disabled
firefox103 --- disabled
firefox104 --- disabled
firefox105 --- fixed

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: nobody → jfkthame
Status: NEW → ASSIGNED
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
Crash Signature: [@ hb_ot_metrics_get_position ]
Crash Signature: [@ hb_ot_metrics_get_position ] → [@ hb_ot_metrics_get_position ] [@ hb_shape_plan_key_t::init ]
Crash Signature: [@ hb_ot_metrics_get_position ] [@ hb_shape_plan_key_t::init ] → [@ hb_ot_metrics_get_position ] [@ hb_shape_plan_key_t::init ] [@ _hb_ot_metrics_get_position_common ]
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
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 105 Branch
Flags: needinfo?(jfkthame)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: