625237 - DecrementAnimationConsumers not always called in nsDocument::RemoveImage

Status: RESOLVED FIXED

(Core :: ImageLib, defect)

Description

[Alon Zakai (:azakai)]

Due to a silly bug with a return before it exiting the function.

Comment 1

[Alon Zakai (:azakai)]

Attachment: patch

Asking for approval since without this patch, we may run animations for images after they are no longer shown.

Comment 2

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Comment on attachment 503344 [details] [diff] [review]
patch

Should we DecrementAnimationConsumers even if UnlockImage fails?  Seems like we should....

Comment 3

[Alon Zakai (:azakai)]

If so then we should change the corresponding case in ::AddImage as well. It seems odd though, to do anything after we have a failure in Lock/UnlockImage. Also, what would the final rv be? If just one fails then that one I guess, but what if both fail?

Comment 4

[Boris Zbarsky [:bzbarsky, bz on IRC]]

> It seems odd though, to do anything after we have a failure in Lock/UnlockImage.

Well, it depends on whether unlock can fail in "sane" situations.  Can it?

> If just one fails then that one I guess, 

Right.

> but what if both fail?

"Pick one".  Callers probably don't care what the error code is much.  ;)

Comment 5

[Alon Zakai (:azakai)]

Attachment: patch v2

Made the requested changes.

Comment 6

[Boris Zbarsky [:bzbarsky, bz on IRC]]

Comment on attachment 504795 [details] [diff] [review]
patch v2

>@@ -8110,32 +8110,34 @@ nsDocument::AddImage(imgIRequest* aImage
>+  if ((oldCount == 0) && mAnimatingImages) {

Don't add the extra parens there, please.

>@@ -8150,27 +8152,31 @@ nsDocument::RemoveImage(imgIRequest* aIm
>+  if ((count == 0) && mAnimatingImages)

Same here.

r=me with those nits fixed.

Comment 7

[Alon Zakai (:azakai)]

Requesting blocking-fennec, since without this patch we may run animations for images after they are no longer shown, which is very bad for battery life (I have seen this happen while debugging another imagelib issue).

Comment 8

[Alon Zakai (:azakai)]

http://hg.mozilla.org/mozilla-central/rev/c2cef09c8c7b