Only lock during decoding for the initial decode

RESOLVED FIXED in Firefox 37

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: seth, Assigned: seth)

Tracking

(Blocks: 1 bug)

unspecified
mozilla38
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox37 fixed, firefox38 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

4 years ago
Bug 1079627 changed the timing of a lot of things in ImageLib, and it ended up revealing some interesting new failure modes. One potential problem that I discovered while testing it is that it is possible for an image to be discarding during the initial decode, before we discover that it is animated. (This is *very* unlikely, but I've seen it happen in the magical environment of the B2G emulators, where time has no meaning.) This causes us to assert later since we assume that if an image is animated, we never discard any of its frames.

Now, I'm not sure that there's really much keeping us from allowing discarding of animated images, but I don't want to make that change right now. So until we flip that on, we need to ensure that they don't get discarded during decoding, and so in bug 1079627 I made us lock the image while a decoder is running.

That causes problems for the patches in bug 1125490 and bug 1125462, though, because they want to discard even if an image is being redecoded because it was in a background tab that got brought back to the foreground.

The thing is, we don't really need to lock during decoding *every* time - just during the initial decode, when we're not sure if we're animated or not. This patch makes us lock the image only during the initial decode. It adds a flag on each decoder that lets us know whether we locked the image when creating that particular decoder; this is the safest way to ensure that the locks and unlocks are balanced.
(Assignee)

Comment 1

4 years ago
Created attachment 8554138 [details] [diff] [review]
Only lock the image during the initial decode

Here's the patch.
Attachment #8554138 - Flags: review?(tnikkel)
(Assignee)

Comment 2

4 years ago
Created attachment 8554139 [details] [diff] [review]
Only lock the image during the initial decode

Fixed a typo.
Attachment #8554139 - Flags: review?(tnikkel)
(Assignee)

Updated

4 years ago
Attachment #8554138 - Attachment is obsolete: true
Attachment #8554138 - Flags: review?(tnikkel)
Attachment #8554139 - Flags: review?(tnikkel) → review+
(Assignee)

Comment 3

4 years ago
Thanks for the review, Timothy. It's confirmed that this results in a bug improvement on AWSY; see bug 1122704 comment 7.

Try job here:

https://tbpl.mozilla.org/?tree=Try&rev=39ff99ddb6a7
(Assignee)

Comment 4

4 years ago
Heh, *big* improvement.
https://hg.mozilla.org/mozilla-central/rev/91836c912d09
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
This resulted in a ~60 MiB drop in the dark blue line ("RSS: After TP5, tabs closed") on AWSY. Hooray!
(Assignee)

Comment 8

4 years ago
Comment on attachment 8554139 [details] [diff] [review]
Only lock the image during the initial decode

Approval Request Comment
[Feature/regressing bug #]: Part of fix for bug 1125272 and bug 1119938.
[User impact if declined]: Already approved.
[Describe test coverage new/current, TreeHerder]: In 38 for a long time.
[Risks and why]: Low risk; in 38 for a long time.
[String/UUID change made/needed]: None.
Attachment #8554139 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 9

4 years ago
Pushed to Aurora:

https://hg.mozilla.org/releases/mozilla-aurora/rev/e19fc07f7163
status-firefox37: --- → fixed
status-firefox38: --- → fixed
Comment on attachment 8554139 [details] [diff] [review]
Only lock the image during the initial decode

Aurora approval previously granted for a collection of ImageLib related bugs that Seth worked on and QE verified before uplift.
Attachment #8554139 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.