Fix HWUI ShaderCache mIDHash eviction race

This patch fixes the ShaderCache eviction race by ensuring the ID
remains in the cache outside of the background thread, during set().

This patch also makes set() a class method, and removes
getBlobCacheLocked() for simplicity as it no longer serves to
"ensure the blob cache is initialized" as initShaderDiskCache() handles
that role now.

Test: atest hwui_unit_tests:ShaderCacheTest
Bug: 290231463
Change-Id: Id69aff95a0cf3bed6b3b7dfc2bd61b2a8a2b555a

2 files changed, 21 insertions, 33 deletions

diff --git a/libs/hwui/pipeline/skia/ShaderCache.cpp b/libs/hwui/pipeline/skia/ShaderCache.cpp
index f71e7289bd37..b87002371775 100644
--- a/libs/hwui/pipeline/skia/ShaderCache.cpp
+++ b/libs/hwui/pipeline/skia/ShaderCache.cpp
@@ -88,6 +88,9 @@ void ShaderCache::initShaderDiskCache(const void* identity, ssize_t size) {
         mBlobCache.reset(new FileBlobCache(maxKeySize, maxValueSize, maxTotalSize, mFilename));
         validateCache(identity, size);
         mInitialized = true;
+        if (identity != nullptr && size > 0 && mIDHash.size()) {
+            set(&sIDKey, sizeof(sIDKey), mIDHash.data(), mIDHash.size());
+        }
     }
 }
 
@@ -96,11 +99,6 @@ void ShaderCache::setFilename(const char* filename) {
     mFilename = filename;
 }
 
-BlobCache* ShaderCache::getBlobCacheLocked() {
-    LOG_ALWAYS_FATAL_IF(!mInitialized, "ShaderCache has not been initialized");
-    return mBlobCache.get();
-}
-
 sk_sp<SkData> ShaderCache::load(const SkData& key) {
     ATRACE_NAME("ShaderCache::load");
     size_t keySize = key.size();
@@ -115,8 +113,7 @@ sk_sp<SkData> ShaderCache::load(const SkData& key) {
     if (!valueBuffer) {
         return nullptr;
     }
-    BlobCache* bc = getBlobCacheLocked();
-    size_t valueSize = bc->get(key.data(), keySize, valueBuffer, mObservedBlobValueSize);
+    size_t valueSize = mBlobCache->get(key.data(), keySize, valueBuffer, mObservedBlobValueSize);
     int maxTries = 3;
     while (valueSize > mObservedBlobValueSize && maxTries > 0) {
         mObservedBlobValueSize = std::min(valueSize, maxValueSize);
@@ -126,7 +123,7 @@ sk_sp<SkData> ShaderCache::load(const SkData& key) {
             return nullptr;
         }
         valueBuffer = newValueBuffer;
-        valueSize = bc->get(key.data(), keySize, valueBuffer, mObservedBlobValueSize);
+        valueSize = mBlobCache->get(key.data(), keySize, valueBuffer, mObservedBlobValueSize);
         maxTries--;
     }
     if (!valueSize) {
@@ -143,16 +140,17 @@ sk_sp<SkData> ShaderCache::load(const SkData& key) {
     return SkData::MakeFromMalloc(valueBuffer, valueSize);
 }
 
-namespace {
-// Helper for BlobCache::set to trace the result.
-void set(BlobCache* cache, const void* key, size_t keySize, const void* value, size_t valueSize) {
-    switch (cache->set(key, keySize, value, valueSize)) {
+void ShaderCache::set(const void* key, size_t keySize, const void* value, size_t valueSize) {
+    switch (mBlobCache->set(key, keySize, value, valueSize)) {
         case BlobCache::InsertResult::kInserted:
             // This is what we expect/hope. It means the cache is large enough.
             return;
         case BlobCache::InsertResult::kDidClean: {
             ATRACE_FORMAT("ShaderCache: evicted an entry to fit {key: %lu value %lu}!", keySize,
                           valueSize);
+            if (mIDHash.size()) {
+                set(&sIDKey, sizeof(sIDKey), mIDHash.data(), mIDHash.size());
+            }
             return;
         }
         case BlobCache::InsertResult::kNotEnoughSpace: {
@@ -172,15 +170,10 @@ void set(BlobCache* cache, const void* key, size_t keySize, const void* value, s
         }
     }
 }
-}  // namespace
 
 void ShaderCache::saveToDiskLocked() {
     ATRACE_NAME("ShaderCache::saveToDiskLocked");
     if (mInitialized && mBlobCache) {
-        if (mIDHash.size()) {
-            auto key = sIDKey;
-            set(mBlobCache.get(), &key, sizeof(key), mIDHash.data(), mIDHash.size());
-        }
         // The most straightforward way to make ownership shared
         mMutex.unlock();
         mMutex.lock_shared();
@@ -209,11 +202,10 @@ void ShaderCache::store(const SkData& key, const SkData& data, const SkString& /
 
     const void* value = data.data();
 
-    BlobCache* bc = getBlobCacheLocked();
     if (mInStoreVkPipelineInProgress) {
         if (mOldPipelineCacheSize == -1) {
             // Record the initial pipeline cache size stored in the file.
-            mOldPipelineCacheSize = bc->get(key.data(), keySize, nullptr, 0);
+            mOldPipelineCacheSize = mBlobCache->get(key.data(), keySize, nullptr, 0);
         }
         if (mNewPipelineCacheSize != -1 && mNewPipelineCacheSize == valueSize) {
             // There has not been change in pipeline cache size. Stop trying to save.
@@ -228,7 +220,7 @@ void ShaderCache::store(const SkData& key, const SkData& data, const SkString& /
         mNewPipelineCacheSize = -1;
         mTryToStorePipelineCache = true;
     }
-    set(bc, key.data(), keySize, value, valueSize);
+    set(key.data(), keySize, value, valueSize);
 
     if (!mSavePending && mDeferredSaveDelayMs > 0) {
         mSavePending = true;
diff --git a/libs/hwui/pipeline/skia/ShaderCache.h b/libs/hwui/pipeline/skia/ShaderCache.h
index 2f91c778b8a0..74955503dbb1 100644
--- a/libs/hwui/pipeline/skia/ShaderCache.h
+++ b/libs/hwui/pipeline/skia/ShaderCache.h
@@ -96,20 +96,18 @@ private:
     void operator=(const ShaderCache&) = delete;
 
     /**
-     * "getBlobCacheLocked" returns the BlobCache object being used to store the
-     * key/value blob pairs.  If the BlobCache object has not yet been created,
-     * this will do so, loading the serialized cache contents from disk if
-     * possible.
-     */
-    BlobCache* getBlobCacheLocked() REQUIRES(mMutex);
-
-    /**
      * "validateCache" updates the cache to match the given identity.  If the
      * cache currently has the wrong identity, all entries in the cache are cleared.
      */
     bool validateCache(const void* identity, ssize_t size) REQUIRES(mMutex);
 
     /**
+     * Helper for BlobCache::set to trace the result and ensure the identity hash
+     * does not get evicted.
+     */
+    void set(const void* key, size_t keySize, const void* value, size_t valueSize) REQUIRES(mMutex);
+
+    /**
      * "saveToDiskLocked" attempts to save the current contents of the cache to
      * disk. If the identity hash exists, we will insert the identity hash into
      * the cache for next validation.
@@ -127,11 +125,9 @@ private:
     bool mInitialized GUARDED_BY(mMutex) = false;
 
     /**
-     * "mBlobCache" is the cache in which the key/value blob pairs are stored.  It
-     * is initially NULL, and will be initialized by getBlobCacheLocked the
-     * first time it's needed.
-     * The blob cache contains the Android build number. We treat version mismatches as an empty
-     * cache (logic implemented in BlobCache::unflatten).
+     * "mBlobCache" is the cache in which the key/value blob pairs are stored.
+     * The blob cache contains the Android build number. We treat version mismatches
+     * as an empty cache (logic implemented in BlobCache::unflatten).
      */
     std::unique_ptr<FileBlobCache> mBlobCache GUARDED_BY(mMutex);