605872 - leak of hb_blob objects when using downloadable fonts with opentype layout tables

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jonathan Kew [:jfkthame]]

See bug 527276 comment 50. It looks like when we squirrel away OpenType tables from downloadable fonts being sanitized, we don't ever release the hb_blob references that are created for storage in the gfxFontEntry.

(Work going on in bug 569770 and 597147 (to use harfbuzz under Linux) may affect how this data is managed, but filing this separately so we don't forget to fix it.)

Comment 1

[Karl Tomlinson (:karlt)]

Thanks for filing this, Jonathan.

Comment 2

[Karl Tomlinson (:karlt)]

It probably makes sense for me to work on this, given that patches in bug 569770 are going to need to work in with it, and I'll let Jonathan review bug 597147 ;)

I'm thinking we can manage the saving/sharing difference by keeping in the hash table both a hb_blob_t and a flag that says whether the table owns a reference.

Comment 3

[Karl Tomlinson (:karlt)]

Attachment: separate saving and sharing in the mFontTableCache

This replaces part 1 and part 2 of bug 569770, doing things a little differently.

For shared tables, weak pointers are used instead of keeping an unnecessary strong reference to the font entry.

For saved tables, the blob simply manages the data.

The hash table entry keeps track of whether the table is shared or saved.

Comment 4

[Karl Tomlinson (:karlt)]

Oh, I should mention there's a small change in shared table behavior here because missing tables now have hashtable entries, preventing subsequent lookups.
That mostly just followed naturally from the hashtable managing code.

Comment 5

[Jonathan Kew [:jfkthame]]

Comment on attachment 486852 [details] [diff] [review]
separate saving and sharing in the mFontTableCache

Hmm. Although this seems ok, I don't like the complexity it involves.... it feels like a fertile ground for future errors due to misunderstanding issues of ownership/referencing. (Fair enough, that's because the existing code simply fails to address ownership adequately, and hence leaks! But I wish we could find a simpler and clearer way to deal with it.)

ISTM the complexity is introduced because of the PreloadFontTable hack that uses the table "cache" (or sharing mechanism, if you prefer) to squirrel away tables for HarfBuzz, when OTS will discard them. So maybe we should consider other approaches to that issue, and keep the fontEntry table cache simpler (as it would only need to deal with a single model of ownership).

Given that this is needed only for (a maximum of) three specific tables per downloaded font entry, not for an open-ended collection of font tables, I'm wondering if it would make more sense to just store the three blob pointers directly into individual fields of the font entry, and let GetTable in the font do a simple switch() on the table tag to check the appropriate one of them. That would avoid introducing such complications into the hashtable and its entries - especially as this saving of layout tables is a temporary hack that we'll want to pull out again once OTS fully supports them.

What do you think? I'm open to persuasion if you have strong view on the best approach here....

Comment 6

[Karl Tomlinson (:karlt)]

To me it would seem unfortunate to have special case checks for tables that
are rarely there, and keep them in sync with which tables are saved.  If the
table exists I feel it might as well be linked via the hash table.

I actually think much of the complexity comes from the sharing and blob
interaction.  I don't know whether that can be reduced much, but I admit it
would be appealing to do so.  Contributing factors there are that the blob might
destroy its user data during creation, and there is no way to query a blob for
its user data.

I considered treating PreloadFontTable blobs like other shared data and keeping
blob references on the font entry, so that we can remove PlainTableBlobData.
However, I doubt that would simplify things are great deal.

I could merge PlainTableBlobData into SharedTableBlobData? (and probably call it something else.)  That would simplify the class hierarchy, mean that only one static destructor is necessary, and the only cost would be a couple of extra member variables on the few preloaded tables.

Comment 7

[Jonathan Kew [:jfkthame]]

(In reply to comment #6)
> To me it would seem unfortunate to have special case checks for tables that
> are rarely there, and keep them in sync with which tables are saved.

True, but I see it as a temporary workaround (until OTS has GDEF/POS/SUB support) that we will eventually want to remove, and as such it'd be nice if it didn't add complexity inside the sharing hashtable code.

> I actually think much of the complexity comes from the sharing and blob
> interaction.  I don't know whether that can be reduced much, but I admit it
> would be appealing to do so.  Contributing factors there are that the blob
> might
> destroy its user data during creation, 

Hmm, yes. But if you don't want the user-data destructor to be called in that situation (e.g. because the blob hasn't actually been put into the hashtable yet), would it be feasible to create the blob with userData pointing to an "empty" BlobData record, and then once creation (and hashtable insertion) is known to be successful, set up the relevant field(s) of the BlobData?

> and there is no way to query a blob for
> its user data.

We could request an hb_blob_get_user_data() API if that would be helpful; it's trivial to implement.

> I could merge PlainTableBlobData into SharedTableBlobData? (and probably call
> it something else.)  That would simplify the class hierarchy, mean that only
> one static destructor is necessary, and the only cost would be a couple of
> extra member variables on the few preloaded tables.

That sounds reasonable. The (small) extra cost on preloaded tables is unimportant; that mechanism is only for use with downloaded fonts, so there will generally not be more than a few such entries at any time.

Comment 8

[Karl Tomlinson (:karlt)]

Attachment: Add hb_blob_get_user_data()

This doesn't provide the read/write and thread-safety functions that are available for the core blob data, but this is user_data, so the user can decide how to manage it.

Comment 9

[Karl Tomlinson (:karlt)]

Attachment: refactor mFontTableCache, providing a sharing/saving distinction v2

When compared to the previous patch, this patch combines the two different
kinds of blob user data into one class and moves that class out of the header
file.  It also merges the hash table value class into the hash table entry
class; the resulting class manages both creation and disassociation of
blobs and is the only class that interacts with blob user data.
hb_blob_get_user_data is used to detect whether blob creation was successful.

The number of lines of code here is similar to what could have been achieved
with the previous approach, but I hope it is simpler to follow.

When compared to the current situation for shared font tables, this patch
provides the following properties.  They may not be vital but I think they
make a cleaner model.

* The blob (instead of the hashtable entry) owns the blob data (so does not
  need to care about the lifetime of the FontEntry).

* The blob user data will never try to remove its hash entry before its blob
  is inserted in the hash table (nor after it has been removed from the hash
  table).

* The hash table entry disassociates the blob user data from the hash table
  entry should the blob ever get replaced by a different blob for the same
  tag.

The saving of tables that get stripped by OTS almost comes for free.

Comment 10

[Jonathan Kew [:jfkthame]]

Comment on attachment 490031 [details] [diff] [review]
refactor mFontTableCache, providing a sharing/saving distinction v2

Looks good to me. We might need to tweak this eventually if Behdad decides to do something different with the blob user-data API, but let's take this for now at least.

Comment 11

[Karl Tomlinson (:karlt)]

blocking2.0? as this is needed for bug 569770, and also for the sake of the small leaks.

Comment 12

[Karl Tomlinson (:karlt)]

Attachment: avoid hb_blob_get_user_data

It sounds like the hb user data API may end up being quite different, so this patch modifies attachment 490031 [details] [diff] [review] such that it does not need hb_blob_get_user_data.

Instead the FontTableHashEntry keeps a pointer to the user_data when necessary, and the FontTableBlobData clears (not removes) the FontTableHashEntry if deleted during blob creation.

This patch could be folded into attachment 490031 [details] [diff] [review] and so attachment 490030 [details] [diff] [review] would not be needed.

Comment 13

[Jonathan Kew [:jfkthame]]

Comment on attachment 492481 [details] [diff] [review]
avoid hb_blob_get_user_data

Ok, I'm fine with doing it this way.

>     // Disconnect from the HashEntry (because the blob has already been
>     // removed from the hashtable) and return true if a HashEntry was being
>     // managed;
>-    PRBool ForgetHashEntry()
>+    void ForgetHashEntry()

The second part of this comment should be removed - the method doesn't return anything any more.

Comment 14

[Jonathan Kew [:jfkthame]]

Comment on attachment 490030 [details] [diff] [review]
Add hb_blob_get_user_data()

Clearing review and marking this obsolete, on the assumption you'll go with the combination of attachment 490031 [details] [diff] [review] + attachment 492481 [details] [diff] [review] instead, so that we don't need to touch the harfbuzz api.

Comment 15

[Karl Tomlinson (:karlt)]

http://hg.mozilla.org/mozilla-central/rev/1a7f8d063d1d