Closed Bug 625237 Opened 9 years ago Closed 9 years ago

DecrementAnimationConsumers not always called in nsDocument::RemoveImage

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
fennec 2.0+ ---

People

(Reporter: azakai, Assigned: azakai)

Details

Attachments

(1 file, 1 obsolete file)

Due to a silly bug with a return before it exiting the function.
Attached patch patch (obsolete) — Splinter Review
Asking for approval since without this patch, we may run animations for images after they are no longer shown.
Attachment #503344 - Flags: review?(bzbarsky)
Attachment #503344 - Flags: approval2.0?
Comment on attachment 503344 [details] [diff] [review]
patch

Should we DecrementAnimationConsumers even if UnlockImage fails?  Seems like we should....
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?
> 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.  ;)
Attached patch patch v2Splinter Review
Made the requested changes.
Attachment #503344 - Attachment is obsolete: true
Attachment #504795 - Flags: review?(bzbarsky)
Attachment #503344 - Flags: review?(bzbarsky)
Attachment #503344 - Flags: approval2.0?
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.
Attachment #504795 - Flags: review?(bzbarsky) → review+
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).
tracking-fennec: --- → ?
tracking-fennec: ? → 2.0+
http://hg.mozilla.org/mozilla-central/rev/c2cef09c8c7b
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.