1207378 - memory corruption from twitter profile images

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: relevant valgrind output

Comment 2

[Seth Fowler [:seth] [:s2h]]

Looks an awful lot like bug 1206744.

Comment 3

[Seth Fowler [:seth] [:s2h]]

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.

Comment 4

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Add support for an offset to Downscaler. r=tn

Comment 5

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Use Downscaler to remove first-frame padding when downscaling GIFs. r=tn

Comment 6

[Seth Fowler [:seth] [:s2h]]

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.

Comment 7

[Seth Fowler [:seth] [:s2h]]

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 "&&".

Comment 8

[Seth Fowler [:seth] [:s2h]]

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.)

Comment 9

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e1f429deb0d

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Yep, that fixes the crash I was seeing.

Comment 11

[Timothy Nikkel (:tnikkel)]

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?

Comment 12

[Seth Fowler [:seth] [:s2h]]

(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.

Comment 13

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Add support for a frame rect to Downscaler

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.)

Comment 14

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Use Downscaler to remove first-frame padding when downscaling GIFs

Same as the old part 2, but now we pass the frame rect instead of just the offset.

Comment 15

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=535d6f733056

Comment 16

[Seth Fowler [:seth] [:s2h]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3d603de6ef4b
https://hg.mozilla.org/integration/mozilla-inbound/rev/649f934c48e6

Comment 18

[Seth Fowler [:seth] [:s2h]]

We want this on Aurora ASAP. I'll request uplift as soon as this gets merged.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3d603de6ef4b
https://hg.mozilla.org/mozilla-central/rev/649f934c48e6

Comment 21

[Seth Fowler [:seth] [:s2h]]

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.

Comment 22

[Liz Henry (:lizzard) (relman/hg->git project)]

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

Comment 23

[Liz Henry (:lizzard) (relman/hg->git project)]

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

Comment 24

[Carsten Book [:Tomcat]]

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!

Comment 25

[Seth Fowler [:seth] [:s2h]]

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.

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e301a8e15078

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e301a8e15078

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d6cad20ab005
https://hg.mozilla.org/releases/mozilla-aurora/rev/9afad3de5ea3
https://hg.mozilla.org/releases/mozilla-aurora/rev/5426b69fd725

Comment 29

[Carsten Book [:Tomcat]]

for comment #27,seth landed this but since pulsebot does not mark aurora/beta etc i did the marking :)

Comment 30

[Seth Fowler [:seth] [:s2h]]

(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!

Comment 32

[Liz Henry (:lizzard) (relman/hg->git project)]

Adding tracking flags for 43+ since this was a regression.