Make imgRequestProxy hold a weak ref to its listener, to prevent reference

cycles.  Bug 196797, r=pavlov, sr=jst

content/html/content/src/nsHTMLImageElement.cpp

@@ -200,6 +200,9 @@ nsHTMLImageElement::nsHTMLImageElement()
 
 nsHTMLImageElement::~nsHTMLImageElement()
 {
+  if (mRequest) {
+    mRequest->Cancel(NS_ERROR_FAILURE);
+  }
 }
 
 
@@ -817,9 +820,9 @@ nsHTMLImageElement::SetSrcInner(nsIURI* aBaseURL,
       // If we have a loader we're in the middle of loading a image,
       // we'll cancel that load and start a new one.
 
-      // if (mRequest) {
+      if (mRequest) {
-      //   mRequest->Cancel() ?? cancel the load?
+        mRequest->Cancel(NS_ERROR_FAILURE);
-      // }
+      }
 
       nsCOMPtr<imgILoader> il(do_GetService("@mozilla.org/image/loader;1"));
       if (!il) {

content/html/document/src/nsImageDocument.cpp

@@ -158,6 +158,7 @@ NS_IMPL_THREADSAFE_ISUPPORTS2(ImageListener,
 NS_IMETHODIMP
 ImageListener::OnStartRequest(nsIRequest* request, nsISupports *ctxt)
 {
+  NS_PRECONDITION(!mDocument->mImageRequest, "OnStartRequest called twice!");
   nsCOMPtr<nsIChannel> channel = do_QueryInterface(request);
   if (!channel) {
     return NS_ERROR_FAILURE;
@@ -217,6 +218,9 @@ nsImageDocument::nsImageDocument()
 
 nsImageDocument::~nsImageDocument()
 {
+  if (mImageRequest) {
+    mImageRequest->Cancel(NS_ERROR_FAILURE);
+  }
 }
 
 NS_IMPL_ADDREF_INHERITED(nsImageDocument, nsHTMLDocument)
@@ -302,10 +306,14 @@ NS_IMETHODIMP
 nsImageDocument::SetScriptGlobalObject(nsIScriptGlobalObject* aScriptGlobalObject)
 {
   if (!aScriptGlobalObject) {
-    // If the global object is being set to null, then it means we are
+    // If the global object is being set to null, then it means we are going
-    // going away soon. Drop our ref to imgRequest so that we don't end
+    // away soon. Drop our ref to imgRequest so that we don't end up leaking
-    // up leaking due to cycles through imgLib
+    // due to cycles through imgLib.  This should not be really necessary
+    // anymore, but it's a belt-and-suspenders thing.
+    if (mImageRequest) {
+      mImageRequest->Cancel(NS_ERROR_FAILURE);
       mImageRequest = nsnull;
+    }
 
     if (mImageResizingEnabled) {
       nsCOMPtr<nsIDOMEventTarget> target = do_QueryInterface(mImageElement);

layout/base/nsImageLoader.cpp

@@ -115,6 +115,9 @@ nsImageLoader::Load(nsIURI *aURI)
     if (eq) {
       return NS_OK;
     }
+
+    // Now cancel the old request so it won't hold a stale ref to us.
+    mRequest->Cancel(NS_ERROR_FAILURE);
   }
 
   nsCOMPtr<imgILoader> il(do_GetService("@mozilla.org/image/loader;1", &rv));

layout/base/src/nsImageLoader.cpp

@@ -115,6 +115,9 @@ nsImageLoader::Load(nsIURI *aURI)
     if (eq) {
       return NS_OK;
     }
+
+    // Now cancel the old request so it won't hold a stale ref to us.
+    mRequest->Cancel(NS_ERROR_FAILURE);
   }
 
   nsCOMPtr<imgILoader> il(do_GetService("@mozilla.org/image/loader;1", &rv));

layout/generic/nsImageFrame.h

@@ -272,6 +272,15 @@ private:
     // broken image and loading image icons
   public:
     IconLoad(nsIPresContext *aPresContext):mRefCount(0),mIconsLoaded(PR_FALSE) { GetPrefs(aPresContext); }
+    ~IconLoad()
+    {
+      if (mIconLoads[0].mRequest) {
+        mIconLoads[0].mRequest->Cancel(NS_ERROR_FAILURE);
+      }
+      if (mIconLoads[1].mRequest) {
+        mIconLoads[1].mRequest->Cancel(NS_ERROR_FAILURE);
+      }
+    }
     void AddRef(void) { ++mRefCount; }
     PRBool Release(void) { return --mRefCount == 0; }
     void GetPrefs(nsIPresContext *aPresContext);

layout/html/base/src/nsImageFrame.h

@@ -272,6 +272,15 @@ private:
     // broken image and loading image icons
   public:
     IconLoad(nsIPresContext *aPresContext):mRefCount(0),mIconsLoaded(PR_FALSE) { GetPrefs(aPresContext); }
+    ~IconLoad()
+    {
+      if (mIconLoads[0].mRequest) {
+        mIconLoads[0].mRequest->Cancel(NS_ERROR_FAILURE);
+      }
+      if (mIconLoads[1].mRequest) {
+        mIconLoads[1].mRequest->Cancel(NS_ERROR_FAILURE);
+      }
+    }
     void AddRef(void) { ++mRefCount; }
     PRBool Release(void) { return --mRefCount == 0; }
     void GetPrefs(nsIPresContext *aPresContext);

layout/xul/base/src/nsImageBoxFrame.cpp

@@ -424,7 +424,10 @@ nsImageBoxFrame::UpdateImage(nsIPresContext*  aPresContext, PRBool& aResize)
     mHasImage = PR_FALSE;
     aResize = PR_TRUE;
 
+    if (mImageRequest) {
+      mImageRequest->Cancel(NS_ERROR_FAILURE);
       mImageRequest = nsnull;
+    }
 
     return;
   }

layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp

@@ -101,6 +101,21 @@
 
 #define ELLIPSIS "..."
 
+// Enumeration function that cancels all the image requests in our cache
+PR_STATIC_CALLBACK(PRBool)
+CancelImageRequest(nsHashKey* aKey, void* aData, void* aClosure)
+{
+  nsISupports* supports = NS_STATIC_CAST(nsISupports*, aData);
+  nsCOMPtr<imgIRequest> request = do_QueryInterface(supports);
+  nsCOMPtr<imgIDecoderObserver> observer;
+  request->GetDecoderObserver(getter_AddRefs(observer));
+  NS_ASSERTION(observer, "No observer?  We're leaking!");
+  request->Cancel(NS_ERROR_FAILURE);
+  imgIDecoderObserver* observer2 = observer;
+  NS_RELEASE(observer2);  // Balance out the addref from GetImage()
+  return PR_TRUE;
+}
+
 // The style context cache impl
 nsStyleContext*
 nsTreeStyleCache::GetStyleContext(nsICSSPseudoComparator* aComparator,
@@ -332,8 +347,11 @@ nsTreeBodyFrame::nsTreeBodyFrame(nsIPresShell* aPresShell)
 // Destructor
 nsTreeBodyFrame::~nsTreeBodyFrame()
 {
+  if (mImageCache) {
+    mImageCache->Enumerate(CancelImageRequest);
     delete mImageCache;
   }
+}
 
 NS_IMETHODIMP_(nsrefcnt) 
 nsTreeBodyFrame::AddRef(void)
@@ -1877,6 +1895,7 @@ nsTreeBodyFrame::GetImage(PRInt32 aRowIndex, const PRUnichar* aColID, PRBool aUs
       return NS_ERROR_OUT_OF_MEMORY;
 
     listener->AddRow(aRowIndex);
+    nsCOMPtr<imgIDecoderObserver> imgDecoderObserver = listener;
 
     nsCOMPtr<nsIURI> baseURI;
     nsCOMPtr<nsIDocument> doc;
@@ -1903,8 +1922,8 @@ nsTreeBodyFrame::GetImage(PRInt32 aRowIndex, const PRUnichar* aColID, PRBool aUs
 
     mImageGuard = PR_TRUE;
     // XXX: initialDocumentURI is NULL!
-    rv = il->LoadImage(srcURI, nsnull, documentURI, nsnull, listener, doc,
+    rv = il->LoadImage(srcURI, nsnull, documentURI, nsnull, imgDecoderObserver,
-                       nsIRequest::LOAD_NORMAL, nsnull, nsnull,
+                       doc, nsIRequest::LOAD_NORMAL, nsnull, nsnull,
                        getter_AddRefs(imageRequest));
     mImageGuard = PR_FALSE;
 
@@ -1920,6 +1939,8 @@ nsTreeBodyFrame::GetImage(PRInt32 aRowIndex, const PRUnichar* aColID, PRBool aUs
         return NS_ERROR_OUT_OF_MEMORY;
     }
     mImageCache->Put(&key, imageRequest);
+    imgIDecoderObserver* decoderObserverPtr = imgDecoderObserver;
+    NS_ADDREF(decoderObserverPtr);  // Will get released when we remove the cache entry.
   }
   return NS_OK;
 }
@@ -3462,7 +3483,10 @@ NS_IMETHODIMP
 nsTreeBodyFrame::ClearStyleAndImageCaches()
 {
   mStyleCache.Clear();
+  if (mImageCache) {
+    mImageCache->Enumerate(CancelImageRequest);
     delete mImageCache;
+  }
   mImageCache = nsnull;
   mScrollbar = nsnull;
   return NS_OK;

modules/libpr0n/public/imgILoader.idl

@@ -58,6 +58,12 @@ interface imgILoader : nsISupports
    *                  image came from a request that had post data
    * @param aRequest A newly created, unused imgIRequest object or NULL for one to
                      be created for you.
+
+
+   * libpr0n does NOT keep a strong ref to the observer; this prevents
+   * reference cycles.  This means that callers of loadImage should
+   * make sure to Cancel() the resulting request before the observer
+   * goes away.
    */
   imgIRequest loadImage(in nsIURI aURI,
                         in nsIURI aInitialDocumentURL,
@@ -74,6 +80,10 @@ interface imgILoader : nsISupports
    * @param uri the URI to load
    * @param aObserver the observer
    * @param cx some random data
+   *
+   * libpr0n does NOT keep a strong ref to the observer; this prevents
+   * reference cycles.  This means that callers of loadImageWithChannel should
+   * make sure to Cancel() the resulting request before the observer goes away.
    */
   imgIRequest loadImageWithChannel(in nsIChannel aChannel, in imgIDecoderObserver aObserver, in nsISupports cx, out nsIStreamListener aListener);
 

modules/libpr0n/src/imgRequestProxy.cpp

@@ -42,6 +42,7 @@ NS_IMPL_THREADSAFE_ISUPPORTS2(imgRequestProxy, imgIRequest, nsIRequest)
 
 imgRequestProxy::imgRequestProxy() :
   mOwner(nsnull),
+  mListener(nsnull),
   mLoadFlags(nsIRequest::LOAD_NORMAL),
   mCanceled(PR_FALSE),
   mIsInLoadGroup(PR_FALSE),
@@ -55,6 +56,7 @@ imgRequestProxy::imgRequestProxy() :
 imgRequestProxy::~imgRequestProxy()
 {
   /* destructor code */
+  NS_PRECONDITION(!mListener, "Someone forgot to properly cancel this request!");
 
   if (mOwner) {
     if (!mCanceled) {
@@ -294,9 +296,12 @@ void imgRequestProxy::FrameChanged(imgIContainer *container, gfxIImageFrame *new
 {
   LOG_FUNC(gImgLog, "imgRequestProxy::FrameChanged");
 
-  if (mListener)
+  if (mListener) {
+    // Hold a ref to the listener while we call it, just in case.
+    nsCOMPtr<imgIDecoderObserver> kungFuDeathGrip(mListener);
     mListener->FrameChanged(container, newframe, dirtyRect);
   }
+}
 
 /** imgIDecoderObserver methods **/
 
@@ -304,57 +309,78 @@ void imgRequestProxy::OnStartDecode()
 {
   LOG_FUNC(gImgLog, "imgRequestProxy::OnStartDecode");
 
-  if (mListener)
+  if (mListener) {
+    // Hold a ref to the listener while we call it, just in case.
+    nsCOMPtr<imgIDecoderObserver> kungFuDeathGrip(mListener);
     mListener->OnStartDecode(this);
   }
+}
 
 void imgRequestProxy::OnStartContainer(imgIContainer *image)
 {
   LOG_FUNC(gImgLog, "imgRequestProxy::OnStartContainer");
 
-  if (mListener)
+  if (mListener) {
+    // Hold a ref to the listener while we call it, just in case.
+    nsCOMPtr<imgIDecoderObserver> kungFuDeathGrip(mListener);
     mListener->OnStartContainer(this, image);
   }
+}
 
 void imgRequestProxy::OnStartFrame(gfxIImageFrame *frame)
 {
   LOG_FUNC(gImgLog, "imgRequestProxy::OnStartFrame");
 
-  if (mListener)
+  if (mListener) {
+    // Hold a ref to the listener while we call it, just in case.
+    nsCOMPtr<imgIDecoderObserver> kungFuDeathGrip(mListener);
     mListener->OnStartFrame(this, frame);
   }
+}
 
 void imgRequestProxy::OnDataAvailable(gfxIImageFrame *frame, const nsRect * rect)
 {
   LOG_FUNC(gImgLog, "imgRequestProxy::OnDataAvailable");
 
-  if (mListener)
+  if (mListener) {
+    // Hold a ref to the listener while we call it, just in case.
+    nsCOMPtr<imgIDecoderObserver> kungFuDeathGrip(mListener);
     mListener->OnDataAvailable(this, frame, rect);
   }
+}
 
 void imgRequestProxy::OnStopFrame(gfxIImageFrame *frame)
 {
   LOG_FUNC(gImgLog, "imgRequestProxy::OnStopFrame");
 
-  if (mListener)
+  if (mListener) {
+    // Hold a ref to the listener while we call it, just in case.
+    nsCOMPtr<imgIDecoderObserver> kungFuDeathGrip(mListener);
     mListener->OnStopFrame(this, frame);
   }
+}
 
 void imgRequestProxy::OnStopContainer(imgIContainer *image)
 {
   LOG_FUNC(gImgLog, "imgRequestProxy::OnStopContainer");
 
-  if (mListener)
+  if (mListener) {
+    // Hold a ref to the listener while we call it, just in case.
+    nsCOMPtr<imgIDecoderObserver> kungFuDeathGrip(mListener);
     mListener->OnStopContainer(this, image);
   }
+}
 
 void imgRequestProxy::OnStopDecode(nsresult status, const PRUnichar *statusArg)
 {
   LOG_FUNC(gImgLog, "imgRequestProxy::OnStopDecode");
 
-  if (mListener)
+  if (mListener) {
+    // Hold a ref to the listener while we call it, just in case.
+    nsCOMPtr<imgIDecoderObserver> kungFuDeathGrip(mListener);
     mListener->OnStopDecode(this, status, statusArg);
   }
+}
 
 
 

modules/libpr0n/src/imgRequestProxy.h

@@ -82,7 +82,7 @@ private:
 
   imgRequest *mOwner;
 
-  nsCOMPtr<imgIDecoderObserver> mListener;
+  imgIDecoderObserver* mListener;  // Weak ref; see imgILoader.idl
   nsCOMPtr<nsILoadGroup> mLoadGroup;
 
   nsLoadFlags mLoadFlags;