[oilpan] Move CSSFontFace and CSSSegmentedFontFace to the managed heap

Source/core/css/CSSFontSelector.cpp

 /*
  * Copyright (C) 2007, 2008, 2011 Apple Inc. All rights reserved.
  *           (C) 2007, 2008 Nikolas Zimmermann <zimmermann@kde.org>
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
@@ skipping 177 matching lines @@
                     traitsMask |= FontVariantSmallCapsMask;
                     break;
                 default:
                     break;
             }
         }
     } else
         traitsMask |= FontVariantMask;
 
     // Each item in the src property's list is a single CSSFontFaceSource. Put them all into a CSSFontFace.
-    RefPtr<CSSFontFace> fontFace;
+    Handle<CSSFontFace> fontFace;
 
     int srcLength = srcList->length();
 
     bool foundSVGFont = false;
 
     for (int i = 0; i < srcLength; i++) {
         // An item in the list either specifies a string (local font name) or a URL (remote font to download).
         CSSFontFaceSrcValue* item = static_cast<CSSFontFaceSrcValue*>(srcList->itemWithoutBoundsCheck(i));
         OwnPtr<CSSFontFaceSource> source;
 
@@ skipping 76 matching lines @@
                     familyName = pictographFamily;
                     break;
                 default:
                     break;
             }
         }
 
         if (familyName.isEmpty())
             continue;
 
-        OwnPtr<Vector<RefPtr<CSSFontFace> > >& familyFontFaces = m_fontFaces.add(familyName, nullptr).iterator->value;
+        // FIXME(oilpan): This is dangerous because the below 'new' can trigger a GC and
+        // some finalizer can get rid of the collection from m_fontFaces.
+        // Then subsequent access to familyFontFaces causes use-after-free.
+        // We should move the collection to the managed heap.
+        CSSFontFaceVectorCollection* familyFontFaces = m_fontFaces.add(familyName, nullptr).iterator->value.get();
         if (!familyFontFaces) {
-            familyFontFaces = adoptPtr(new Vector<RefPtr<CSSFontFace> >);
+            familyFontFaces = new CSSFontFaceVectorCollection;
+            m_fontFaces.set(familyName, adoptPtr(familyFontFaces));
 
             ASSERT(!m_locallyInstalledFontFaces.contains(familyName));
 
             Vector<unsigned> locallyInstalledFontsTraitsMasks;
             fontCache()->getTraitsInFamily(familyName, locallyInstalledFontsTraitsMasks);
             if (unsigned numLocallyInstalledFaces = locallyInstalledFontsTraitsMasks.size()) {
-                OwnPtr<Vector<RefPtr<CSSFontFace> > > familyLocallyInstalledFaces = adoptPtr(new Vector<RefPtr<CSSFontFace> >);
+                OwnPtr<CSSFontFaceVectorCollection> familyLocallyInstalledFaces = adoptPtr(new CSSFontFaceVectorCollection);
 
                 for (unsigned i = 0; i < numLocallyInstalledFaces; ++i) {
-                    RefPtr<CSSFontFace> locallyInstalledFontFace = CSSFontFace::create(static_cast<FontTraitsMask>(locallyInstalledFontsTraitsMasks[i]), nullptr, true);
+                    HandleScope scope;
+                    Handle<CSSFontFace> locallyInstalledFontFace = CSSFontFace::create(static_cast<FontTraitsMask>(locallyInstalledFontsTraitsMasks[i]), nullptr, true);
                     locallyInstalledFontFace->addSource(adoptPtr(new CSSFontFaceSource(familyName)));
                     ASSERT(locallyInstalledFontFace->isValid());
-                    familyLocallyInstalledFaces->append(locallyInstalledFontFace);
+                    (*familyLocallyInstalledFaces)->append(locallyInstalledFontFace);
                 }
 
                 m_locallyInstalledFontFaces.set(familyName, familyLocallyInstalledFaces.release());
             }
         }
 
-        familyFontFaces->append(fontFace);
+        (*familyFontFaces)->append(fontFace);
         
         ++m_version;
     }
 }
 
 void CSSFontSelector::registerForInvalidationCallbacks(FontSelectorClient* client)
 {
     m_clients.add(client);
 }
 
@@ skipping 57 matching lines @@
          genericFamily = settings->standardFontFamily(script);
 
     if (!genericFamily.isEmpty())
         return fontCache()->getCachedFontData(fontDescription, genericFamily);
 
     return 0;
 }
 
 static FontTraitsMask desiredTraitsMaskForComparison;
 
-static inline bool compareFontFaces(CSSFontFace* first, CSSFontFace* second)
+static inline bool compareFontFaces(Handle<CSSFontFace> first, Handle<CSSFontFace> second)
 {
     FontTraitsMask firstTraitsMask = first->traitsMask();
     FontTraitsMask secondTraitsMask = second->traitsMask();
 
     bool firstHasDesiredVariant = firstTraitsMask & desiredTraitsMaskForComparison & FontVariantMask;
     bool secondHasDesiredVariant = secondTraitsMask & desiredTraitsMaskForComparison & FontVariantMask;
 
     if (firstHasDesiredVariant != secondHasDesiredVariant)
         return firstHasDesiredVariant;
 
@@ skipping 69 matching lines @@
 PassRefPtr<FontData> CSSFontSelector::getFontData(const FontDescription& fontDescription, const AtomicString& familyName)
 {
     if (m_fontFaces.isEmpty()) {
         if (familyName.startsWith("-webkit-"))
             return fontDataForGenericFamily(m_document, fontDescription, familyName);
         if (fontDescription.genericFamily() == FontDescription::StandardFamily && !fontDescription.isSpecifiedFont())
             return fontDataForGenericFamily(m_document, fontDescription, "-webkit-standard");
         return 0;
     }
 
-    CSSSegmentedFontFace* face = getFontFace(fontDescription, familyName);
+    Handle<CSSSegmentedFontFace> face = getFontFace(fontDescription, familyName);
     // If no face was found, then return 0 and let the OS come up with its best match for the name.
     if (!face) {
         // If we were handed a generic family, but there was no match, go ahead and return the correct font based off our
         // settings.
         if (fontDescription.genericFamily() == FontDescription::StandardFamily && !fontDescription.isSpecifiedFont())
             return fontDataForGenericFamily(m_document, fontDescription, "-webkit-standard");
         return fontDataForGenericFamily(m_document, fontDescription, familyName);
     }
 
     // We have a face. Ask it for a font data. If it cannot produce one, it will fail, and the OS will take over.
     return face->getFontData(fontDescription);
 }
 
-CSSSegmentedFontFace* CSSFontSelector::getFontFace(const FontDescription& fontDescription, const AtomicString& family)
+Result<CSSSegmentedFontFace> CSSFontSelector::getFontFace(const FontDescription& fontDescription, const AtomicString& family)
 {
-    Vector<RefPtr<CSSFontFace> >* familyFontFaces = m_fontFaces.get(family);
-    if (!familyFontFaces || familyFontFaces->isEmpty())
-        return 0;
+    CSSFontFaceVectorCollection* familyFontFaces = m_fontFaces.get(family);
+    if (!familyFontFaces || (*familyFontFaces)->isEmpty())
+        return nullptr;
 
-    OwnPtr<HashMap<unsigned, RefPtr<CSSSegmentedFontFace> > >& segmentedFontFaceCache = m_fonts.add(family, nullptr).iterator->value;
-    if (!segmentedFontFaceCache)
-        segmentedFontFaceCache = adoptPtr(new HashMap<unsigned, RefPtr<CSSSegmentedFontFace> >);
+    // FIXME(oilpan): This is dangerous because the below 'new' can trigger a GC and
+    // some finalizer can get rid of the hashmap from m_fonts.
+    // Then subsequent access to segmentedFontFaceCache causes use-after-free.
+    // We should move the hashmap to the managed heap.
+    HashMap<unsigned, Persistent<CSSSegmentedFontFace> >* segmentedFontFaceCache = m_fonts.add(family, nullptr).iterator->value.get();
+    if (!segmentedFontFaceCache) {
+        segmentedFontFaceCache = new HashMap<unsigned, Persistent<CSSSegmentedFontFace> >;
+        m_fonts.set(family, adoptPtr(segmentedFontFaceCache));
+    }
 
     FontTraitsMask traitsMask = fontDescription.traitsMask();
 
-    RefPtr<CSSSegmentedFontFace>& face = segmentedFontFaceCache->add(traitsMask, 0).iterator->value;
-    if (!face) {
-        face = CSSSegmentedFontFace::create(this);
+    Handle<CSSSegmentedFontFace> face = segmentedFontFaceCache->add(traitsMask, nullptr).iterator->value;
+    if (face)
+        return face;
 
-        // Collect all matching faces and sort them in order of preference.
-        Vector<CSSFontFace*, 32> candidateFontFaces;
-        for (int i = familyFontFaces->size() - 1; i >= 0; --i) {
-            CSSFontFace* candidate = familyFontFaces->at(i).get();
+    face = CSSSegmentedFontFace::create(this);
+    segmentedFontFaceCache->set(traitsMask, face);
+
+    // Collect all matching faces and sort them in order of preference.
+    Vector<Handle<CSSFontFace>, 32> candidateFontFaces;

[haraken, 2013/07/03 12:03:23]

This is wrong. Handles in the vector are freed in the HandleScope in the below
for-loop. This should be CollectionRoot<Vector<Member<CSSFonFace>>>.

(I hope this kind of errors are auto-detected by a verification framework.)

[Mads Ager (google), 2013/07/03 12:18:38]

Yes, good catch.

+    for (int i = (*familyFontFaces)->size() - 1; i >= 0; --i) {
+        HandleScope scope;
+        Handle<CSSFontFace> candidate = (*familyFontFaces)->at(i);
+        unsigned candidateTraitsMask = candidate->traitsMask();
+        if ((traitsMask & FontStyleNormalMask) && !(candidateTraitsMask & FontStyleNormalMask))
+            continue;
+        if ((traitsMask & FontVariantNormalMask) && !(candidateTraitsMask & FontVariantNormalMask))
+            continue;
+#if ENABLE(SVG_FONTS)
+        // For SVG Fonts that specify that they only support the "normal" variant, we will assume they are incapable
+        // of small-caps synthesis and just ignore the font face as a candidate.
+        if (candidate->hasSVGFontFaceSource() && (traitsMask & FontVariantSmallCapsMask) && !(candidateTraitsMask & FontVariantSmallCapsMask))
+            continue;
+#endif
+        candidateFontFaces.append(candidate);
+    }
+
+    CSSFontFaceVectorCollection* familyLocallyInstalledFontFaces = m_locallyInstalledFontFaces.get(family);
+    if (familyLocallyInstalledFontFaces) {
+        unsigned numLocallyInstalledFontFaces = (*familyLocallyInstalledFontFaces)->size();
+        for (unsigned i = 0; i < numLocallyInstalledFontFaces; ++i) {
+            Handle<CSSFontFace> candidate = (*familyLocallyInstalledFontFaces)->at(i);
             unsigned candidateTraitsMask = candidate->traitsMask();
             if ((traitsMask & FontStyleNormalMask) && !(candidateTraitsMask & FontStyleNormalMask))
                 continue;
             if ((traitsMask & FontVariantNormalMask) && !(candidateTraitsMask & FontVariantNormalMask))
                 continue;
-#if ENABLE(SVG_FONTS)
-            // For SVG Fonts that specify that they only support the "normal" variant, we will assume they are incapable
-            // of small-caps synthesis and just ignore the font face as a candidate.
-            if (candidate->hasSVGFontFaceSource() && (traitsMask & FontVariantSmallCapsMask) && !(candidateTraitsMask & FontVariantSmallCapsMask))
-                continue;
-#endif
             candidateFontFaces.append(candidate);
         }
+    }
 
-        if (Vector<RefPtr<CSSFontFace> >* familyLocallyInstalledFontFaces = m_locallyInstalledFontFaces.get(family)) {
-            unsigned numLocallyInstalledFontFaces = familyLocallyInstalledFontFaces->size();
-            for (unsigned i = 0; i < numLocallyInstalledFontFaces; ++i) {
-                CSSFontFace* candidate = familyLocallyInstalledFontFaces->at(i).get();
-                unsigned candidateTraitsMask = candidate->traitsMask();
-                if ((traitsMask & FontStyleNormalMask) && !(candidateTraitsMask & FontStyleNormalMask))
-                    continue;
-                if ((traitsMask & FontVariantNormalMask) && !(candidateTraitsMask & FontVariantNormalMask))
-                    continue;
-                candidateFontFaces.append(candidate);
-            }
-        }
-
-        desiredTraitsMaskForComparison = traitsMask;
-        stable_sort(candidateFontFaces.begin(), candidateFontFaces.end(), compareFontFaces);
-        unsigned numCandidates = candidateFontFaces.size();
-        for (unsigned i = 0; i < numCandidates; ++i)
-            face->appendFontFace(candidateFontFaces[i]);
-    }
-    return face.get();
+    desiredTraitsMaskForComparison = traitsMask;
+    stable_sort(candidateFontFaces.begin(), candidateFontFaces.end(), compareFontFaces);
+    unsigned numCandidates = candidateFontFaces.size();
+    for (unsigned i = 0; i < numCandidates; ++i)
+        face->appendFontFace(candidateFontFaces[i]);
+    return face;
 }
 
 void CSSFontSelector::clearDocument()
 {
     if (!m_document) {
         ASSERT(!m_beginLoadingTimer.isActive());
         ASSERT(m_fontsToBeginLoading.isEmpty());
         return;
     }
 
@@ skipping 39 matching lines @@
     }
     // Ensure that if the request count reaches zero, the frame loader will know about it.
     cachedResourceLoader->loadDone(0);
     // New font loads may be triggered by layout after the document load is complete but before we have dispatched
     // didFinishLoading for the frame. Make sure the delegate is always dispatched by checking explicitly.
     if (m_document && m_document->frame())
         m_document->frame()->loader()->checkLoadComplete();
 }
 
 }