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

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[Jonathan Kew [:jfkthame]]

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.

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: Ensure gfxHarfBuzzShaper only loads the vmtx table once, to avoid leaking it

Comment 2

[John Daggett (:jtd)]

Hmmm, any way we can track hb_blob objects so we don't hit this sort of thing in the future?

Comment 3

[Jonathan Kew [:jfkthame]]

(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.

Comment 4

[Jonathan Kew [:jfkthame]]

I've filed bug 1139824 with a patch to improve this tracking.

Comment 5

[Jonathan Kew [:jfkthame]]

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

Comment 6

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/778ecd7236a2

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/778ecd7236a2