Closed Bug 1207378 Opened 5 years ago Closed 5 years ago

memory corruption from twitter profile images

Categories

(Core :: ImageLib, defect, major)

43 Branch
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox42 --- unaffected
firefox43 + fixed
firefox44 + fixed

People

(Reporter: dbaron, Assigned: seth)

References

Details

(Keywords: crash, regression, sec-critical)

Attachments

(3 files, 2 obsolete files)

I'm crashing reliably in a Linux debug build loading https://twitter.com/dakami/status/646423477176565760 .

It's a build on changeset https://hg.mozilla.org/mozilla-central/rev/a1ccea59e254 plus my local patch queue.
Looks an awful lot like bug 1206744.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1206744
I think this is actually a different problem than bug 1206744, despite the stack looking the same.

The downscaler is just not taking the first-frame padding of this GIF into account. This can be fixed easily just by allocating a frame for the full size of the first frame (ignoring the padding) and then adding an "offset" feature to Downscaler, allowing it to skip over rows and columns that don't contain data due to padding.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Blocks: 1194058
Comment on attachment 8665153 [details] [diff] [review]
(Part 1) - Add support for an offset to Downscaler. r=tn

Part 1 adds an aOffset parameter to Downscaler::BeginFrame(). The idea is to implement padding by skipping rows vertically (using calls to CommitRow() with empty rows) and by skipping columns horizontally (by adding an offset in the return value of RowBuffer()).

Note that we shouldn't need to clear the skipped columns in RowBuffer(), because callers never get a pointer to them and so won't be able to write anything over the default all-black, zero alpha pixels. That's not true for the vertical skipping, of course, so we do clear the buffer in that case.
Attachment #8665153 - Flags: review?(tnikkel)
Comment on attachment 8665154 [details] [diff] [review]
(Part 2) - Use Downscaler to remove first-frame padding when downscaling GIFs. r=tn

This patch uses part 1 to handle first-frame padding when downscaling GIFs. (GIFs are the only format where this can happen.)

The only tricky thing is that we have to remember (via mSawTransparency) that we did this, so that we don't optimize out the transparency later in EndImageFrame(). Because the GIF may not be transparent other than the padding, we need to change the "||" in the condition in EndImageFrame() to an "&&".
Attachment #8665154 - Flags: review?(tnikkel)
We should have a test for this, but again, it's not easy to write without adding a new framework that doesn't belong in this bug. I'll add a suite of DDD gtests this week. (In fact, I plan to start on it today.)
See Also: → 1207830
Yep, that fixes the crash I was seeing.
Comment on attachment 8665154 [details] [diff] [review]
(Part 2) - Use Downscaler to remove first-frame padding when downscaling GIFs. r=tn

Hmm, so if there is padding at the bottom of the frame don't we need to commit enough rows (of transparent pixels) after the frame (in the padding portion on the bottom) to the downscaler so the filter can output the rows of the frame that do have non-transparent image data?
(In reply to Timothy Nikkel (:tn) from comment #11)
> Comment on attachment 8665154 [details] [diff] [review]
> (Part 2) - Use Downscaler to remove first-frame padding when downscaling
> GIFs. r=tn
> 
> Hmm, so if there is padding at the bottom of the frame don't we need to
> commit enough rows (of transparent pixels) after the frame (in the padding
> portion on the bottom) to the downscaler so the filter can output the rows
> of the frame that do have non-transparent image data?

Hmm... actually we do need to, yeah. I'll update the patch.
OK, I've reworked the patch so that Downscaler::BeginFrame() takes a frame rect
instead of just an offset, and we use CommitRow() to add padding at the bottom
of the frame as well. (By calling SkipToRow() in CommitRow() when we detect that
we just hit the bottom.)
Attachment #8666217 - Flags: review?(tnikkel)
Assignee: nobody → seth
Status: REOPENED → ASSIGNED
Attachment #8665153 - Attachment is obsolete: true
Attachment #8665153 - Flags: review?(tnikkel)
Attachment #8665154 - Attachment is obsolete: true
Attachment #8665154 - Flags: review?(tnikkel)
Same as the old part 2, but now we pass the frame rect instead of just the offset.
Attachment #8666222 - Flags: review?(tnikkel)
Attachment #8666217 - Flags: review?(tnikkel) → review+
Attachment #8666222 - Flags: review?(tnikkel) → review+
Duplicate of this bug: 1208696
We want this on Aurora ASAP. I'll request uplift as soon as this gets merged.
Duplicate of this bug: 1208764
https://hg.mozilla.org/mozilla-central/rev/3d603de6ef4b
https://hg.mozilla.org/mozilla-central/rev/649f934c48e6
Status: ASSIGNED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Comment on attachment 8666217 [details] [diff] [review]
(Part 1) - Add support for a frame rect to Downscaler

Approval Request Comment
[Feature/regressing bug #]: Bug 1194058.
[User impact if declined]: Crashes on pages with certain types of GIFs.
[Describe test coverage new/current, TreeHerder]: On m-c.
[Risks and why]: Fairly low risk.
[String/UUID change made/needed]: None.
Attachment #8666217 - Flags: approval-mozilla-aurora?
Attachment #8666222 - Flags: approval-mozilla-aurora?
Comment on attachment 8666217 [details] [diff] [review]
(Part 1) - Add support for a frame rect to Downscaler

Approved for uplift to aurora, let's not crash with gif avatars
Attachment #8666217 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8666222 [details] [diff] [review]
(Part 2) - Use Downscaler to remove first-frame padding when downscaling GIFs

Part 2 should also be uplifted to aurora, fix for crashes on gif download
Attachment #8666222 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1210264
Hi Seth, this cause conflicts during uplift:

grafting 303930:3d603de6ef4b "Bug 1207378 (Part 1) - Add support for a frame rect to Downscaler. r=tn"
^[[Cmerging image/decoders/nsBMPDecoder.cpp
merging image/decoders/nsGIFDecoder2.cpp
merging image/decoders/nsICODecoder.cpp
warning: conflicts during merge.
merging image/decoders/nsICODecoder.cpp incomplete! (edit conflicts, then use 'hg resolve --mark')
merging image/decoders/nsIconDecoder.cpp
merging image/decoders/nsJPEGDecoder.cpp
merging image/decoders/nsPNGDecoder.cpp
abort: unresolved conflicts, can't continue
(use hg resolve and hg graft --continue)

could you take a look, thanks!
Flags: needinfo?(seth)
Hmm OK, that's not surprising in retrospect. Ideally we'd uplift bug 1201796 to Aurora at the same time as this bug. Let me take a look and see if I think we're ready to uplift that yet; if not, I'll just uplift this manually and fix the merge conflict.
Flags: needinfo?(seth)
for comment #27,seth landed this but since pulsebot does not mark aurora/beta etc i did the marking :)
(In reply to Carsten Book [:Tomcat] from comment #29)
> for comment #27,seth landed this but since pulsebot does not mark
> aurora/beta etc i did the marking :)

Yes, my mistake; I thought bzpost would take care of that. Thanks!
Depends on: 1210136
Duplicate of this bug: 1208936
Adding tracking flags for 43+ since this was a regression.
Depends on: 1216178
Depends on: 1224185
Depends on: 1223465
Depends on: 1224100
Depends on: 1233651
Depends on: 1207958
You need to log in before you can comment on or make changes to this bug.