Browse Source

refactor: Add `ElectronBrowserContext::BrowserContexts()` (35-x-y backport) (#46161)

refactor: Add `ElectronBrowserContext::BrowserContexts()`

* refactor: add ElectronBrowserContext::BrowserContexts()

* refactor: use ElectronBrowserContext::BrowserContexts() in ElectronBrowserMainParts::PostMainMessageLoopRun()

* refactor: use ElectronBrowserContext::BrowserContexts() in ElectronExtensionsBrowserClient::IsValidContext()

* refactor: use ElectronBrowserContext::BrowserContexts() in ElectronExtensionsBrowserClient::BroadcastEventToRenderers()

* refactor: move PartitionKey, BrowserContextMap private

* refactor: add ElectronBrowserContext::IsValidContext()

decouple ElectronExtensionsBrowserClient from the internals of ElectronBrowserContext
Charles Kerr 1 month ago
parent
commit
998de7aa6c

+ 47 - 7
shell/browser/electron_browser_context.cc

@@ -12,6 +12,7 @@
 #include "base/barrier_closure.h"
 #include "base/base_paths.h"
 #include "base/command_line.h"
+#include "base/containers/to_vector.h"
 #include "base/files/file_path.h"
 #include "base/no_destructor.h"
 #include "base/path_service.h"
@@ -308,14 +309,53 @@ bool DoesDeviceMatch(const base::Value& device,
   return false;
 }
 
+// partition_id => browser_context
+struct PartitionKey {
+  PartitionKey(const std::string_view partition, bool in_memory)
+      : type_{Type::Partition}, location_{partition}, in_memory_{in_memory} {}
+
+  explicit PartitionKey(const base::FilePath& file_path)
+      : type_{Type::Path},
+        location_{file_path.AsUTF8Unsafe()},
+        in_memory_{false} {}
+
+  friend auto operator<=>(const PartitionKey&, const PartitionKey&) = default;
+
+ private:
+  enum class Type { Partition, Path };
+
+  Type type_;
+  std::string location_;
+  bool in_memory_;
+};
+
+[[nodiscard]] auto& ContextMap() {
+  static base::NoDestructor<
+      std::map<PartitionKey, std::unique_ptr<ElectronBrowserContext>>>
+      map;
+  return *map;
+}
+
 }  // namespace
 
 // static
-ElectronBrowserContext::BrowserContextMap&
-ElectronBrowserContext::browser_context_map() {
-  static base::NoDestructor<ElectronBrowserContext::BrowserContextMap>
-      browser_context_map;
-  return *browser_context_map;
+std::vector<ElectronBrowserContext*> ElectronBrowserContext::BrowserContexts() {
+  return base::ToVector(ContextMap(),
+                        [](auto& iter) { return iter.second.get(); });
+}
+
+bool ElectronBrowserContext::IsValidContext(const void* context) {
+  return std::ranges::any_of(ContextMap(), [context](const auto& iter) {
+    return iter.second.get() == context;
+  });
+}
+
+// static
+void ElectronBrowserContext::DestroyAllContexts() {
+  auto& map = ContextMap();
+  // Avoid UAF by destroying the default context last. See ba629e3 for info.
+  const auto extracted = map.extract(PartitionKey{"", false});
+  map.clear();
 }
 
 ElectronBrowserContext::ElectronBrowserContext(
@@ -833,7 +873,7 @@ ElectronBrowserContext* ElectronBrowserContext::From(
     const std::string& partition,
     bool in_memory,
     base::Value::Dict options) {
-  auto& context = browser_context_map()[PartitionKey(partition, in_memory)];
+  auto& context = ContextMap()[PartitionKey(partition, in_memory)];
   if (!context) {
     context.reset(new ElectronBrowserContext{std::cref(partition), in_memory,
                                              std::move(options)});
@@ -844,7 +884,7 @@ ElectronBrowserContext* ElectronBrowserContext::From(
 ElectronBrowserContext* ElectronBrowserContext::FromPath(
     const base::FilePath& path,
     base::Value::Dict options) {
-  auto& context = browser_context_map()[PartitionKey(path)];
+  auto& context = ContextMap()[PartitionKey(path)];
   if (!context) {
     context.reset(
         new ElectronBrowserContext{std::cref(path), false, std::move(options)});

+ 3 - 22
shell/browser/electron_browser_context.h

@@ -61,28 +61,9 @@ class ElectronBrowserContext : public content::BrowserContext {
   ElectronBrowserContext(const ElectronBrowserContext&) = delete;
   ElectronBrowserContext& operator=(const ElectronBrowserContext&) = delete;
 
-  // partition_id => browser_context
-  struct PartitionKey {
-    PartitionKey(const std::string_view partition, bool in_memory)
-        : type_{Type::Partition}, location_{partition}, in_memory_{in_memory} {}
+  [[nodiscard]] static std::vector<ElectronBrowserContext*> BrowserContexts();
 
-    explicit PartitionKey(const base::FilePath& file_path)
-        : type_{Type::Path},
-          location_{file_path.AsUTF8Unsafe()},
-          in_memory_{false} {}
-
-    friend auto operator<=>(const PartitionKey&, const PartitionKey&) = default;
-
-   private:
-    enum class Type { Partition, Path };
-
-    Type type_;
-    std::string location_;
-    bool in_memory_;
-  };
-
-  using BrowserContextMap =
-      std::map<PartitionKey, std::unique_ptr<ElectronBrowserContext>>;
+  [[nodiscard]] static bool IsValidContext(const void* context);
 
   // Get or create the BrowserContext according to its |partition| and
   // |in_memory|. The |options| will be passed to constructor when there is no
@@ -97,7 +78,7 @@ class ElectronBrowserContext : public content::BrowserContext {
   static ElectronBrowserContext* FromPath(const base::FilePath& path,
                                           base::Value::Dict options = {});
 
-  static BrowserContextMap& browser_context_map();
+  static void DestroyAllContexts();
 
   void SetUserAgent(const std::string& user_agent);
   std::string GetUserAgent() const;

+ 3 - 9
shell/browser/electron_browser_main_parts.cc

@@ -554,11 +554,9 @@ void ElectronBrowserMainParts::PostMainMessageLoopRun() {
 
   // Shutdown the DownloadManager before destroying Node to prevent
   // DownloadItem callbacks from crashing.
-  for (auto& iter : ElectronBrowserContext::browser_context_map()) {
-    auto* download_manager = iter.second.get()->GetDownloadManager();
-    if (download_manager) {
+  for (auto* browser_context : ElectronBrowserContext::BrowserContexts()) {
+    if (auto* download_manager = browser_context->GetDownloadManager())
       download_manager->Shutdown();
-    }
   }
 
   // Shutdown utility process created with Electron API before
@@ -598,11 +596,7 @@ void ElectronBrowserMainParts::PostMainMessageLoopRun() {
   node_bindings_->set_uv_env(nullptr);
   node_env_.reset();
 
-  auto default_context_key = ElectronBrowserContext::PartitionKey("", false);
-  std::unique_ptr<ElectronBrowserContext> default_context = std::move(
-      ElectronBrowserContext::browser_context_map()[default_context_key]);
-  ElectronBrowserContext::browser_context_map().clear();
-  default_context.reset();
+  ElectronBrowserContext::DestroyAllContexts();
 
   fake_browser_process_->PostMainMessageLoopRun();
   content::DevToolsAgentHost::StopRemoteDebuggingPipeHandler();

+ 5 - 13
shell/browser/extensions/electron_extensions_browser_client.cc

@@ -85,12 +85,7 @@ bool ElectronExtensionsBrowserClient::AreExtensionsDisabled(
 }
 
 bool ElectronExtensionsBrowserClient::IsValidContext(void* context) {
-  auto& context_map = ElectronBrowserContext::browser_context_map();
-  for (auto const& entry : context_map) {
-    if (entry.second && entry.second.get() == context)
-      return true;
-  }
-  return false;
+  return ElectronBrowserContext::IsValidContext(context);
 }
 
 bool ElectronExtensionsBrowserClient::IsSameContext(BrowserContext* first,
@@ -341,13 +336,10 @@ void ElectronExtensionsBrowserClient::BroadcastEventToRenderers(
     return;
   }
 
-  for (auto const& [key, browser_context] :
-       ElectronBrowserContext::browser_context_map()) {
-    if (browser_context) {
-      extensions::EventRouter::Get(browser_context.get())
-          ->BroadcastEvent(std::make_unique<extensions::Event>(
-              histogram_value, event_name, args.Clone()));
-    }
+  for (auto* browser_context : ElectronBrowserContext::BrowserContexts()) {
+    extensions::EventRouter::Get(browser_context)
+        ->BroadcastEvent(std::make_unique<extensions::Event>(
+            histogram_value, event_name, args.Clone()));
   }
 }