Bug 385123. Don't insert spaces after holes while we build a textrun. r=vlad

gfx/thebes/public/gfxTextRunWordCache.h

@@ -134,6 +134,7 @@ protected:
                       PRUint32 aStart, PRUint32 aEnd, PRUint32 aHash,
                       nsTArray<DeferredWord>* aDeferredWords);
     void FinishTextRun(gfxTextRun *aTextRun, gfxTextRun *aNewRun,
+                       gfxContext *aContext,
                        const nsTArray<DeferredWord>& aDeferredWords,
                        PRBool aSuccessful);
     void RemoveWord(gfxTextRun *aTextRun, PRUint32 aStart,

gfx/thebes/src/gfxTextRunWordCache.cpp

@@ -37,6 +37,8 @@
 
 #include "gfxTextRunWordCache.h"
 
+static PRLogModuleInfo *gWordCacheLog = PR_NewLogModule("wordCache");
+
 static inline PRUint32
 HashMix(PRUint32 aHash, PRUnichar aCh)
 {
@@ -122,11 +124,13 @@ gfxTextRunWordCache::LookupWord(gfxTextRun *aTextRun, gfxFont *aFirstFont,
     if (fontEntry->mTextRun) {
         existingEntry = fontEntry;
     } else {
+        PR_LOG(gWordCacheLog, PR_LOG_DEBUG, ("%p(%d-%d,%d): added using font", aTextRun, aStart, aEnd - aStart, aHash));
         key.mFontOrGroup = aTextRun->GetFontGroup();
         CacheHashEntry *groupEntry = mCache.GetEntry(key);
         if (groupEntry) {
             existingEntry = groupEntry;
             mCache.RawRemoveEntry(fontEntry);
+            PR_LOG(gWordCacheLog, PR_LOG_DEBUG, ("%p(%d-%d,%d): removed using font", aTextRun, aStart, aEnd - aStart, aHash));
             fontEntry = nsnull;
         }
     }
@@ -169,6 +173,7 @@ gfxTextRunWordCache::LookupWord(gfxTextRun *aTextRun, gfxFont *aFirstFont,
  */
 void
 gfxTextRunWordCache::FinishTextRun(gfxTextRun *aTextRun, gfxTextRun *aNewRun,
+                                   gfxContext *aContext,
                                    const nsTArray<DeferredWord>& aDeferredWords,
                                    PRBool aSuccessful)
 {
@@ -194,14 +199,18 @@ gfxTextRunWordCache::FinishTextRun(gfxTextRun *aTextRun, gfxTextRun *aNewRun,
                 NS_ASSERTION(mCache.GetEntry(key),
                              "This entry should have been added previously!");
                 mCache.RemoveEntry(key);
+                PR_LOG(gWordCacheLog, PR_LOG_DEBUG, ("%p(%d-%d,%d): removed using font", aTextRun, word->mDestOffset, word->mLength, word->mHash));
                 
                 if (aSuccessful) {
                     key.mFontOrGroup = fontGroup;
                     CacheHashEntry *groupEntry = mCache.PutEntry(key);
                     if (groupEntry) {
+                        PR_LOG(gWordCacheLog, PR_LOG_DEBUG, ("%p(%d-%d,%d): added using fontgroup", aTextRun, word->mDestOffset, word->mLength, word->mHash));
                         groupEntry->mTextRun = aTextRun;
                         groupEntry->mWordOffset = word->mDestOffset;
                         groupEntry->mHashedByFont = PR_FALSE;
+                        NS_ASSERTION(mCache.GetEntry(key),
+                                     "We should find the thing we just added!");
                     }
                 }
             }
@@ -213,6 +222,20 @@ gfxTextRunWordCache::FinishTextRun(gfxTextRun *aTextRun, gfxTextRun *aNewRun,
             aTextRun->CopyGlyphDataFrom(source,
                 word->mSourceOffset, word->mLength, word->mDestOffset,
                 source == aNewRun);
+            // Fill in additional spaces
+            PRUint32 endCharIndex;
+            if (i + 1 < aDeferredWords.Length()) {
+                endCharIndex = aDeferredWords[i + 1].mDestOffset;
+            } else {
+                endCharIndex = aTextRun->GetLength();
+            }
+            PRUint32 charIndex;
+            for (charIndex = word->mDestOffset + word->mLength;
+                 charIndex < endCharIndex; ++charIndex) {
+                if (IsBoundarySpace(aTextRun->GetChar(charIndex))) {
+                    aTextRun->SetSpaceGlyph(font, aContext, charIndex);
+                }
+            }
         }
     }
 }
@@ -250,7 +273,7 @@ gfxTextRunWordCache::MakeTextRun(const PRUnichar *aText, PRUint32 aLength,
                 PRUint32 length = i - wordStart;
                 PRUnichar *chars = tempString.AppendElements(length);
                 if (!chars) {
-                    FinishTextRun(textRun, nsnull, deferredWords, PR_FALSE);
+                    FinishTextRun(textRun, nsnull, nsnull, deferredWords, PR_FALSE);
                     return nsnull;
                 }
                 memcpy(chars, aText + wordStart, length*sizeof(PRUnichar));
@@ -258,10 +281,12 @@ gfxTextRunWordCache::MakeTextRun(const PRUnichar *aText, PRUint32 aLength,
                 deferredWords.AppendElement(word);
             }
             
+            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!
+            }
             hash = 0;
             wordStart = i + 1;
         } else {
@@ -285,7 +310,7 @@ gfxTextRunWordCache::MakeTextRun(const PRUnichar *aText, PRUint32 aLength,
     newRun = aFontGroup->MakeTextRun(tempString.Elements(), tempString.Length(),
                                      &params, aFlags | gfxTextRunFactory::TEXT_IS_PERSISTENT);
 
-    FinishTextRun(textRun, newRun, deferredWords, newRun != nsnull);
+    FinishTextRun(textRun, newRun, aParams->mContext, deferredWords, newRun != nsnull);
     return textRun.forget();
 }
 
@@ -323,7 +348,7 @@ gfxTextRunWordCache::MakeTextRun(const PRUint8 *aText, PRUint32 aLength,
                 PRUint32 length = i - wordStart;
                 PRUint8 *chars = tempString.AppendElements(length);
                 if (!chars) {
-                    FinishTextRun(textRun, nsnull, deferredWords, PR_FALSE);
+                    FinishTextRun(textRun, nsnull, nsnull, deferredWords, PR_FALSE);
                     return nsnull;
                 }
                 memcpy(chars, aText + wordStart, length*sizeof(PRUint8));
@@ -331,10 +356,12 @@ gfxTextRunWordCache::MakeTextRun(const PRUint8 *aText, PRUint32 aLength,
                 deferredWords.AppendElement(word);
             }
             
+            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!
+            }
             hash = 0;
             wordStart = i + 1;
         } else {
@@ -358,7 +385,7 @@ gfxTextRunWordCache::MakeTextRun(const PRUint8 *aText, PRUint32 aLength,
     newRun = aFontGroup->MakeTextRun(tempString.Elements(), tempString.Length(),
                                      &params, aFlags | gfxTextRunFactory::TEXT_IS_PERSISTENT);
 
-    FinishTextRun(textRun, newRun, deferredWords, newRun != nsnull);
+    FinishTextRun(textRun, newRun, aParams->mContext, deferredWords, newRun != nsnull);
     return textRun.forget();
 }
 
@@ -379,6 +406,9 @@ gfxTextRunWordCache::RemoveWord(gfxTextRun *aTextRun, PRUint32 aStart,
         // XXX would like to use RawRemoveEntry here plus some extra method
         // that conditionally shrinks the hashtable
         mCache.RemoveEntry(key);
+        PR_LOG(gWordCacheLog, PR_LOG_DEBUG, ("%p(%d-%d,%d): removed using %s",
+            aTextRun, aStart, aEnd - aStart, aHash,
+            key.mFontOrGroup == aTextRun->GetFontGroup() ? "fontgroup" : "font"));
     }
 }
 

gfx/thebes/test/Makefile.in

@@ -59,6 +59,7 @@ CPPSRCS = \
 	gfxSurfaceRefCountTest.cpp \
 	gfxFontSelectionTest.cpp \
 	gfxTextRunPerfTest.cpp \
+	gfxWordCacheTest.cpp \
 	$(NULL)
 
 ifeq ($(MOZ_WIDGET_TOOLKIT),cocoa)

gfx/thebes/test/gfxFontSelectionTest.cpp

@@ -47,6 +47,7 @@
 #include "gfxContext.h"
 #include "gfxFont.h"
 #include "gfxPlatform.h"
+#include "gfxTextRunWordCache.h"
 
 #include "gfxFontTest.h"
 
@@ -63,6 +64,10 @@ enum {
     S_ASCII = 1
 };
 
+class FrameTextRunCache;
+
+static gfxTextRunWordCache *gTextRunCache;
+
 struct LiteralArray {
     LiteralArray (unsigned long l1) {
         data.AppendElement(l1);
@@ -294,21 +299,24 @@ RunTest (TestEntry *test, gfxContext *ctx) {
         flags |= gfxTextRunFactory::TEXT_IS_RTL;
     }
     PRUint32 length;
+    PRBool isInCache;
     if (test->stringType == S_ASCII) {
         flags |= gfxTextRunFactory::TEXT_IS_ASCII | gfxTextRunFactory::TEXT_IS_8BIT;
         length = strlen(test->string);
-        textRun = fontGroup->MakeTextRun(NS_REINTERPRET_CAST(PRUint8*, test->string), length, &params, flags);
+        textRun = gTextRunCache->MakeTextRun(NS_REINTERPRET_CAST(PRUint8*, test->string), length, fontGroup, &params, flags, &isInCache);
     } else {
         flags |= gfxTextRunFactory::TEXT_HAS_SURROGATES; // just in case
         NS_ConvertUTF8toUTF16 str(nsDependentCString(test->string));
         length = str.Length();
-        textRun = fontGroup->MakeTextRun(str.get(), length, &params, flags);
+        textRun = gTextRunCache->MakeTextRun(str.get(), length, fontGroup, &params, flags, &isInCache);
     }
 
     gfxFontTestStore::NewStore();
     textRun->Draw(ctx, gfxPoint(0,0), 0, length, nsnull, nsnull, nsnull);
     gfxFontTestStore *s = gfxFontTestStore::CurrentStore();
 
+    gTextRunCache->RemoveTextRun(textRun);
+
     if (!test->Check(s)) {
         DumpStore(s);
         printf ("  expected:\n");
@@ -340,6 +348,8 @@ main (int argc, char **argv) {
     if (NS_FAILED(rv))
         return -1;
 
+    gTextRunCache = new gfxTextRunWordCache();
+
     // let's get all the xpcom goop out of the system
     fflush (stderr);
     fflush (stdout);

gfx/thebes/test/gfxWordCacheTest.cpp

@@ -0,0 +1,198 @@
+/* -*- Mode: C++; tab-width: 20; indent-tabs-mode: nil; c-basic-offset: 4 -*-
+ * ***** BEGIN LICENSE BLOCK *****
+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
+ *
+ * The contents of this file are subject to the Mozilla Public License Version
+ * 1.1 (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ * http://www.mozilla.org/MPL/
+ *
+ * Software distributed under the License is distributed on an "AS IS" basis,
+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
+ * for the specific language governing rights and limitations under the
+ * License.
+ *
+ * The Original Code is Mozilla Corporation code.
+ *
+ * The Initial Developer of the Original Code is Mozilla Corporation.
+ * Portions created by the Initial Developer are Copyright (C) 2007
+ * the Initial Developer. All Rights Reserved.
+ *
+ * Contributor(s):
+ *   Vladimir Vukicevic <vladimir@pobox.com>
+ *
+ * Alternatively, the contents of this file may be used under the terms of
+ * either the GNU General Public License Version 2 or later (the "GPL"), or
+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
+ * in which case the provisions of the GPL or the LGPL are applicable instead
+ * of those above. If you wish to allow use of your version of this file only
+ * under the terms of either the GPL or the LGPL, and not to allow others to
+ * use your version of this file under the terms of the MPL, indicate your
+ * decision by deleting the provisions above and replace them with the notice
+ * and other provisions required by the GPL or the LGPL. If you do not delete
+ * the provisions above, a recipient may use your version of this file under
+ * the terms of any one of the MPL, the GPL or the LGPL.
+ *
+ * ***** END LICENSE BLOCK ***** */
+
+#include "nsCOMPtr.h"
+#include "nsTArray.h"
+#include "nsString.h"
+#include "nsDependentString.h"
+
+#include "prinrval.h"
+
+#include "nsServiceManagerUtils.h"
+#include "nsIPrefService.h"
+#include "nsIPrefBranch.h"
+
+#include "gfxContext.h"
+#include "gfxFont.h"
+#include "gfxPlatform.h"
+
+#include "gfxFontTest.h"
+
+#include "gfxTextRunWordCache.h"
+
+#if defined(XP_MACOSX)
+#include "gfxTestCocoaHelper.h"
+#endif
+
+#ifdef MOZ_WIDGET_GTK2
+#include "gtk/gtk.h"
+#endif
+
+class FrameTextRunCache;
+
+static FrameTextRunCache *gTextRuns = nsnull;
+
+/*
+ * Cache textruns and expire them after 3*10 seconds of no use.
+ */
+class FrameTextRunCache : public nsExpirationTracker<gfxTextRun,3> {
+public:
+ enum { TIMEOUT_SECONDS = 10 };
+ FrameTextRunCache()
+     : nsExpirationTracker<gfxTextRun,3>(TIMEOUT_SECONDS*1000) {}
+ ~FrameTextRunCache() {
+   AgeAllGenerations();
+ }
+
+ void RemoveFromCache(gfxTextRun* aTextRun) {
+   if (aTextRun->GetExpirationState()->IsTracked()) {
+     RemoveObject(aTextRun);
+   }
+   mCache.RemoveTextRun(aTextRun);
+ }
+
+ // This gets called when the timeout has expired on a gfxTextRun
+ virtual void NotifyExpired(gfxTextRun* aTextRun) {
+   RemoveFromCache(aTextRun);
+   delete aTextRun;
+ }
+
+ gfxTextRunWordCache mCache;
+};
+
+static gfxTextRun *
+MakeTextRun(const PRUnichar *aText, PRUint32 aLength,
+           gfxFontGroup *aFontGroup, const gfxFontGroup::Parameters* aParams,
+           PRUint32 aFlags)
+{
+   nsAutoPtr<gfxTextRun> textRun;
+   if (aLength == 0) {
+       textRun = aFontGroup->MakeEmptyTextRun(aParams, aFlags);
+   } else if (aLength == 1 && aText[0] == ' ') {
+       textRun = aFontGroup->MakeSpaceTextRun(aParams, aFlags);
+   } else {
+       PRBool isInCache;
+       textRun = gTextRuns->mCache.MakeTextRun(aText, aLength, aFontGroup,
+           aParams, aFlags, &isInCache);
+       if (!isInCache && textRun) {
+       }
+   }
+   if (!textRun)
+       return nsnull;
+   nsresult rv = gTextRuns->AddObject(textRun);
+   if (NS_FAILED(rv)) {
+       gTextRuns->RemoveFromCache(textRun);
+       return nsnull;
+   }
+   return textRun.forget();
+}
+
+already_AddRefed<gfxContext>
+MakeContext ()
+{
+   const int size = 200;
+
+   nsRefPtr<gfxASurface> surface;
+
+   surface = gfxPlatform::GetPlatform()->CreateOffscreenSurface(gfxIntSize(size, size), gfxASurface::ImageFormatRGB24);
+   gfxContext *ctx = new gfxContext(surface);
+   NS_IF_ADDREF(ctx);
+   return ctx;
+}
+
+int
+main (int argc, char **argv) {
+#ifdef MOZ_WIDGET_GTK2
+   gtk_init(&argc, &argv);
+#endif
+#ifdef XP_MACOSX
+   CocoaPoolInit();
+#endif
+
+   // Initialize XPCOM
+   nsresult rv = NS_InitXPCOM2(nsnull, nsnull, nsnull);
+   if (NS_FAILED(rv))
+       return -1;
+
+   rv = gfxPlatform::Init();
+   if (NS_FAILED(rv))
+       return -1;
+
+   // let's get all the xpcom goop out of the system
+   fflush (stderr);
+   fflush (stdout);
+
+   gTextRuns = new FrameTextRunCache();
+
+   nsRefPtr<gfxContext> ctx = MakeContext();
+   {
+       gfxFontStyle style (FONT_STYLE_NORMAL,
+                           139,
+                           10.0,
+                           nsDependentCString("x-western"),
+                           0.0,
+                           PR_FALSE, PR_FALSE);
+
+       nsRefPtr<gfxFontGroup> fontGroup =
+           gfxPlatform::GetPlatform()->CreateFontGroup(NS_LITERAL_STRING("Geneva, MS Sans Serif, Helvetica,serif"), &style);
+
+       gfxTextRunFactory::Parameters params = {
+           ctx, nsnull, nsnull, nsnull, 0, 60
+       };
+
+       PRUint32 flags = gfxTextRunFactory::TEXT_IS_PERSISTENT;
+
+       // First load an Arabic word into the cache
+       const char cString[] = "\xd8\xaa\xd9\x85";
+       nsDependentCString cStr(cString);
+       NS_ConvertUTF8toUTF16 str(cStr);
+       gfxTextRun *tr = MakeTextRun(str.get(), str.Length(), fontGroup, &params, flags);
+
+       // Now try to trigger an assertion with a word cache bug. The first
+       // word is in the cache so it gets added to the new textrun directly.
+       // The second word is not in the cache 
+       const char cString2[] = "\xd8\xaa\xd9\x85\n\xd8\xaa\xd8\x85 ";
+       nsDependentCString cStr2(cString2);
+       NS_ConvertUTF8toUTF16 str2(cStr2);
+       gfxTextRun *tr2 = MakeTextRun(str2.get(), str2.Length(), fontGroup, &params, flags);
+
+       tr2->GetAdvanceWidth(0, str2.Length(), nsnull);
+   }
+
+   fflush (stderr);
+   fflush (stdout);
+}