Fix for bug 7234184 F/TextLayoutCache: Failed to put an entry...

This bug was triggered by user code concurrently mutating the character
array while calling into a drawText method in another thread. When the
value of the array changed, it caused inconsistent state, leading to
assert failures.

This is arguably bad behavior by the user code, but it shouldn't cause a
native crash. The fix is to do a defensive copy of the text into the
key, so the value is guaranteed to remain constant throughout the text
layout process. The change is mostly deletion of code, because there was
an optimization to try to avoid such a copy. That optimization was not
actually effective, however, because the indexOfKey() operation in the
KeyedVector underlying the TextLayoutCache did the copy anyway. Thus,
even though this change looks like it's introducing a copy where there
wasn't one before, the actual performance impact should be nil.

Note that the ability to handle a mutating argument is now part of the
contract for TextLayoutEngine::getValue(), and is now documented. That
contract may change, as the result of future optimization. Also, care
was taken to only use the value after the copy.

Other performance issues with TextLayoutCache are tracked in bug
7271109.

Change-Id: I9c90e8e4d501f3f37e2f22a7851f032808d46fbe

2 files changed, 16 insertions, 28 deletions

diff --git a/core/jni/android/graphics/TextLayoutCache.cpp b/core/jni/android/graphics/TextLayoutCache.cpp
index ba8cea475574..4669c378365b 100644
--- a/core/jni/android/graphics/TextLayoutCache.cpp
+++ b/core/jni/android/graphics/TextLayoutCache.cpp
@@ -111,7 +111,7 @@ sp<TextLayoutValue> TextLayoutCache::getValue(const SkPaint* paint,
 
         // Compute advances and store them
         mShaper->computeValues(value.get(), paint,
-                reinterpret_cast<const UChar*>(text), start, count,
+                reinterpret_cast<const UChar*>(key.getText()), start, count,
                 size_t(contextCount), int(dirFlags));
 
         if (mDebugEnabled) {
@@ -139,15 +139,12 @@ sp<TextLayoutValue> TextLayoutCache::getValue(const SkPaint* paint,
             // Update current cache size
             mSize += size;
 
-            // Copy the text when we insert the new entry
-            key.internalTextCopy();
-
             bool putOne = mCache.put(key, value);
             LOG_ALWAYS_FATAL_IF(!putOne, "Failed to put an entry into the cache.  "
                     "This indicates that the cache already has an entry with the "
                     "same key but it should not since we checked earlier!"
                     " - start = %d, count = %d, contextCount = %d - Text = '%s'",
-                    start, count, contextCount, String8(text + start, count).string());
+                    start, count, contextCount, String8(key.getText() + start, count).string());
 
             if (mDebugEnabled) {
                 nsecs_t totalTime = systemTime(SYSTEM_TIME_MONOTONIC) - startTime;
@@ -158,7 +155,7 @@ sp<TextLayoutValue> TextLayoutCache::getValue(const SkPaint* paint,
                         value.get(), start, count, contextCount, size, mMaxSize - mSize,
                         value->getElapsedTime() * 0.000001f,
                         (totalTime - value->getElapsedTime()) * 0.000001f,
-                        String8(text + start, count).string());
+                        String8(key.getText() + start, count).string());
             }
         } else {
             if (mDebugEnabled) {
@@ -168,7 +165,7 @@ sp<TextLayoutValue> TextLayoutCache::getValue(const SkPaint* paint,
                         " - Compute time %0.6f ms - Text = '%s'",
                         start, count, contextCount, size, mMaxSize - mSize,
                         value->getElapsedTime() * 0.000001f,
-                        String8(text + start, count).string());
+                        String8(key.getText() + start, count).string());
             }
         }
     } else {
@@ -188,7 +185,7 @@ sp<TextLayoutValue> TextLayoutCache::getValue(const SkPaint* paint,
                         value->getElapsedTime() * 0.000001f,
                         elapsedTimeThruCacheGet * 0.000001f,
                         deltaPercent,
-                        String8(text + start, count).string());
+                        String8(key.getText() + start, count).string());
             }
             if (mCacheHitCount % DEFAULT_DUMP_STATS_CACHE_HIT_INTERVAL == 0) {
                 dumpCacheStats();
@@ -225,15 +222,16 @@ void TextLayoutCache::dumpCacheStats() {
 /**
  * TextLayoutCacheKey
  */
-TextLayoutCacheKey::TextLayoutCacheKey(): text(NULL), start(0), count(0), contextCount(0),
+TextLayoutCacheKey::TextLayoutCacheKey(): start(0), count(0), contextCount(0),
         dirFlags(0), typeface(NULL), textSize(0), textSkewX(0), textScaleX(0), flags(0),
         hinting(SkPaint::kNo_Hinting), variant(SkPaint::kDefault_Variant), language()  {
 }
 
 TextLayoutCacheKey::TextLayoutCacheKey(const SkPaint* paint, const UChar* text,
         size_t start, size_t count, size_t contextCount, int dirFlags) :
-            text(text), start(start), count(count), contextCount(contextCount),
+            start(start), count(count), contextCount(contextCount),
             dirFlags(dirFlags) {
+    textCopy.setTo(text, contextCount);
     typeface = paint->getTypeface();
     textSize = paint->getTextSize();
     textSkewX = paint->getTextSkewX();
@@ -245,7 +243,6 @@ TextLayoutCacheKey::TextLayoutCacheKey(const SkPaint* paint, const UChar* text,
 }
 
 TextLayoutCacheKey::TextLayoutCacheKey(const TextLayoutCacheKey& other) :
-        text(NULL),
         textCopy(other.textCopy),
         start(other.start),
         count(other.count),
@@ -259,9 +256,6 @@ TextLayoutCacheKey::TextLayoutCacheKey(const TextLayoutCacheKey& other) :
         hinting(other.hinting),
         variant(other.variant),
         language(other.language) {
-    if (other.text) {
-        textCopy.setTo(other.text, other.contextCount);
-    }
 }
 
 int TextLayoutCacheKey::compare(const TextLayoutCacheKey& lhs, const TextLayoutCacheKey& rhs) {
@@ -304,11 +298,6 @@ int TextLayoutCacheKey::compare(const TextLayoutCacheKey& lhs, const TextLayoutC
     return memcmp(lhs.getText(), rhs.getText(), lhs.contextCount * sizeof(UChar));
 }
 
-void TextLayoutCacheKey::internalTextCopy() {
-    textCopy.setTo(text, contextCount);
-    text = NULL;
-}
-
 size_t TextLayoutCacheKey::getSize() const {
     return sizeof(TextLayoutCacheKey) + sizeof(UChar) * contextCount;
 }
diff --git a/core/jni/android/graphics/TextLayoutCache.h b/core/jni/android/graphics/TextLayoutCache.h
index 1f4e22c8274f..9994393a091e 100644
--- a/core/jni/android/graphics/TextLayoutCache.h
+++ b/core/jni/android/graphics/TextLayoutCache.h
@@ -77,20 +77,15 @@ public:
     TextLayoutCacheKey(const TextLayoutCacheKey& other);
 
     /**
-     * We need to copy the text when we insert the key into the cache itself.
-     * We don't need to copy the text when we are only comparing keys.
-     */
-    void internalTextCopy();
-
-    /**
      * Get the size of the Cache key.
      */
     size_t getSize() const;
 
     static int compare(const TextLayoutCacheKey& lhs, const TextLayoutCacheKey& rhs);
 
+    inline const UChar* getText() const { return textCopy.string(); }
+
 private:
-    const UChar* text; // if text is NULL, use textCopy
     String16 textCopy;
     size_t start;
     size_t count;
@@ -105,8 +100,6 @@ private:
     SkPaint::FontVariant variant;
     SkLanguage language;
 
-    inline const UChar* getText() const { return text ? text : textCopy.string(); }
-
 }; // TextLayoutCacheKey
 
 inline int strictly_order_type(const TextLayoutCacheKey& lhs, const TextLayoutCacheKey& rhs) {
@@ -316,6 +309,12 @@ public:
     TextLayoutEngine();
     virtual ~TextLayoutEngine();
 
+    /**
+     * Note: this method currently does a defensive copy of the text argument, in case
+     * there is concurrent mutation of it. The contract may change, and may in the
+     * future require the caller to guarantee that the contents will not change during
+     * the call. Be careful of this when doing optimization.
+     **/
     sp<TextLayoutValue> getValue(const SkPaint* paint, const jchar* text, jint start,
             jint count, jint contextCount, jint dirFlags);