Avoid snapshotting ContextLifecycleObservers when iterating.

third_party/WebKit/Source/platform/LifecycleNotifier.h

 /*
  * Copyright (C) 2008 Apple Inc. All Rights Reserved.
  * Copyright (C) 2013 Google Inc. All Rights Reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
@@ skipping 35 matching lines @@
     //
     // When contextDestroyed() is called, the observer's lifecycleContext()
     // is still valid and safe to use during the notification.
     virtual void notifyContextDestroyed();
 
     DEFINE_INLINE_VIRTUAL_TRACE()
     {
         visitor->trace(m_observers);
     }
 
-    bool isIteratingOverObservers() const { return m_iterating != IteratingNone; }
+    bool isIteratingOverObservers() const { return m_iterationState != NotIterating; }
 
 protected:
     LifecycleNotifier()
-        : m_iterating(IteratingNone)
+        : m_iterationState(NotIterating)
     {
     }
 
-    enum IterationType {
-        IteratingNone,
-        IteratingOverAll,
-    };
-
-    IterationType m_iterating;
-
-protected:
-    using ObserverSet = HeapHashSet<WeakMember<Observer>>;
-
-    ObserverSet m_observers;
-
 #if DCHECK_IS_ON()
     T* context() { return static_cast<T*>(this); }
 #endif
+
+    using ObserverSet = HeapHashSet<WeakMember<Observer>>;
+
+    enum IterationState {
+        AllowingNone = 0,
+        AllowingAddition,
+        AllowingRemoval,
+        NotIterating = AllowingAddition | AllowingRemoval,
+    };
+
+    class IterationScope {
+        STACK_ALLOCATED();
+    public:
+        explicit IterationScope(LifecycleNotifier* notifier, IterationState allowable = AllowingNone)

[haraken, 2016/06/27 01:06:34]

Is the second parameter used somewhere?

[sof, 2016/06/27 05:14:14]

Line 85

+            : m_notifier(notifier)
+        {
+            // Don't support nested iterations.
+            RELEASE_ASSERT(m_notifier->m_iterationState == NotIterating);
+            m_notifier->m_iterationState = allowable;
+
+            // Swap out the notifier's weak collection to this stack allocated
+            // object, ensuring that collection references are strongified across
+            // conservative GCs.
+            //
+            // If not done, weak processing could mutate the underlying table
+            // while it is being iterated over.
+            m_observers.swap(m_notifier->m_observers);

[haraken, 2016/06/27 01:06:34]

I don't fully understand why we need to explicitly swap m_observers. The caller
side holds an iterator while iterating the hash set. Wouldn't it be enough to
keep the hash set alive?

[sof, 2016/06/27 05:14:14]

I was thinking about that overnight, this being too conservative. And looking at
HeapTest.HeapWeakCollectionTypes, we can rely on iterators strongifying the
collection. So let me simplify away IterationScope.

[sof, 2016/06/27 06:36:54]

Now done.

+        }
+        ~IterationScope()
+        {
+            m_notifier->m_iterationState = NotIterating;
+            m_observers.swap(m_notifier->m_observers);
+        }
+
+        const typename LifecycleNotifier::ObserverSet& observers() const { return m_observers; }
+
+    private:
+        Member<LifecycleNotifier> m_notifier;
+        typename LifecycleNotifier::ObserverSet m_observers;
+    };
+
+    friend class IterationScope;
+
+    // Iteration state is recorded while iterating the observer set,
+    // optionally barring add or remove mutations while it runs.
+    IterationState m_iterationState;
+    ObserverSet m_observers;
 };
 
 template<typename T, typename Observer>
 inline LifecycleNotifier<T, Observer>::~LifecycleNotifier()
 {
     // FIXME: Enable the following ASSERT. Also see a FIXME in Document::detach().
     // ASSERT(!m_observers.size());
 }
 
 template<typename T, typename Observer>
 inline void LifecycleNotifier<T, Observer>::notifyContextDestroyed()
 {
-    TemporaryChange<IterationType> scope(m_iterating, IteratingOverAll);
+    // Observer unregistration is allowed, but effectively a no-op.
+    TemporaryChange<IterationState> scope(m_iterationState, AllowingRemoval);
     ObserverSet observers;
     m_observers.swap(observers);
     for (Observer* observer : observers) {
         ASSERT(observer->lifecycleContext() == context());
         observer->contextDestroyed();
     }
 }
 
 template<typename T, typename Observer>
 inline void LifecycleNotifier<T, Observer>::addObserver(Observer* observer)
 {
-    RELEASE_ASSERT(m_iterating != IteratingOverAll);
+    RELEASE_ASSERT(m_iterationState & AllowingAddition);
     m_observers.add(observer);
 }
 
 template<typename T, typename Observer>
 inline void LifecycleNotifier<T, Observer>::removeObserver(Observer* observer)
 {
+    RELEASE_ASSERT(m_iterationState & AllowingRemoval);
     m_observers.remove(observer);
 }
 
 } // namespace blink
 
 #endif // LifecycleNotifier_h