bug 609691 - check result of gfxFontGroup::MakeTextRun for failure. r=karlt a=joe
authorJonathan Kew <jfkthame@gmail.com>
Wed, 01 Dec 2010 11:53:45 +0000
changeset 58436 ee61ea93a2faf4f5da1f58d2eaf0c87fe1e47ecc
parent 58435 d1228efe31dd443de570c4570ee848bb8514949e
child 58437 e1983b9db75d1a16fb3c86b07f4a17e04edbecfe
push id1
push usershaver@mozilla.com
push dateTue, 04 Jan 2011 17:58:04 +0000
reviewerskarlt, joe
bugs609691
milestone2.0b8pre
bug 609691 - check result of gfxFontGroup::MakeTextRun for failure. r=karlt a=joe
gfx/thebes/gfxTextRunWordCache.cpp
--- a/gfx/thebes/gfxTextRunWordCache.cpp
+++ b/gfx/thebes/gfxTextRunWordCache.cpp
@@ -447,32 +447,39 @@ TextRunWordCache::FinishTextRun(gfxTextR
     for (i = 0; i < aDeferredWords.Length(); ++i) {
         const DeferredWord *word = &aDeferredWords[i];
         gfxTextRun *source = word->mSourceTextRun;
         if (!source) {
             source = aNewRun;
         }
         // If the word starts inside a cluster we don't want this word
         // in the cache, so we'll remove the associated cache entry
-        PRBool wordStartsInsideCluster =
-            !source->IsClusterStart(word->mSourceOffset);
-        PRBool wordStartsInsideLigature =
-            !source->IsLigatureGroupStart(word->mSourceOffset);
+        PRBool wordStartsInsideCluster;
+        PRBool wordStartsInsideLigature;
+        if (aSuccessful) {
+            wordStartsInsideCluster =
+                !source->IsClusterStart(word->mSourceOffset);
+            wordStartsInsideLigature =
+                !source->IsLigatureGroupStart(word->mSourceOffset);
+        }
         if (source == aNewRun) {
             // We created a cache entry for this word based on the assumption
             // that the word matches GetFontAt(0). If this assumption is false,
             // we need to remove that cache entry and replace it with an entry
             // keyed off the fontgroup.
-            PRBool rekeyWithFontGroup =
-                GetWordFontOrGroup(aNewRun, word->mSourceOffset, word->mLength) != font && !useFontGroup;
-            if (!aSuccessful || rekeyWithFontGroup ||
-                wordStartsInsideCluster || wordStartsInsideLigature) {
+            PRBool removeFontKey = !aSuccessful ||
+                wordStartsInsideCluster || wordStartsInsideLigature ||
+                (!useFontGroup && font != GetWordFontOrGroup(aNewRun,
+                                                             word->mSourceOffset,
+                                                             word->mLength));
+            if (removeFontKey) {
                 // We need to remove the current placeholder cache entry
-                CacheHashKey key(aTextRun, (useFontGroup ? (void*)fontGroup : (void*)font), word->mDestOffset, word->mLength,
-                                 word->mHash);
+                CacheHashKey key(aTextRun,
+                                 (useFontGroup ? (void*)fontGroup : (void*)font),
+                                 word->mDestOffset, word->mLength, word->mHash);
                 NS_ASSERTION(mCache.GetEntry(key),
                              "This entry should have been added previously!");
                 mCache.RemoveEntry(key);
 #ifdef DEBUG
                 --aTextRun->mCachedWords;
 #endif
                 PR_LOG(gWordCacheLog, PR_LOG_DEBUG, ("%p(%d-%d,%d): removed using font", aTextRun, word->mDestOffset, word->mLength, word->mHash));
                 
@@ -520,19 +527,30 @@ TextRunWordCache::FinishTextRun(gfxTextR
                     // had to prepend a space just so we could detect this
                     // situation (so we can keep this "word" out of the
                     // cache). But now the data in aNewRun is no use to us.
                     // We need to find out what the platform would do
                     // if the characters were at the start of the text.
                     tmpTextRun = aNewRun->GetFontGroup()->MakeTextRun(
                         source->GetTextUnicode() + sourceOffset, length, aParams,
                         aNewRun->GetFlags());
-                    source = tmpTextRun;
-                    sourceOffset = 0;
-                    stealData = PR_TRUE;
+                    if (tmpTextRun) {
+                        source = tmpTextRun;
+                        sourceOffset = 0;
+                        stealData = PR_TRUE;
+                    } else {
+                        // If we failed to create the temporary run (OOM),
+                        // skip the word, as if aSuccessful had been FALSE.
+                        // (In practice this is only likely to occur if
+                        // we're on the verge of an OOM crash anyhow.
+                        // But ignoring gfxFontGroup::MakeTextRun() failure
+                        // is bad because it means we'd be using an invalid
+                        // source pointer.)
+                        continue;
+                    }
                 }
             }
             aTextRun->CopyGlyphDataFrom(source, sourceOffset, length,
                 destOffset, stealData);
             // Fill in additional spaces
             PRUint32 endCharIndex;
             if (i + 1 < aDeferredWords.Length()) {
                 endCharIndex = aDeferredWords[i + 1].mDestOffset;
@@ -633,21 +651,33 @@ TextRunWordCache::MakeTextRun(const PRUn
                                                       mBidiNumeral);
                 }
                 // now we make a transient textRun for the transformed word; this will not be cached
                 gfxTextRun *numRun;
                 numRun =
                     aFontGroup->MakeTextRun(numString.get(), length, aParams,
                                             aFlags & ~(gfxTextRunFactory::TEXT_IS_PERSISTENT |
                                                        gfxTextRunFactory::TEXT_IS_8BIT));
-                DeferredWord word = { numRun, 0, wordStart, length, hash };
-                deferredWords.AppendElement(word);
-                transientRuns.AppendElement(numRun);
-                seenDigitToModify = PR_FALSE;
-            } else {
+                // If MakeTextRun failed, numRun will be null, which is bad...
+                // we'll just pretend there wasn't a digit to process.
+                // This means we won't have the correct numerals, but at least
+                // we're not trying to copy glyph data from an invalid source.
+                // In practice it's unlikely to happen unless we're very close
+                // to crashing due to OOM.
+                if (numRun) {
+                    DeferredWord word = { numRun, 0, wordStart, length, hash };
+                    deferredWords.AppendElement(word);
+                    transientRuns.AppendElement(numRun);
+                } else {
+                    seenDigitToModify = PR_FALSE;
+                }
+            }
+
+            if (!seenDigitToModify) {
+                // didn't need to modify digits (or failed to do so)
                 PRBool hit = LookupWord(textRun, font, wordStart, i, hash,
                                         deferredWords.Length() == 0 ? nsnull : &deferredWords);
                 if (!hit) {
                     // Always put a space before the word so we can detect
                     // combining characters at the start of a word
                     tempString.AppendElement(' ');
                     PRUint32 offset = tempString.Length();
                     PRUint32 length = i - wordStart;
@@ -662,17 +692,20 @@ TextRunWordCache::MakeTextRun(const PRUn
                 }
 
                 if (deferredWords.Length() == 0) {
                     if (IsBoundarySpace(ch) && i < aLength) {
                         textRun->SetSpaceGlyph(font, aParams->mContext, i);
                     } // else we should set this character to be invisible missing,
                       // but it already is because the textrun is blank!
                 }
+            } else {
+                seenDigitToModify = PR_FALSE;
             }
+
             hash = 0;
             wordStart = i + 1;
         } else {
             hash = HashMix(hash, ch);
         }
     }
 
     if (deferredWords.Length() == 0) {
@@ -757,21 +790,26 @@ TextRunWordCache::MakeTextRun(const PRUi
                                                       mBidiNumeral);
                 }
                 // now we make a transient textRun for the transformed word; this will not be cached
                 gfxTextRun *numRun;
                 numRun =
                     aFontGroup->MakeTextRun(numString.get(), length, aParams,
                                             aFlags & ~(gfxTextRunFactory::TEXT_IS_PERSISTENT |
                                                        gfxTextRunFactory::TEXT_IS_8BIT));
-                DeferredWord word = { numRun, 0, wordStart, length, hash };
-                deferredWords.AppendElement(word);
-                transientRuns.AppendElement(numRun);
-                seenDigitToModify = PR_FALSE;
-            } else {
+                if (numRun) {
+                    DeferredWord word = { numRun, 0, wordStart, length, hash };
+                    deferredWords.AppendElement(word);
+                    transientRuns.AppendElement(numRun);
+                } else {
+                    seenDigitToModify = PR_FALSE;
+                }
+            }
+
+            if (!seenDigitToModify) {
                 PRBool hit = LookupWord(textRun, font, wordStart, i, hash,
                                         deferredWords.Length() == 0 ? nsnull : &deferredWords);
                 if (!hit) {
                     if (tempString.Length() > 0) {
                         tempString.AppendElement(' ');
                     }
                     PRUint32 offset = tempString.Length();
                     PRUint32 length = i - wordStart;
@@ -786,17 +824,20 @@ TextRunWordCache::MakeTextRun(const PRUi
                 }
 
                 if (deferredWords.Length() == 0) {
                     if (IsBoundarySpace(ch) && i < aLength) {
                         textRun->SetSpaceGlyph(font, aParams->mContext, i);
                     } // else we should set this character to be invisible missing,
                       // but it already is because the textrun is blank!
                 }
+            } else {
+                seenDigitToModify = PR_FALSE;
             }
+
             hash = 0;
             wordStart = i + 1;
         } else {
             hash = HashMix(hash, ch);
         }
     }
 
     if (deferredWords.Length() == 0) {