Do not remove directories in purge (flutter#21478)

Otherwise, the persistent cache may hold invalid directories and make subsequent store fail. The newly added unit tests would fail without the fix.
esarbanis · Oct 6, 2020 · e59e893 · e59e893
1 parent 4569aab
commit e59e893
Show file tree

Hide file tree

Showing 5 changed files with 80 additions and 21 deletions.
diff --git a/shell/common/persistent_cache.cc b/shell/common/persistent_cache.cc
@@ -85,15 +85,24 @@ bool PersistentCache::Purge() {
   FML_CHECK(GetWorkerTaskRunner());
 
   std::promise<bool> removed;
-  GetWorkerTaskRunner()->PostTask(
-      [&removed, cache_directory = cache_directory_]() {
-        if (cache_directory->is_valid()) {
-          FML_LOG(INFO) << "Purge persistent cache.";
-          removed.set_value(RemoveFilesInDirectory(*cache_directory));
-        } else {
-          removed.set_value(false);
+  GetWorkerTaskRunner()->PostTask([&removed,
+                                   cache_directory = cache_directory_]() {
+    if (cache_directory->is_valid()) {
+      // Only remove files but not directories.
+      FML_LOG(INFO) << "Purge persistent cache.";
+      fml::FileVisitor delete_file = [](const fml::UniqueFD& directory,
+                                        const std::string& filename) {
+        // Do not delete directories. Return true to continue with other files.
+        if (fml::IsDirectory(directory, filename.c_str())) {
+          return true;
         }
-      });
+        return fml::UnlinkFile(directory, filename.c_str());
+      };
+      removed.set_value(VisitFilesRecursively(*cache_directory, delete_file));
+    } else {
+      removed.set_value(false);
+    }
+  });
   return removed.get_future().get();
 }
 
@@ -279,20 +288,18 @@ static void PersistentCacheStore(fml::RefPtr<fml::TaskRunner> worker,
                                  std::shared_ptr<fml::UniqueFD> cache_directory,
                                  std::string key,
                                  std::unique_ptr<fml::Mapping> value) {
-  auto task =
-      fml::MakeCopyable([cache_directory,             //
-                         file_name = std::move(key),  //
-                         mapping = std::move(value)   //
+  auto task = fml::MakeCopyable([cache_directory,             //
+                                 file_name = std::move(key),  //
+                                 mapping = std::move(value)   //
   ]() mutable {
-        TRACE_EVENT0("flutter", "PersistentCacheStore");
-        if (!fml::WriteAtomically(*cache_directory,   //
-                                  file_name.c_str(),  //
-                                  *mapping)           //
-        ) {
-          FML_DLOG(WARNING)
-              << "Could not write cache contents to persistent store.";
-        }
-      });
+    TRACE_EVENT0("flutter", "PersistentCacheStore");
+    if (!fml::WriteAtomically(*cache_directory,   //
+                              file_name.c_str(),  //
+                              *mapping)           //
+    ) {
+      FML_LOG(WARNING) << "Could not write cache contents to persistent store.";
+    }
+  });
 
   if (!worker) {
     FML_LOG(WARNING)

diff --git a/shell/common/persistent_cache.h b/shell/common/persistent_cache.h
@@ -15,6 +15,10 @@
 #include "flutter/fml/unique_fd.h"
 #include "third_party/skia/include/gpu/GrContextOptions.h"
 
+namespace testing {
+class ShellTest;
+}
+
 namespace flutter {
 
 /// A cache of SkData that gets stored to disk.
@@ -120,6 +124,8 @@ class PersistentCache : public GrContextOptions::PersistentCache {
 
   fml::RefPtr<fml::TaskRunner> GetWorkerTaskRunner() const;
 
+  friend class testing::ShellTest;
+
   FML_DISALLOW_COPY_AND_ASSIGN(PersistentCache);
 };
 

diff --git a/shell/common/persistent_cache_unittests.cc b/shell/common/persistent_cache_unittests.cc
@@ -273,5 +273,38 @@ TEST_F(ShellTest, CanPurgePersistentCache) {
   DestroyShell(std::move(shell));
 }
 
+TEST_F(ShellTest, PurgeAllowsFutureSkSLCache) {
+  sk_sp<SkData> shader_key = SkData::MakeWithCString("key");
+  sk_sp<SkData> shader_value = SkData::MakeWithCString("value");
+  std::string shader_filename = PersistentCache::SkKeyToFilePath(*shader_key);
+
+  fml::ScopedTemporaryDirectory base_dir;
+  ASSERT_TRUE(base_dir.fd().is_valid());
+  PersistentCache::SetCacheDirectoryPath(base_dir.path());
+  PersistentCache::ResetCacheForProcess();
+
+  // Run engine with purge_persistent_cache and cache_sksl.
+  auto settings = CreateSettingsForFixture();
+  settings.purge_persistent_cache = true;
+  settings.cache_sksl = true;
+  auto config = RunConfiguration::InferFromSettings(settings);
+  std::unique_ptr<Shell> shell = CreateShell(settings);
+  RunEngine(shell.get(), std::move(config));
+  auto persistent_cache = PersistentCache::GetCacheForProcess();
+  ASSERT_EQ(persistent_cache->LoadSkSLs().size(), 0u);
+
+  // Store the cache and verify it's valid.
+  StorePersistentCache(persistent_cache, *shader_key, *shader_value);
+  std::promise<bool> io_flushed;
+  shell->GetTaskRunners().GetIOTaskRunner()->PostTask(
+      [&io_flushed]() { io_flushed.set_value(true); });
+  io_flushed.get_future().get();  // Wait for the IO thread to flush the file.
+  ASSERT_GT(persistent_cache->LoadSkSLs().size(), 0u);
+
+  // Cleanup
+  fml::RemoveFilesInDirectory(base_dir.fd());
+  DestroyShell(std::move(shell));
+}
+
 }  // namespace testing
 }  // namespace flutter
diff --git a/shell/common/shell_test.cc b/shell/common/shell_test.cc
@@ -231,6 +231,12 @@ bool ShellTest::GetNeedsReportTimings(Shell* shell) {
   return shell->needs_report_timings_;
 }
 
+void ShellTest::StorePersistentCache(PersistentCache* cache,
+                                     const SkData& key,
+                                     const SkData& value) {
+  cache->store(key, value);
+}
+
 void ShellTest::OnServiceProtocol(
     Shell* shell,
     ServiceProtocolEnum some_protocol,

diff --git a/shell/common/shell_test.h b/shell/common/shell_test.h
@@ -85,6 +85,13 @@ class ShellTest : public FixtureTest {
   static bool GetNeedsReportTimings(Shell* shell);
   static void SetNeedsReportTimings(Shell* shell, bool value);
 
+  // Declare |StorePersistentCache| inside |ShellTest| so |PersistentCache| can
+  // friend |ShellTest| and allow us to call private |PersistentCache::store| in
+  // unit tests.
+  static void StorePersistentCache(PersistentCache* cache,
+                                   const SkData& key,
+                                   const SkData& value);
+
   enum ServiceProtocolEnum {
     kGetSkSLs,
     kEstimateRasterCacheMemory,