Closed
Bug 1100725
Opened 10 years ago
Closed 10 years ago
Ensure that we always consume all the progress and invalidations a Decoder generates
Categories
(Core :: Graphics: ImageLib, defect)
Core
Graphics: ImageLib
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: seth, Assigned: seth)
References
Details
Attachments
(4 files, 3 obsolete files)
1.41 KB,
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
3.24 KB,
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
1.13 KB,
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
5.82 KB,
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
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)
Assignee | ||
Comment 2•10 years ago
|
||
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)
Assignee | ||
Comment 3•10 years ago
|
||
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)
Assignee | ||
Comment 4•10 years ago
|
||
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
Assignee | ||
Comment 5•10 years ago
|
||
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)
Assignee | ||
Comment 6•10 years ago
|
||
Rebased part 3 and renumbered it to part 4.
Attachment #8524333 -
Flags: review?(tnikkel)
Assignee | ||
Updated•10 years ago
|
Attachment #8524262 -
Attachment is obsolete: true
Attachment #8524262 -
Flags: review?(tnikkel)
Assignee | ||
Updated•10 years ago
|
Attachment #8524331 -
Attachment is obsolete: true
Attachment #8524331 -
Flags: review?(tnikkel)
Assignee | ||
Updated•10 years ago
|
Attachment #8524333 -
Attachment is obsolete: true
Attachment #8524333 -
Flags: review?(tnikkel)
Assignee | ||
Comment 9•10 years ago
|
||
New try job here: https://tbpl.mozilla.org/?tree=Try&rev=4fc5683cb7ad
Comment 10•10 years ago
|
||
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?
Updated•10 years ago
|
Attachment #8524259 -
Flags: review?(tnikkel) → review+
Updated•10 years ago
|
Attachment #8524334 -
Flags: review?(tnikkel) → review+
Updated•10 years ago
|
Attachment #8524335 -
Flags: review?(tnikkel) → review+
Comment 11•10 years ago
|
||
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+
Assignee | ||
Comment 12•10 years ago
|
||
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
Assignee | ||
Comment 13•10 years ago
|
||
(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.
Assignee | ||
Comment 14•10 years ago
|
||
Try looks good. Pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/7c1a12019ef0 https://hg.mozilla.org/integration/mozilla-inbound/rev/09a58fd3c8c6 https://hg.mozilla.org/integration/mozilla-inbound/rev/5c28ff9fa687 https://hg.mozilla.org/integration/mozilla-inbound/rev/ba86bdcc4a75
Comment 15•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7c1a12019ef0 https://hg.mozilla.org/mozilla-central/rev/09a58fd3c8c6 https://hg.mozilla.org/mozilla-central/rev/5c28ff9fa687 https://hg.mozilla.org/mozilla-central/rev/ba86bdcc4a75
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•