From 45d28f9b7e80fd416f9c3859678694d46e4d3ed5 Mon Sep 17 00:00:00 2001
From: Alvin Ji <alvinji@chromium.org>
Date: Wed, 03 Jun 2026 15:33:14 -0700
Subject: [PATCH] serial: Isolate SerialPort caching per DOMWrapperWorld

This CL resolves a cross-world V8 Promise leak by isolating the caching
of SerialPort instances to their respective V8 execution worlds.
Previously, the Serial globally cached SerialPort and WritableStream
objects per frame, which allowed a webpage in the main world to retrieve
a pending abort promise from an extension's isolated world, leading to
prototype pollution. By introducing a new world-keyed cache structure
inside the Serial supplement, each DOMWrapperWorld now manages and
retrieves its own isolated SerialPort instance.

Bug: 517406035
Change-Id: I6a6ad0c687374429d601601284a8573ab9f816bd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7884417
Commit-Queue: Alvin Ji <alvinji@chromium.org>
Reviewed-by: Matt Reynolds <mattreynolds@chromium.org>
Reviewed-by: Koji Ishii <kojii@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1641261}
---

diff --git a/content/child/runtime_features.cc b/content/child/runtime_features.cc
index f95c36a7..203fa08 100644
--- a/content/child/runtime_features.cc
+++ b/content/child/runtime_features.cc
@@ -366,6 +366,8 @@
           {"RelatedWebsitePartitionAPI",
            raw_ref(net::features::kRelatedWebsitePartitionAPI)},
           {"SerialPortConnected", raw_ref(features::kSerialPortConnected)},
+          {"WebSerialWorldIsolatedCache",
+           raw_ref(features::kWebSerialWorldIsolatedCache)},
           {"SplitViewLinkOpen", raw_ref(features::kSplitViewLinkOpen)},
 #if BUILDFLAG(IS_MAC)
           {"SystemDefaultAccentColors",
diff --git a/services/device/public/cpp/device_features.cc b/services/device/public/cpp/device_features.cc
index 0e89d75..05f70d9 100644
--- a/services/device/public/cpp/device_features.cc
+++ b/services/device/public/cpp/device_features.cc
@@ -31,6 +31,10 @@
 #endif  // !BUILDFLAG(IS_ANDROID)
 );
 
+// Restricts the sharing of C++ SerialPort and WritableStream instances across
+// different DOMWrapperWorld contexts to prevent cross-world leaks.
+BASE_FEATURE(kWebSerialWorldIsolatedCache, base::FEATURE_ENABLED_BY_DEFAULT);
+
 // Avoid triggering the macOS Bluetooth permission prompt when
 // navigator.serial.getPorts() is called and permission is undetermined.
 BASE_FEATURE(kAvoidBluetoothPromptInGetPorts, base::FEATURE_ENABLED_BY_DEFAULT);
diff --git a/services/device/public/cpp/device_features.h b/services/device/public/cpp/device_features.h
index 393545f4..e223c3dc 100644
--- a/services/device/public/cpp/device_features.h
+++ b/services/device/public/cpp/device_features.h
@@ -25,6 +25,7 @@
 DEVICE_FEATURES_EXPORT BASE_DECLARE_FEATURE(
     kSeverSensorConnectionsOnPermissionRevocation);
 DEVICE_FEATURES_EXPORT BASE_DECLARE_FEATURE(kSerialPortConnected);
+DEVICE_FEATURES_EXPORT BASE_DECLARE_FEATURE(kWebSerialWorldIsolatedCache);
 DEVICE_FEATURES_EXPORT BASE_DECLARE_FEATURE(kAvoidBluetoothPromptInGetPorts);
 DEVICE_FEATURES_EXPORT BASE_DECLARE_FEATURE(kWebUsbBlocklist);
 DEVICE_FEATURES_EXPORT BASE_DECLARE_FEATURE(
diff --git a/third_party/blink/renderer/modules/serial/serial.cc b/third_party/blink/renderer/modules/serial/serial.cc
index f6b4b25f..f655e76 100644
--- a/third_party/blink/renderer/modules/serial/serial.cc
+++ b/third_party/blink/renderer/modules/serial/serial.cc
@@ -12,8 +12,10 @@
 #include "services/network/public/mojom/permissions_policy/permissions_policy_feature.mojom-blink.h"
 #include "third_party/blink/public/mojom/serial/serial.mojom-blink.h"
 #include "third_party/blink/public/platform/browser_interface_broker_proxy.h"
+#include "third_party/blink/renderer/bindings/core/v8/local_window_proxy.h"
 #include "third_party/blink/renderer/bindings/core/v8/script_promise.h"
 #include "third_party/blink/renderer/bindings/core/v8/script_promise_resolver.h"
+#include "third_party/blink/renderer/bindings/core/v8/worker_or_worklet_script_controller.h"
 #include "third_party/blink/renderer/bindings/modules/v8/v8_serial_port_filter.h"
 #include "third_party/blink/renderer/bindings/modules/v8/v8_serial_port_request_options.h"
 #include "third_party/blink/renderer/core/dom/document.h"
@@ -25,10 +27,13 @@
 #include "third_party/blink/renderer/core/frame/local_frame.h"
 #include "third_party/blink/renderer/core/workers/dedicated_worker_global_scope.h"
 #include "third_party/blink/renderer/core/workers/worker_global_scope.h"
+#include "third_party/blink/renderer/core/workers/worker_or_worklet_global_scope.h"
 #include "third_party/blink/renderer/modules/bluetooth/bluetooth_uuid.h"
 #include "third_party/blink/renderer/modules/event_target_modules_names.h"
 #include "third_party/blink/renderer/modules/serial/serial_port.h"
+#include "third_party/blink/renderer/platform/bindings/dom_wrapper_world.h"
 #include "third_party/blink/renderer/platform/heap/garbage_collected.h"
+#include "third_party/blink/renderer/platform/runtime_enabled_features.h"
 
 namespace blink {
 
@@ -123,19 +128,84 @@
 }
 
 void Serial::ContextDestroyed() {
-  for (auto& entry : port_cache_)
+  for (auto& cache_entry : port_caches_) {
+    for (auto& entry : cache_entry.value->port_cache()) {
+      entry.value->ContextDestroyed();
+    }
+  }
+  for (auto& entry : port_cache_) {
     entry.value->ContextDestroyed();
+  }
 }
 
 void Serial::OnPortConnectedStateChanged(
     mojom::blink::SerialPortInfoPtr port_info) {
   bool connected = port_info->connected;
-  SerialPort* port = GetOrCreatePort(std::move(port_info));
-  port->set_connected(connected);
-  if (connected) {
-    port->DispatchEvent(*Event::CreateBubble(event_type_names::kConnect));
+  const AtomicString& event_type =
+      connected ? event_type_names::kConnect : event_type_names::kDisconnect;
+
+  if (RuntimeEnabledFeatures::WebSerialWorldIsolatedCacheEnabled()) {
+    ExecutionContext* context = GetExecutionContext();
+    if (!context) {
+      return;
+    }
+
+    if (context->IsWindow()) {
+      LocalDOMWindow* window = To<LocalDOMWindow>(context);
+      LocalFrame* frame = window->GetFrame();
+      if (!frame) {
+        return;
+      }
+
+      // Handle scope is required to manage temporary V8 handles allocated
+      // during world iteration, preventing V8 memory leaks.
+      v8::Isolate* isolate = context->GetIsolate();
+      v8::HandleScope handle_scope(isolate);
+      HeapVector<Member<DOMWrapperWorld>> worlds;
+      DOMWrapperWorld::AllWorldsInIsolate(isolate, worlds);
+
+      for (DOMWrapperWorld* world : worlds) {
+        // A world is only active in this frame if it has an initialized window
+        // proxy context. This prevents dispatching events or creating port
+        // objects for worlds (like extensions) that exist in the isolate but
+        // are not actively running on this specific page.
+        LocalWindowProxy* window_proxy =
+            frame->WindowProxyMaybeUninitialized(*world);
+        if (window_proxy && !window_proxy->ContextIfInitialized().IsEmpty()) {
+          SerialPort* port = GetOrCreatePort(*world, port_info->Clone());
+          port->set_connected(connected);
+          port->DispatchEvent(*Event::CreateBubble(event_type));
+        }
+      }
+    } else if (context->IsWorkerGlobalScope()) {
+      auto* worker_global_scope = To<WorkerOrWorkletGlobalScope>(context);
+      // Workers run in a single-world environment (no isolated
+      // worlds/extensions). Thus, we don't need to loop over worlds and can
+      // retrieve the single active worker world directly from the script
+      // controller.
+      WorkerOrWorkletScriptController* script_controller =
+          worker_global_scope->ScriptController();
+      // Only proceed if the JS execution context is initialized. If the worker
+      // is terminating or not fully started, we should not dispatch events.
+      if (script_controller && script_controller->IsContextInitialized()) {
+        v8::Isolate* isolate = context->GetIsolate();
+        v8::HandleScope handle_scope(isolate);
+        ScriptState* script_state = script_controller->GetScriptState();
+        if (script_state) {
+          DOMWrapperWorld& world = script_state->World();
+          SerialPort* port = GetOrCreatePort(world, std::move(port_info));
+          port->set_connected(connected);
+          port->DispatchEvent(*Event::CreateBubble(event_type));
+        }
+      }
+    }
   } else {
-    port->DispatchEvent(*Event::CreateBubble(event_type_names::kDisconnect));
+    // Legacy fallback path when WebSerialWorldIsolatedCache is disabled.
+    // Uses the shared port_cache_ instead of the world-isolated port_caches_.
+    // This block can be safely removed when the feature flag is cleaned up.
+    SerialPort* port = GetOrCreatePort(std::move(port_info));
+    port->set_connected(connected);
+    port->DispatchEvent(*Event::CreateBubble(event_type));
   }
 }
 
@@ -277,11 +347,16 @@
   service_->ForgetPort(token, std::move(callback));
 }
 
+void Serial::SerialPortCache::Trace(Visitor* visitor) const {
+  visitor->Trace(port_cache_);
+}
+
 void Serial::Trace(Visitor* visitor) const {
   visitor->Trace(service_);
   visitor->Trace(receiver_);
   visitor->Trace(get_ports_promises_);
   visitor->Trace(request_port_promises_);
+  visitor->Trace(port_caches_);
   visitor->Trace(port_cache_);
   EventTarget::Trace(visitor);
   Supplement<NavigatorBase>::Trace(visitor);

diff --git a/third_party/blink/renderer/modules/serial/serial_unittest.cc b/third_party/blink/renderer/modules/serial/serial_unittest.cc
index 1e5cc33..82b9023 100644
--- a/third_party/blink/renderer/modules/serial/serial_unittest.cc
+++ b/third_party/blink/renderer/modules/serial/serial_unittest.cc
@@ -8,8 +8,17 @@
 #include "third_party/blink/renderer/bindings/core/v8/v8_binding_for_testing.h"
 #include "third_party/blink/renderer/bindings/core/v8/v8_union_string_unsignedlong.h"
 #include "third_party/blink/renderer/bindings/modules/v8/v8_serial_port_filter.h"
+#include "third_party/blink/renderer/core/frame/local_dom_window.h"
+#include "third_party/blink/renderer/core/frame/local_frame.h"
+#include "third_party/blink/renderer/core/frame/navigator.h"
+#include "third_party/blink/renderer/modules/serial/serial_port.h"
+#include "third_party/blink/renderer/platform/bindings/dom_wrapper_world.h"
 #include "third_party/blink/renderer/platform/bindings/exception_code.h"
+#include "third_party/blink/renderer/platform/bindings/script_state.h"
 #include "third_party/blink/renderer/platform/heap/garbage_collected.h"
+#include "third_party/blink/renderer/platform/heap/persistent.h"
+#include "third_party/blink/renderer/platform/heap/thread_state.h"
+#include "third_party/blink/renderer/platform/testing/runtime_enabled_features_test_helpers.h"
 #include "third_party/blink/renderer/platform/testing/task_environment.h"
 
 namespace blink {
@@ -141,4 +150,205 @@
             scope.GetExceptionState().Code());
 }
 
+class SerialTestHelper {
+ public:
+  static SerialPort* GetOrCreatePort(Serial* serial,
+                                     ScriptState* script_state,
+                                     mojom::blink::SerialPortInfoPtr info) {
+    return serial->GetOrCreatePort(script_state, std::move(info));
+  }
+  static size_t CacheSize(Serial* serial) {
+    return serial->port_caches_.size();
+  }
+  static size_t CacheSizeForWorld(Serial* serial, DOMWrapperWorld& world) {
+    auto it = serial->port_caches_.find(&world);
+    if (it != serial->port_caches_.end()) {
+      return it->value->port_cache().size();
+    }
+    return 0;
+  }
+  static size_t DefaultCacheSize(Serial* serial) {
+    return serial->port_cache_.size();
+  }
+};
+
+// Verifies that when WebSerialWorldIsolatedCache is enabled, SerialPort
+// instances are cached per-world. Requesting the same port token in different
+// worlds (main vs. isolated) must return different C++ objects.
+TEST(SerialTest, WorldIsolatedCache) {
+  test::TaskEnvironment task_environment;
+  V8TestingScope scope;
+  v8::Isolate* isolate = scope.GetIsolate();
+
+  // Enable feature
+  ScopedWebSerialWorldIsolatedCacheForTest feature_helper(true);
+
+  Navigator* navigator = scope.GetFrame().DomWindow()->navigator();
+  Serial* serial = Serial::serial(*navigator);
+  ASSERT_TRUE(serial);
+
+  // Create main world script state
+  ScriptState* main_script_state = scope.GetScriptState();
+  DOMWrapperWorld& main_world = main_script_state->World();
+  EXPECT_TRUE(main_world.IsMainWorld());
+
+  // Create isolated world
+  DOMWrapperWorld* isolated_world =
+      DOMWrapperWorld::EnsureIsolatedWorld(isolate, 1);
+  EXPECT_TRUE(isolated_world->IsIsolatedWorld());
+
+  // We need a ScriptState for the isolated world.
+  // To do that, we can get the WindowProxy for the isolated world.
+  scope.GetFrame().GetWindowProxy(*isolated_world);  // Force initialization
+  ScriptState* isolated_script_state =
+      ToScriptState(&scope.GetFrame(), *isolated_world);
+  ASSERT_TRUE(isolated_script_state);
+
+  // We use the same token for both port infos to simulate two different worlds
+  // requesting access to the exact same physical serial device.
+  auto token = base::UnguessableToken::Create();
+
+  // Create port info
+  auto info1 = mojom::blink::SerialPortInfo::New();
+  info1->token = token;
+  info1->connected = true;
+
+  auto info2 = mojom::blink::SerialPortInfo::New();
+  info2->token = token;
+  info2->connected = true;
+
+  // GetOrCreatePort in main world
+  SerialPort* port_main = SerialTestHelper::GetOrCreatePort(
+      serial, main_script_state, std::move(info1));
+  ASSERT_TRUE(port_main);
+
+  // GetOrCreatePort in isolated world
+  SerialPort* port_isolated = SerialTestHelper::GetOrCreatePort(
+      serial, isolated_script_state, std::move(info2));
+  ASSERT_TRUE(port_isolated);
+
+  // They should be different C++ objects
+  EXPECT_NE(port_main, port_isolated);
+
+  // Cache sizes should be updated
+  EXPECT_EQ(SerialTestHelper::CacheSize(serial), 2U);
+  EXPECT_EQ(SerialTestHelper::DefaultCacheSize(serial), 0U);
+}
+
+// Verifies that when WebSerialWorldIsolatedCache is disabled, the cache
+// falls back to the legacy behavior where requesting the same port token in
+// different worlds returns the exact same C++ object.
+TEST(SerialTest, WorldIsolatedCacheDisabled) {
+  test::TaskEnvironment task_environment;
+  V8TestingScope scope;
+  v8::Isolate* isolate = scope.GetIsolate();
+
+  // Disable feature
+  ScopedWebSerialWorldIsolatedCacheForTest feature_helper(false);
+
+  Navigator* navigator = scope.GetFrame().DomWindow()->navigator();
+  Serial* serial = Serial::serial(*navigator);
+  ASSERT_TRUE(serial);
+
+  ScriptState* main_script_state = scope.GetScriptState();
+
+  DOMWrapperWorld* isolated_world =
+      DOMWrapperWorld::EnsureIsolatedWorld(isolate, 1);
+  scope.GetFrame().GetWindowProxy(*isolated_world);  // Force initialization
+  ScriptState* isolated_script_state =
+      ToScriptState(&scope.GetFrame(), *isolated_world);
+  ASSERT_TRUE(isolated_script_state);
+
+  // We use the same token for both port infos to simulate two different worlds
+  // requesting access to the exact same physical serial device.
+  auto token = base::UnguessableToken::Create();
+
+  auto info1 = mojom::blink::SerialPortInfo::New();
+  info1->token = token;
+  info1->connected = true;
+
+  auto info2 = mojom::blink::SerialPortInfo::New();
+  info2->token = token;
+  info2->connected = true;
+
+  // GetOrCreatePort in main world
+  SerialPort* port_main = SerialTestHelper::GetOrCreatePort(
+      serial, main_script_state, std::move(info1));
+  ASSERT_TRUE(port_main);
+
+  // GetOrCreatePort in isolated world
+  SerialPort* port_isolated = SerialTestHelper::GetOrCreatePort(
+      serial, isolated_script_state, std::move(info2));
+  ASSERT_TRUE(port_isolated);
+
+  // They should be the SAME C++ object
+  EXPECT_EQ(port_main, port_isolated);
+
+  // Cache sizes should be updated
+  EXPECT_EQ(SerialTestHelper::CacheSize(serial), 0U);
+  EXPECT_EQ(SerialTestHelper::DefaultCacheSize(serial), 1U);
+}
+
+// Verifies the entire lifecycle and memory safety of the weak-caching
+// mechanism. Specifically, it ensures that:
+// 1. SerialPort objects can be garbage-collected when no longer in use.
+// 2. The cache automatically removes dead entries to prevent memory leaks.
+// 3. Requesting a previously GCed port safely triggers new instance creation.
+TEST(SerialTest, GCedPortIsRecreated) {
+  test::TaskEnvironment task_environment;
+  V8TestingScope scope;
+
+  Navigator* navigator = scope.GetFrame().DomWindow()->navigator();
+  Serial* serial = Serial::serial(*navigator);
+  ASSERT_TRUE(serial);
+
+  ScriptState* script_state = scope.GetScriptState();
+  DOMWrapperWorld& main_world = script_state->World();
+  auto token = base::UnguessableToken::Create();
+
+  // We store the port in a WeakPersistent, which does not keep the port alive
+  // during GC.
+  WeakPersistent<SerialPort> weak_port;
+  {
+    auto info = mojom::blink::SerialPortInfo::New();
+    info->token = token;
+    info->connected = true;
+    // Requesting the port creates it and caches it.
+    SerialPort* port = SerialTestHelper::GetOrCreatePort(serial, script_state,
+                                                         std::move(info));
+    ASSERT_TRUE(port);
+    weak_port = port;
+  }
+
+  // Exiting this scope destroys the strong C++ pointer 'port'. Now, only
+  // weak references (in the cache and 'weak_port') point to the SerialPort.
+  EXPECT_EQ(SerialTestHelper::CacheSizeForWorld(serial, main_world), 1U);
+
+  // Force Oilpan garbage collection. Since there are no active strong
+  // references (JS or C++) to the SerialPort, it will be collected, and
+  // Oilpan's weak processing should automatically remove the dead entry from
+  // the cache map.
+  ThreadState::Current()->CollectAllGarbageForTesting(
+      ThreadState::StackState::kNoHeapPointers);
+
+  // The port should be GCed
+  EXPECT_FALSE(weak_port);
+
+  // Cache should be empty now
+  EXPECT_EQ(SerialTestHelper::CacheSizeForWorld(serial, main_world), 0U);
+
+  // GetOrCreatePort again
+  auto info2 = mojom::blink::SerialPortInfo::New();
+  info2->token = token;
+  info2->connected = true;
+  SerialPort* port2 =
+      SerialTestHelper::GetOrCreatePort(serial, script_state, std::move(info2));
+
+  // It should be non-null (a new port)
+  ASSERT_TRUE(port2);
+  EXPECT_NE(port2, weak_port);
+
+  EXPECT_EQ(SerialTestHelper::CacheSizeForWorld(serial, main_world), 1U);
+}
+
 }  // namespace blink