Bug 393936 - nsIRequest::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). Note, however, that this new method shouldn't be used in any new code; it exists only to support code that relied on the broken behaviour prior to this checkin. r=bzbarsky sr=vlad

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,16 @@ 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.
+   *
+   * NOTE: You should not use this in any new code; instead, use cancel(). Note
+   * that cancel() is asynchronous, which means that some time after you call
+   * it, the listener/observer will get an OnStopRequest(). This means that, if
+   * you're the observer, you can't call cancel() from your destructor.
+   */
+  void cancelAndForgetObserver(in nsresult aStatus);
 };
 

modules/libpr0n/src/imgRequestProxy.cpp

@@ -74,6 +74,7 @@ imgRequestProxy::~imgRequestProxy()
 {
   /* destructor code */
   NS_PRECONDITION(!mListener, "Someone forgot to properly cancel this request!");
+
   // Explicitly set mListener to null to ensure that the RemoveProxy
   // call below can't send |this| to an arbitrary listener while |this|
   // is being destroyed.  This is all belt-and-suspenders in view of the
@@ -97,8 +98,6 @@ imgRequestProxy::~imgRequestProxy()
   }
 }
 
-
-
 nsresult imgRequestProxy::Init(imgRequest *request, nsILoadGroup *aLoadGroup, imgIDecoderObserver *aObserver)
 {
   NS_PRECONDITION(!mOwner && !mListener, "imgRequestProxy is already initialized");
@@ -197,6 +196,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,11 +216,35 @@ 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();
+}
+
+/* void cancelAndForgetObserver (in nsresult aStatus); */
+NS_IMETHODIMP imgRequestProxy::CancelAndForgetObserver(nsresult aStatus)
+{
+  if (mCanceled || !mOwner)
+    return NS_ERROR_FAILURE;
+
+  LOG_SCOPE(gImgLog, "imgRequestProxy::CancelAndForgetObserver");
+
+  mCanceled = PR_TRUE;
+
+  // Passing false to aNotify means that mListener will still get
+  // OnStopRequest, if needed.
+  mOwner->RemoveProxy(this, aStatus, PR_FALSE);
+
+  NullOutListener();
 
   return NS_OK;
 }
@@ -400,7 +425,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 +438,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 +449,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 +460,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 +471,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 +482,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 +493,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 +504,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 +521,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);
 }