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>

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
 }
 

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 {}

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);
             }
         }
     }

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",

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

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();
+}