Bug 1150916 - Be sure we aren't iterating over a hashtable while it is modified in nsObserverService::UnmarkGrayStrongObservers(). r=froydnj
authorAndrew McCreight <continuation@gmail.com>
Wed, 08 Apr 2015 17:16:30 -0700
changeset 268106 074e4fb089b1871e63dd1f00499c809a24d14295
parent 268105 ce72894838de40bfaef21bb514011a553ebf3160
child 268107 f0a8286209ed0796317149253efc35537e368d83
push id4830
push userjlund@mozilla.com
push dateMon, 29 Jun 2015 20:18:48 +0000
treeherdermozilla-beta@4c2175bb0420 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfroydnj
bugs1150916
milestone40.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1150916 - Be sure we aren't iterating over a hashtable while it is modified in nsObserverService::UnmarkGrayStrongObservers(). r=froydnj The QI in xpc_TryUnmarkWrappedGrayObject() might somehow trigger a GC, which in turn could destroy an observer, that would in turn call RemoveObserver() which mutates the hash table that we're enumerating.
xpcom/ds/nsObserverList.cpp
xpcom/ds/nsObserverList.h
xpcom/ds/nsObserverService.cpp
--- a/xpcom/ds/nsObserverList.cpp
+++ b/xpcom/ds/nsObserverList.cpp
@@ -84,40 +84,40 @@ nsObserverList::FillObserverArray(nsCOMA
       }
     } else {
       aArray.AppendObject(observers[i].asObserver());
     }
   }
 }
 
 void
+nsObserverList::AppendStrongObservers(nsCOMArray<nsIObserver>& aArray)
+{
+  aArray.SetCapacity(aArray.Length() + mObservers.Length());
+
+  for (int32_t i = mObservers.Length() - 1; i >= 0; --i) {
+    if (!mObservers[i].isWeakRef) {
+      aArray.AppendObject(mObservers[i].asObserver());
+    }
+  }
+}
+
+void
 nsObserverList::NotifyObservers(nsISupports* aSubject,
                                 const char* aTopic,
                                 const char16_t* someData)
 {
   nsCOMArray<nsIObserver> observers;
   FillObserverArray(observers);
 
   for (int32_t i = 0; i < observers.Count(); ++i) {
     observers[i]->Observe(aSubject, aTopic, someData);
   }
 }
 
-void
-nsObserverList::UnmarkGrayStrongObservers()
-{
-#if !defined(MOZILLA_XPCOMRT_API)
-  for (uint32_t i = 0; i < mObservers.Length(); ++i) {
-    if (!mObservers[i].isWeakRef) {
-      xpc_TryUnmarkWrappedGrayObject(mObservers[i].asObserver());
-    }
-  }
-#endif // !defined(MOZILLA_XPCOMRT_API)
-}
-
 NS_IMPL_ISUPPORTS(nsObserverEnumerator, nsISimpleEnumerator)
 
 nsObserverEnumerator::nsObserverEnumerator(nsObserverList* aObserverList)
   : mIndex(0)
 {
   aObserverList->FillObserverArray(mObservers);
 }
 
--- a/xpcom/ds/nsObserverList.h
+++ b/xpcom/ds/nsObserverList.h
@@ -66,19 +66,18 @@ public:
                        const char* aTopic,
                        const char16_t* aSomeData);
   nsresult GetObserverList(nsISimpleEnumerator** aEnumerator);
 
   // Fill an array with the observers of this category.
   // The array is filled in last-added-first order.
   void FillObserverArray(nsCOMArray<nsIObserver>& aArray);
 
-  // Unmark any strongly held observers implemented in JS so the cycle
-  // collector will not traverse them.
-  void UnmarkGrayStrongObservers();
+  // Like FillObserverArray(), but only for strongly held observers.
+  void AppendStrongObservers(nsCOMArray<nsIObserver>& aArray);
 
 private:
   nsTArray<ObserverRef> mObservers;
 };
 
 class nsObserverEnumerator final : public nsISimpleEnumerator
 {
 public:
--- a/xpcom/ds/nsObserverService.cpp
+++ b/xpcom/ds/nsObserverService.cpp
@@ -339,26 +339,35 @@ NS_IMETHODIMP nsObserverService::NotifyO
     observerList->NotifyObservers(aSubject, aTopic, aSomeData);
   }
 #endif
 
   return NS_OK;
 }
 
 static PLDHashOperator
-UnmarkGrayObserverEntry(nsObserverList* aObserverList, void* aClosure)
+AppendStrongObservers(nsObserverList* aObserverList, void* aClosure)
 {
+  nsCOMArray<nsIObserver>* array = static_cast<nsCOMArray<nsIObserver>*>(aClosure);
+
   if (aObserverList) {
-    aObserverList->UnmarkGrayStrongObservers();
+    aObserverList->AppendStrongObservers(*array);
   }
   return PL_DHASH_NEXT;
 }
 
 NS_IMETHODIMP
 nsObserverService::UnmarkGrayStrongObservers()
 {
   NS_ENSURE_VALIDCALL
 
-  mObserverTopicTable.EnumerateEntries(UnmarkGrayObserverEntry, nullptr);
+#if !defined(MOZILLA_XPCOMRT_API)
+  nsCOMArray<nsIObserver> strongObservers;
+  mObserverTopicTable.EnumerateEntries(AppendStrongObservers, &strongObservers);
+
+  for (uint32_t i = 0; i < strongObservers.Length(); ++i) {
+    xpc_TryUnmarkWrappedGrayObject(strongObservers[i]);
+  }
+#endif // !defined(MOZILLA_XPCOMRT_API)
 
   return NS_OK;
 }