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.

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

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

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

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

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

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

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

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

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!

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.

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

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!

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