Blink: Introduce LifecycleNotifier::ForEachObserver().

LifecycleNotifier currently exposes its internal state (observers_,
iteration_state_) to subclasses, and relies on the subclasses to
correctly update iteration_state_ while iterating over observers_, like
PageVisibilityNotifier::NotifyPageVisibilityChanged() does. Sadly, it's
easy to get this wrong, as shown by SynchronousMutationNotifier (does
not update iteration_state_ at all) and ContextLifecycleNotifier (only
updates iteration_state_ in some cases).

This CL makes the LifecycleNotifier state private, and adds a
ForEachObserver() method that safely iterates over observers_,
encapsulating the needed changes to iteration_state_ and a cast from
LifecycleObserverBase* to the desired observer type.

Change-Id: Ibb23f2c7e2bba9bdf08d49e8569a354934ed4e11
Reviewed-on: https://chromium-review.googlesource.com/1132674Reviewed-by: Kent Tamura <tkent@chromium.org>
Commit-Queue: Victor Costan <pwnall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574076}

third_party/blink/renderer/core/dom/context_lifecycle_notifier.cc

@@ -33,67 +33,58 @@
 namespace blink {
 
 void ContextLifecycleNotifier::NotifyResumingPausableObjects() {
-  base::AutoReset<IterationState> scope(&iteration_state_, kAllowingNone);
-  for (LifecycleObserverBase* observer_base : observers_) {
-    ContextLifecycleObserver* observer =
-        static_cast<ContextLifecycleObserver*>(observer_base);
+  ForEachObserver([&](ContextLifecycleObserver* observer) {
     if (observer->ObserverType() !=
         ContextLifecycleObserver::kPausableObjectType)
-      continue;
+      return;
     PausableObject* pausable_object = static_cast<PausableObject*>(observer);
 #if DCHECK_IS_ON()
     DCHECK_EQ(pausable_object->GetExecutionContext(), Context());
     DCHECK(pausable_object->PauseIfNeededCalled());
 #endif
     pausable_object->Unpause();
-  }
+  });
 }
 
 void ContextLifecycleNotifier::NotifySuspendingPausableObjects() {
-  base::AutoReset<IterationState> scope(&iteration_state_, kAllowingNone);
-  for (LifecycleObserverBase* observer_base : observers_) {
-    ContextLifecycleObserver* observer =
-        static_cast<ContextLifecycleObserver*>(observer_base);
+  ForEachObserver([&](ContextLifecycleObserver* observer) {
     if (observer->ObserverType() !=
         ContextLifecycleObserver::kPausableObjectType)
-      continue;
+      return;
     PausableObject* pausable_object = static_cast<PausableObject*>(observer);
 #if DCHECK_IS_ON()
     DCHECK_EQ(pausable_object->GetExecutionContext(), Context());
     DCHECK(pausable_object->PauseIfNeededCalled());
 #endif
     pausable_object->Pause();
-  }
+  });
 }
 
 unsigned ContextLifecycleNotifier::PausableObjectCount() const {
   DCHECK(!IsIteratingOverObservers());
   unsigned pausable_objects = 0;
-  for (LifecycleObserverBase* observer_base : observers_) {
-    ContextLifecycleObserver* observer =
-        static_cast<ContextLifecycleObserver*>(observer_base);
+  ForEachObserver([&](ContextLifecycleObserver* observer) {
     if (observer->ObserverType() !=
         ContextLifecycleObserver::kPausableObjectType)
-      continue;
+      return;
     pausable_objects++;
-  }
+  });
   return pausable_objects;
 }
 
 #if DCHECK_IS_ON()
 bool ContextLifecycleNotifier::Contains(PausableObject* object) const {
   DCHECK(!IsIteratingOverObservers());
-  for (LifecycleObserverBase* observer_base : observers_) {
-    ContextLifecycleObserver* observer =
-        static_cast<ContextLifecycleObserver*>(observer_base);
+  bool found = false;
+  ForEachObserver([&](ContextLifecycleObserver* observer) {
     if (observer->ObserverType() !=
         ContextLifecycleObserver::kPausableObjectType)
-      continue;
+      return;
     PausableObject* pausable_object = static_cast<PausableObject*>(observer);
     if (pausable_object == object)
-      return true;
-  }
-  return false;
+      found = true;
+  });
+  return found;
 }
 #endif
 

third_party/blink/renderer/core/dom/synchronous_mutation_notifier.cc

@@ -13,40 +13,32 @@ SynchronousMutationNotifier::SynchronousMutationNotifier() = default;
 
 void SynchronousMutationNotifier::NotifyChangeChildren(
     const ContainerNode& container) {
-  for (LifecycleObserverBase* observer_base : observers_) {
-    SynchronousMutationObserver* observer =
-        static_cast<SynchronousMutationObserver*>(observer_base);
+  ForEachObserver([&](SynchronousMutationObserver* observer) {
     observer->DidChangeChildren(container);
-  }
+  });
 }
 
 void SynchronousMutationNotifier::NotifyMergeTextNodes(
     const Text& node,
     const NodeWithIndex& node_to_be_removed_with_index,
     unsigned old_length) {
-  for (LifecycleObserverBase* observer_base : observers_) {
-    SynchronousMutationObserver* observer =
-        static_cast<SynchronousMutationObserver*>(observer_base);
+  ForEachObserver([&](SynchronousMutationObserver* observer) {
     observer->DidMergeTextNodes(node, node_to_be_removed_with_index,
                                 old_length);
-  }
+  });
 }
 
 void SynchronousMutationNotifier::NotifyMoveTreeToNewDocument(
     const Node& root) {
-  for (LifecycleObserverBase* observer_base : observers_) {
-    SynchronousMutationObserver* observer =
-        static_cast<SynchronousMutationObserver*>(observer_base);
+  ForEachObserver([&](SynchronousMutationObserver* observer) {
     observer->DidMoveTreeToNewDocument(root);
-  }
+  });
 }
 
 void SynchronousMutationNotifier::NotifySplitTextNode(const Text& node) {
-  for (LifecycleObserverBase* observer_base : observers_) {
-    SynchronousMutationObserver* observer =
-        static_cast<SynchronousMutationObserver*>(observer_base);
+  ForEachObserver([&](SynchronousMutationObserver* observer) {
     observer->DidSplitTextNode(node);
-  }
+  });
 }
 
 void SynchronousMutationNotifier::NotifyUpdateCharacterData(
@@ -54,29 +46,23 @@ void SynchronousMutationNotifier::NotifyUpdateCharacterData(
     unsigned offset,
     unsigned old_length,
     unsigned new_length) {
-  for (LifecycleObserverBase* observer_base : observers_) {
-    SynchronousMutationObserver* observer =
-        static_cast<SynchronousMutationObserver*>(observer_base);
+  ForEachObserver([&](SynchronousMutationObserver* observer) {
     observer->DidUpdateCharacterData(character_data, offset, old_length,
                                      new_length);
-  }
+  });
 }
 
 void SynchronousMutationNotifier::NotifyNodeChildrenWillBeRemoved(
     ContainerNode& container) {
-  for (LifecycleObserverBase* observer_base : observers_) {
-    SynchronousMutationObserver* observer =
-        static_cast<SynchronousMutationObserver*>(observer_base);
+  ForEachObserver([&](SynchronousMutationObserver* observer) {
     observer->NodeChildrenWillBeRemoved(container);
-  }
+  });
 }
 
 void SynchronousMutationNotifier::NotifyNodeWillBeRemoved(Node& node) {
-  for (LifecycleObserverBase* observer_base : observers_) {
-    SynchronousMutationObserver* observer =
-        static_cast<SynchronousMutationObserver*>(observer_base);
+  ForEachObserver([&](SynchronousMutationObserver* observer) {
     observer->NodeWillBeRemoved(node);
-  }
+  });
 }
 
 }  // namespace blink

third_party/blink/renderer/core/page/page_visibility_notifier.cc

@@ -31,12 +31,9 @@
 namespace blink {
 
 void PageVisibilityNotifier::NotifyPageVisibilityChanged() {
-  base::AutoReset<IterationState> scope(&iteration_state_, kAllowingNone);
-  for (LifecycleObserverBase* observer_base : observers_) {
-    PageVisibilityObserver* observer =
-        static_cast<PageVisibilityObserver*>(observer_base);
+  ForEachObserver([](PageVisibilityObserver* observer) {
     observer->PageVisibilityChanged();
-  }
+  });
 }
 
 }  // namespace blink

third_party/blink/renderer/platform/lifecycle_notifier.h

@@ -63,6 +63,25 @@ class LifecycleNotifier : public GarbageCollectedMixin {
 
   T* Context() { return static_cast<T*>(this); }
 
+  // Safely iterate over the registered lifecycle observers.
+  //
+  // Adding or removing observers is not allowed during iteration. The callable
+  // will only be called synchronously inside ForEachObserver().
+  //
+  // Sample usage:
+  //     ForEachObserver([](ObserverType* observer) {
+  //       observer->SomeMethod();
+  //     });
+  template <typename ForEachCallable>
+  void ForEachObserver(const ForEachCallable& callable) const {
+    base::AutoReset<IterationState> scope(&iteration_state_, kAllowingNone);
+    for (LifecycleObserverBase* observer_base : observers_) {
+      Observer* observer = static_cast<Observer*>(observer_base);
+      callable(observer);
+    }
+  }
+
+ private:
   using ObserverSet = HeapHashSet<WeakMember<LifecycleObserverBase>>;
 
   enum IterationState {
@@ -74,7 +93,7 @@ class LifecycleNotifier : public GarbageCollectedMixin {
 
   // Iteration state is recorded while iterating the observer set,
   // optionally barring add or remove mutations.
-  IterationState iteration_state_;
+  mutable IterationState iteration_state_;
   ObserverSet observers_;
 };