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

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

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

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

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<nsImageBoxListener*>(mListener.get())->SetFrame(nsnull); // set the frame to null so we don't send messages to a dead object.

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

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<nsIRunnable> 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<imgIDecoderObserver> 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<imgIDecoderObserver> 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<imgIDecoderObserver> 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<imgIDecoderObserver> 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<imgIDecoderObserver> 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<imgIDecoderObserver> 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<imgIDecoderObserver> 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<imgIDecoderObserver> 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<imgIDecoderObserver> kungFuDeathGrip(mListener);

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<imgRequestProxy> 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();
   

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