fix bug 369214 (ASSERTION: The observer list changed while being iterated over!: 'count == mObservers.Count()' in libpr0n) by switching the observers list to use nsTObserverArray. r=stuart a=1.9 blocker

modules/libpr0n/src/imgRequest.cpp

@@ -124,23 +124,19 @@ nsresult imgRequest::Init(nsIURI *aURI,
   return NS_OK;
 }
 
-nsresult imgRequest::AddProxy(imgRequestProxy *proxy, PRBool aNotify)
+nsresult imgRequest::AddProxy(imgRequestProxy *proxy)
 {
+  NS_PRECONDITION(proxy, "null imgRequestProxy passed in");
   LOG_SCOPE_WITH_PARAM(gImgLog, "imgRequest::AddProxy", "proxy", proxy);
 
-  mObservers.AppendElement(static_cast<void*>(proxy));
+  return mObservers.AppendObserver(proxy) ? NS_OK : NS_ERROR_OUT_OF_MEMORY;
-
-  if (aNotify)
-    NotifyProxyListener(proxy);
-
-  return NS_OK;
 }
 
 nsresult imgRequest::RemoveProxy(imgRequestProxy *proxy, nsresult aStatus, PRBool aNotify)
 {
   LOG_SCOPE_WITH_PARAM(gImgLog, "imgRequest::RemoveProxy", "proxy", proxy);
 
-  mObservers.RemoveElement(static_cast<void*>(proxy));
+  mObservers.RemoveObserver(proxy);
 
   /* Check mState below before we potentially call Cancel() below. Since
      Cancel() may result in OnStopRequest being called back before Cancel()
@@ -167,7 +163,7 @@ nsresult imgRequest::RemoveProxy(imgRequestProxy *proxy, nsresult aStatus, PRBoo
     mImage->StopAnimation();
   }
 
-  if (mObservers.Count() == 0) {
+  if (mObservers.IsEmpty()) {
     /* If |aStatus| is a failure code, then cancel the load if it is still in progress.
        Otherwise, let the load continue, keeping 'this' in the cache with no observers.
        This way, if a proxy is destroyed without calling cancel on it, it won't leak
@@ -323,8 +319,9 @@ void imgRequest::RemoveFromCache()
 
 PRBool imgRequest::HaveProxyWithObserver(imgRequestProxy* aProxyToIgnore) const
 {
-  for (PRInt32 i = 0; i < mObservers.Count(); ++i) {
+  nsTObserverArray<imgRequestProxy>::ForwardIterator iter(mObservers);
-    imgRequestProxy *proxy = static_cast<imgRequestProxy*>(mObservers[i]);
+  imgRequestProxy* proxy;
+  while ((proxy = iter.GetNext())) {
     if (proxy == aProxyToIgnore) {
       continue;
     }
@@ -355,7 +352,7 @@ void imgRequest::AdjustPriority(imgRequestProxy *proxy, PRInt32 delta)
   // concern though is that image loads remain lower priority than other pieces
   // of content such as link clicks, CSS, and JS.
   //
-  if (mObservers[0] != proxy)
+  if (mObservers.SafeObserverAt(0) != proxy)
     return;
 
   nsCOMPtr<nsISupportsPriority> p = do_QueryInterface(mRequest);
@@ -401,15 +398,10 @@ NS_IMETHODIMP imgRequest::FrameChanged(imgIContainer *container,
 {
   LOG_SCOPE(gImgLog, "imgRequest::FrameChanged");
 
-  PRInt32 count = mObservers.Count();
+  nsTObserverArray<imgRequestProxy>::ForwardIterator iter(mObservers);
-  for (PRInt32 i = 0; i < count; i++) {
+  imgRequestProxy* proxy;
-    imgRequestProxy *proxy = static_cast<imgRequestProxy*>(mObservers[i]);
+  while ((proxy = iter.GetNext())) {
-    if (proxy) proxy->FrameChanged(container, newframe, dirtyRect);
+    proxy->FrameChanged(container, newframe, dirtyRect);
-
-    // If this assertion fires, it means that imgRequest notifications could
-    // be dropped!
-    NS_ASSERTION(count == mObservers.Count(),
-                 "The observer list changed while being iterated over!");
   }
 
   return NS_OK;
@@ -424,15 +416,10 @@ NS_IMETHODIMP imgRequest::OnStartDecode(imgIRequest *request)
 
   mState |= onStartDecode;
 
-  PRInt32 count = mObservers.Count();
+  nsTObserverArray<imgRequestProxy>::ForwardIterator iter(mObservers);
-  for (PRInt32 i = 0; i < count; i++) {
+  imgRequestProxy* proxy;
-    imgRequestProxy *proxy = static_cast<imgRequestProxy*>(mObservers[i]);
+  while ((proxy = iter.GetNext())) {
-    if (proxy) proxy->OnStartDecode();
+    proxy->OnStartDecode();
-
-    // If this assertion fires, it means that imgRequest notifications could
-    // be dropped!
-    NS_ASSERTION(count == mObservers.Count(), 
-                 "The observer list changed while being iterated over!");
   }
 
   /* In the case of streaming jpegs, it is possible to get multiple OnStartDecodes which
@@ -464,16 +451,10 @@ NS_IMETHODIMP imgRequest::OnStartContainer(imgIRequest *request, imgIContainer *
 
   mImageStatus |= imgIRequest::STATUS_SIZE_AVAILABLE;
 
-  PRInt32 count = mObservers.Count();
+  nsTObserverArray<imgRequestProxy>::ForwardIterator iter(mObservers);
-  for (PRInt32 i = 0; i < count; i++) {
+  imgRequestProxy* proxy;
-    imgRequestProxy *proxy = static_cast<imgRequestProxy*>(mObservers[i]);
+  while ((proxy = iter.GetNext())) {
-    if (proxy) proxy->OnStartContainer(image);
+    proxy->OnStartContainer(image);
-
-    // If this assertion fires, it means that imgRequest notifications could
-    // be dropped!
-    NS_ASSERTION(count == mObservers.Count(), 
-                 "The observer list changed while being iterated over!");
-
   }
 
   return NS_OK;
@@ -485,15 +466,10 @@ NS_IMETHODIMP imgRequest::OnStartFrame(imgIRequest *request,
 {
   LOG_SCOPE(gImgLog, "imgRequest::OnStartFrame");
 
-  PRInt32 count = mObservers.Count();
+  nsTObserverArray<imgRequestProxy>::ForwardIterator iter(mObservers);
-  for (PRInt32 i = 0; i < count; i++) {
+  imgRequestProxy* proxy;
-    imgRequestProxy *proxy = static_cast<imgRequestProxy*>(mObservers[i]);
+  while ((proxy = iter.GetNext())) {
-    if (proxy) proxy->OnStartFrame(frame);
+    proxy->OnStartFrame(frame);
-
-    // If this assertion fires, it means that imgRequest notifications could
-    // be dropped!
-    NS_ASSERTION(count == mObservers.Count(), 
-                 "The observer list changed while being iterated over!");
   }
 
   return NS_OK;
@@ -506,15 +482,10 @@ NS_IMETHODIMP imgRequest::OnDataAvailable(imgIRequest *request,
 {
   LOG_SCOPE(gImgLog, "imgRequest::OnDataAvailable");
 
-  PRInt32 count = mObservers.Count();
+  nsTObserverArray<imgRequestProxy>::ForwardIterator iter(mObservers);
-  for (PRInt32 i = 0; i < count; i++) {
+  imgRequestProxy* proxy;
-    imgRequestProxy *proxy = static_cast<imgRequestProxy*>(mObservers[i]);
+  while ((proxy = iter.GetNext())) {
-    if (proxy) proxy->OnDataAvailable(frame, rect);
+    proxy->OnDataAvailable(frame, rect);
-
-    // If this assertion fires, it means that imgRequest notifications could
-    // be dropped!
-    NS_ASSERTION(count == mObservers.Count(), 
-                 "The observer list changed while being iterated over!");
   }
 
   return NS_OK;
@@ -541,15 +512,10 @@ NS_IMETHODIMP imgRequest::OnStopFrame(imgIRequest *request,
     mCacheEntry->SetDataSize(cacheSize + imageSize);
   }
 
-  PRInt32 count = mObservers.Count();
+  nsTObserverArray<imgRequestProxy>::ForwardIterator iter(mObservers);
-  for (PRInt32 i = 0; i < count; i++) {
+  imgRequestProxy* proxy;
-    imgRequestProxy *proxy = static_cast<imgRequestProxy*>(mObservers[i]);
+  while ((proxy = iter.GetNext())) {
-    if (proxy) proxy->OnStopFrame(frame);
+    proxy->OnStopFrame(frame);
-
-    // If this assertion fires, it means that imgRequest notifications could
-    // be dropped!
-    NS_ASSERTION(count == mObservers.Count(), 
-                 "The observer list changed while being iterated over!");
   }
 
   return NS_OK;
@@ -563,15 +529,10 @@ NS_IMETHODIMP imgRequest::OnStopContainer(imgIRequest *request,
 
   mState |= onStopContainer;
 
-  PRInt32 count = mObservers.Count();
+  nsTObserverArray<imgRequestProxy>::ForwardIterator iter(mObservers);
-  for (PRInt32 i = 0; i < count; i++) {
+  imgRequestProxy* proxy;
-    imgRequestProxy *proxy = static_cast<imgRequestProxy*>(mObservers[i]);
+  while ((proxy = iter.GetNext())) {
-    if (proxy) proxy->OnStopContainer(image);
+    proxy->OnStopContainer(image);
-
-    // If this assertion fires, it means that imgRequest notifications could
-    // be dropped!
-    NS_ASSERTION(count == mObservers.Count(), 
-                 "The observer list changed while being iterated over!");
   }
 
   return NS_OK;
@@ -592,15 +553,10 @@ NS_IMETHODIMP imgRequest::OnStopDecode(imgIRequest *aRequest,
     mImageStatus |= imgIRequest::STATUS_ERROR;
   }
 
-  PRInt32 count = mObservers.Count();
+  nsTObserverArray<imgRequestProxy>::ForwardIterator iter(mObservers);
-  for (PRInt32 i = 0; i < count; i++) {
+  imgRequestProxy* proxy;
-    imgRequestProxy *proxy = static_cast<imgRequestProxy*>(mObservers[i]);
+  while ((proxy = iter.GetNext())) {
-    if (proxy) proxy->OnStopDecode(GetResultFromImageStatus(mImageStatus), aStatusArg);
+    proxy->OnStopDecode(GetResultFromImageStatus(mImageStatus), aStatusArg);
-
-    // If this assertion fires, it means that imgRequest notifications could
-    // be dropped!
-    NS_ASSERTION(count == mObservers.Count(), 
-                 "The observer list changed while being iterated over!");
   }
 
   return NS_OK;
@@ -637,15 +593,10 @@ NS_IMETHODIMP imgRequest::OnStartRequest(nsIRequest *aRequest, nsISupports *ctxt
   mLoading = PR_TRUE;
 
   /* notify our kids */
-  PRInt32 count = mObservers.Count();
+  nsTObserverArray<imgRequestProxy>::ForwardIterator iter(mObservers);
-  for (PRInt32 i = 0; i < count; i++) {
+  imgRequestProxy* proxy;
-    imgRequestProxy *proxy = static_cast<imgRequestProxy*>(mObservers[i]);
+  while ((proxy = iter.GetNext())) {
-    if (proxy) proxy->OnStartRequest(aRequest, ctxt);
+    proxy->OnStartRequest(aRequest, ctxt);
-
-    // If this assertion fires, it means that imgRequest notifications could
-    // be dropped!
-    NS_ASSERTION(count == mObservers.Count(), 
-                 "The observer list changed while being iterated over!");
   }
 
   nsCOMPtr<nsIChannel> chan(do_QueryInterface(aRequest));
@@ -703,7 +654,7 @@ NS_IMETHODIMP imgRequest::OnStartRequest(nsIRequest *aRequest, nsISupports *ctxt
 
 
   // Shouldn't we be dead already if this gets hit?  Probably multipart/x-mixed-replace...
-  if (mObservers.Count() == 0) {
+  if (mObservers.IsEmpty()) {
     this->Cancel(NS_IMAGELIB_ERROR_FAILURE);
   }
 
@@ -762,13 +713,13 @@ NS_IMETHODIMP imgRequest::OnStopRequest(nsIRequest *aRequest, nsISupports *ctxt,
   }
 
   /* notify the kids */
-  PRInt32 count = mObservers.Count();
+  nsTObserverArray<imgRequestProxy>::ForwardIterator iter(mObservers);
-  for (PRInt32 i = count-1; i>=0; i--) {
+  imgRequestProxy* proxy;
-    imgRequestProxy *proxy = static_cast<imgRequestProxy*>(mObservers[i]);
+  while ((proxy = iter.GetNext())) {
     /* calling OnStopRequest may result in the death of |proxy| so don't use the
        pointer after this call.
      */
-    if (proxy) proxy->OnStopRequest(aRequest, ctxt, status, mHadLastPart);
+    proxy->OnStopRequest(aRequest, ctxt, status, mHadLastPart);
   }
 
   return NS_OK;

modules/libpr0n/src/imgRequest.h

@@ -56,7 +56,7 @@
 #include "nsCategoryCache.h"
 #include "nsCOMPtr.h"
 #include "nsString.h"
-#include "nsVoidArray.h"
+#include "nsTObserverArray.h"
 #include "nsWeakReference.h"
 
 class imgCacheValidator;
@@ -89,9 +89,8 @@ public:
                 void *aCacheId,
                 void *aLoadId);
 
-  // Callers that pass aNotify==PR_FALSE must call NotifyProxyListener
+  // Callers must call NotifyProxyListener later.
-  // later.
+  nsresult AddProxy(imgRequestProxy *proxy);
-  nsresult AddProxy   (imgRequestProxy *proxy, PRBool aNotify);
 
   // aNotify==PR_FALSE still sends OnStopRequest.
   nsresult RemoveProxy(imgRequestProxy *proxy, nsresult aStatus, PRBool aNotify);
@@ -156,7 +155,7 @@ private:
   nsCOMPtr<imgIDecoder> mDecoder;
   nsCOMPtr<nsIProperties> mProperties;
 
-  nsAutoVoidArray mObservers;
+  nsTObserverArray<imgRequestProxy> mObservers;
 
   PRPackedBool mLoading;
   PRPackedBool mProcessing;

modules/libpr0n/src/imgRequestProxy.cpp

@@ -92,8 +92,6 @@ imgRequestProxy::~imgRequestProxy()
        */
       mOwner->RemoveProxy(this, NS_OK, PR_FALSE);
     }
-
-    NS_RELEASE(mOwner);
   }
 }
 
@@ -101,6 +99,7 @@ imgRequestProxy::~imgRequestProxy()
 
 nsresult imgRequestProxy::Init(imgRequest *request, nsILoadGroup *aLoadGroup, imgIDecoderObserver *aObserver)
 {
+  NS_PRECONDITION(!mOwner && !mListener, "imgRequestProxy is already initialized");
   NS_PRECONDITION(request, "no request");
   if (!request)
     return NS_ERROR_NULL_POINTER;
@@ -108,13 +107,11 @@ nsresult imgRequestProxy::Init(imgRequest *request, nsILoadGroup *aLoadGroup, im
   LOG_SCOPE_WITH_PARAM(gImgLog, "imgRequestProxy::Init", "request", request);
 
   mOwner = request;
-  NS_ADDREF(mOwner);
-
   mListener = aObserver;
-
   mLoadGroup = aLoadGroup;
 
-  request->AddProxy(this, PR_FALSE); // Pass PR_FALSE here so that AddProxy doesn't send all the On* notifications immediatly
+  // Note: AddProxy won't send all the On* notifications immediatly
+  request->AddProxy(this);
 
   return NS_OK;
 }
@@ -127,12 +124,10 @@ nsresult imgRequestProxy::ChangeOwner(imgRequest *aNewOwner)
   // Passing false to aNotify means that mListener will still get
   // OnStopRequest, if needed.
   mOwner->RemoveProxy(this, NS_IMAGELIB_CHANGING_OWNER, PR_FALSE);
-  NS_RELEASE(mOwner);
 
   mOwner = aNewOwner;
-  NS_ADDREF(mOwner);
 
-  mOwner->AddProxy(this, PR_FALSE);
+  mOwner->AddProxy(this);
 
   return NS_OK;
 }

modules/libpr0n/src/imgRequestProxy.h

@@ -47,6 +47,7 @@
 #include "nsILoadGroup.h"
 #include "nsISupportsPriority.h"
 #include "nsCOMPtr.h"
+#include "nsAutoPtr.h"
 
 #include "imgRequest.h"
 
@@ -105,7 +106,13 @@ protected:
 private:
   friend class imgCacheValidator;
 
-  imgRequest *mOwner;
+  // We maintain the following invariant:
+  // The proxy is registered at most with a single imgRequest as an observer,
+  // and whenever it is, mOwner points to that object. This helps ensure that
+  // imgRequestProxy::~imgRequestProxy unregisters the proxy as an observer
+  // from whatever request it was registered with (if any). This, in turn,
+  // means that imgRequest::mObservers will not have any stale pointers in it.
+  nsRefPtr<imgRequest> mOwner;
 
   imgIDecoderObserver* mListener;  // Weak ref; see imgILoader.idl
   nsCOMPtr<nsILoadGroup> mLoadGroup;

xpcom/glue/nsTObserverArray.h

@@ -49,7 +49,7 @@ class NS_COM_GLUE nsTObserverArray_base {
       protected:
         friend class nsTObserverArray_base;
 
-        Iterator_base(PRInt32 aPosition, nsTObserverArray_base& aArray)
+        Iterator_base(PRInt32 aPosition, const nsTObserverArray_base& aArray)
           : mPosition(aPosition),
             mNext(aArray.mIterators),
             mArray(aArray) {
@@ -80,7 +80,7 @@ class NS_COM_GLUE nsTObserverArray_base {
         Iterator_base* mNext;
 
         // The array we're iterating
-        nsTObserverArray_base& mArray;
+        const nsTObserverArray_base& mArray;
     };
 
     /**
@@ -104,7 +104,7 @@ class NS_COM_GLUE nsTObserverArray_base {
      */
     void AdjustIterators(PRInt32 aModPos, PRInt32 aAdjustment);
 
-    Iterator_base* mIterators;
+    mutable Iterator_base* mIterators;
     nsVoidArray mObservers;
 };
 
@@ -182,7 +182,7 @@ class nsTObserverArray : public nsTObserverArray_base {
     // to GetNext
     class ForwardIterator : public nsTObserverArray_base::Iterator_base {
       public:
-        ForwardIterator(nsTObserverArray<T>& aArray)
+        ForwardIterator(const nsTObserverArray<T>& aArray)
           : Iterator_base(0, aArray) {
         }