From 6bfc4bd6231232e0088b4f6966d7fb88798260aa Mon Sep 17 00:00:00 2001 From: "asqueella@gmail.com" Date: Sat, 22 Sep 2007 12:40:57 -0700 Subject: [PATCH] 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 | 149 ++++++++---------------- modules/libpr0n/src/imgRequest.h | 9 +- modules/libpr0n/src/imgRequestProxy.cpp | 13 +-- modules/libpr0n/src/imgRequestProxy.h | 9 +- xpcom/glue/nsTObserverArray.h | 8 +- 5 files changed, 70 insertions(+), 118 deletions(-) diff --git a/modules/libpr0n/src/imgRequest.cpp b/modules/libpr0n/src/imgRequest.cpp index c492deef8e1..3e267150c0d 100644 --- a/modules/libpr0n/src/imgRequest.cpp +++ b/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(proxy)); - - if (aNotify) - NotifyProxyListener(proxy); - - return NS_OK; + return mObservers.AppendObserver(proxy) ? NS_OK : NS_ERROR_OUT_OF_MEMORY; } nsresult imgRequest::RemoveProxy(imgRequestProxy *proxy, nsresult aStatus, PRBool aNotify) { LOG_SCOPE_WITH_PARAM(gImgLog, "imgRequest::RemoveProxy", "proxy", proxy); - mObservers.RemoveElement(static_cast(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) { - imgRequestProxy *proxy = static_cast(mObservers[i]); + nsTObserverArray::ForwardIterator iter(mObservers); + 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 p = do_QueryInterface(mRequest); @@ -401,15 +398,10 @@ NS_IMETHODIMP imgRequest::FrameChanged(imgIContainer *container, { LOG_SCOPE(gImgLog, "imgRequest::FrameChanged"); - PRInt32 count = mObservers.Count(); - for (PRInt32 i = 0; i < count; i++) { - imgRequestProxy *proxy = static_cast(mObservers[i]); - if (proxy) 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!"); + nsTObserverArray::ForwardIterator iter(mObservers); + imgRequestProxy* proxy; + while ((proxy = iter.GetNext())) { + proxy->FrameChanged(container, newframe, dirtyRect); } return NS_OK; @@ -424,15 +416,10 @@ NS_IMETHODIMP imgRequest::OnStartDecode(imgIRequest *request) mState |= onStartDecode; - PRInt32 count = mObservers.Count(); - for (PRInt32 i = 0; i < count; i++) { - imgRequestProxy *proxy = static_cast(mObservers[i]); - if (proxy) 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!"); + nsTObserverArray::ForwardIterator iter(mObservers); + imgRequestProxy* proxy; + while ((proxy = iter.GetNext())) { + proxy->OnStartDecode(); } /* 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(); - for (PRInt32 i = 0; i < count; i++) { - imgRequestProxy *proxy = static_cast(mObservers[i]); - if (proxy) 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!"); - + nsTObserverArray::ForwardIterator iter(mObservers); + imgRequestProxy* proxy; + while ((proxy = iter.GetNext())) { + proxy->OnStartContainer(image); } return NS_OK; @@ -485,15 +466,10 @@ NS_IMETHODIMP imgRequest::OnStartFrame(imgIRequest *request, { LOG_SCOPE(gImgLog, "imgRequest::OnStartFrame"); - PRInt32 count = mObservers.Count(); - for (PRInt32 i = 0; i < count; i++) { - imgRequestProxy *proxy = static_cast(mObservers[i]); - if (proxy) 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!"); + nsTObserverArray::ForwardIterator iter(mObservers); + imgRequestProxy* proxy; + while ((proxy = iter.GetNext())) { + proxy->OnStartFrame(frame); } return NS_OK; @@ -506,15 +482,10 @@ NS_IMETHODIMP imgRequest::OnDataAvailable(imgIRequest *request, { LOG_SCOPE(gImgLog, "imgRequest::OnDataAvailable"); - PRInt32 count = mObservers.Count(); - for (PRInt32 i = 0; i < count; i++) { - imgRequestProxy *proxy = static_cast(mObservers[i]); - if (proxy) 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!"); + nsTObserverArray::ForwardIterator iter(mObservers); + imgRequestProxy* proxy; + while ((proxy = iter.GetNext())) { + proxy->OnDataAvailable(frame, rect); } return NS_OK; @@ -541,15 +512,10 @@ NS_IMETHODIMP imgRequest::OnStopFrame(imgIRequest *request, mCacheEntry->SetDataSize(cacheSize + imageSize); } - PRInt32 count = mObservers.Count(); - for (PRInt32 i = 0; i < count; i++) { - imgRequestProxy *proxy = static_cast(mObservers[i]); - if (proxy) 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!"); + nsTObserverArray::ForwardIterator iter(mObservers); + imgRequestProxy* proxy; + while ((proxy = iter.GetNext())) { + proxy->OnStopFrame(frame); } return NS_OK; @@ -563,15 +529,10 @@ NS_IMETHODIMP imgRequest::OnStopContainer(imgIRequest *request, mState |= onStopContainer; - PRInt32 count = mObservers.Count(); - for (PRInt32 i = 0; i < count; i++) { - imgRequestProxy *proxy = static_cast(mObservers[i]); - if (proxy) 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!"); + nsTObserverArray::ForwardIterator iter(mObservers); + imgRequestProxy* proxy; + while ((proxy = iter.GetNext())) { + proxy->OnStopContainer(image); } return NS_OK; @@ -592,15 +553,10 @@ NS_IMETHODIMP imgRequest::OnStopDecode(imgIRequest *aRequest, mImageStatus |= imgIRequest::STATUS_ERROR; } - PRInt32 count = mObservers.Count(); - for (PRInt32 i = 0; i < count; i++) { - imgRequestProxy *proxy = static_cast(mObservers[i]); - if (proxy) 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!"); + nsTObserverArray::ForwardIterator iter(mObservers); + imgRequestProxy* proxy; + while ((proxy = iter.GetNext())) { + proxy->OnStopDecode(GetResultFromImageStatus(mImageStatus), aStatusArg); } 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(); - for (PRInt32 i = 0; i < count; i++) { - imgRequestProxy *proxy = static_cast(mObservers[i]); - if (proxy) 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!"); + nsTObserverArray::ForwardIterator iter(mObservers); + imgRequestProxy* proxy; + while ((proxy = iter.GetNext())) { + proxy->OnStartRequest(aRequest, ctxt); } nsCOMPtr 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(); - for (PRInt32 i = count-1; i>=0; i--) { - imgRequestProxy *proxy = static_cast(mObservers[i]); + nsTObserverArray::ForwardIterator iter(mObservers); + imgRequestProxy* proxy; + 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; diff --git a/modules/libpr0n/src/imgRequest.h b/modules/libpr0n/src/imgRequest.h index fd1e7fb4d8d..3de9607f59d 100644 --- a/modules/libpr0n/src/imgRequest.h +++ b/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 - // later. - nsresult AddProxy (imgRequestProxy *proxy, PRBool aNotify); + // Callers must call NotifyProxyListener later. + nsresult AddProxy(imgRequestProxy *proxy); // aNotify==PR_FALSE still sends OnStopRequest. nsresult RemoveProxy(imgRequestProxy *proxy, nsresult aStatus, PRBool aNotify); @@ -156,7 +155,7 @@ private: nsCOMPtr mDecoder; nsCOMPtr mProperties; - nsAutoVoidArray mObservers; + nsTObserverArray mObservers; PRPackedBool mLoading; PRPackedBool mProcessing; diff --git a/modules/libpr0n/src/imgRequestProxy.cpp b/modules/libpr0n/src/imgRequestProxy.cpp index 8548cb1aea9..f960a28f736 100644 --- a/modules/libpr0n/src/imgRequestProxy.cpp +++ b/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; } diff --git a/modules/libpr0n/src/imgRequestProxy.h b/modules/libpr0n/src/imgRequestProxy.h index 5bca7de22c7..3c5f3fbab88 100644 --- a/modules/libpr0n/src/imgRequestProxy.h +++ b/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 mOwner; imgIDecoderObserver* mListener; // Weak ref; see imgILoader.idl nsCOMPtr mLoadGroup; diff --git a/xpcom/glue/nsTObserverArray.h b/xpcom/glue/nsTObserverArray.h index e40de4e5030..c1dc472ce8f 100644 --- a/xpcom/glue/nsTObserverArray.h +++ b/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& aArray) + ForwardIterator(const nsTObserverArray& aArray) : Iterator_base(0, aArray) { }