gfxHarfBuzzShaper leaks the font's vmtx table (if present) when doing vertical shaping

RESOLVED FIXED in Firefox 39

Status

()

Core
Graphics: Text
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jfkthame, Assigned: jfkthame)

Tracking

unspecified
mozilla39
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
This is the source of the WinXP debug reftest orange that occurred when trying to land bug 1134598. It doesn't show up on platforms that don't use the gfxFontEntry table cache (e.g. OS X and Windows with DirectWrite), because our leak checking doesn't know about harfbuzz hb_blob_t objects; but the font table cache that we use with the GDI backend detects it.

The problem arises because gfxHarfBuzzShaper::InitializeVertical(), which loads the vmtx table, is only intended to be called once; there's even a boolean flag mVerticalInitialized provided to support this. But it fails to set/check that flag at all! The result is that we call InitializeVertical on every ShapeText(), and thus bump up the reference count of the vmtx table's hb_blob_t so that it doesn't get deleted at the appropriate time.
(Assignee)

Comment 1

3 years ago
Created attachment 8572895 [details] [diff] [review]
Ensure gfxHarfBuzzShaper only loads the vmtx table once, to avoid leaking it
Attachment #8572895 - Flags: review?(smontagu)
(Assignee)

Updated

3 years ago
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED

Comment 2

3 years ago
Hmmm, any way we can track hb_blob objects so we don't hit this sort of thing in the future?
(Assignee)

Comment 3

3 years ago
(In reply to John Daggett (:jtd) from comment #2)
> Hmmm, any way we can track hb_blob objects so we don't hit this sort of
> thing in the future?

We could use some kind of smart pointer instead of plain hb_blob_t* for our members/variables, which could have prevented the leak here by automatically destroying the old reference when assigning a new value to the mVmtxTable field. But that would have silently concealed the fact that we're actually wasting time on every ShapeText() call by repeatedly doing the InitializeVertical() work.

I suppose we could have made the problem here visible across all platforms if we ensure that whenever we create an hb_blob in the various platform overrides of gfxFontEntry::GetFontTable(), we add an explicitly-tracked object to its userData. For Windows/DWrite, we can simply add logging to the FontTableRec that we attach, but for OS X we'd need to wrap the CFDataRef in an additional object to achieve this. I wouldn't want to do that for release builds; we could do it for debug builds within an #ifdef, I guess.

Let's consider that in a followup; I'd be prepared to work on (or review) a patch to improve tracking/reporting, but in any case we need to fix the specific issue here so that other work (getting tests enabled!) can move forward.
(Assignee)

Comment 4

3 years ago
I've filed bug 1139824 with a patch to improve this tracking.
(Assignee)

Comment 5

3 years ago
Tryserver run confirming this resolves the WinXP reftest leak that caused bug 1134598 to bounce when I tried to land it previously:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=0b9ff650d0d7
Attachment #8572895 - Flags: review?(smontagu) → review+
https://hg.mozilla.org/mozilla-central/rev/778ecd7236a2
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.