467838 - gfxFontEntry::mUserFontData pointer may be copied and each copy used in delete

Status: NEW

(Core :: Graphics: Text, defect)

Description

[Karl Tomlinson (:karlt)]

gfxFontEntry(const gfxFontEntry& aEntry) : 
        mName(aEntry.mName), mItalic(aEntry.mItalic), 
        mFixedPitch(aEntry.mFixedPitch), mUnicodeFont(aEntry.mUnicodeFont), 
        mSymbolFont(aEntry.mSymbolFont), mTrueType(aEntry.mTrueType),
        mIsType1(aEntry.mIsType1), mIsProxy(aEntry.mIsProxy), 
        mIsValid(aEntry.mIsValid), mIsBadUnderlineFont(aEntry.mIsBadUnderlineFont),
        mWeight(aEntry.mWeight), mCmapInitialized(aEntry.mCmapInitialized),
        mCharacterMap(aEntry.mCharacterMap), mUserFontData(aEntry.mUserFontData)
    { }

    gfxUserFontData* mUserFontData;


gfxFontEntry::~gfxFontEntry() 
{
    if (mUserFontData)
        delete mUserFontData;
}

I don't know whether we use the copy constructor, but perhaps mUserFontData should be ref counted.

Comment 1

[John Daggett (:jtd)]

Actually, I would go the opposite way, in general we shouldn't need to make copies of font entries so we should probably enforce that by making the copy constructor private.

Comment 2

[Karl Tomlinson (:karlt)]

Perhaps, but two not entirely unreasonable use cases for copy constructors that I could imagine are:

1) copying a base font entry for a synthetic font entry,

2) copying a gfxProxyFontEntry to get its properties.

I'm thinking perhaps the best solution is to move the mUserFontData to the specific subclass implementations.  Then the subclasses could have private copy constructor and assignment operators, if appropriate.

This may also be more type safe, and could reduce the number of objects (allocations) through storing directly on the font entry rather than the gfxUserFontData intermediate object.