Allow ActiveDOMObjects to be destructed while iterating the list of ActiveDOMObjects

Source/core/dom/ContextLifecycleNotifier.cpp

Index: Source/core/dom/ContextLifecycleNotifier.cpp
diff --git a/Source/core/dom/ContextLifecycleNotifier.cpp b/Source/core/dom/ContextLifecycleNotifier.cpp
index 048978c735435f9f54fe8aaa07f7e51944ee6664..5abb5a282987272c423b904b3894b528bb4365b5 100644
--- a/Source/core/dom/ContextLifecycleNotifier.cpp
+++ b/Source/core/dom/ContextLifecycleNotifier.cpp
@@ -59,7 +59,6 @@ void ContextLifecycleNotifier::removeObserver(ContextLifecycleNotifier::Observer
 
     RELEASE_ASSERT(m_iterating != IteratingOverContextObservers);
     if (observer->observerType() == Observer::ActiveDOMObjectType) {
-        RELEASE_ASSERT(m_iterating != IteratingOverActiveDOMObjects);
         m_activeDOMObjects.remove(static_cast<ActiveDOMObject*>(observer));
     }
 }
@@ -67,44 +66,62 @@ void ContextLifecycleNotifier::removeObserver(ContextLifecycleNotifier::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 in a case where resume() allocates an on-heap object

[Mads Ager (chromium), 2014/05/09 08:05:44]

NIT: I think you should leave out the part of the comment that talks about
allocation. Based on the stack trace that we are seeing I don't think this is
related to allocation. I think the point is that a DatabaseThread is being
stopped via an ActiveDOMObject stop call:

Main thread:

1) Document::detach
2) notifyStoppingActiveDOMObjects
3) DatabaseContext::stop
4) DatabaseContext::stopDatabases
5) Post task to database thread and block until it is terminated; this is a safe
point for GC

Database thread:

6) DatabaseThread::cleanupDatabaseThreadCompleted
7) ThreadState::detach
8) Heap::collectAllGarbage (to run all destructors); this sets pending sweep for
all threads including the main thread.
9) TaskSynchronizer::taskCompleted()

Main thread:

10) wake up from safepoint; sweep requested; sweep and finalize dead
ActiveDOMObjects

So, when we have a database thread we are use that we will get a sweep here even
if there are no allocations.

[haraken, 2014/05/09 08:12:36]

Yeah, your observation is correct. An allocation would be another case that will
cause the same issue. For simplicity, I just removed the part from the comment.

+        // and it triggers a GC. 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();
+        }
     }
 }
 
 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;
 }