Closed Bug 1100725 Opened 6 years ago Closed 6 years ago

Ensure that we always consume all the progress and invalidations a Decoder generates

Categories

(Core :: ImageLib, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: seth, Assigned: seth)

References

Details

Attachments

(4 files, 3 obsolete files)

Right now there are situations where we may destroy a Decoder without consuming all of the progress and invalidations it generates. When this happens, it's almost guaranteed to lead to a bug.

Let's fix the situations we currently know about, and then add assertions to discover any other ways this problem might manifest.
So one way this problem can happen is if we shut down the decoder in RasterImage::DoImageDataComplete. We don't grab its progress there, like we do in RasterImage::FinishedSomeDecoding, so we can miss changes. We *could* just grab the progress there and return it to OnImageDataComplete, but I actually don't think the code in question is necessary at all. FinishedSomeDecoding should also do the job of shutting down the decoder, and it does it right. It's easier to reason about this stuff if we only shut down the decoder in one place, so I think the best fix here is just to delete the code that shuts down the decoder and requests a new decode, and let FinishedSomeDecoding handle it.
Attachment #8524258 - Flags: review?(tnikkel)
I found another problem in nsICODecoder. (What else is new; half of my bugs come from nsICODecoder, padding, or multipart/x-mixed-replace.) nsICODecoder needs to consume the contained decoder's invalidations.
Attachment #8524259 - Flags: review?(tnikkel)
Parts 1 and 2 cover the bugs I'm aware of so far. But we still need to add assertions to make sure there aren't any more issues lurking. This part adds those assertions. To do that, it replaces Decoder::GetProgress with Decoder::TakeProgress, which resets Decoder::mProgress to NoChange afterwards. This makes it possible to assert in Decoder's destructor that we actually consumed all the progress changes and invalidations it generated.
Attachment #8524262 - Flags: review?(tnikkel)
Here's a try job. I cast a fairly wide net because if there are many more problems lurking, I want to catch them.

https://tbpl.mozilla.org/?tree=Try&rev=76773efc1d45
The try job located another problem - we need to grab invalidations after shutting down the decoder in FinishedSomeDecoding. The BMP decoder in particular seems to love to produce invalidations in Decoder::Finish! The patch is trivial.
Attachment #8524331 - Flags: review?(tnikkel)
Rebased part 3 and renumbered it to part 4.
Attachment #8524333 - Flags: review?(tnikkel)
Attachment #8524262 - Attachment is obsolete: true
Attachment #8524262 - Flags: review?(tnikkel)
Attachment #8524331 - Attachment is obsolete: true
Attachment #8524331 - Flags: review?(tnikkel)
Attachment #8524333 - Attachment is obsolete: true
Attachment #8524333 - Flags: review?(tnikkel)
Comment on attachment 8524258 [details] [diff] [review]
(Part 1) - Don't shut down decoder in DoImageDataComplete

The request decode that FinishedSomeDecoding does is only for full decodes that were requested during a size decode. The one in DoImageDataComplete is for size decodes that don't complete. But since we have all the source data, can that only happen when there is a decode error?
Attachment #8524259 - Flags: review?(tnikkel) → review+
Attachment #8524334 - Flags: review?(tnikkel) → review+
Attachment #8524335 - Flags: review?(tnikkel) → review+
Comment on attachment 8524258 [details] [diff] [review]
(Part 1) - Don't shut down decoder in DoImageDataComplete

I'll grant r+ so you can land sooner assuming that comment 10 is correct.
Attachment #8524258 - Flags: review?(tnikkel) → review+
Hmm, that try job was build-only. That's what I get for submitting try jobs late at night. =) Here's a proper try job:

https://tbpl.mozilla.org/?tree=Try&rev=2fd0f2d66f3e
(In reply to Timothy Nikkel (:tn) from comment #10)
> Comment on attachment 8524258 [details] [diff] [review]
> (Part 1) - Don't shut down decoder in DoImageDataComplete
> 
> The request decode that FinishedSomeDecoding does is only for full decodes
> that were requested during a size decode. The one in DoImageDataComplete is
> for size decodes that don't complete. But since we have all the source data,
> can that only happen when there is a decode error?

That's how it looks to me, yeah. It doesn't seem like, if DecodeUntilSizeAvailable fails in DoImageDataComplete, that RequestDecode is ever going to do anything useful.
Depends on: 1137615
You need to log in before you can comment on or make changes to this bug.