Bug 359608 - Animated GIFs are animated even when user navigates to another page.r=bholley,bz;sr=bz;a=blocker
authorAlon Zakai <azakai@mozilla.com>
Tue, 07 Sep 2010 17:33:02 -0700
changeset 52149 fd33aa85de93cd0ff059b48f40b28d67438804b8
parent 52148 d35c0d22fa7150b9c9c28a897bd9372456dff6ba
child 52150 7fda546db6a6b671f3baebcf02e470d6ea88ae7b
push idunknown
push userunknown
push dateunknown
reviewersbholley, bz, bz, blocker
bugs359608
milestone2.0b6pre
Bug 359608 - Animated GIFs are animated even when user navigates to another page.r=bholley,bz;sr=bz;a=blocker
content/base/src/nsDocument.cpp
content/base/src/nsDocument.h
layout/base/nsImageLoader.cpp
layout/generic/nsBulletFrame.cpp
layout/generic/nsImageFrame.cpp
layout/xul/base/src/nsImageBoxFrame.cpp
layout/xul/base/src/tree/src/nsTreeImageListener.cpp
modules/libpr0n/public/imgIContainer.idl
modules/libpr0n/public/imgIRequest.idl
modules/libpr0n/src/Image.cpp
modules/libpr0n/src/Image.h
modules/libpr0n/src/RasterImage.cpp
modules/libpr0n/src/RasterImage.h
modules/libpr0n/src/imgRequest.cpp
modules/libpr0n/src/imgRequestProxy.cpp
modules/libpr0n/src/imgRequestProxy.h
--- a/content/base/src/nsDocument.cpp
+++ b/content/base/src/nsDocument.cpp
@@ -1522,16 +1522,17 @@ nsDOMImplementation::CreateHTMLDocument(
 // =
 // ==================================================================
 
   // NOTE! nsDocument::operator new() zeroes out all members, so don't
   // bother initializing members to 0.
 
 nsDocument::nsDocument(const char* aContentType)
   : nsIDocument()
+  , mAnimatingImages(PR_TRUE)
 {
   SetContentTypeInternal(nsDependentCString(aContentType));
   
 #ifdef PR_LOGGING
   if (!gDocumentLeakPRLog)
     gDocumentLeakPRLog = PR_NewLogModule("DocumentLeak");
 
   if (gDocumentLeakPRLog)
@@ -7374,16 +7375,21 @@ nsDocument::OnPageShow(PRBool aPersisted
     mIsShowing = PR_TRUE;
   }
  
 #ifdef MOZ_SMIL
   if (mAnimationController) {
     mAnimationController->OnPageShow();
   }
 #endif
+
+  if (aPersisted) {
+    SetImagesNeedAnimating(PR_TRUE);
+  }
+
   nsCOMPtr<nsPIDOMEventTarget> target =
     aDispatchStartTarget ? do_QueryInterface(aDispatchStartTarget) :
                            do_QueryInterface(GetWindow());
   DispatchPageTransition(target, NS_LITERAL_STRING("pageshow"), aPersisted);
 }
 
 static PRBool
 NotifyPageHide(nsIDocument* aDocument, void* aData)
@@ -7424,16 +7430,20 @@ nsDocument::OnPageHide(PRBool aPersisted
   }
 
 #ifdef MOZ_SMIL
   if (mAnimationController) {
     mAnimationController->OnPageHide();
   }
 #endif
   
+  if (aPersisted) {
+    SetImagesNeedAnimating(PR_FALSE);
+  }
+
   // Now send out a PageHide event.
   nsCOMPtr<nsPIDOMEventTarget> target =
     aDispatchStartTarget ? do_QueryInterface(aDispatchStartTarget) :
                            do_QueryInterface(GetWindow());
   DispatchPageTransition(target, NS_LITERAL_STRING("pagehide"), aPersisted);
 
   mVisible = PR_FALSE;
   EnumerateExternalResources(NotifyPageHide, &aPersisted);
@@ -8088,17 +8098,24 @@ nsDocument::AddImage(imgIRequest* aImage
   if (!success)
     return NS_ERROR_OUT_OF_MEMORY;
 
   // If this is the first insertion and we're locking images, lock this image
   // too.
   if ((oldCount == 0) && mLockingImages) {
     nsresult rv = aImage->LockImage();
     NS_ENSURE_SUCCESS(rv, rv);
-    return aImage->RequestDecode();
+    rv = aImage->RequestDecode();
+    NS_ENSURE_SUCCESS(rv, rv);
+  }
+
+  // If this is the first insertion and we're animating images, request
+  // that this image be animated too.
+  if (oldCount == 0 && mAnimatingImages) {
+    return aImage->IncrementAnimationConsumers();
   }
 
   return NS_OK;
 }
 
 nsresult
 nsDocument::RemoveImage(imgIRequest* aImage)
 {
@@ -8121,16 +8138,21 @@ nsDocument::RemoveImage(imgIRequest* aIm
     mImageTracker.Put(aImage, count);
   }
 
   // If we removed the image from the tracker and we're locking images, unlock
   // this image.
   if ((count == 0) && mLockingImages)
     return aImage->UnlockImage();
 
+  // If we removed the image from the tracker and we're animating images,
+  // remove our request to animate this image.
+  if (count == 0 && mAnimatingImages)
+    return aImage->DecrementAnimationConsumers();
+
   return NS_OK;
 }
 
 PLDHashOperator LockEnumerator(imgIRequest* aKey,
                                PRUint32 aData,
                                void*    userArg)
 {
   aKey->LockImage();
@@ -8159,8 +8181,40 @@ nsDocument::SetImageLockingState(PRBool 
                                       : UnlockEnumerator,
                               nsnull);
 
   // Update state.
   mLockingImages = aLocked;
 
   return NS_OK;
 }
+
+PLDHashOperator IncrementAnimationEnumerator(imgIRequest* aKey,
+                                             PRUint32 aData,
+                                             void*    userArg)
+{
+  aKey->IncrementAnimationConsumers();
+  return PL_DHASH_NEXT;
+}
+
+PLDHashOperator DecrementAnimationEnumerator(imgIRequest* aKey,
+                                             PRUint32 aData,
+                                             void*    userArg)
+{
+  aKey->DecrementAnimationConsumers();
+  return PL_DHASH_NEXT;
+}
+
+void
+nsDocument::SetImagesNeedAnimating(PRBool aAnimating)
+{
+  // If there's no change, there's nothing to do.
+  if (mAnimatingImages == aAnimating)
+    return;
+
+  // Otherwise, iterate over our images and perform the appropriate action.
+  mImageTracker.EnumerateRead(aAnimating ? IncrementAnimationEnumerator
+                                         : DecrementAnimationEnumerator,
+                              nsnull);
+
+  // Update state.
+  mAnimatingImages = aAnimating;
+}
--- a/content/base/src/nsDocument.h
+++ b/content/base/src/nsDocument.h
@@ -1139,16 +1139,19 @@ protected:
   // This flag is only set in nsXMLDocument, for e.g. documents used in XBL. We
   // don't want animations to play in such documents, so we need to store the
   // flag here so that we can check it in nsDocument::GetAnimationController.
   PRPackedBool mLoadedAsInteractiveData:1;
 
   // Whether we're currently holding a lock on all of our images.
   PRPackedBool mLockingImages:1;
 
+  // Whether we currently require our images to animate
+  PRPackedBool mAnimatingImages:1;
+
   PRUint8 mXMLDeclarationBits;
 
   PRUint8 mDefaultElementType;
 
   nsInterfaceHashtable<nsVoidPtrHashKey, nsPIBoxObject> *mBoxObjectTable;
 
   // The channel that got passed to StartDocumentLoad(), if any
   nsCOMPtr<nsIChannel> mChannel;
@@ -1245,16 +1248,22 @@ private:
 
   // Tracking for images in the document.
   nsDataHashtable< nsPtrHashKey<imgIRequest>, PRUint32> mImageTracker;
 
 #ifdef DEBUG
 protected:
   PRBool mWillReparent;
 #endif
+
+protected:
+  // Makes the images on this document capable of having their animation
+  // active or suspended. An Image will animate as long as at least one of its
+  // owning Documents needs it to animate; otherwise it can suspend.
+  void SetImagesNeedAnimating(PRBool aAnimating);
 };
 
 #define NS_DOCUMENT_INTERFACE_TABLE_BEGIN(_class)                             \
   NS_NODE_OFFSET_AND_INTERFACE_TABLE_BEGIN(_class)                            \
   NS_INTERFACE_TABLE_ENTRY_AMBIGUOUS(_class, nsIDOMDocument, nsDocument)      \
   NS_INTERFACE_TABLE_ENTRY_AMBIGUOUS(_class, nsIDOMNSDocument, nsDocument)    \
   NS_INTERFACE_TABLE_ENTRY_AMBIGUOUS(_class, nsIDOMDocumentEvent, nsDocument) \
   NS_INTERFACE_TABLE_ENTRY_AMBIGUOUS(_class, nsIDOMDocumentView, nsDocument)  \
--- a/layout/base/nsImageLoader.cpp
+++ b/layout/base/nsImageLoader.cpp
@@ -142,18 +142,16 @@ NS_IMETHODIMP nsImageLoader::OnStartCont
   NS_ABORT_IF_FALSE(aImage, "Who's calling us then?");
 
   /* Get requested animation policy from the pres context:
    *   normal = 0
    *   one frame = 1
    *   one loop = 2
    */
   aImage->SetAnimationMode(mFrame->PresContext()->ImageAnimationMode());
-  // Ensure the animation (if any) is started.
-  aImage->StartAnimation();
 
   return NS_OK;
 }
 
 NS_IMETHODIMP nsImageLoader::OnStopFrame(imgIRequest *aRequest,
                                          PRUint32 aFrame)
 {
   if (!mFrame)
--- a/layout/generic/nsBulletFrame.cpp
+++ b/layout/generic/nsBulletFrame.cpp
@@ -1463,19 +1463,20 @@ NS_IMETHODIMP nsBulletFrame::OnStartCont
     if (shell) {
       shell->FrameNeedsReflow(this, nsIPresShell::eStyleChange,
                               NS_FRAME_IS_DIRTY);
     }
   }
 
   // Handle animations
   aImage->SetAnimationMode(presContext->ImageAnimationMode());
-  // Ensure the animation (if any) is started.
-  aImage->StartAnimation();
-
+  // Ensure the animation (if any) is started. Note: There is no
+  // corresponding call to Decrement for this. This Increment will be
+  // 'cleaned up' by the Request when it is destroyed, but only then.
+  aRequest->IncrementAnimationConsumers();
   
   return NS_OK;
 }
 
 NS_IMETHODIMP nsBulletFrame::OnDataAvailable(imgIRequest *aRequest,
                                              PRBool aCurrentFrame,
                                              const nsIntRect *aRect)
 {
--- a/layout/generic/nsImageFrame.cpp
+++ b/layout/generic/nsImageFrame.cpp
@@ -256,18 +256,16 @@ nsImageFrame::Init(nsIContent*      aCon
 
   // If we already have an image container, OnStartContainer won't be called
   // Set the animation mode here
   if (currentRequest) {
     nsCOMPtr<imgIContainer> image;
     currentRequest->GetImage(getter_AddRefs(image));
     if (image) {
       image->SetAnimationMode(aPresContext->ImageAnimationMode());
-      // Ensure the animation (if any) is started.
-      image->StartAnimation();
     }
   }
 
   return rv;
 }
 
 PRBool
 nsImageFrame::UpdateIntrinsicSize(imgIContainer* aImage)
@@ -475,18 +473,16 @@ nsImageFrame::OnStartContainer(imgIReque
 
   /* Get requested animation policy from the pres context:
    *   normal = 0
    *   one frame = 1
    *   one loop = 2
    */
   nsPresContext *presContext = PresContext();
   aImage->SetAnimationMode(presContext->ImageAnimationMode());
-  // Ensure the animation (if any) is started.
-  aImage->StartAnimation();
 
   if (IsPendingLoad(aRequest)) {
     // We don't care
     return NS_OK;
   }
   
   UpdateIntrinsicSize(aImage);
 
--- a/layout/xul/base/src/nsImageBoxFrame.cpp
+++ b/layout/xul/base/src/nsImageBoxFrame.cpp
@@ -504,18 +504,20 @@ nsImageBoxFrame::GetFrameName(nsAString&
 #endif
 
 
 NS_IMETHODIMP nsImageBoxFrame::OnStartContainer(imgIRequest *request,
                                                 imgIContainer *image)
 {
   NS_ENSURE_ARG_POINTER(image);
 
-  // Ensure the animation (if any) is started
-  image->StartAnimation();
+  // Ensure the animation (if any) is started. Note: There is no
+  // corresponding call to Decrement for this. This Increment will be
+  // 'cleaned up' by the Request when it is destroyed, but only then.
+  request->IncrementAnimationConsumers();
 
   nscoord w, h;
   image->GetWidth(&w);
   image->GetHeight(&h);
 
   mIntrinsicSize.SizeTo(nsPresContext::CSSPixelsToAppUnits(w),
                         nsPresContext::CSSPixelsToAppUnits(h));
 
--- a/layout/xul/base/src/tree/src/nsTreeImageListener.cpp
+++ b/layout/xul/base/src/tree/src/nsTreeImageListener.cpp
@@ -34,16 +34,17 @@
  * and other provisions required by the GPL or the LGPL. If you do not delete
  * the provisions above, a recipient may use your version of this file under
  * the terms of any one of the MPL, the GPL or the LGPL.
  *
  * ***** END LICENSE BLOCK ***** */
 
 #include "nsTreeImageListener.h"
 #include "nsITreeBoxObject.h"
+#include "imgIRequest.h"
 #include "imgIContainer.h"
 
 NS_IMPL_ISUPPORTS3(nsTreeImageListener, imgIDecoderObserver, imgIContainerObserver, nsITreeImageListener)
 
 nsTreeImageListener::nsTreeImageListener(nsITreeBoxObject* aTree)
   : mTree(aTree),
     mInvalidationSuppressed(PR_TRUE),
     mInvalidationArea(nsnull)
@@ -53,18 +54,20 @@ nsTreeImageListener::nsTreeImageListener
 nsTreeImageListener::~nsTreeImageListener()
 {
   delete mInvalidationArea;
 }
 
 NS_IMETHODIMP nsTreeImageListener::OnStartContainer(imgIRequest *aRequest,
                                                     imgIContainer *aImage)
 {
-  // Ensure the animation (if any) is started
-  aImage->StartAnimation();                                                     
+  // Ensure the animation (if any) is started. Note: There is no
+  // corresponding call to Decrement for this. This Increment will be
+  // 'cleaned up' by the Request when it is destroyed, but only then.
+  aRequest->IncrementAnimationConsumers();
   return NS_OK;
 }
 
 NS_IMETHODIMP nsTreeImageListener::OnDataAvailable(imgIRequest *aRequest,
                                                    PRBool aCurrentFrame,
                                                    const nsIntRect *aRect)
 {
   Invalidate();
--- a/modules/libpr0n/public/imgIContainer.idl
+++ b/modules/libpr0n/public/imgIContainer.idl
@@ -66,17 +66,17 @@ native gfxGraphicsFilter(gfxPattern::Gra
 /**
  * imgIContainer is the interface that represents an image. It allows
  * access to frames as Thebes surfaces, and permits users to extract subregions
  * as other imgIContainers. It also allows drawing of images on to Thebes
  * contexts.
  *
  * Internally, imgIContainer also manages animation of images.
  */
-[scriptable, uuid(362e5b5f-f677-49e0-9a3c-03249c794624)]
+[scriptable, uuid(8bb94fa2-f57a-482c-bef8-e0b0424b0b3c)]
 interface imgIContainer : nsISupports
 {
   /**
    * The width of the container rectangle.
    */
   readonly attribute PRInt32 width;
 
   /**
@@ -237,12 +237,10 @@ interface imgIContainer : nsISupports
    */
   const short kNormalAnimMode   = 0;
   const short kDontAnimMode     = 1;
   const short kLoopOnceAnimMode = 2;
 
   attribute unsigned short animationMode;
 
   /* Methods to control animation */
-  void startAnimation();
-  void stopAnimation();
   void resetAnimation();
 };
--- a/modules/libpr0n/public/imgIRequest.idl
+++ b/modules/libpr0n/public/imgIRequest.idl
@@ -47,17 +47,17 @@ interface nsIPrincipal;
 
 /**
  * imgIRequest interface
  *
  * @author Stuart Parmenter <stuart@mozilla.com>
  * @version 0.1
  * @see imagelib2
  */
-[scriptable, uuid(ebde51c9-cc11-4b6a-99c3-d7f568c7481b)]
+[scriptable, uuid(62f58f12-3076-44fc-a742-5b648dac21bc)]
 interface imgIRequest : nsIRequest
 {
   /**
    * the image container...
    * @return the image object associated with the request.
    * @attention NEED DOCS
    */
   readonly attribute imgIContainer image;
@@ -174,10 +174,24 @@ interface imgIRequest : nsIRequest
   void unlockImage();
 
   /**
    * If this request is for an animated image, the method creates a new
    * request which contains the current frame of the image.
    * Otherwise returns the same request.
    */
   imgIRequest getStaticRequest();
+
+  /**
+   * Requests that the image animate (if it has an animation).
+   *
+   * @see Image::IncrementAnimationConsumers for documentation of the underlying call.
+   */
+  void incrementAnimationConsumers();
+
+  /**
+   * Tell the image it can forget about a request that the image animate.
+   *
+   * @see Image::DecrementAnimationConsumers for documentation of the underlying call.
+   */
+  void decrementAnimationConsumers();
 };
 
--- a/modules/libpr0n/src/Image.cpp
+++ b/modules/libpr0n/src/Image.cpp
@@ -37,17 +37,19 @@
 
 #include "Image.h"
 
 namespace mozilla {
 namespace imagelib {
 
 // Constructor
 Image::Image(imgStatusTracker* aStatusTracker) :
-  mInitialized(PR_FALSE)
+  mAnimationConsumers(0),
+  mInitialized(PR_FALSE),
+  mAnimating(PR_FALSE)
 {
   if (aStatusTracker) {
     mStatusTracker = aStatusTracker;
     mStatusTracker->SetImage(this);
   } else {
     mStatusTracker = new imgStatusTracker(this);
   }
 }
@@ -93,11 +95,37 @@ Image::GetDecoderType(const char *aMimeT
 
   // Icon
   else if (!strcmp(aMimeType, "image/icon"))
     rv = eDecoderType_icon;
 
   return rv;
 }
 
+void
+Image::IncrementAnimationConsumers()
+{
+  mAnimationConsumers++;
+  EvaluateAnimation();
+}
+
+void
+Image::DecrementAnimationConsumers()
+{
+  NS_ABORT_IF_FALSE(mAnimationConsumers >= 1, "Invalid no. of animation consumers!");
+  mAnimationConsumers--;
+  EvaluateAnimation();
+}
+
+void
+Image::EvaluateAnimation()
+{
+  if (!mAnimating && ShouldAnimate()) {
+    nsresult rv = StartAnimation();
+    mAnimating = NS_SUCCEEDED(rv);
+  } else if (mAnimating && !ShouldAnimate()) {
+    StopAnimation();
+    mAnimating = PR_FALSE;
+  }
+}
 
 } // namespace imagelib
 } // namespace mozilla
--- a/modules/libpr0n/src/Image.h
+++ b/modules/libpr0n/src/Image.h
@@ -101,20 +101,43 @@ public:
     eDecoderType_jpeg    = 2,
     eDecoderType_bmp     = 3,
     eDecoderType_ico     = 4,
     eDecoderType_icon    = 5,
     eDecoderType_unknown = 6
   };
   static eDecoderType GetDecoderType(const char *aMimeType);
 
+  void IncrementAnimationConsumers();
+  void DecrementAnimationConsumers();
+#ifdef DEBUG
+  PRUint32 GetAnimationConsumers() { return mAnimationConsumers; }
+#endif
+
 protected:
   Image(imgStatusTracker* aStatusTracker);
 
+  /**
+   * Decides whether animation should or should not be happening,
+   * and makes sure the right thing is being done.
+   */
+  virtual void EvaluateAnimation();
+
+  virtual nsresult StartAnimation() = 0;
+  virtual nsresult StopAnimation() = 0;
+
   // Member data shared by all implementations of this abstract class
   nsAutoPtr<imgStatusTracker> mStatusTracker;
+  PRUint32                    mAnimationConsumers;
   PRPackedBool                mInitialized;   // Have we been initalized?
+  PRPackedBool                mAnimating;
+
+  /**
+   * Extended by child classes, if they have additional
+   * conditions for being able to animate
+   */
+  virtual PRBool ShouldAnimate() { return mAnimationConsumers > 0; }
 };
 
 } // namespace imagelib
 } // namespace mozilla
 
 #endif // MOZILLA_IMAGELIB_IMAGE_H_
--- a/modules/libpr0n/src/RasterImage.cpp
+++ b/modules/libpr0n/src/RasterImage.cpp
@@ -185,17 +185,18 @@ RasterImage::RasterImage(imgStatusTracke
   mDecodeOnDraw(PR_FALSE),
   mMultipart(PR_FALSE),
   mDiscardable(PR_FALSE),
   mHasSourceData(PR_FALSE),
   mDecoded(PR_FALSE),
   mHasBeenDecoded(PR_FALSE),
   mWorkerPending(PR_FALSE),
   mInDecoder(PR_FALSE),
-  mError(PR_FALSE)
+  mError(PR_FALSE),
+  mAnimationFinished(PR_FALSE)
 {
   // Set up the discard tracker node.
   mDiscardTrackerNode.curr = this;
   mDiscardTrackerNode.prev = mDiscardTrackerNode.next = nsnull;
 
   // Statistics
   num_containers++;
 }
@@ -797,21 +798,18 @@ RasterImage::InternalAddFrame(PRUint32 f
   // We only need to refresh that small area when Frame 0 comes around again
   nsIntRect frameRect = frame->GetRect();
   mAnim->firstFrameRefreshArea.UnionRect(mAnim->firstFrameRefreshArea, 
                                          frameRect);
   
   rv = InternalAddFrameHelper(framenum, frame.forget(), imageData, imageLength,
                               paletteData, paletteLength);
   
-  // If this is our second frame (We've just added our second frame above),
-  // count should now be 2.  This must be called after we AppendObject 
-  // because StartAnimation checks for > 1 frames
-  if (mFrames.Length() == 2)
-    StartAnimation();
+  // We may be able to start animating, if we now have enough frames
+  EvaluateAnimation();
   
   return rv;
 }
 
 nsresult
 RasterImage::AppendFrame(PRInt32 aX, PRInt32 aY, PRInt32 aWidth,
                          PRInt32 aHeight,
                          gfxASurface::gfxImageFormat aFormat,
@@ -1094,86 +1092,69 @@ RasterImage::SetAnimationMode(PRUint16 a
   if (mError)
     return NS_ERROR_FAILURE;
 
   NS_ASSERTION(aAnimationMode == kNormalAnimMode ||
                aAnimationMode == kDontAnimMode ||
                aAnimationMode == kLoopOnceAnimMode,
                "Wrong Animation Mode is being set!");
   
-  switch (mAnimationMode = aAnimationMode) {
-    case kDontAnimMode:
-      StopAnimation();
-      break;
-    case kNormalAnimMode:
-      if (mLoopCount != 0 || 
-          (mAnim && (mAnim->currentAnimationFrameIndex + 1 < mFrames.Length())))
-        StartAnimation();
-      break;
-    case kLoopOnceAnimMode:
-      if (mAnim && (mAnim->currentAnimationFrameIndex + 1 < mFrames.Length()))
-        StartAnimation();
-      break;
-  }
-  
+  mAnimationMode = aAnimationMode;
+
+  EvaluateAnimation();
+
   return NS_OK;
 }
 
 //******************************************************************************
-/* void startAnimation () */
-NS_IMETHODIMP
+/* void StartAnimation () */
+nsresult
 RasterImage::StartAnimation()
 {
   if (mError)
     return NS_ERROR_FAILURE;
 
-  if (mAnimationMode == kDontAnimMode || 
-      (mAnim && (mAnim->timer || mAnim->animating)))
-    return NS_OK;
+  NS_ABORT_IF_FALSE(ShouldAnimate(), "Should not animate!");
+
+  if (!ensureAnimExists())
+    return NS_ERROR_OUT_OF_MEMORY;
+
+  NS_ABORT_IF_FALSE(mAnim && !mAnim->timer, "Anim must exist and not have a timer yet");
   
-  if (mFrames.Length() > 1) {
-    if (!ensureAnimExists())
-      return NS_ERROR_OUT_OF_MEMORY;
-    
-    // Default timeout to 100: the timer notify code will do the right
-    // thing, so just get that started.
-    PRInt32 timeout = 100;
-    imgFrame *currentFrame = GetCurrentImgFrame();
-    if (currentFrame) {
-      timeout = currentFrame->GetTimeout();
-      if (timeout <= 0) // -1 means display this frame forever
-        return NS_OK;
+  // Default timeout to 100: the timer notify code will do the right
+  // thing, so just get that started.
+  PRInt32 timeout = 100;
+  imgFrame *currentFrame = GetCurrentImgFrame();
+  if (currentFrame) {
+    timeout = currentFrame->GetTimeout();
+    if (timeout < 0) { // -1 means display this frame forever
+      mAnimationFinished = PR_TRUE;
+      return NS_ERROR_ABORT;
     }
-    
-    mAnim->timer = do_CreateInstance("@mozilla.org/timer;1");
-    NS_ENSURE_TRUE(mAnim->timer, NS_ERROR_OUT_OF_MEMORY);
-    
-    // The only way animating becomes true is if the timer is created
-    mAnim->animating = PR_TRUE;
-    mAnim->timer->InitWithCallback(static_cast<nsITimerCallback*>(this),
-                                   timeout, nsITimer::TYPE_REPEATING_SLACK);
   }
   
+  mAnim->timer = do_CreateInstance("@mozilla.org/timer;1");
+  NS_ENSURE_TRUE(mAnim->timer, NS_ERROR_OUT_OF_MEMORY);
+  mAnim->timer->InitWithCallback(static_cast<nsITimerCallback*>(this),
+                                 timeout, nsITimer::TYPE_REPEATING_SLACK);
+  
   return NS_OK;
 }
 
 //******************************************************************************
 /* void stopAnimation (); */
-NS_IMETHODIMP
+nsresult
 RasterImage::StopAnimation()
 {
+  NS_ABORT_IF_FALSE(mAnimating, "Should be animating!");
+
   if (mError)
     return NS_ERROR_FAILURE;
 
-  if (mAnim) {
-    mAnim->animating = PR_FALSE;
-
-    if (!mAnim->timer)
-      return NS_OK;
-
+  if (mAnim->timer) {
     mAnim->timer->Cancel();
     mAnim->timer = nsnull;
   }
 
   return NS_OK;
 }
 
 //******************************************************************************
@@ -1183,36 +1164,33 @@ RasterImage::ResetAnimation()
 {
   if (mError)
     return NS_ERROR_FAILURE;
 
   if (mAnimationMode == kDontAnimMode || 
       !mAnim || mAnim->currentAnimationFrameIndex == 0)
     return NS_OK;
 
-  PRBool oldAnimating = mAnim->animating;
-
-  if (mAnim->animating) {
-    nsresult rv = StopAnimation();
-    NS_ENSURE_SUCCESS(rv, rv);
-  }
+  if (mAnimating)
+    StopAnimation();
 
   mAnim->lastCompositedFrameIndex = -1;
   mAnim->currentAnimationFrameIndex = 0;
 
   // Note - We probably want to kick off a redecode somewhere around here when
   // we fix bug 500402.
 
   // Update display if we were animating before
   nsCOMPtr<imgIContainerObserver> observer(do_QueryReferent(mObserver));
-  if (oldAnimating && observer)
+  if (mAnimating && observer)
     observer->FrameChanged(this, &(mAnim->firstFrameRefreshArea));
 
-  if (oldAnimating)
-    return StartAnimation();
+  if (ShouldAnimate())
+    StartAnimation();
+
   return NS_OK;
 }
 
 void
 RasterImage::SetLoopCount(PRInt32 aLoopCount)
 {
   if (mError)
     return;
@@ -1425,27 +1403,31 @@ RasterImage::SetSourceSizeHint(PRUint32 
 
 //******************************************************************************
 /* void notify(in nsITimer timer); */
 NS_IMETHODIMP
 RasterImage::Notify(nsITimer *timer)
 {
   // This should never happen since the timer is only set up in StartAnimation()
   // after mAnim is checked to exist.
-  NS_ENSURE_TRUE(mAnim, NS_ERROR_UNEXPECTED);
-  NS_ASSERTION(mAnim->timer == timer,
-               "RasterImage::Notify() called with incorrect timer");
-
-  if (!mAnim->animating || !mAnim->timer)
+  NS_ABORT_IF_FALSE(mAnim, "Need anim for Notify()");
+  NS_ABORT_IF_FALSE(timer, "Need timer for Notify()");
+  NS_ABORT_IF_FALSE(mAnim->timer == timer,
+                    "RasterImage::Notify() called with incorrect timer");
+
+  if (!mAnimating || !ShouldAnimate())
     return NS_OK;
 
   nsCOMPtr<imgIContainerObserver> observer(do_QueryReferent(mObserver));
   if (!observer) {
     // the imgRequest that owns us is dead, we should die now too.
-    StopAnimation();
+    NS_ABORT_IF_FALSE(mAnimationConsumers == 0,
+                      "If no observer, should have no consumers");
+    if (mAnimating)
+      StopAnimation();
     return NS_OK;
   }
 
   if (mFrames.Length() == 0)
     return NS_OK;
   
   imgFrame *nextFrame = nsnull;
   PRInt32 previousFrameIndex = mAnim->currentAnimationFrameIndex;
@@ -1458,17 +1440,18 @@ RasterImage::Notify(nsITimer *timer)
   // finished decoding (see EndFrameDecode)
   if (mAnim->doneDecoding || 
       (nextFrameIndex < mAnim->currentDecodingFrameIndex)) {
     if (mFrames.Length() == nextFrameIndex) {
       // End of Animation
 
       // If animation mode is "loop once", it's time to stop animating
       if (mAnimationMode == kLoopOnceAnimMode || mLoopCount == 0) {
-        StopAnimation();
+        mAnimationFinished = PR_TRUE;
+        EvaluateAnimation();
         return NS_OK;
       } else {
         // We may have used compositingFrame to build a frame, and then copied
         // it back into mFrames[..].  If so, delete composite to save memory
         if (mAnim->compositingFrame && mAnim->lastCompositedFrameIndex == -1)
           mAnim->compositingFrame = nsnull;
       }
 
@@ -1501,18 +1484,20 @@ RasterImage::Notify(nsITimer *timer)
       mAnim->timer->SetDelay(100);
       return NS_OK;
     }
     timeout = nextFrame->GetTimeout();
   }
 
   if (timeout > 0)
     mAnim->timer->SetDelay(timeout);
-  else
-    StopAnimation();
+  else {
+    mAnimationFinished = PR_TRUE;
+    EvaluateAnimation();
+  }
 
   nsIntRect dirtyRect;
   imgFrame *frameToUse = nsnull;
 
   if (nextFrameIndex == 0) {
     frameToUse = nextFrame;
     dirtyRect = mAnim->firstFrameRefreshArea;
   } else {
@@ -2735,10 +2720,17 @@ RasterImage::WriteToRasterImage(nsIInput
   (void) image->AddSourceData(aFromRawSegment, aCount);
 
   // We wrote everything we got
   *aWriteCount = aCount;
 
   return NS_OK;
 }
 
+PRBool
+RasterImage::ShouldAnimate()
+{
+  return Image::ShouldAnimate() && mFrames.Length() >= 2 &&
+         mAnimationMode != kDontAnimMode && !mAnimationFinished;
+}
+
 } // namespace imagelib
 } // namespace mozilla
--- a/modules/libpr0n/src/RasterImage.h
+++ b/modules/libpr0n/src/RasterImage.h
@@ -81,18 +81,18 @@ class nsIInputStream;
  *
  *
  * @par A Quick Walk Through
  * The decoder initializes this class and calls AppendFrame() to add a frame.
  * Once RasterImage detects more than one frame, it starts the animation
  * with StartAnimation().
  *
  * @par
- * StartAnimation() checks if animating is allowed, and creates a timer.  The
- * timer calls Notify when the specified frame delay time is up.
+ * StartAnimation() creates a timer.  The timer calls Notify when the
+ * specified frame delay time is up.
  *
  * @par
  * Notify() moves on to the next frame, sets up the new timer delay, destroys
  * the old frame, and forces a redraw via observer->FrameChanged().
  *
  * @par
  * Each frame can have a different method of removing itself. These are
  * listed as imgIContainer::cDispose... constants.  Notify() calls 
@@ -154,16 +154,19 @@ public:
   NS_DECL_ISUPPORTS
   NS_DECL_IMGICONTAINER
   NS_DECL_NSITIMERCALLBACK
   NS_DECL_NSIPROPERTIES
 
   RasterImage(imgStatusTracker* aStatusTracker = nsnull);
   virtual ~RasterImage();
 
+  virtual nsresult StartAnimation();
+  virtual nsresult StopAnimation();
+
   // C++-only version of imgIContainer::GetType, for convenience
   virtual PRUint16 GetType() { return imgIContainer::TYPE_RASTER; }
 
   // Methods inherited from Image
   nsresult Init(imgIDecoderObserver* aObserver,
                 const char* aMimeType,
                 PRUint32 aFlags);
   void     GetCurrentFrameRect(nsIntRect& aRect);
@@ -321,26 +324,23 @@ private:
      * when it's done with the current frame.
      */
     nsAutoPtr<imgFrame>        compositingPrevFrame;
     //! Timer to animate multiframed images
     nsCOMPtr<nsITimer>         timer;
     //! Whether we can assume there will be no more frames
     //! (and thus loop the animation)
     PRPackedBool               doneDecoding;
-    //! Are we currently animating the image?
-    PRPackedBool               animating;
 
     Anim() :
       firstFrameRefreshArea(),
       currentDecodingFrameIndex(0),
       currentAnimationFrameIndex(0),
       lastCompositedFrameIndex(-1),
-      doneDecoding(PR_FALSE),
-      animating(PR_FALSE)
+      doneDecoding(PR_FALSE)
     {
       ;
     }
     ~Anim()
     {
       if (timer)
         timer->Cancel();
     }
@@ -485,16 +485,20 @@ private: // data
   PRPackedBool               mHasBeenDecoded:1;
 
   // Helpers for decoder
   PRPackedBool               mWorkerPending:1;
   PRPackedBool               mInDecoder:1;
 
   PRPackedBool               mError:1;  // Error handling
 
+  // Whether the animation can stop, due to running out
+  // of frames, or no more owning request
+  PRPackedBool               mAnimationFinished:1;
+
   // Decoding
   nsresult WantDecodedFrames();
   nsresult SyncDecode();
   nsresult InitDecoder(bool aDoSizeDecode);
   nsresult WriteToDecoder(const char *aBuffer, PRUint32 aCount);
   nsresult DecodeSomeData(PRUint32 aMaxBytes);
   PRBool   IsDecodeFinished();
 
@@ -508,16 +512,18 @@ private: // data
   nsresult ShutdownDecoder(eShutdownIntent aIntent);
 
   // Helpers
   void DoError();
   PRBool CanDiscard();
   PRBool DiscardingActive();
   PRBool StoringSourceData();
 
+protected:
+  PRBool ShouldAnimate();
 };
 
 // XXXdholbert These helper classes should move to be inside the
 // scope of the RasterImage class.
 // Decoding Helper Class
 //
 // We use this class to mimic the interactivity benefits of threading
 // in a single-threaded event loop. We want to progressively decode
--- a/modules/libpr0n/src/imgRequest.cpp
+++ b/modules/libpr0n/src/imgRequest.cpp
@@ -306,32 +306,40 @@ nsresult imgRequest::AddProxy(imgRequest
   return mObservers.AppendElementUnlessExists(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);
 
+  NS_ABORT_IF_FALSE(!mImage || HaveProxyWithObserver(nsnull) ||
+                    mImage->GetAnimationConsumers() == 0,
+    "How can we have an image with animation consumers, but no observer?");
+
+  // This will remove our animation consumers, so after removing
+  // this proxy, we don't end up without proxies with observers, but still
+  // have animation consumers.
+  proxy->ClearAnimationConsumers();
+
   mObservers.RemoveElement(proxy);
 
+  // Check that after our changes, we are still in a consistent state
+  NS_ABORT_IF_FALSE(!mImage || HaveProxyWithObserver(nsnull) ||
+                    mImage->GetAnimationConsumers() == 0,
+    "How can we have an image with animation consumers, but no observer?");
+
   // Let the status tracker do its thing before we potentially call Cancel()
   // below, because Cancel() may result in OnStopRequest being called back
   // before Cancel() returns, leaving the image in a different state then the
   // one it was in at this point.
 
   imgStatusTracker& statusTracker = GetStatusTracker();
   statusTracker.EmulateRequestFinished(proxy, aStatus, !aNotify);
 
-  if (mImage && !HaveProxyWithObserver(nsnull)) {
-    LOG_MSG(gImgLog, "imgRequest::RemoveProxy", "stopping animation");
-
-    mImage->StopAnimation();
-  }
-
   if (mObservers.IsEmpty()) {
     // If we have no observers, there's nothing holding us alive. If we haven't
     // been cancelled and thus removed from the cache, tell the image loader so
     // we can be evicted from the cache.
     if (mCacheEntry) {
       NS_ABORT_IF_FALSE(mKeyURI, "Removing last observer without key uri.");
 
       imgLoader::SetHasNoProxies(mKeyURI, mCacheEntry);
@@ -389,21 +397,16 @@ void imgRequest::CancelAndAbort(nsresult
 }
 
 void imgRequest::Cancel(nsresult aStatus)
 {
   /* The Cancel() method here should only be called by this class. */
 
   LOG_SCOPE(gImgLog, "imgRequest::Cancel");
 
-  LOG_MSG(gImgLog, "imgRequest::Cancel", "stopping animation");
-  if (mImage) {
-    mImage->StopAnimation();
-  }
-
   imgStatusTracker& statusTracker = GetStatusTracker();
   statusTracker.RecordCancel();
 
   RemoveFromCache();
 
   if (mRequest && statusTracker.IsLoading())
     mRequest->Cancel(aStatus);
 }
--- a/modules/libpr0n/src/imgRequestProxy.cpp
+++ b/modules/libpr0n/src/imgRequestProxy.cpp
@@ -63,16 +63,17 @@ NS_IMPL_ISUPPORTS4(imgRequestProxy, imgI
 imgRequestProxy::imgRequestProxy() :
   mOwner(nsnull),
   mURI(nsnull),
   mImage(nsnull),
   mPrincipal(nsnull),
   mListener(nsnull),
   mLoadFlags(nsIRequest::LOAD_NORMAL),
   mLockCount(0),
+  mAnimationConsumers(0),
   mCanceled(PR_FALSE),
   mIsInLoadGroup(PR_FALSE),
   mListenerIsStrongRef(PR_FALSE),
   mDecodeRequested(PR_FALSE),
   mDeferNotifications(PR_FALSE),
   mSentStartContainer(PR_FALSE)
 {
   /* member initializers and constructor code */
@@ -84,16 +85,18 @@ imgRequestProxy::~imgRequestProxy()
   /* destructor code */
   NS_PRECONDITION(!mListener, "Someone forgot to properly cancel this request!");
 
   // Unlock the image the proper number of times if we're holding locks on it.
   // Note that UnlockImage() decrements mLockCount each time it's called.
   while (mLockCount)
     UnlockImage();
 
+  ClearAnimationConsumers();
+
   // 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
   // above assert.
   NullOutListener();
 
   if (mOwner) {
     if (!mCanceled) {
@@ -114,16 +117,18 @@ imgRequestProxy::~imgRequestProxy()
 
 nsresult imgRequestProxy::Init(imgRequest* request, nsILoadGroup* aLoadGroup, Image* aImage,
                                nsIURI* aURI, imgIDecoderObserver* aObserver)
 {
   NS_PRECONDITION(!mOwner && !mListener, "imgRequestProxy is already initialized");
 
   LOG_SCOPE_WITH_PARAM(gImgLog, "imgRequestProxy::Init", "request", request);
 
+  NS_ABORT_IF_FALSE(mAnimationConsumers == 0, "Cannot have animation before Init");
+
   mOwner = request;
   mListener = aObserver;
   // Make sure to addref mListener before the AddProxy call below, since
   // that call might well want to release it if the imgRequest has
   // already seen OnStopRequest.
   if (mListener) {
     mListenerIsStrongRef = PR_TRUE;
     NS_ADDREF(mListener);
@@ -144,24 +149,32 @@ nsresult imgRequestProxy::ChangeOwner(im
   NS_PRECONDITION(mOwner, "Cannot ChangeOwner on a proxy without an owner!");
 
   // If we're holding locks, unlock the old image.
   // Note that UnlockImage decrements mLockCount each time it's called.
   PRUint32 oldLockCount = mLockCount;
   while (mLockCount)
     UnlockImage();
 
+  // If we're holding animation requests, undo them.
+  PRUint32 oldAnimationConsumers = mAnimationConsumers;
+  ClearAnimationConsumers();
+
   // Even if we are cancelled, we MUST change our image, because the image
   // holds our status, and the status must always be correct.
   mImage = aNewOwner->mImage;
 
   // If we were locked, apply the locks here
   for (PRUint32 i = 0; i < oldLockCount; i++)
     LockImage();
 
+  // If we had animation requests, apply them here
+  for (PRUint32 i = 0; i < oldAnimationConsumers; i++)
+    IncrementAnimationConsumers();
+
   if (mCanceled)
     return NS_OK;
 
   // Were we decoded before?
   PRBool wasDecoded = PR_FALSE;
   if (mImage &&
       (mImage->GetStatusTracker().GetImageStatus() &
        imgIRequest::STATUS_FRAME_COMPLETE)) {
@@ -329,16 +342,64 @@ imgRequestProxy::UnlockImage()
   NS_ABORT_IF_FALSE(mLockCount > 0, "calling unlock but no locks!");
 
   mLockCount--;
   if (mImage)
     return mImage->UnlockImage();
   return NS_OK;
 }
 
+NS_IMETHODIMP
+imgRequestProxy::IncrementAnimationConsumers()
+{
+  // Without an observer, we should not animate
+  if (!HasObserver())
+    return NS_OK;
+
+  mAnimationConsumers++;
+  if (mImage)
+    mImage->IncrementAnimationConsumers();
+  return NS_OK;
+}
+
+NS_IMETHODIMP
+imgRequestProxy::DecrementAnimationConsumers()
+{
+  if (!HasObserver()) {
+    NS_ABORT_IF_FALSE(mAnimationConsumers == 0,
+                      "How can we have animation consumers without an observer?");
+
+    // Without an observer, we have no consumers anyhow
+    return NS_OK;
+  }
+
+  // We may get here if some responsible code called Increment,
+  // then called us, but we have meanwhile called ClearAnimationConsumers
+  // because we needed to get rid of them earlier (see
+  // imgRequest::RemoveProxy), and hence have nothing left to
+  // decrement. (In such a case we got rid of the animation consumers
+  // early, but not the observer.)
+  if (mAnimationConsumers > 0) {
+    mAnimationConsumers--;
+    if (mImage)
+      mImage->DecrementAnimationConsumers();
+  }
+  return NS_OK;
+}
+
+void
+imgRequestProxy::ClearAnimationConsumers()
+{
+  NS_ABORT_IF_FALSE(HasObserver() || mAnimationConsumers == 0,
+                    "How can we have animation consumers without an observer?");
+
+  while (mAnimationConsumers > 0)
+    DecrementAnimationConsumers();
+}
+
 /* void suspend (); */
 NS_IMETHODIMP imgRequestProxy::Suspend()
 {
     return NS_ERROR_NOT_IMPLEMENTED;
 }
 
 /* void resume (); */
 NS_IMETHODIMP imgRequestProxy::Resume()
@@ -686,16 +747,20 @@ void imgRequestProxy::OnStopRequest(PRBo
     imgIDecoderObserver* obs = mListener;
     mListenerIsStrongRef = PR_FALSE;
     NS_RELEASE(obs);
   }
 }
 
 void imgRequestProxy::NullOutListener()
 {
+  // If we have animation consumers, then they don't matter anymore
+  if (mListener)
+    ClearAnimationConsumers();
+
   if (mListenerIsStrongRef) {
     // Releasing could do weird reentery stuff, so just play it super-safe
     nsCOMPtr<imgIDecoderObserver> obs;
     obs.swap(mListener);
     mListenerIsStrongRef = PR_FALSE;
   } else {
     mListener = nsnull;
   }
@@ -778,16 +843,20 @@ imgRequestProxy::SetImage(Image* aImage)
   NS_ABORT_IF_FALSE(aImage,  "Setting null image");
   NS_ABORT_IF_FALSE(!mImage, "Setting image when we already have one");
 
   mImage = aImage;
 
   // Apply any locks we have
   for (PRUint32 i = 0; i < mLockCount; ++i)
     mImage->LockImage();
+
+  // Apply any animation consumers we have
+  for (PRUint32 i = 0; i < mAnimationConsumers; i++)
+    mImage->IncrementAnimationConsumers();
 }
 
 imgStatusTracker&
 imgRequestProxy::GetStatusTracker()
 {
   // NOTE: It's possible that our mOwner has an Image that it didn't notify
   // us about, if we were Canceled before its Image was constructed.
   // (Canceling removes us as an observer, so mOwner has no way to notify us).
--- a/modules/libpr0n/src/imgRequestProxy.h
+++ b/modules/libpr0n/src/imgRequestProxy.h
@@ -122,16 +122,22 @@ public:
   {
     mDeferNotifications = aDeferNotifications;
   }
 
   // Setter for our |mImage| pointer, for imgRequest to use, once it
   // instantiates an Image.
   void SetImage(mozilla::imagelib::Image* aImage);
 
+  // Removes all animation consumers that were created with
+  // IncrementAnimationConsumers. This is necessary since we need
+  // to do it before the proxy itself is destroyed. See
+  // imgRequest::RemoveProxy
+  void ClearAnimationConsumers();
+
 protected:
   friend class imgStatusTracker;
   friend class imgStatusNotifyRunnable;
   friend class imgRequestNotifyRunnable;
 
   class imgCancelRunnable;
   friend class imgCancelRunnable;
 
@@ -215,16 +221,17 @@ private:
   // mListener is only promised to be a weak ref (see imgILoader.idl),
   // but we actually keep a strong ref to it until we've seen our
   // first OnStopRequest.
   imgIDecoderObserver* mListener;
   nsCOMPtr<nsILoadGroup> mLoadGroup;
 
   nsLoadFlags mLoadFlags;
   PRUint32    mLockCount;
+  PRUint32    mAnimationConsumers;
   PRPackedBool mCanceled;
   PRPackedBool mIsInLoadGroup;
   PRPackedBool mListenerIsStrongRef;
   PRPackedBool mDecodeRequested;
 
   // Whether we want to defer our notifications by the non-virtual Observer
   // interfaces as image loads proceed.
   PRPackedBool mDeferNotifications;