fix: change ASAR archive cache to per-process to fix leak (#29536)

* fix: change ASAR archive cache to per-process to fix leak (#29292)

* chore: address code review comments

* chore: tighten up thread-safety

* chore: better address code review comments

* chore: more code review changes

Co-authored-by: David Sanders <dsanders11@ucsbalum.com>

shell/browser/electron_browser_main_parts.cc

@@ -44,7 +44,6 @@
 #include "shell/browser/ui/devtools_manager_delegate.h"
 #include "shell/common/api/electron_bindings.h"
 #include "shell/common/application_info.h"
-#include "shell/common/asar/asar_util.h"
 #include "shell/common/electron_paths.h"
 #include "shell/common/gin_helper/trackable_object.h"
 #include "shell/common/node_bindings.h"
@@ -209,9 +208,7 @@ ElectronBrowserMainParts::ElectronBrowserMainParts(
   self_ = this;
 }
 
-ElectronBrowserMainParts::~ElectronBrowserMainParts() {
-  asar::ClearArchives();
-}
+ElectronBrowserMainParts::~ElectronBrowserMainParts() = default;
 
 // static
 ElectronBrowserMainParts* ElectronBrowserMainParts::Get() {

shell/common/asar/archive.cc

@@ -8,6 +8,7 @@
 #include <utility>
 #include <vector>
 
+#include "base/check.h"
 #include "base/files/file.h"
 #include "base/files/file_util.h"
 #include "base/json/json_reader.h"
@@ -118,7 +119,7 @@ bool FillFileInfoWithNode(Archive::FileInfo* info,
 }  // namespace
 
 Archive::Archive(const base::FilePath& path)
-    : path_(path), file_(base::File::FILE_OK) {
+    : initialized_(false), path_(path), file_(base::File::FILE_OK) {
   base::ThreadRestrictions::ScopedAllowIO allow_io;
   file_.Initialize(path_, base::File::FLAG_OPEN | base::File::FLAG_READ);
 #if defined(OS_WIN)
@@ -141,6 +142,10 @@ Archive::~Archive() {
 }
 
 bool Archive::Init() {
+  // Should only be initialized once
+  CHECK(!initialized_);
+  initialized_ = true;
+
   if (!file_.IsValid()) {
     if (file_.error_details() != base::File::FILE_ERROR_NOT_FOUND) {
       LOG(WARNING) << "Opening " << path_.value() << ": "
@@ -198,7 +203,7 @@ bool Archive::Init() {
   return true;
 }
 
-bool Archive::GetFileInfo(const base::FilePath& path, FileInfo* info) {
+bool Archive::GetFileInfo(const base::FilePath& path, FileInfo* info) const {
   if (!header_)
     return false;
 
@@ -213,7 +218,7 @@ bool Archive::GetFileInfo(const base::FilePath& path, FileInfo* info) {
   return FillFileInfoWithNode(info, header_size_, node);
 }
 
-bool Archive::Stat(const base::FilePath& path, Stats* stats) {
+bool Archive::Stat(const base::FilePath& path, Stats* stats) const {
   if (!header_)
     return false;
 
@@ -237,7 +242,7 @@ bool Archive::Stat(const base::FilePath& path, Stats* stats) {
 }
 
 bool Archive::Readdir(const base::FilePath& path,
-                      std::vector<base::FilePath>* files) {
+                      std::vector<base::FilePath>* files) const {
   if (!header_)
     return false;
 
@@ -257,7 +262,8 @@ bool Archive::Readdir(const base::FilePath& path,
   return true;
 }
 
-bool Archive::Realpath(const base::FilePath& path, base::FilePath* realpath) {
+bool Archive::Realpath(const base::FilePath& path,
+                       base::FilePath* realpath) const {
   if (!header_)
     return false;
 
@@ -276,6 +282,11 @@ bool Archive::Realpath(const base::FilePath& path, base::FilePath* realpath) {
 }
 
 bool Archive::CopyFileOut(const base::FilePath& path, base::FilePath* out) {
+  if (!header_)
+    return false;
+
+  base::AutoLock auto_lock(external_files_lock_);
+
   auto it = external_files_.find(path.value());
   if (it != external_files_.end()) {
     *out = it->second->path();

shell/common/asar/archive.h

@@ -11,6 +11,7 @@
 
 #include "base/files/file.h"
 #include "base/files/file_path.h"
+#include "base/synchronization/lock.h"
 
 namespace base {
 class DictionaryValue;
@@ -21,7 +22,7 @@ namespace asar {
 class ScopedTemporaryFile;
 
 // This class represents an asar package, and provides methods to read
-// information from it.
+// information from it. It is thread-safe after |Init| has been called.
 class Archive {
  public:
   struct FileInfo {
@@ -46,16 +47,17 @@ class Archive {
   bool Init();
 
   // Get the info of a file.
-  bool GetFileInfo(const base::FilePath& path, FileInfo* info);
+  bool GetFileInfo(const base::FilePath& path, FileInfo* info) const;
 
   // Fs.stat(path).
-  bool Stat(const base::FilePath& path, Stats* stats);
+  bool Stat(const base::FilePath& path, Stats* stats) const;
 
   // Fs.readdir(path).
-  bool Readdir(const base::FilePath& path, std::vector<base::FilePath>* files);
+  bool Readdir(const base::FilePath& path,
+               std::vector<base::FilePath>* files) const;
 
   // Fs.realpath(path).
-  bool Realpath(const base::FilePath& path, base::FilePath* realpath);
+  bool Realpath(const base::FilePath& path, base::FilePath* realpath) const;
 
   // Copy the file into a temporary file, and return the new path.
   // For unpacked file, this method will return its real path.
@@ -65,16 +67,17 @@ class Archive {
   int GetFD() const;
 
   base::FilePath path() const { return path_; }
-  base::DictionaryValue* header() const { return header_.get(); }
 
  private:
-  base::FilePath path_;
+  bool initialized_;
+  const base::FilePath path_;
   base::File file_;
   int fd_ = -1;
   uint32_t header_size_ = 0;
   std::unique_ptr<base::DictionaryValue> header_;
 
   // Cached external temporary files.
+  base::Lock external_files_lock_;
   std::unordered_map<base::FilePath::StringType,
                      std::unique_ptr<ScopedTemporaryFile>>
       external_files_;

shell/common/asar/asar_util.cc

@@ -6,11 +6,14 @@
 
 #include <map>
 #include <string>
+#include <utility>
 
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/lazy_instance.h"
+#include "base/no_destructor.h"
 #include "base/stl_util.h"
+#include "base/synchronization/lock.h"
 #include "base/threading/thread_local.h"
 #include "base/threading/thread_restrictions.h"
 #include "shell/common/asar/archive.h"
@@ -19,30 +22,41 @@ namespace asar {
 
 namespace {
 
-// The global instance of ArchiveMap, will be destroyed on exit.
 typedef std::map<base::FilePath, std::shared_ptr<Archive>> ArchiveMap;
-base::LazyInstance<base::ThreadLocalPointer<ArchiveMap>>::Leaky
-    g_archive_map_tls = LAZY_INSTANCE_INITIALIZER;
 
 const base::FilePath::CharType kAsarExtension[] = FILE_PATH_LITERAL(".asar");
 
-std::map<base::FilePath, bool> g_is_directory_cache;
-
 bool IsDirectoryCached(const base::FilePath& path) {
-  auto it = g_is_directory_cache.find(path);
-  if (it != g_is_directory_cache.end()) {
+  static base::NoDestructor<std::map<base::FilePath, bool>>
+      s_is_directory_cache;
+  static base::NoDestructor<base::Lock> lock;
+
+  base::AutoLock auto_lock(*lock);
+  auto& is_directory_cache = *s_is_directory_cache;
+
+  auto it = is_directory_cache.find(path);
+  if (it != is_directory_cache.end()) {
     return it->second;
   }
   base::ThreadRestrictions::ScopedAllowIO allow_io;
-  return g_is_directory_cache[path] = base::DirectoryExists(path);
+  return is_directory_cache[path] = base::DirectoryExists(path);
 }
 
 }  // namespace
 
+ArchiveMap& GetArchiveCache() {
+  static base::NoDestructor<ArchiveMap> s_archive_map;
+  return *s_archive_map;
+}
+
+base::Lock& GetArchiveCacheLock() {
+  static base::NoDestructor<base::Lock> lock;
+  return *lock;
+}
+
 std::shared_ptr<Archive> GetOrCreateAsarArchive(const base::FilePath& path) {
-  if (!g_archive_map_tls.Pointer()->Get())
-    g_archive_map_tls.Pointer()->Set(new ArchiveMap);
-  ArchiveMap& map = *g_archive_map_tls.Pointer()->Get();
+  base::AutoLock auto_lock(GetArchiveCacheLock());
+  ArchiveMap& map = GetArchiveCache();
 
   // if we have it, return it
   const auto lower = map.lower_bound(path);
@@ -61,8 +75,10 @@ std::shared_ptr<Archive> GetOrCreateAsarArchive(const base::FilePath& path) {
 }
 
 void ClearArchives() {
-  if (g_archive_map_tls.Pointer()->Get())
-    delete g_archive_map_tls.Pointer()->Get();
+  base::AutoLock auto_lock(GetArchiveCacheLock());
+  ArchiveMap& map = GetArchiveCache();
+
+  map.clear();
 }
 
 bool GetAsarArchivePath(const base::FilePath& full_path,

shell/common/asar/asar_util.h

@@ -16,7 +16,7 @@ namespace asar {
 
 class Archive;
 
-// Gets or creates a new Archive from the path.
+// Gets or creates and caches a new Archive from the path.
 std::shared_ptr<Archive> GetOrCreateAsarArchive(const base::FilePath& path);
 
 // Destroy cached Archive objects.

shell/renderer/electron_renderer_client.cc

@@ -11,7 +11,6 @@
 #include "content/public/renderer/render_frame.h"
 #include "electron/buildflags/buildflags.h"
 #include "shell/common/api/electron_bindings.h"
-#include "shell/common/asar/asar_util.h"
 #include "shell/common/gin_helper/dictionary.h"
 #include "shell/common/gin_helper/event_emitter_caller.h"
 #include "shell/common/node_bindings.h"
@@ -39,9 +38,7 @@ ElectronRendererClient::ElectronRendererClient()
           NodeBindings::Create(NodeBindings::BrowserEnvironment::kRenderer)),
       electron_bindings_(new ElectronBindings(node_bindings_->uv_loop())) {}
 
-ElectronRendererClient::~ElectronRendererClient() {
-  asar::ClearArchives();
-}
+ElectronRendererClient::~ElectronRendererClient() = default;
 
 void ElectronRendererClient::RenderFrameCreated(
     content::RenderFrame* render_frame) {

shell/renderer/web_worker_observer.cc

@@ -7,7 +7,6 @@
 #include "base/lazy_instance.h"
 #include "base/threading/thread_local.h"
 #include "shell/common/api/electron_bindings.h"
-#include "shell/common/asar/asar_util.h"
 #include "shell/common/gin_helper/event_emitter_caller.h"
 #include "shell/common/node_bindings.h"
 #include "shell/common/node_includes.h"
@@ -39,7 +38,6 @@ WebWorkerObserver::~WebWorkerObserver() {
   lazy_tls.Pointer()->Set(nullptr);
   node::FreeEnvironment(node_bindings_->uv_env());
   node::FreeIsolateData(node_bindings_->isolate_data());
-  asar::ClearArchives();
 }
 
 void WebWorkerObserver::WorkerScriptReadyForEvaluation(