From eeb6de683a7dda045119ce040e80ea2723b6ef76 Mon Sep 17 00:00:00 2001 From: Joe Drew Date: Fri, 7 Nov 2008 14:35:22 -0500 Subject: [PATCH] Bug 393936 - Cancel() must not notify anything sync. Make removal from the loadgroup async, and add another call CancelAndForgetObserver() that removes the listener immediately (since some callsites expect that). r=bz, sr=pavlov, a=blocking1.9.1+ --- content/base/src/nsImageLoadingContent.cpp | 4 +- layout/base/nsImageLoader.cpp | 6 +-- layout/generic/nsImageFrame.h | 4 +- layout/xul/base/src/nsImageBoxFrame.cpp | 2 +- modules/libpr0n/public/imgIRequest.idl | 6 +++ modules/libpr0n/src/imgRequestProxy.cpp | 44 +++++++++++++++++----- modules/libpr0n/src/imgRequestProxy.h | 26 +++++++++++++ widget/src/cocoa/nsMenuItemIconX.mm | 2 +- 8 files changed, 75 insertions(+), 19 deletions(-) diff --git a/content/base/src/nsImageLoadingContent.cpp b/content/base/src/nsImageLoadingContent.cpp index f6f89284151..9b78c9152d1 100644 --- a/content/base/src/nsImageLoadingContent.cpp +++ b/content/base/src/nsImageLoadingContent.cpp @@ -121,11 +121,11 @@ nsImageLoadingContent::DestroyImageLoadingContent() { // Cancel our requests so they won't hold stale refs to us if (mCurrentRequest) { - mCurrentRequest->Cancel(NS_ERROR_FAILURE); + mCurrentRequest->CancelAndForgetObserver(NS_ERROR_FAILURE); mCurrentRequest = nsnull; } if (mPendingRequest) { - mPendingRequest->Cancel(NS_ERROR_FAILURE); + mPendingRequest->CancelAndForgetObserver(NS_ERROR_FAILURE); mPendingRequest = nsnull; } } diff --git a/layout/base/nsImageLoader.cpp b/layout/base/nsImageLoader.cpp index f917dbb115c..90f4fb78fa4 100644 --- a/layout/base/nsImageLoader.cpp +++ b/layout/base/nsImageLoader.cpp @@ -74,7 +74,7 @@ nsImageLoader::~nsImageLoader() mPresContext = nsnull; if (mRequest) { - mRequest->Cancel(NS_ERROR_FAILURE); + mRequest->CancelAndForgetObserver(NS_ERROR_FAILURE); } } @@ -95,7 +95,7 @@ nsImageLoader::Destroy() mPresContext = nsnull; if (mRequest) { - mRequest->Cancel(NS_ERROR_FAILURE); + mRequest->CancelAndForgetObserver(NS_ERROR_FAILURE); } mRequest = nsnull; @@ -122,7 +122,7 @@ nsImageLoader::Load(imgIRequest *aImage) } // Now cancel the old request so it won't hold a stale ref to us. - mRequest->Cancel(NS_ERROR_FAILURE); + mRequest->CancelAndForgetObserver(NS_ERROR_FAILURE); mRequest = nsnull; } diff --git a/layout/generic/nsImageFrame.h b/layout/generic/nsImageFrame.h index c1e66ae4576..fd17a3eb22d 100644 --- a/layout/generic/nsImageFrame.h +++ b/layout/generic/nsImageFrame.h @@ -307,11 +307,11 @@ private: { // in case the pref service releases us later if (mLoadingImage) { - mLoadingImage->Cancel(NS_ERROR_FAILURE); + mLoadingImage->CancelAndForgetObserver(NS_ERROR_FAILURE); mLoadingImage = nsnull; } if (mBrokenImage) { - mBrokenImage->Cancel(NS_ERROR_FAILURE); + mBrokenImage->CancelAndForgetObserver(NS_ERROR_FAILURE); mBrokenImage = nsnull; } } diff --git a/layout/xul/base/src/nsImageBoxFrame.cpp b/layout/xul/base/src/nsImageBoxFrame.cpp index cdab38ee8c9..25b4127a914 100644 --- a/layout/xul/base/src/nsImageBoxFrame.cpp +++ b/layout/xul/base/src/nsImageBoxFrame.cpp @@ -207,7 +207,7 @@ nsImageBoxFrame::Destroy() { // Release image loader first so that it's refcnt can go to zero if (mImageRequest) - mImageRequest->Cancel(NS_ERROR_FAILURE); + mImageRequest->CancelAndForgetObserver(NS_ERROR_FAILURE); if (mListener) reinterpret_cast(mListener.get())->SetFrame(nsnull); // set the frame to null so we don't send messages to a dead object. diff --git a/modules/libpr0n/public/imgIRequest.idl b/modules/libpr0n/public/imgIRequest.idl index 11b32a02e46..82b78970651 100644 --- a/modules/libpr0n/public/imgIRequest.idl +++ b/modules/libpr0n/public/imgIRequest.idl @@ -104,5 +104,11 @@ interface imgIRequest : nsIRequest * The principal gotten from the channel the image was loaded from. */ readonly attribute nsIPrincipal imagePrincipal; + + /** + * Cancels this request as in nsIRequest::Cancel(); further, also nulls out + * decoderObserver so it gets no further notifications from us. + */ + void cancelAndForgetObserver(in nsresult aStatus); }; diff --git a/modules/libpr0n/src/imgRequestProxy.cpp b/modules/libpr0n/src/imgRequestProxy.cpp index 0bebed2a0eb..a92235b4a09 100644 --- a/modules/libpr0n/src/imgRequestProxy.cpp +++ b/modules/libpr0n/src/imgRequestProxy.cpp @@ -197,6 +197,8 @@ NS_IMETHODIMP imgRequestProxy::IsPending(PRBool *_retval) /* readonly attribute nsresult status; */ NS_IMETHODIMP imgRequestProxy::GetStatus(nsresult *aStatus) { + // XXXbz this is wrong... Canceling with a status should make that + // status the status of the request, generally. if (!mOwner) return NS_ERROR_FAILURE; @@ -215,13 +217,18 @@ NS_IMETHODIMP imgRequestProxy::Cancel(nsresult status) mCanceled = PR_TRUE; + nsCOMPtr ev = new imgCancelRunnable(this, status); + return NS_DispatchToCurrentThread(ev); +} + +void +imgRequestProxy::DoCancel(nsresult status) +{ // Passing false to aNotify means that mListener will still get // OnStopRequest, if needed. mOwner->RemoveProxy(this, status, PR_FALSE); NullOutListener(); - - return NS_OK; } /* void suspend (); */ @@ -360,6 +367,21 @@ NS_IMETHODIMP imgRequestProxy::GetImagePrincipal(nsIPrincipal **aPrincipal) return mOwner->GetPrincipal(aPrincipal); } +/* void cancelAndForgetObserver (in nsresult aStatus); */ +NS_IMETHODIMP imgRequestProxy::CancelAndForgetObserver(nsresult aStatus) +{ + if (mCanceled || !mOwner) + return NS_ERROR_FAILURE; + + LOG_SCOPE(gImgLog, "imgRequestProxy::CancelAndForgetObserver"); + + nsresult rv = Cancel(aStatus); + + NullOutListener(); + + return rv; +} + /** nsISupportsPriority methods **/ NS_IMETHODIMP imgRequestProxy::GetPriority(PRInt32 *priority) @@ -400,7 +422,7 @@ void imgRequestProxy::FrameChanged(imgIContainer *container, gfxIImageFrame *new { LOG_FUNC(gImgLog, "imgRequestProxy::FrameChanged"); - if (mListener) { + if (mListener && !mCanceled) { // Hold a ref to the listener while we call it, just in case. nsCOMPtr kungFuDeathGrip(mListener); mListener->FrameChanged(container, newframe, dirtyRect); @@ -413,7 +435,7 @@ void imgRequestProxy::OnStartDecode() { LOG_FUNC(gImgLog, "imgRequestProxy::OnStartDecode"); - if (mListener) { + if (mListener && !mCanceled) { // Hold a ref to the listener while we call it, just in case. nsCOMPtr kungFuDeathGrip(mListener); mListener->OnStartDecode(this); @@ -424,7 +446,7 @@ void imgRequestProxy::OnStartContainer(imgIContainer *image) { LOG_FUNC(gImgLog, "imgRequestProxy::OnStartContainer"); - if (mListener) { + if (mListener && !mCanceled) { // Hold a ref to the listener while we call it, just in case. nsCOMPtr kungFuDeathGrip(mListener); mListener->OnStartContainer(this, image); @@ -435,7 +457,7 @@ void imgRequestProxy::OnStartFrame(gfxIImageFrame *frame) { LOG_FUNC(gImgLog, "imgRequestProxy::OnStartFrame"); - if (mListener) { + if (mListener && !mCanceled) { // Hold a ref to the listener while we call it, just in case. nsCOMPtr kungFuDeathGrip(mListener); mListener->OnStartFrame(this, frame); @@ -446,7 +468,7 @@ void imgRequestProxy::OnDataAvailable(gfxIImageFrame *frame, const nsIntRect * r { LOG_FUNC(gImgLog, "imgRequestProxy::OnDataAvailable"); - if (mListener) { + if (mListener && !mCanceled) { // Hold a ref to the listener while we call it, just in case. nsCOMPtr kungFuDeathGrip(mListener); mListener->OnDataAvailable(this, frame, rect); @@ -457,7 +479,7 @@ void imgRequestProxy::OnStopFrame(gfxIImageFrame *frame) { LOG_FUNC(gImgLog, "imgRequestProxy::OnStopFrame"); - if (mListener) { + if (mListener && !mCanceled) { // Hold a ref to the listener while we call it, just in case. nsCOMPtr kungFuDeathGrip(mListener); mListener->OnStopFrame(this, frame); @@ -468,7 +490,7 @@ void imgRequestProxy::OnStopContainer(imgIContainer *image) { LOG_FUNC(gImgLog, "imgRequestProxy::OnStopContainer"); - if (mListener) { + if (mListener && !mCanceled) { // Hold a ref to the listener while we call it, just in case. nsCOMPtr kungFuDeathGrip(mListener); mListener->OnStopContainer(this, image); @@ -479,7 +501,7 @@ void imgRequestProxy::OnStopDecode(nsresult status, const PRUnichar *statusArg) { LOG_FUNC(gImgLog, "imgRequestProxy::OnStopDecode"); - if (mListener) { + if (mListener && !mCanceled) { // Hold a ref to the listener while we call it, just in case. nsCOMPtr kungFuDeathGrip(mListener); mListener->OnStopDecode(this, status, statusArg); @@ -496,6 +518,8 @@ void imgRequestProxy::OnStartRequest(nsIRequest *request, nsISupports *ctxt) LOG_FUNC_WITH_PARAM(gImgLog, "imgRequestProxy::OnStartRequest", "name", name.get()); #endif + // Notify even if mCanceled, since OnStartRequest is guaranteed by the + // nsIStreamListener contract so it makes sense to do the same here. if (mListener) { // Hold a ref to the listener while we call it, just in case. nsCOMPtr kungFuDeathGrip(mListener); diff --git a/modules/libpr0n/src/imgRequestProxy.h b/modules/libpr0n/src/imgRequestProxy.h index 5f79e200bb4..727829e7668 100644 --- a/modules/libpr0n/src/imgRequestProxy.h +++ b/modules/libpr0n/src/imgRequestProxy.h @@ -49,6 +49,7 @@ #include "nsISupportsPriority.h" #include "nsCOMPtr.h" #include "nsAutoPtr.h" +#include "nsThreadUtils.h" #include "imgRequest.h" @@ -85,6 +86,28 @@ public: protected: friend class imgRequest; + class imgCancelRunnable; + friend class imgCancelRunnable; + + class imgCancelRunnable : public nsRunnable + { + public: + imgCancelRunnable(imgRequestProxy* owner, nsresult status) + : mOwner(owner), mStatus(status) + {} + + NS_IMETHOD Run() { + mOwner->DoCancel(mStatus); + return NS_OK; + } + + private: + nsRefPtr mOwner; + nsresult mStatus; + }; + + + /* non-virtual imgIDecoderObserver methods */ void OnStartDecode (); void OnStartContainer(imgIContainer *aContainer); @@ -105,6 +128,9 @@ protected: return mListener != nsnull; } + /* Finish up canceling ourselves */ + void DoCancel(nsresult status); + /* Do the proper refcount management to null out mListener */ void NullOutListener(); diff --git a/widget/src/cocoa/nsMenuItemIconX.mm b/widget/src/cocoa/nsMenuItemIconX.mm index c2c26cc059b..9058ee53cbd 100644 --- a/widget/src/cocoa/nsMenuItemIconX.mm +++ b/widget/src/cocoa/nsMenuItemIconX.mm @@ -98,7 +98,7 @@ nsMenuItemIconX::nsMenuItemIconX(nsMenuObjectX* aMenuItem, nsMenuItemIconX::~nsMenuItemIconX() { if (mIconRequest) - mIconRequest->Cancel(NS_BINDING_ABORTED); + mIconRequest->CancelAndForgetObserver(NS_BINDING_ABORTED); }