Use iterators that deal with observers being removed to iterate the observers array when notifying Bug 340733, r+sr+branch181=sicking

content/base/src/nsDocument.cpp

@@ -631,6 +631,73 @@ nsDOMImplementation::Init(nsIURI* aDocumentURI, nsIURI* aBaseURI,
   return NS_OK;
 }
 
+PRBool
+nsDocumentObserverList::PrependElement(nsIDocumentObserver* aObserver)
+{
+  PRBool prepended = mObservers.InsertElementAt(aObserver, 0);
+
+  // This introduces an inconsistency -- forward iterators will not see the new
+  // element, while backwards ones will.  That's kinda inherent in the
+  // different iteration orders, though.
+  if (prepended) {
+    for (Iterator* iter = mIterators; iter; iter = iter->mNext) {
+      iter->mPosition++;
+    }
+  }
+
+  return prepended;
+}
+
+PRBool
+nsDocumentObserverList::RemoveElement(nsIDocumentObserver* aElement)
+{
+  PRInt32 index = mObservers.IndexOf(aElement);
+  if (index == -1) {
+    return PR_FALSE;
+  }
+
+#ifdef DEBUG
+  PRBool removed =
+#endif
+    mObservers.RemoveElementAt(index);
+  NS_ASSERTION(removed, "How could we fail to remove by index?");
+
+  for (Iterator* iter = mIterators; iter; iter = iter->mNext) {
+    // If iter->mPosition == index then forward iterators are safe, since in
+    // that case the position is not affected by the removal; all that's
+    // affected is what element is at that position.  Backward iterators,
+    // however, need to decrement mPosition in that case.
+    if (iter->mPosition > index ||
+        (iter->mPosition == index && iter->mStep < 0)) {
+      iter->mPosition--;
+    }
+  }
+  
+  return PR_TRUE;
+}
+
+void
+nsDocumentObserverList::Clear()
+{
+  mObservers.Clear(); 
+
+  // Reset all iterators to a bogus position so they don't return
+  // anything next time they're called.
+  for (Iterator* iter = mIterators; iter; iter = iter->mNext) {
+    iter->mPosition = -1;
+  }
+}
+
+nsIDocumentObserver*
+nsDocumentObserverList::Iterator::GetNext()
+{
+  nsIDocumentObserver* ret =
+    NS_STATIC_CAST(nsIDocumentObserver*,
+                   mList.mObservers.SafeElementAt(mPosition));
+  mPosition += mStep;
+  return ret;
+}
+
 // ==================================================================
 // =
 // ==================================================================
@@ -826,8 +893,7 @@ nsDocument::Init()
   mBindingManager = bindingManager;
 
   // The binding manager must always be the first observer of the document.
-  // (static cast to the correct interface pointer)
-  mObservers.InsertElementAt(NS_STATIC_CAST(nsIDocumentObserver*, bindingManager), 0);
+  mObservers.PrependElement(bindingManager);
 
   mOnloadBlocker = new nsOnloadBlocker();
   NS_ENSURE_TRUE(mOnloadBlocker, NS_ERROR_OUT_OF_MEMORY);
@@ -2113,7 +2179,7 @@ void
 nsDocument::AddObserver(nsIDocumentObserver* aObserver)
 {
   // XXX Make sure the observer isn't already in the list
-  if (mObservers.IndexOf(aObserver) == -1) {
+  if (!mObservers.Contains(aObserver)) {
     mObservers.AppendElement(aObserver);
   }
 }
@@ -2129,22 +2195,9 @@ nsDocument::RemoveObserver(nsIDocumentObserver* aObserver)
     return mObservers.RemoveElement(aObserver);
   }
 
-  return (mObservers.IndexOf(aObserver) != -1);
+  return mObservers.Contains(aObserver);
 }
 
-void
-nsDocument::CopyObserversTo(nsCOMArray<nsIDocumentObserver>& aDestination)
-{
-  PRInt32 count = mObservers.Count();
-  aDestination.SetCapacity(count);
-  // If we run out of memory, we just won't notify some of the observers.
-  for (PRInt32 i = 0; i < count; ++i) {
-    aDestination.AppendObject(
-      NS_STATIC_CAST(nsIDocumentObserver*,mObservers[i]));
-  }
-}
-
-
 void
 nsDocument::BeginUpdate(nsUpdateType aUpdateType)
 {
@@ -2325,11 +2378,9 @@ nsDocument::ContentAppended(nsIContent* aContainer,
   // observer list in a forward order
   // XXXldb So one should notify the other rather than both being
   // registered.
-  nsCOMArray<nsIDocumentObserver> observers;
-  CopyObserversTo(observers);
-  for (PRInt32 i = 0, i_end = observers.Count(); i < i_end; ++i) {
-    observers[i]->ContentAppended(this, aContainer, aNewIndexInContainer);
-  }
+  NS_DOCUMENT_FORWARD_NOTIFY_OBSERVERS(ContentAppended,
+                                       (this, aContainer,
+                                        aNewIndexInContainer));
 }
 
 void
@@ -2343,11 +2394,9 @@ nsDocument::ContentInserted(nsIContent* aContainer, nsIContent* aChild,
   // in a forward order
   // XXXldb So one should notify the other rather than both being
   // registered.
-  nsCOMArray<nsIDocumentObserver> observers;
-  CopyObserversTo(observers);
-  for (PRInt32 i = 0, i_end = observers.Count(); i < i_end; ++i) {
-    observers[i]->ContentInserted(this, aContainer, aChild, aIndexInContainer);
-  }
+  NS_DOCUMENT_FORWARD_NOTIFY_OBSERVERS(ContentInserted,
+                                       (this, aContainer, aChild,
+                                        aIndexInContainer));
 }
 
 void
@@ -2361,11 +2410,8 @@ nsDocument::ContentRemoved(nsIContent* aContainer, nsIContent* aChild,
   // observer list in a reverse order
   // XXXldb So one should notify the other rather than both being
   // registered.
-  nsCOMArray<nsIDocumentObserver> observers;
-  CopyObserversTo(observers);
-  for (PRInt32 i = observers.Count() - 1; i >= 0; --i) {
-    observers[i]->ContentRemoved(this, aContainer, aChild, aIndexInContainer);
-  }
+  NS_DOCUMENT_NOTIFY_OBSERVERS(ContentRemoved,
+                               (this, aContainer, aChild, aIndexInContainer));
 }
 
 void

content/base/src/nsDocument.h

@@ -262,6 +262,94 @@ private:
   ~nsOnloadBlocker() {}
 };
 
+/**
+ * nsDocumentObserverList is the list of nsIDocumentObservers for a document.
+ * It doesn't allow direct reading of the list; all access must take place
+ * through stack-allocated nsDocumentObserverList::ForwardIterator or
+ * nsDocumentObserverList::ReverseIterator objects.
+ */
+class nsDocumentObserverList
+{
+public:
+  nsDocumentObserverList() :
+    mIterators(nsnull)
+  {}
+
+  class Iterator;
+  friend class Iterator;
+
+  class Iterator
+  {
+  public:
+    nsIDocumentObserver* GetNext();
+
+  protected:
+    Iterator(PRInt32 aStep, nsDocumentObserverList& aList) :
+      mPosition(aStep > 0 ? 0 : aList.mObservers.Count() - 1),
+      mStep(aStep),
+      mList(aList),
+      mNext(aList.mIterators)
+    {
+      NS_ASSERTION(mStep == 1 || mStep == -1, "Invalid step size");
+      aList.mIterators = this;
+    }
+
+    ~Iterator() {
+      NS_ASSERTION(mList.mIterators == this, "Destroyed out of order?");
+      mList.mIterators = mNext;
+    }
+
+    friend class nsDocumentObserverList;
+
+    // Our current position in mObservers
+    PRInt32 mPosition;
+    
+  private:
+    // Which direction to move in
+    PRInt32 mStep;
+
+    // The observer array to work with
+    nsDocumentObserverList& mList;
+
+    // Our next iterator.
+    Iterator* mNext;
+  };
+
+  class ForwardIterator : public Iterator
+  {
+  public:
+    ForwardIterator(nsDocumentObserverList& aList) :
+      Iterator(1, aList)
+    {}
+  };
+
+  class ReverseIterator : public Iterator
+  {
+  public:
+    ReverseIterator(nsDocumentObserverList& aList) :
+      Iterator(-1, aList)
+    {}
+  };
+
+  PRBool PrependElement(nsIDocumentObserver* aObserver);
+
+  PRInt32 Contains(nsIDocumentObserver* aPossibleObserver) const {
+    return mObservers.IndexOf(aPossibleObserver) != -1;
+  }
+
+  PRBool AppendElement(nsIDocumentObserver* aElement) {
+    return mObservers.AppendElement(aElement);
+  }
+
+  PRBool RemoveElement(nsIDocumentObserver* aElement);
+
+  void Clear();
+
+private:
+  nsAutoVoidArray mObservers;
+  Iterator* mIterators;
+};
+
 // Base class for our document implementations.
 //
 // Note that this class *implements* nsIDOMXMLDocument, but it's not
@@ -682,19 +770,26 @@ protected:
   // Dispatch an event to the ScriptGlobalObject for this document
   void DispatchEventToWindow(nsEvent *aEvent);
 
-  // Copy |mObservers| to an nsCOMArray in preparation so we can notify
-  // the list of observers set up at one point in time.
-  void CopyObserversTo(nsCOMArray<nsIDocumentObserver>& aDestination);
-
+  // NS_DOCUMENT_NOTIFY_OBSERVERS goes backwards for now for backwards compat.
+  // If you change this, update ContentAppended/Inserted/Removed accordingly.
 #define NS_DOCUMENT_NOTIFY_OBSERVERS(func_, params_)                          \
   do {                                                                        \
-    nsCOMArray<nsIDocumentObserver> observers_;                               \
-    CopyObserversTo(observers_);                                              \
-    for (PRInt32 i_ = observers_.Count() - 1; i_ >= 0; --i_) {                \
-      observers_[i_] -> func_ params_ ;                                       \
+    nsDocumentObserverList::ReverseIterator iter_(mObservers);                \
+    nsCOMPtr<nsIDocumentObserver> obs_;                                       \
+    while ((obs_ = iter_.GetNext())) {                                        \
+      obs_ -> func_ params_ ;                                                 \
     }                                                                         \
   } while (0)
 
+#define NS_DOCUMENT_FORWARD_NOTIFY_OBSERVERS(func_, params_)                  \
+  do {                                                                        \
+    nsDocumentObserverList::ForwardIterator iter_(mObservers);                \
+    nsCOMPtr<nsIDocumentObserver> obs_;                                       \
+    while ((obs_ = iter_.GetNext())) {                                        \
+      obs_ -> func_ params_ ;                                                 \
+    }                                                                         \
+  } while (0)
+ 
 #ifdef DEBUG
   void VerifyRootContentState();
 #endif
@@ -720,7 +815,7 @@ protected:
   nsCOMArray<nsIStyleSheet> mCatalogSheets;
 
   // Basically always has at least 1 entry
-  nsAutoVoidArray mObservers;
+  nsDocumentObserverList mObservers;
 
   // The document's script global object, the object from which the
   // document can get its script context and scope. This is the