Allow ActiveDOMObjects to be destructed while iterating the list of ActiveDOMObjects

Source/core/dom/ContextLifecycleNotifier.cpp

 /*
  * 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 41 matching lines @@
         m_activeDOMObjects.add(static_cast<ActiveDOMObject*>(observer));
     }
 }
 
 void ContextLifecycleNotifier::removeObserver(ContextLifecycleNotifier::Observer* observer)
 {
     LifecycleNotifier<ExecutionContext>::removeObserver(observer);
 
     RELEASE_ASSERT(m_iterating != IteratingOverContextObservers);
     if (observer->observerType() == Observer::ActiveDOMObjectType) {
-        RELEASE_ASSERT(m_iterating != IteratingOverActiveDOMObjects);
         m_activeDOMObjects.remove(static_cast<ActiveDOMObject*>(observer));
     }
 }
 
 void ContextLifecycleNotifier::notifyResumingActiveDOMObjects()
 {
     TemporaryChange<IterationType> scope(this->m_iterating, IteratingOverActiveDOMObjects);
-    ActiveDOMObjectSet::iterator activeObjectsEnd = m_activeDOMObjects.end();
-    for (ActiveDOMObjectSet::iterator iter = m_activeDOMObjects.begin(); iter != activeObjectsEnd; ++iter) {
-        ASSERT((*iter)->executionContext() == context());
-        ASSERT((*iter)->suspendIfNeededCalled());
-        (*iter)->resume();
+    Vector<ActiveDOMObject*> snapshotOfActiveDOMObjects;
+    copyToVector(m_activeDOMObjects, snapshotOfActiveDOMObjects);
+    for (Vector<ActiveDOMObject*>::iterator iter = snapshotOfActiveDOMObjects.begin(); iter != snapshotOfActiveDOMObjects.end(); iter++) {
+        // FIXME: Oilpan: At the moment, it's possible that the ActiveDOMObject is destructed
+        // during the iteration. Once we move ActiveDOMObject to the heap and
+        // make m_activeDOMObjects a HeapHashSet<WeakMember<ActiveDOMObject>>,
+        // it's no longer possible that ActiveDOMObject is destructed during the iteration,
+        // so we can remove the hack (i.e., we can just iterate m_activeDOMObjects without
+        // taking a snapshot). For more details, see https://codereview.chromium.org/247253002/.
+        if (m_activeDOMObjects.contains(*iter)) {
+            ASSERT((*iter)->executionContext() == context());
+            ASSERT((*iter)->suspendIfNeededCalled());
+            (*iter)->resume();
+        }
     }
 }
 
 void ContextLifecycleNotifier::notifySuspendingActiveDOMObjects()
 {
     TemporaryChange<IterationType> scope(this->m_iterating, IteratingOverActiveDOMObjects);
-    ActiveDOMObjectSet::iterator activeObjectsEnd = m_activeDOMObjects.end();
-    for (ActiveDOMObjectSet::iterator iter = m_activeDOMObjects.begin(); iter != activeObjectsEnd; ++iter) {
-        ASSERT((*iter)->executionContext() == context());
-        ASSERT((*iter)->suspendIfNeededCalled());
-        (*iter)->suspend();
+    Vector<ActiveDOMObject*> snapshotOfActiveDOMObjects;
+    copyToVector(m_activeDOMObjects, snapshotOfActiveDOMObjects);
+    for (Vector<ActiveDOMObject*>::iterator iter = snapshotOfActiveDOMObjects.begin(); iter != snapshotOfActiveDOMObjects.end(); iter++) {
+        // It's possible that the ActiveDOMObject is already destructed.
+        // See a FIXME above.
+        if (m_activeDOMObjects.contains(*iter)) {
+            ASSERT((*iter)->executionContext() == context());
+            ASSERT((*iter)->suspendIfNeededCalled());
+            (*iter)->suspend();
+        }
     }
 }
 
 void ContextLifecycleNotifier::notifyStoppingActiveDOMObjects()
 {
     TemporaryChange<IterationType> scope(this->m_iterating, IteratingOverActiveDOMObjects);
-    ActiveDOMObjectSet::iterator activeObjectsEnd = m_activeDOMObjects.end();
-    for (ActiveDOMObjectSet::iterator iter = m_activeDOMObjects.begin(); iter != activeObjectsEnd; ++iter) {
-        ASSERT((*iter)->executionContext() == context());
-        ASSERT((*iter)->suspendIfNeededCalled());
-        (*iter)->stop();
+    Vector<ActiveDOMObject*> snapshotOfActiveDOMObjects;
+    copyToVector(m_activeDOMObjects, snapshotOfActiveDOMObjects);
+    for (Vector<ActiveDOMObject*>::iterator iter = snapshotOfActiveDOMObjects.begin(); iter != snapshotOfActiveDOMObjects.end(); iter++) {
+        // It's possible that the ActiveDOMObject is already destructed.
+        // See a FIXME above.
+        if (m_activeDOMObjects.contains(*iter)) {
+            ASSERT((*iter)->executionContext() == context());
+            ASSERT((*iter)->suspendIfNeededCalled());
+            (*iter)->stop();
+        }
     }

[abarth-chromium, 2014/05/09 13:54:39]

I'm worried that ActiveDOMObjects created during this iteration will never get
the stop notification.  Can we forbid additions during these iterations but
allow removals?

[haraken, 2014/05/09 14:05:15]

Creation during iteration is already forbidden by the RELEASE_ASSERT(m_iterating
!= IteratingOverContextObservers) at line 49.

- Previously both creation and destruction are forbidden.
- This CL makes destruction OK. The creation is still forbidden.

 }
 
 bool ContextLifecycleNotifier::hasPendingActivity() const
 {
-    ActiveDOMObjectSet::const_iterator activeObjectsEnd = activeDOMObjects().end();
-    for (ActiveDOMObjectSet::const_iterator iter = activeDOMObjects().begin(); iter != activeObjectsEnd; ++iter) {
+    for (ActiveDOMObjectSet::const_iterator iter = m_activeDOMObjects.begin(); iter != m_activeDOMObjects.end(); ++iter) {
         if ((*iter)->hasPendingActivity())
             return true;
     }
-
     return false;
 }
 
 } // namespace WebCore