fonts: Fix memory accounting for deserialized glyphs.

When deserializing glyphs in the SkRemoteGlyphCache, we allocate from
the arena for the SkGlyphCache but don't account for it in the total
memory used by the cache. Fix that and avoid exposing the SkArenaAlloc
from SkGlyphCache, since that can result in such brittle use.

R=herb@google.com

Bug: 829622
Change-Id: Iecff9ce6e0ed2c641957535363edec3e3fad178d
Reviewed-on: https://skia-review.googlesource.com/128112
Reviewed-by: Herb Derby <herb@google.com>
Commit-Queue: Khusal Sagar <khushalsagar@chromium.org>

6 files changed, 74 insertions, 17 deletions

diff --git a/src/core/SkGlyphCache.cpp b/src/core/SkGlyphCache.cpp
index 6930b37e6a..d9d8d41eb1 100644
--- a/src/core/SkGlyphCache.cpp
+++ b/src/core/SkGlyphCache.cpp
@@ -15,6 +15,12 @@
 #include "SkTypeface.h"
 #include <cctype>
 
+namespace {
+size_t compute_path_size(const SkPath& path) {
+    return sizeof(SkPath) + path.countPoints() * sizeof(SkPoint);
+}
+}  // namespace
+
 SkGlyphCache::SkGlyphCache(
     const SkDescriptor& desc,
     std::unique_ptr<SkScalerContext> scaler,
@@ -188,6 +194,18 @@ const void* SkGlyphCache::findImage(const SkGlyph& glyph) {
     return glyph.fImage;
 }
 
+void SkGlyphCache::initializeImage(const volatile void* data, size_t size, SkGlyph* glyph) {
+    if (glyph->fWidth > 0 && glyph->fWidth < kMaxGlyphWidth) {
+        size_t allocSize = glyph->allocImage(&fAlloc);
+        // check that alloc() actually succeeded
+        if (glyph->fImage) {
+            SkAssertResult(size == allocSize);
+            memcpy(glyph->fImage, const_cast<const void*>(data), size);
+            fMemoryUsed += size;
+        }
+    }
+}
+
 const SkPath* SkGlyphCache::findPath(const SkGlyph& glyph) {
     if (glyph.fWidth) {
         if (glyph.fPathData == nullptr) {
@@ -197,7 +215,7 @@ const SkPath* SkGlyphCache::findPath(const SkGlyph& glyph) {
             SkPath* path = new SkPath;
             if (fScalerContext->getPath(glyph.getPackedID(), path)) {
                 pathData->fPath = path;
-                fMemoryUsed += sizeof(SkPath) + path->countPoints() * sizeof(SkPoint);
+                fMemoryUsed += compute_path_size(*path);
             } else {
                 pathData->fPath = nullptr;
                 delete path;
@@ -392,17 +410,23 @@ void SkGlyphCache::dump() const {
 }
 
 #ifdef SK_DEBUG
+void SkGlyphCache::forceValidate() const {
+    size_t memoryUsed = sizeof(*this);
+    fGlyphMap.foreach ([&memoryUsed](const SkGlyph& glyph) {
+        memoryUsed += sizeof(SkGlyph);
+        if (glyph.fImage) {
+            memoryUsed += glyph.computeImageSize();
+        }
+        if (glyph.fPathData && glyph.fPathData->fPath) {
+            memoryUsed += compute_path_size(*glyph.fPathData->fPath);
+        }
+    });
+    SkASSERT(fMemoryUsed == memoryUsed);
+}
 
 void SkGlyphCache::validate() const {
 #ifdef SK_DEBUG_GLYPH_CACHE
-    int count = fGlyphArray.count();
-    for (int i = 0; i < count; i++) {
-        const SkGlyph* glyph = &fGlyphArray[i];
-        SkASSERT(glyph);
-        if (glyph->fImage) {
-            SkASSERT(fGlyphAlloc.contains(glyph->fImage));
-        }
-    }
+    forceValidate();
 #endif
 }
 
diff --git a/src/core/SkGlyphCache.h b/src/core/SkGlyphCache.h
index b9234df74f..e069efd905 100644
--- a/src/core/SkGlyphCache.h
+++ b/src/core/SkGlyphCache.h
@@ -44,11 +44,6 @@ public:
     /**  Return a glyph that has no information if it is not already filled out. */
     SkGlyph* getRawGlyphByID(SkPackedGlyphID);
 
-    /** Return the Strike's SkArenaAlloc. */
-    SkArenaAlloc* getAlloc() {
-        return &fAlloc;
-    }
-
     /** Returns a glyph with valid fAdvance and fDevKern fields. The remaining fields may be
         valid, but that is not guaranteed. If you require those, call getUnicharMetrics or
         getGlyphIDMetrics instead.
@@ -93,6 +88,10 @@ public:
     */
     const void* findImage(const SkGlyph&);
 
+    /** Initializes the image associated with the glyph with |data|.
+     */
+    void initializeImage(const volatile void* data, size_t size, SkGlyph*);
+
     /** If the advance axis intersects the glyph's path, append the positions scaled and offset
         to the array (if non-null), and set the count to the updated array length.
     */
@@ -126,6 +125,7 @@ public:
     SkScalerContext* getScalerContext() const { return fScalerContext.get(); }
 
 #ifdef SK_DEBUG
+    void forceValidate() const;
     void validate() const;
 #else
     void validate() const {}
diff --git a/src/core/SkRemoteGlyphCache.cpp b/src/core/SkRemoteGlyphCache.cpp
index 0643aff2b3..5e21f8f292 100644
--- a/src/core/SkRemoteGlyphCache.cpp
+++ b/src/core/SkRemoteGlyphCache.cpp
@@ -668,8 +668,7 @@ bool SkStrikeClient::readStrikeData(const volatile void* memory, size_t memorySi
             if (imageSize != 0) {
                 image = deserializer.readArray<uint8_t>(imageSize);
                 if (!image.data()) READ_FAILURE
-                allocatedGlyph->allocImage(strike->getAlloc());
-                memcpy(allocatedGlyph->fImage, image.data(), image.size());
+                strike->initializeImage(image.data(), image.size(), allocatedGlyph);
             }
         }
     }
diff --git a/src/core/SkStrikeCache.cpp b/src/core/SkStrikeCache.cpp
index 04a1a80030..2e5bd89b79 100644
--- a/src/core/SkStrikeCache.cpp
+++ b/src/core/SkStrikeCache.cpp
@@ -155,6 +155,14 @@ void SkStrikeCache::PurgeAll() {
     get_globals().purgeAll();
 }
 
+void SkStrikeCache::Validate() {
+#ifdef SK_DEBUG
+    auto visitor = [](const SkGlyphCache& cache) { cache.forceValidate(); };
+
+    get_globals().forEachStrike(visitor);
+#endif
+}
+
 void SkStrikeCache::Dump() {
     SkDebugf("GlyphCache [     used    budget ]\n");
     SkDebugf("    bytes  [ %8zu  %8zu ]\n",
diff --git a/src/core/SkStrikeCache.h b/src/core/SkStrikeCache.h
index df048e33be..f2c9820799 100644
--- a/src/core/SkStrikeCache.h
+++ b/src/core/SkStrikeCache.h
@@ -92,7 +92,7 @@ public:
             const SkDescriptor&, const SkScalerContextEffects&, const SkTypeface&);
 
     static void PurgeAll();
-
+    static void Validate();
     static void Dump();
 
     // Dump memory usage statistics of all the attaches caches in the process using the
diff --git a/tests/SkRemoteGlyphCacheTest.cpp b/tests/SkRemoteGlyphCacheTest.cpp
index e653b7d2c2..dbe36d1e6a 100644
--- a/tests/SkRemoteGlyphCacheTest.cpp
+++ b/tests/SkRemoteGlyphCacheTest.cpp
@@ -220,3 +220,29 @@ DEF_TEST(SkRemoteGlyphCache_StrikePinningClient, reporter) {
     SkGraphics::PurgeFontCache();
     REPORTER_ASSERT(reporter, clientTf->unique());
 }
+
+DEF_TEST(SkRemoteGlyphCache_ClientMemoryAccounting, reporter) {
+    sk_sp<DiscardableManager> discardableManager = sk_make_sp<DiscardableManager>();
+    SkStrikeServer server(discardableManager.get());
+    SkStrikeClient client(discardableManager);
+
+    // Server.
+    auto serverTf = SkTypeface::MakeFromName("monospace", SkFontStyle());
+    auto serverTfData = server.serializeTypeface(serverTf.get());
+
+    int glyphCount = 10;
+    auto serverBlob = buildTextBlob(serverTf, glyphCount);
+
+    const SkSurfaceProps props(SkSurfaceProps::kLegacyFontHost_InitType);
+    SkTextBlobCacheDiffCanvas cache_diff_canvas(10, 10, SkMatrix::I(), props, &server);
+    SkPaint paint;
+    cache_diff_canvas.drawTextBlob(serverBlob.get(), 0, 0, paint);
+
+    std::vector<uint8_t> serverStrikeData;
+    server.writeStrikeData(&serverStrikeData);
+
+    // Client.
+    REPORTER_ASSERT(reporter,
+                    client.readStrikeData(serverStrikeData.data(), serverStrikeData.size()));
+    SkStrikeCache::Validate();
+}