[oilpan] Move CSSFontFace and CSSSegmentedFontFace to the managed heap

Source/core/css/CSSFontSelector.cpp

Index: Source/core/css/CSSFontSelector.cpp
diff --git a/Source/core/css/CSSFontSelector.cpp b/Source/core/css/CSSFontSelector.cpp
index 3871cdf714f693ee1c50d4f81ede65514fe17b17..7d6ed022bcf60ce0f3da9acae5e005d9c8057f9f 100644
--- a/Source/core/css/CSSFontSelector.cpp
+++ b/Source/core/css/CSSFontSelector.cpp
@@ -195,7 +195,7 @@ void CSSFontSelector::addFontFaceRule(const Handle<StyleRuleFontFace>& fontFaceR
         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();
 
@@ -292,29 +292,30 @@ void CSSFontSelector::addFontFaceRule(const Handle<StyleRuleFontFace>& fontFaceR
         if (familyName.isEmpty())
             continue;
 
-        OwnPtr<Vector<RefPtr<CSSFontFace> > >& familyFontFaces = m_fontFaces.add(familyName, nullptr).iterator->value;
+        OwnPtr<CSSFontFaceVectorCollection>& familyFontFaces = m_fontFaces.add(familyName, nullptr).iterator->value;

[haraken, 2013/07/03 04:39:20]

What do you think about this? We cannot simply change this to:

  OwnPtr<CSSFontFaceVectorCollection> familyFontFaces = ...

because OwnPtr is not copyable.

One way to solve this issue is to store CSSFontFaceVectorCollection instead of
OwnPtr<CSSFontFaceVectorCollection>. However, in that case, we have to copy
CollectionRoot a lot of times.

[Mads Ager (chromium), 2013/07/03 06:30:42]

Yeah, this one is a little annoying. Another way would be to pull out the actual
vector pointer and operate on that directly:

CSSFontFaceVectorCollection* familyFontFaces = m_fontFaces.add(familyName,
nullptr).iterator->value.get();
if (!familyFontFaces) {
    familyFontFaces = new CSSFontFaceVectorCollection;
    m_fontFaces.set(familyName, adoptPtr(familyFontFaces));
    ...
}
...

We should consider how to deal with these heap allocated vectors. If we start
allocating them on the oilpan heap we could replace the OwnPtr with Member and
extract them in a Handle.

[haraken, 2013/07/03 07:46:02]

Done.

         if (!familyFontFaces) {
-            familyFontFaces = adoptPtr(new Vector<RefPtr<CSSFontFace> >);
+            familyFontFaces = adoptPtr(new CSSFontFaceVectorCollection);
 
             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;
     }
@@ -392,7 +393,7 @@ static PassRefPtr<FontData> fontDataForGenericFamily(Document* document, const F
 
 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();
@@ -482,7 +483,7 @@ PassRefPtr<FontData> CSSFontSelector::getFontData(const FontDescription& fontDes
         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
@@ -496,26 +497,27 @@ PassRefPtr<FontData> CSSFontSelector::getFontData(const FontDescription& fontDes
     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;
+    OwnPtr<HashMap<unsigned, Persistent<CSSSegmentedFontFace> > >& segmentedFontFaceCache = m_fonts.add(family, nullptr).iterator->value;
     if (!segmentedFontFaceCache)
-        segmentedFontFaceCache = adoptPtr(new HashMap<unsigned, RefPtr<CSSSegmentedFontFace> >);
+        segmentedFontFaceCache = adoptPtr(new HashMap<unsigned, Persistent<CSSSegmentedFontFace> >);
 
     FontTraitsMask traitsMask = fontDescription.traitsMask();
 
-    RefPtr<CSSSegmentedFontFace>& face = segmentedFontFaceCache->add(traitsMask, 0).iterator->value;
+    Persistent<CSSSegmentedFontFace>& face = segmentedFontFaceCache->add(traitsMask, nullptr).iterator->value;

[Mads Ager (chromium), 2013/07/02 14:59:23]

I don't think we should be using this pattern. The problem is that the create
call below can allocate. That allocation can cause a GC which can call
finalizers. If one of those finalizers changes the segmentedFontFaceCache we can
get a memory corruption here. Instead, we should do something like:

Handle<CSSSegmentedFontFace> face = segmentedFontFaceCache->add(traitsMask,
nullptr).iterator->value;
if (value)
    return value;
face = CSSSegmentedFontFace::create(this);
segmentedFontFaceCache->set(traitsMask, face);
...

[haraken, 2013/07/03 04:39:20]

Done.
  

     if (!face) {
         face = CSSSegmentedFontFace::create(this);
 
         // 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();
+        Vector<Handle<CSSFontFace>, 32> candidateFontFaces;
+        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;
@@ -530,10 +532,11 @@ CSSSegmentedFontFace* CSSFontSelector::getFontFace(const FontDescription& fontDe
             candidateFontFaces.append(candidate);
         }
 
-        if (Vector<RefPtr<CSSFontFace> >* familyLocallyInstalledFontFaces = m_locallyInstalledFontFaces.get(family)) {
-            unsigned numLocallyInstalledFontFaces = familyLocallyInstalledFontFaces->size();
+        CSSFontFaceVectorCollection* familyLocallyInstalledFontFaces = m_locallyInstalledFontFaces.get(family);
+        if (familyLocallyInstalledFontFaces) {
+            unsigned numLocallyInstalledFontFaces = (*familyLocallyInstalledFontFaces)->size();
             for (unsigned i = 0; i < numLocallyInstalledFontFaces; ++i) {
-                CSSFontFace* candidate = familyLocallyInstalledFontFaces->at(i).get();
+                Handle<CSSFontFace> candidate = (*familyLocallyInstalledFontFaces)->at(i);
                 unsigned candidateTraitsMask = candidate->traitsMask();
                 if ((traitsMask & FontStyleNormalMask) && !(candidateTraitsMask & FontStyleNormalMask))
                     continue;
@@ -549,7 +552,7 @@ CSSSegmentedFontFace* CSSFontSelector::getFontFace(const FontDescription& fontDe
         for (unsigned i = 0; i < numCandidates; ++i)
             face->appendFontFace(candidateFontFaces[i]);
     }
-    return face.get();
+    return face;
 }
 
 void CSSFontSelector::clearDocument()