843588 - font table cache-entry management in gfxFontEntry is broken

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[Jonathan Kew [:jfkthame]]

In gfxFontEntry, we have a hashtable-based cache for font tables so that they can be shared among multiple instantiations of the same font (e.g. at different sizes). This is used on platforms where our font object can't get a direct reference to shared table data in an OS cache, so instead the font entry manages a copy of the data.
The font data is wrapped in a harfbuzz "blob", which uses a delete callback to remove itself from the fontEntry's hashtable when the last reference to it is released (i.e. the last font instance that was using it goes away). See bug 605872.

However, this mechanism is broken, in that the FontTableBlobData attached to the blob relies on a -pointer- to the hashtable entry in order to remove the entry when the table is released; see:

http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxFont.cpp#277

This is fundamentally unsafe, because the hash entry can move if the hashtable decides that it needs to reallocate its storage. When that happens, the entry pointer held in the blob data is left pointing to garbage.

This doesn't immediately cause any harm, but later, when the blob is deleted and tries to remove its reference from the hashtable, it no longer finds the right key, and so the removal fails.

This -also- doesn't immediately cause any harm, but the fontEntry is left with a table cache entry that refers to a deleted blob. So next time we instantiate a font using that same entry, and want to access that table, we find a matching entry in the hash...but its blob no longer exists. Boom.

Currently, we're mostly getting away with this because we normally only put a handful of entries into the fontEntry table cache, and so the hashtable doesn't actually need to reallocate.

The graphite shaper, however, accesses additional tables and so when we enable graphite, suddenly it becomes much more likely that we'll hit this scenario. This is why we saw a bunch of (intermittent but frequent) reftest crashes last time I tried to land the graphite testcases in bug 700022.

(I suspect this issue may have already caused a few of our intermittent reftest crashes on android/b2g, though I don't have proof of that. Currently, we only have a handful of tests in the tree that actually use graphite and hence exercise the table cache more heavily, so most of the time we get "lucky" and don't actually crash.)

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: font table cache management should not rely on hashtable entry pointers remaining valid

Instead of recording the hash entry pointer (which may change) in the FontTableBlobData, this records the table tag; given this, we can reliably remove the entry from the hashtable when the blob is destroyed. Incidentally, we can also remove the FontTableHashEntry::SaveTable method, as we no longer rely on saving unsanitized tables in this cache to bypass OTS.

Comment 2

[Jonathan Kew [:jfkthame]]

Tryserver run: https://tbpl.mozilla.org/?tree=Try&rev=0a14e1714e0c. There are various "normal" intermittent failures there, but the issue of crashiness in the text reftests (Android R4) during or shortly after the graphite testcases is -not- present.

(Compare https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=833978b3dadf, where the graphite reftests were landed -without- this fix to the font table cache.)

Comment 3

[Karl Tomlinson (:karlt)]

Comment on attachment 716582 [details] [diff] [review]
font table cache management should not rely on hashtable entry pointers remaining valid

Thanks for the analysis and fix.

Comment 4

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/99387b4b9731

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/99387b4b9731