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

This commit is contained in:
asqueella@gmail.com 2007-09-22 12:40:57 -07:00
parent 24c1dcb520
commit 70469d1cb5
5 changed files with 70 additions and 118 deletions

View File

@ -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));
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<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) {
imgRequestProxy *proxy = static_cast<imgRequestProxy*>(mObservers[i]);
nsTObserverArray<imgRequestProxy>::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<nsISupportsPriority> 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<imgRequestProxy*>(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<imgRequestProxy>::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<imgRequestProxy*>(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<imgRequestProxy>::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<imgRequestProxy*>(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<imgRequestProxy>::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<imgRequestProxy*>(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<imgRequestProxy>::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<imgRequestProxy*>(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<imgRequestProxy>::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<imgRequestProxy*>(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<imgRequestProxy>::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<imgRequestProxy*>(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<imgRequestProxy>::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<imgRequestProxy*>(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<imgRequestProxy>::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<imgRequestProxy*>(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<imgRequestProxy>::ForwardIterator iter(mObservers);
imgRequestProxy* proxy;
while ((proxy = iter.GetNext())) {
proxy->OnStartRequest(aRequest, ctxt);
}
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();
for (PRInt32 i = count-1; i>=0; i--) {
imgRequestProxy *proxy = static_cast<imgRequestProxy*>(mObservers[i]);
nsTObserverArray<imgRequestProxy>::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;

View File

@ -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<imgIDecoder> mDecoder;
nsCOMPtr<nsIProperties> mProperties;
nsAutoVoidArray mObservers;
nsTObserverArray<imgRequestProxy> mObservers;
PRPackedBool mLoading;
PRPackedBool mProcessing;

View File

@ -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;
}

View File

@ -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;

View File

@ -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) {
}