From f1741c27cf27099a330ea6bc2bd55f267c7a4f52 Mon Sep 17 00:00:00 2001 From: Alon Zakai Date: Tue, 7 Sep 2010 17:33:02 -0700 Subject: [PATCH] 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 | 56 +++++++- content/base/src/nsDocument.h | 9 ++ layout/base/nsImageLoader.cpp | 2 - layout/generic/nsBulletFrame.cpp | 7 +- layout/generic/nsImageFrame.cpp | 4 - layout/xul/base/src/nsImageBoxFrame.cpp | 6 +- .../base/src/tree/src/nsTreeImageListener.cpp | 7 +- modules/libpr0n/public/imgIContainer.idl | 4 +- modules/libpr0n/public/imgIRequest.idl | 16 ++- modules/libpr0n/src/Image.cpp | 30 +++- modules/libpr0n/src/Image.h | 23 +++ modules/libpr0n/src/RasterImage.cpp | 132 ++++++++---------- modules/libpr0n/src/RasterImage.h | 18 ++- modules/libpr0n/src/imgRequest.cpp | 25 ++-- modules/libpr0n/src/imgRequestProxy.cpp | 69 +++++++++ modules/libpr0n/src/imgRequestProxy.h | 7 + 16 files changed, 309 insertions(+), 106 deletions(-) diff --git a/content/base/src/nsDocument.cpp b/content/base/src/nsDocument.cpp index 1b84f1a27f5d..e50b14a24139 100644 --- a/content/base/src/nsDocument.cpp +++ b/content/base/src/nsDocument.cpp @@ -1527,6 +1527,7 @@ nsDOMImplementation::CreateHTMLDocument(const nsAString& aTitle, nsDocument::nsDocument(const char* aContentType) : nsIDocument() + , mAnimatingImages(PR_TRUE) { SetContentTypeInternal(nsDependentCString(aContentType)); @@ -7379,6 +7380,11 @@ nsDocument::OnPageShow(PRBool aPersisted, mAnimationController->OnPageShow(); } #endif + + if (aPersisted) { + SetImagesNeedAnimating(PR_TRUE); + } + nsCOMPtr target = aDispatchStartTarget ? do_QueryInterface(aDispatchStartTarget) : do_QueryInterface(GetWindow()); @@ -7429,6 +7435,10 @@ nsDocument::OnPageHide(PRBool aPersisted, } #endif + if (aPersisted) { + SetImagesNeedAnimating(PR_FALSE); + } + // Now send out a PageHide event. nsCOMPtr target = aDispatchStartTarget ? do_QueryInterface(aDispatchStartTarget) : @@ -8093,7 +8103,14 @@ nsDocument::AddImage(imgIRequest* aImage) 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; @@ -8126,6 +8143,11 @@ nsDocument::RemoveImage(imgIRequest* aImage) 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; } @@ -8164,3 +8186,35 @@ nsDocument::SetImageLockingState(PRBool 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; +} diff --git a/content/base/src/nsDocument.h b/content/base/src/nsDocument.h index 763e38737658..9b84561a9069 100644 --- a/content/base/src/nsDocument.h +++ b/content/base/src/nsDocument.h @@ -1144,6 +1144,9 @@ protected: // 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; @@ -1250,6 +1253,12 @@ private: 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) \ diff --git a/layout/base/nsImageLoader.cpp b/layout/base/nsImageLoader.cpp index f806b1a67cbf..f450a8e0421a 100644 --- a/layout/base/nsImageLoader.cpp +++ b/layout/base/nsImageLoader.cpp @@ -147,8 +147,6 @@ NS_IMETHODIMP nsImageLoader::OnStartContainer(imgIRequest *aRequest, * one loop = 2 */ aImage->SetAnimationMode(mFrame->PresContext()->ImageAnimationMode()); - // Ensure the animation (if any) is started. - aImage->StartAnimation(); return NS_OK; } diff --git a/layout/generic/nsBulletFrame.cpp b/layout/generic/nsBulletFrame.cpp index cd21cb3027f8..3f54a3598dd7 100644 --- a/layout/generic/nsBulletFrame.cpp +++ b/layout/generic/nsBulletFrame.cpp @@ -1468,9 +1468,10 @@ NS_IMETHODIMP nsBulletFrame::OnStartContainer(imgIRequest *aRequest, // 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; } diff --git a/layout/generic/nsImageFrame.cpp b/layout/generic/nsImageFrame.cpp index 7237bb8ee7ac..dd5d31a7d3c6 100644 --- a/layout/generic/nsImageFrame.cpp +++ b/layout/generic/nsImageFrame.cpp @@ -261,8 +261,6 @@ nsImageFrame::Init(nsIContent* aContent, currentRequest->GetImage(getter_AddRefs(image)); if (image) { image->SetAnimationMode(aPresContext->ImageAnimationMode()); - // Ensure the animation (if any) is started. - image->StartAnimation(); } } @@ -480,8 +478,6 @@ nsImageFrame::OnStartContainer(imgIRequest *aRequest, imgIContainer *aImage) */ nsPresContext *presContext = PresContext(); aImage->SetAnimationMode(presContext->ImageAnimationMode()); - // Ensure the animation (if any) is started. - aImage->StartAnimation(); if (IsPendingLoad(aRequest)) { // We don't care diff --git a/layout/xul/base/src/nsImageBoxFrame.cpp b/layout/xul/base/src/nsImageBoxFrame.cpp index 7cc786d0f941..1638f8861383 100644 --- a/layout/xul/base/src/nsImageBoxFrame.cpp +++ b/layout/xul/base/src/nsImageBoxFrame.cpp @@ -509,8 +509,10 @@ NS_IMETHODIMP nsImageBoxFrame::OnStartContainer(imgIRequest *request, { 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); diff --git a/layout/xul/base/src/tree/src/nsTreeImageListener.cpp b/layout/xul/base/src/tree/src/nsTreeImageListener.cpp index b944f55f4e5d..c99ecdb44b9d 100644 --- a/layout/xul/base/src/tree/src/nsTreeImageListener.cpp +++ b/layout/xul/base/src/tree/src/nsTreeImageListener.cpp @@ -39,6 +39,7 @@ #include "nsTreeImageListener.h" #include "nsITreeBoxObject.h" +#include "imgIRequest.h" #include "imgIContainer.h" NS_IMPL_ISUPPORTS3(nsTreeImageListener, imgIDecoderObserver, imgIContainerObserver, nsITreeImageListener) @@ -58,8 +59,10 @@ nsTreeImageListener::~nsTreeImageListener() 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; } diff --git a/modules/libpr0n/public/imgIContainer.idl b/modules/libpr0n/public/imgIContainer.idl index 477f9fb23e2e..e9df900b039c 100644 --- a/modules/libpr0n/public/imgIContainer.idl +++ b/modules/libpr0n/public/imgIContainer.idl @@ -71,7 +71,7 @@ native gfxGraphicsFilter(gfxPattern::GraphicsFilter); * * Internally, imgIContainer also manages animation of images. */ -[scriptable, uuid(362e5b5f-f677-49e0-9a3c-03249c794624)] +[scriptable, uuid(8bb94fa2-f57a-482c-bef8-e0b0424b0b3c)] interface imgIContainer : nsISupports { /** @@ -242,7 +242,5 @@ interface imgIContainer : nsISupports attribute unsigned short animationMode; /* Methods to control animation */ - void startAnimation(); - void stopAnimation(); void resetAnimation(); }; diff --git a/modules/libpr0n/public/imgIRequest.idl b/modules/libpr0n/public/imgIRequest.idl index 835f5d6662e6..bc644d081ced 100644 --- a/modules/libpr0n/public/imgIRequest.idl +++ b/modules/libpr0n/public/imgIRequest.idl @@ -52,7 +52,7 @@ interface nsIPrincipal; * @version 0.1 * @see imagelib2 */ -[scriptable, uuid(ebde51c9-cc11-4b6a-99c3-d7f568c7481b)] +[scriptable, uuid(62f58f12-3076-44fc-a742-5b648dac21bc)] interface imgIRequest : nsIRequest { /** @@ -179,5 +179,19 @@ interface imgIRequest : nsIRequest * 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(); }; diff --git a/modules/libpr0n/src/Image.cpp b/modules/libpr0n/src/Image.cpp index 75d0c4cba01f..604b9badd00a 100644 --- a/modules/libpr0n/src/Image.cpp +++ b/modules/libpr0n/src/Image.cpp @@ -42,7 +42,9 @@ namespace imagelib { // Constructor Image::Image(imgStatusTracker* aStatusTracker) : - mInitialized(PR_FALSE) + mAnimationConsumers(0), + mInitialized(PR_FALSE), + mAnimating(PR_FALSE) { if (aStatusTracker) { mStatusTracker = aStatusTracker; @@ -98,6 +100,32 @@ Image::GetDecoderType(const char *aMimeType) 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 diff --git a/modules/libpr0n/src/Image.h b/modules/libpr0n/src/Image.h index e3468760250f..75d3869a3949 100644 --- a/modules/libpr0n/src/Image.h +++ b/modules/libpr0n/src/Image.h @@ -106,12 +106,35 @@ public: }; 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 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 diff --git a/modules/libpr0n/src/RasterImage.cpp b/modules/libpr0n/src/RasterImage.cpp index 6ee6a2ed869b..c86739773f6e 100644 --- a/modules/libpr0n/src/RasterImage.cpp +++ b/modules/libpr0n/src/RasterImage.cpp @@ -190,7 +190,8 @@ RasterImage::RasterImage(imgStatusTracker* aStatusTracker) : 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; @@ -802,11 +803,8 @@ RasterImage::InternalAddFrame(PRUint32 framenum, 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; } @@ -1099,76 +1097,59 @@ RasterImage::SetAnimationMode(PRUint16 aAnimationMode) 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(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(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; } @@ -1188,12 +1169,8 @@ RasterImage::ResetAnimation() !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; @@ -1203,11 +1180,12 @@ RasterImage::ResetAnimation() // Update display if we were animating before nsCOMPtr 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; } @@ -1430,17 +1408,21 @@ 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"); + 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 (!mAnim->animating || !mAnim->timer) + if (!mAnimating || !ShouldAnimate()) return NS_OK; nsCOMPtr 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; } @@ -1463,7 +1445,8 @@ RasterImage::Notify(nsITimer *timer) // 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 @@ -1506,8 +1489,10 @@ RasterImage::Notify(nsITimer *timer) if (timeout > 0) mAnim->timer->SetDelay(timeout); - else - StopAnimation(); + else { + mAnimationFinished = PR_TRUE; + EvaluateAnimation(); + } nsIntRect dirtyRect; imgFrame *frameToUse = nsnull; @@ -2740,5 +2725,12 @@ RasterImage::WriteToRasterImage(nsIInputStream* /* unused */, return NS_OK; } +PRBool +RasterImage::ShouldAnimate() +{ + return Image::ShouldAnimate() && mFrames.Length() >= 2 && + mAnimationMode != kDontAnimMode && !mAnimationFinished; +} + } // namespace imagelib } // namespace mozilla diff --git a/modules/libpr0n/src/RasterImage.h b/modules/libpr0n/src/RasterImage.h index 240a15dac6ce..9c32deeede77 100644 --- a/modules/libpr0n/src/RasterImage.h +++ b/modules/libpr0n/src/RasterImage.h @@ -86,8 +86,8 @@ class nsIInputStream; * 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 @@ -159,6 +159,9 @@ public: 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; } @@ -326,16 +329,13 @@ private: //! 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) { ; } @@ -490,6 +490,10 @@ private: // data 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(); @@ -513,6 +517,8 @@ private: // data PRBool DiscardingActive(); PRBool StoringSourceData(); +protected: + PRBool ShouldAnimate(); }; // XXXdholbert These helper classes should move to be inside the diff --git a/modules/libpr0n/src/imgRequest.cpp b/modules/libpr0n/src/imgRequest.cpp index 4ef07f19c9c2..8b624790910b 100644 --- a/modules/libpr0n/src/imgRequest.cpp +++ b/modules/libpr0n/src/imgRequest.cpp @@ -311,8 +311,22 @@ nsresult imgRequest::RemoveProxy(imgRequestProxy *proxy, nsresult aStatus, PRBoo { 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 @@ -321,12 +335,6 @@ nsresult imgRequest::RemoveProxy(imgRequestProxy *proxy, nsresult aStatus, PRBoo 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 @@ -394,11 +402,6 @@ void imgRequest::Cancel(nsresult aStatus) LOG_SCOPE(gImgLog, "imgRequest::Cancel"); - LOG_MSG(gImgLog, "imgRequest::Cancel", "stopping animation"); - if (mImage) { - mImage->StopAnimation(); - } - imgStatusTracker& statusTracker = GetStatusTracker(); statusTracker.RecordCancel(); diff --git a/modules/libpr0n/src/imgRequestProxy.cpp b/modules/libpr0n/src/imgRequestProxy.cpp index 48fc0ad53bcc..ee95388dab61 100644 --- a/modules/libpr0n/src/imgRequestProxy.cpp +++ b/modules/libpr0n/src/imgRequestProxy.cpp @@ -68,6 +68,7 @@ imgRequestProxy::imgRequestProxy() : mListener(nsnull), mLoadFlags(nsIRequest::LOAD_NORMAL), mLockCount(0), + mAnimationConsumers(0), mCanceled(PR_FALSE), mIsInLoadGroup(PR_FALSE), mListenerIsStrongRef(PR_FALSE), @@ -89,6 +90,8 @@ imgRequestProxy::~imgRequestProxy() 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 @@ -119,6 +122,8 @@ nsresult imgRequestProxy::Init(imgRequest* request, nsILoadGroup* aLoadGroup, Im 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 @@ -149,6 +154,10 @@ nsresult imgRequestProxy::ChangeOwner(imgRequest *aNewOwner) 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; @@ -157,6 +166,10 @@ nsresult imgRequestProxy::ChangeOwner(imgRequest *aNewOwner) 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; @@ -334,6 +347,54 @@ imgRequestProxy::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() { @@ -691,6 +752,10 @@ void imgRequestProxy::OnStopRequest(PRBool lastPart) 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 obs; @@ -783,6 +848,10 @@ imgRequestProxy::SetImage(Image* 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& diff --git a/modules/libpr0n/src/imgRequestProxy.h b/modules/libpr0n/src/imgRequestProxy.h index 5f7e1c680843..80da9c3bf98d 100644 --- a/modules/libpr0n/src/imgRequestProxy.h +++ b/modules/libpr0n/src/imgRequestProxy.h @@ -127,6 +127,12 @@ public: // 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; @@ -220,6 +226,7 @@ private: nsLoadFlags mLoadFlags; PRUint32 mLockCount; + PRUint32 mAnimationConsumers; PRPackedBool mCanceled; PRPackedBool mIsInLoadGroup; PRPackedBool mListenerIsStrongRef;