From db073cce765ddee969aaf0ef65d27bf841c47091 Mon Sep 17 00:00:00 2001
From: Kent Tamura <tkent@chromium.org>
Date: Thu, 22 Oct 2026 00:04:19 -0700
Subject: [PATCH] Forbid modifying weak LinkedHashSet during iteration

LinkedHashSet::IteratorWrapper holds a HashMap iterator (map_iterator_)
to strongify WeakMember entries in value_to_index_ during conservative
GC. Modifying a HeapLinkedHashSet<WeakMember<T>> or
GCedHeapLinkedHashSet<WeakMember<T>> while an iterator is active can
rehash value_to_index_ and leave map_iterator_ pointing to the old
backing store.

Call CheckModifications() from LinkedHashSet::IteratorWrapper when
IsWeakV<Value> is true, update documentation comments, and snapshot
weak LinkedHashSets before iterating in the following functions:
* DOMTaskSignal::SignalPriorityChange()
* Page::SetVisibilityState()
* LazyLoadMediaObserver::LoadAllImagesAndBlockLoadEvent()
* FullscreenController::NotifyFramesOfFullscreenEntry()

Fixed: 575851252
Change-Id: I3eefbcf737ee04ebfdd9685565235877b9668042
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8618181
Reviewed-by: Keishi Hattori <keishi@chromium.org>
Auto-Submit: Kent Tamura <tkent@chromium.org>
Commit-Queue: Kent Tamura <tkent@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1724484}
---
diff --git a/third_party/blink/renderer/core/frame/fullscreen_controller.cc b/third_party/blink/renderer/core/frame/fullscreen_controller.cc
index d40fce39..c4572fb 100644
--- a/third_party/blink/renderer/core/frame/fullscreen_controller.cc
+++ b/third_party/blink/renderer/core/frame/fullscreen_controller.cc
@@ -366,7 +366,9 @@
 void FullscreenController::NotifyFramesOfFullscreenEntry(bool granted) {
   // Notify all pending local frames in order whether or not we successfully
   // entered fullscreen.
-  for (LocalFrame* frame : *pending_frames_) {
+  HeapVector<Member<LocalFrame>> pending_frames(*pending_frames_);
+  pending_frames_->clear();
+  for (LocalFrame* frame : pending_frames) {
     if (frame) {
       if (Document* document = frame->GetDocument()) {
         Fullscreen::DidResolveEnterFullscreenRequest(*document, granted);
@@ -384,7 +386,6 @@
       Fullscreen::DidResolveEnterFullscreenRequest(*document, granted);
     }
   }
-  pending_frames_->clear();
 }
 
 void FullscreenController::EnterFullscreenCallback(bool granted) {
diff --git a/third_party/blink/renderer/core/html/media/lazy_load_media_observer.cc b/third_party/blink/renderer/core/html/media/lazy_load_media_observer.cc
index c2752e7..62567b9 100644
--- a/third_party/blink/renderer/core/html/media/lazy_load_media_observer.cc
+++ b/third_party/blink/renderer/core/html/media/lazy_load_media_observer.cc
@@ -81,14 +81,13 @@
     if (!IsDescendantOrSameDocument(element->GetDocument(), for_document)) {
       continue;
     }
-    if (auto* image_element = DynamicTo<HTMLImageElement>(element)) {
-      const_cast<HTMLImageElement*>(image_element)
-          ->LoadDeferredImageBlockingLoad();
-      resources_have_started_loading = true;
-    }
     to_be_unobserved.push_back(element);
   }
   for (Element* element : to_be_unobserved) {
+    if (auto* image_element = DynamicTo<HTMLImageElement>(element)) {
+      image_element->LoadDeferredImageBlockingLoad();
+      resources_have_started_loading = true;
+    }
     lazy_load_intersection_observer_->unobserve(element);
   }
   return resources_have_started_loading;
diff --git a/third_party/blink/renderer/core/page/page.cc b/third_party/blink/renderer/core/page/page.cc
index 2255eec..b85152c 100644
--- a/third_party/blink/renderer/core/page/page.cc
+++ b/third_party/blink/renderer/core/page/page.cc
@@ -821,8 +821,12 @@
   if (is_initial_state)
     return;
 
-  for (auto& observer : page_visibility_observer_set_) {
-    observer->PageVisibilityChanged();
+  HeapVector<Member<PageVisibilityObserver>> observers(
+      page_visibility_observer_set_);
+  for (auto& observer : observers) {
+    if (page_visibility_observer_set_.Contains(observer)) {
+      observer->PageVisibilityChanged();
+    }
   }
 
   if (main_frame_) {
diff --git a/third_party/blink/renderer/core/scheduler/dom_task_signal.cc b/third_party/blink/renderer/core/scheduler/dom_task_signal.cc
index 89dad7a..abc3f678 100644
--- a/third_party/blink/renderer/core/scheduler/dom_task_signal.cc
+++ b/third_party/blink/renderer/core/scheduler/dom_task_signal.cc
@@ -116,7 +116,8 @@
   const V8TaskPriority::Enum previous_priority = priority_;
   priority_ = priority;
 
-  for (AlgorithmHandle* handle : priority_change_algorithms_) {
+  HeapVector<Member<AlgorithmHandle>> algorithms(priority_change_algorithms_);
+  for (AlgorithmHandle* handle : algorithms) {
     handle->GetAlgorithm()->Run();
   }
 
@@ -127,12 +128,13 @@
 
   if (auto* source_signal_manager = DynamicTo<SourceSignalCompositionManager>(
           *priority_composition_manager_.Get())) {
-    // Dependents can be added while dispatching events, but none are removed
-    // since having an active iterator will strongify weak references, making
-    // the following iteration safe. Signaling priority change on newly added
-    // dependent signals has no effect since the new priority is already set.
-    for (auto& abort_signal : source_signal_manager->GetDependentSignals()) {
-      To<DOMTaskSignal>(abort_signal.Get())
+    // Dependents can be added while dispatching events. Signaling priority
+    // change on newly added dependent signals has no effect since the new
+    // priority is already set, so iterating a snapshot is sufficient.
+    HeapVector<Member<AbortSignal>> dependent_signals(
+        source_signal_manager->GetDependentSignals());
+    for (AbortSignal* abort_signal : dependent_signals) {
+      To<DOMTaskSignal>(abort_signal)
           ->SignalPriorityChange(priority, exception_state);
     }
   }
diff --git a/third_party/blink/renderer/platform/wtf/hash_table.h b/third_party/blink/renderer/platform/wtf/hash_table.h
index cf4e8a5..90dbf81 100644
--- a/third_party/blink/renderer/platform/wtf/hash_table.h
+++ b/third_party/blink/renderer/platform/wtf/hash_table.h
@@ -308,21 +308,21 @@
 #endif
   }
 
+ public:
+  constexpr HashTableConstIterator() = default;
+
   void CheckModifications() const {
 #if DCHECK_IS_ON()
     // HashTable and collections that build on it do not support
     // modifications while there is an iterator in use. The exception is
-    // LinkedHashSet, which has its own iterators that tolerate modification
-    // of the underlying set.
+    // non-weak LinkedHashSet, which has its own iterators that tolerate
+    // modification of the underlying set.
 
     DCHECK_EQ(container_modifications_, container_->Modifications());
     DCHECK(!container_->AccessForbidden());
 #endif
   }
 
- public:
-  constexpr HashTableConstIterator() = default;
-
   GetType Get() const {
     CheckModifications();
     return position_;
@@ -777,8 +777,8 @@
   void RegisterModification() { modifications_++; }
   // HashTable and collections that build on it do not support modifications
   // while there is an iterator in use. The exception is
-  // LinkedHashSet, which has its own iterators that tolerate modification
-  // of the underlying set.
+  // non-weak LinkedHashSet, which has its own iterators that tolerate
+  // modification of the underlying set.
   void CheckModifications(int64_t mods) const {
     DCHECK_EQ(mods, modifications_);
   }
diff --git a/third_party/blink/renderer/platform/wtf/linked_hash_set.h b/third_party/blink/renderer/platform/wtf/linked_hash_set.h
index b0f8c9d..7da88c9 100644
--- a/third_party/blink/renderer/platform/wtf/linked_hash_set.h
+++ b/third_party/blink/renderer/platform/wtf/linked_hash_set.h
@@ -49,6 +49,10 @@
 // long as you don't remove the current item. Moving items does not invalidate
 // iterator, so that it may cause unexpected behavior (i.e. loop unexpectedly
 // ends when moving the current item to last).
+// However, this does NOT apply to HeapLinkedHashSet<WeakMember<T>> and
+// GCedHeapLinkedHashSet<WeakMember<T>>: modifying a weak LinkedHashSet while an
+// iterator is active is not allowed because rehashing the underlying map
+// invalidates the internal iterator used to strongify weak entries during GC.
 //
 // Note: empty/deleted values as defined in HashTraits are not allowed.
 template <typename ValueArg,
@@ -99,15 +103,23 @@
     IteratorWrapper(const IteratorWrapper&) = default;
     IteratorWrapper& operator=(const IteratorWrapper&) = default;
 
-    const Value& operator*() const { return *iterator_; }
-    const Value* operator->() const { return &*iterator_; }
+    const Value& operator*() const {
+      CheckModifications();
+      return *iterator_;
+    }
+    const Value* operator->() const {
+      CheckModifications();
+      return &*iterator_;
+    }
 
     IteratorWrapper& operator++() {
+      CheckModifications();
       ++iterator_;
       return *this;
     }
 
     IteratorWrapper& operator--() {
+      CheckModifications();
       --iterator_;
       return *this;
     }
@@ -135,6 +147,12 @@
     IteratorWrapper(const T& it, const Map& map)
         : iterator_(it), map_iterator_(map.begin()) {}
 
+    void CheckModifications() const {
+      if constexpr (IsWeakV<Value>) {
+        map_iterator_.impl_.CheckModifications();
+      }
+    }
+
     // LinkedHashSet::list_ iterator.
     T iterator_;
 
diff --git a/third_party/blink/renderer/platform/heap/test/weakness_marking_test.cc b/third_party/blink/renderer/platform/heap/test/weakness_marking_test.cc
index c05b3f2a..bfbf8c13 100644
--- a/third_party/blink/renderer/platform/heap/test/weakness_marking_test.cc
+++ b/third_party/blink/renderer/platform/heap/test/weakness_marking_test.cc
@@ -2,6 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/test/gtest_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/blink/renderer/platform/heap/collection_support/heap_hash_map.h"
 #include "third_party/blink/renderer/platform/heap/collection_support/heap_hash_set.h"
@@ -229,6 +230,45 @@
   CHECK_EQ(weak_set->size(), kNumberOfWeakEntries);
 }
 
+TEST_F(WeaknessMarkingTest, StrongLinkedHashSetIteratorAcrossRehashAndGC) {
+  using StrongSet = GCedHeapLinkedHashSet<Member<IntegerObject>>;
+  Persistent<StrongSet> set = MakeGarbageCollected<StrongSet>();
+  Persistent<IntegerObject> first = MakeGarbageCollected<IntegerObject>(1);
+  set->insert(first.Get());
+  set->insert(MakeGarbageCollected<IntegerObject>(2));
+
+  size_t visited = 0;
+  for (auto it = set->begin(); it != set->end();) {
+    EXPECT_NE(it->Get(), nullptr);
+    if (visited == 0) {
+      // Trigger rehash of the underlying LinkedHashSet::value_to_index_ map.
+      for (int j = 10; j < 40; ++j) {
+        set->insert(MakeGarbageCollected<IntegerObject>(j));
+      }
+    }
+    ++it;
+    TestSupportingGC::ConservativelyCollectGarbage();
+    ++visited;
+  }
+  EXPECT_EQ(visited, 32u);
+}
+
+TEST_F(WeaknessMarkingTest, WeakLinkedHashSetModificationDuringIteration) {
+  using WeakSet = GCedHeapLinkedHashSet<WeakMember<IntegerObject>>;
+  Persistent<WeakSet> set = MakeGarbageCollected<WeakSet>();
+  Persistent<IntegerObject> first = MakeGarbageCollected<IntegerObject>(1);
+  Persistent<IntegerObject> second = MakeGarbageCollected<IntegerObject>(2);
+  set->insert(first.Get());
+  set->insert(second.Get());
+
+  auto it = set->begin();
+  EXPECT_EQ(it->Get(), first.Get());
+  set->insert(MakeGarbageCollected<IntegerObject>(3));
+  EXPECT_DCHECK_DEATH(++it);
+  EXPECT_DCHECK_DEATH(*it);
+  EXPECT_DCHECK_DEATH(it->Get());
+}
+
 }  // namespace weakness_marking_test
 
 }  // namespace blink

diff --git a/third_party/blink/renderer/platform/heap/test/weakness_marking_test.cc b/third_party/blink/renderer/platform/heap/test/weakness_marking_test.cc
index c05b3f2a..bfbf8c13 100644
--- a/third_party/blink/renderer/platform/heap/test/weakness_marking_test.cc
+++ b/third_party/blink/renderer/platform/heap/test/weakness_marking_test.cc
@@ -2,6 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/test/gtest_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/blink/renderer/platform/heap/collection_support/heap_hash_map.h"
 #include "third_party/blink/renderer/platform/heap/collection_support/heap_hash_set.h"
@@ -229,6 +230,45 @@
   CHECK_EQ(weak_set->size(), kNumberOfWeakEntries);
 }
 
+TEST_F(WeaknessMarkingTest, StrongLinkedHashSetIteratorAcrossRehashAndGC) {
+  using StrongSet = GCedHeapLinkedHashSet<Member<IntegerObject>>;
+  Persistent<StrongSet> set = MakeGarbageCollected<StrongSet>();
+  Persistent<IntegerObject> first = MakeGarbageCollected<IntegerObject>(1);
+  set->insert(first.Get());
+  set->insert(MakeGarbageCollected<IntegerObject>(2));
+
+  size_t visited = 0;
+  for (auto it = set->begin(); it != set->end();) {
+    EXPECT_NE(it->Get(), nullptr);
+    if (visited == 0) {
+      // Trigger rehash of the underlying LinkedHashSet::value_to_index_ map.
+      for (int j = 10; j < 40; ++j) {
+        set->insert(MakeGarbageCollected<IntegerObject>(j));
+      }
+    }
+    ++it;
+    TestSupportingGC::ConservativelyCollectGarbage();
+    ++visited;
+  }
+  EXPECT_EQ(visited, 32u);
+}
+
+TEST_F(WeaknessMarkingTest, WeakLinkedHashSetModificationDuringIteration) {
+  using WeakSet = GCedHeapLinkedHashSet<WeakMember<IntegerObject>>;
+  Persistent<WeakSet> set = MakeGarbageCollected<WeakSet>();
+  Persistent<IntegerObject> first = MakeGarbageCollected<IntegerObject>(1);
+  Persistent<IntegerObject> second = MakeGarbageCollected<IntegerObject>(2);
+  set->insert(first.Get());
+  set->insert(second.Get());
+
+  auto it = set->begin();
+  EXPECT_EQ(it->Get(), first.Get());
+  set->insert(MakeGarbageCollected<IntegerObject>(3));
+  EXPECT_DCHECK_DEATH(++it);
+  EXPECT_DCHECK_DEATH(*it);
+  EXPECT_DCHECK_DEATH(it->Get());
+}
+
 }  // namespace weakness_marking_test
 
 }  // namespace blink