Closed
Bug 721510
Opened 12 years ago
Closed 12 years ago
Always decode at least one chunk when decoding a rasterimage
Categories
(Core :: Graphics: ImageLib, defect)
Core
Graphics: ImageLib
Tracking
()
RESOLVED
FIXED
mozilla13
People
(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)
Details
Attachments
(1 file, 2 obsolete files)
2.59 KB,
patch
|
joe
:
review+
|
Details | Diff | Splinter Review |
Right now, an unlucky context switch could cause us to decode 0 chunks of a rasterimage. But for correctness purposes, we need to decode at least one chunk when doing an UNTIL_SIZE decode. Anyway, this will make more sense with a patch. I'm also going to fix bug 715308 comment 124 at the same time.
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → justin.lebar+bug
Assignee | ||
Comment 1•12 years ago
|
||
Review: :joedrew!
Assignee | ||
Updated•12 years ago
|
Attachment #591945 -
Flags: review?(joe)
Assignee | ||
Comment 2•12 years ago
|
||
Attachment #591956 -
Flags: review?(joe)
Assignee | ||
Updated•12 years ago
|
Attachment #591945 -
Attachment is obsolete: true
Attachment #591945 -
Flags: review?(joe)
Comment 3•12 years ago
|
||
Comment on attachment 591956 [details] [diff] [review] Patch v2 Review of attachment 591956 [details] [diff] [review]: ----------------------------------------------------------------- ::: image/src/RasterImage.cpp @@ +2974,5 @@ > if (!aImg->mDecoder) > return NS_OK; > > + if (aImg->mDecoded) > + return NS_OK; This can be folded into the condition above (with suitable adjustment of the comment), since it's really for the same case. @@ +2999,5 @@ > // We keep decoding chunks until one of events occurs: > + // 1) We're an UNTIL_SIZE decode and we get the size > + // 2) We don't have any data left to decode > + // 3) The decode completes > + // 4) We hit the deadline You should probably move this documentation to right above the while condition. @@ +3012,2 @@ > } > + while ((aDecodeType != DECODE_TYPE_UNTIL_SIZE || !aImg->mHasSize) && I think it actually might be easier to understand if you separate this condition out of the while condition. I don't feel strongly about it, though, so do what makes the most sense to you.
Attachment #591956 -
Flags: review?(joe) → review+
Comment 4•12 years ago
|
||
Landed on inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/b4decc9b812f But backed out: https://hg.mozilla.org/releases/mozilla-aurora/rev/d5db8b8f8c8a because of intermittent but frequent reftest failures in bmp-corrupted/wrapper.html?invalid-compression.bmp, invalid-compression-RLE8.bmp, and invalid-compression-RLE4.bmp: https://tbpl.mozilla.org/php/getParsedLog.php?id=8889722&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=8889344&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=8887615&tree=Mozilla-Inbound For the failure I looked at, image 1 shows a blue square while image 2 is blank white.
Assignee | ||
Comment 5•12 years ago
|
||
I'm kind of surprised this caused these failures (modulo any obvious bugs I'm missing); if anything, I'd expect bug 715308 to have caused them. But let's leave this out for a while and see if there are more test failures.
Comment 6•12 years ago
|
||
The failures did show up in a couple of changesets without this patch, so it looks like this can re-land once the tree is open again. I filed bug 721917 for the test failures. Sorry for the trouble!
Comment 7•12 years ago
|
||
Re-landed on inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/158354889007
Target Milestone: --- → mozilla12
Comment 8•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/158354889007
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 9•12 years ago
|
||
[Approval Request Comment] See comment 131. User impact if declined: Minimal; crash is low-frequency, reftest failure may not be human-visible. Testing completed (on m-c, etc.): Backout. Risk to taking this patch (and alternatives if risky): Very low risk; backing out new patch, reverting to known-good state. String changes made by this patch: none
Attachment #593526 -
Flags: review?(joe)
Attachment #593526 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 10•12 years ago
|
||
Comment on attachment 593526 [details] Placeholder patch - Back out this and bug 721510 Dang, wrong bug.
Attachment #593526 -
Flags: review?(joe)
Attachment #593526 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•12 years ago
|
Attachment #593526 -
Attachment is obsolete: true
Comment 11•12 years ago
|
||
Backed out by: https://hg.mozilla.org/mozilla-central/rev/350ba395c507
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla12 → ---
Comment 12•12 years ago
|
||
Relanded by https://hg.mozilla.org/mozilla-central/rev/581a0260b08b
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in
before you can comment on or make changes to this bug.
Description
•