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

Thanks for filing this, Jonathan.

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.

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.

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

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.

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

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.

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

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

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