Last Comment Bug 721510 - Always decode at least one chunk when decoding a rasterimage
: Always decode at least one chunk when decoding a rasterimage
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla13
Assigned To: Justin Lebar (not reading bugmail)
:
: Milan Sreckovic [:milan]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-26 13:37 PST by Justin Lebar (not reading bugmail)
Modified: 2012-02-15 10:44 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (1.59 KB, patch)
2012-01-26 14:23 PST, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
Patch v2 (2.59 KB, patch)
2012-01-26 14:53 PST, Justin Lebar (not reading bugmail)
joe: review+
Details | Diff | Splinter Review
Placeholder patch - Back out this and bug 721510 (142 bytes, text/plain)
2012-02-01 11:08 PST, Justin Lebar (not reading bugmail)
no flags Details

Description Justin Lebar (not reading bugmail) 2012-01-26 13:37:17 PST
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.
Comment 1 Justin Lebar (not reading bugmail) 2012-01-26 14:23:13 PST
Created attachment 591945 [details] [diff] [review]
Patch v1

Review: :joedrew!
Comment 2 Justin Lebar (not reading bugmail) 2012-01-26 14:53:05 PST
Created attachment 591956 [details] [diff] [review]
Patch v2
Comment 3 Joe Drew (not getting mail) 2012-01-26 15:39:08 PST
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.
Comment 4 Matt Brubeck (:mbrubeck) 2012-01-27 14:56:53 PST
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.
Comment 5 Justin Lebar (not reading bugmail) 2012-01-27 15:02:35 PST
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 Matt Brubeck (:mbrubeck) 2012-01-27 16:45:34 PST
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 Matt Brubeck (:mbrubeck) 2012-01-27 18:56:28 PST
Re-landed on inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/158354889007
Comment 8 Joe Drew (not getting mail) 2012-01-28 18:59:13 PST
https://hg.mozilla.org/mozilla-central/rev/158354889007
Comment 9 Justin Lebar (not reading bugmail) 2012-02-01 11:08:20 PST
Created attachment 593526 [details]
Placeholder patch - Back out this and bug 721510

[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
Comment 10 Justin Lebar (not reading bugmail) 2012-02-01 11:09:25 PST
Comment on attachment 593526 [details]
Placeholder patch - Back out this and bug 721510

Dang, wrong bug.
Comment 11 Ed Morley [:emorley] 2012-02-03 11:15:44 PST
Backed out by:
https://hg.mozilla.org/mozilla-central/rev/350ba395c507
Comment 12 Ed Morley [:emorley] 2012-02-10 05:27:59 PST
Relanded by https://hg.mozilla.org/mozilla-central/rev/581a0260b08b

Note You need to log in before you can comment on or make changes to this bug.