682568 - Fix ICO crash with decodeondraw = true and BMPs with alpha bitmasks

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Brian R. Bondy [:bbondy]]

Originally reported by [e-mail] inside Bug 600556:

"
On a mozilla-inbound nightly, Vista OS:
http://hg.mozilla.org/integration/mozilla-inbound/rev/c70c539f2e93

(Very) dirty profile, Session Manager extension in use, opening a session of a half dozen or so windows with ~ 75 tabs total, I crash at

[@ mozilla::imagelib::nsICODecoder::WriteInternal(char const*, unsigned int) ]

https://crash-stats.mozilla.com/report/index/bp-9f797b78-e5df-4402-b70c-e08052110826
https://crash-stats.mozilla.com/report/index/bp-0d3d66a2-bf2e-4b80-8f45-108882110826

Session Manager UI got borked in the process, so haven't tracked down which page(s) cause the problem (yet).

The crash seems to depend on some combination of open tabs, Session Manager version, restored vs recreated-via-bookmarks session, maybe cache or cookies, and probably the phase of the moon, among other possibilities.
"

Comment 1

[Brian R. Bondy [:bbondy]]

Attachment: Possible fix for crash

I think this is the error but I'll try to create an additional patch with a reference test to ensure it's not something related to needing to check the buffer size.

Comment 2

[bomfog]

I narrowed it down some. I had Google reader (all items, "list" display vs expanded) open in a background tab, with GoogleReaderPlus 0.5.2.7 which adds favicons to the items and sidebar. The foreground tab could just be blank.

Restoring the session with google reader in the foreground worked.
Turning off favicons in the extension worked.
Cache-clearing, extension and/or browser, seemed to have an effect, but I'm not sure what.  Too many variables, and I'm too disorganised.

The Session Manager extension borkage was coincidental, fixed in the most recent update version.

So I'd guess it wasn't a malformed favicon, but rather the extension's handling of them?

(Is this bug's x86_64 platform classification intentional?)

Comment 3

[Brian R. Bondy [:bbondy]]

I'll try to reproduce, but I fear that it depends on which feeds you have.

> (Is this bug's x86_64 platform classification intentional?)

No, fixed.

Comment 4

[Brian R. Bondy [:bbondy]]

Yes it seems like I will need to know which of the favicons for one of the feeds it is.  Would it be possible to post here or email me the list of feeds you have? I can check them one by one myself. 

To get the list:
Once logged into Google reader, under the subscriptions box on the bottom right there is a little arrow going down next to the text "Subscriptions".  Please click that and then copy the list of URLs (and any other text that it happens to select).

Comment 5

[bomfog]

Attachment: Page Info -> Media copy/paste

Comment 6

[bomfog]

Attachment: Google subscriptions copy/paste

Sure, but that's 1200 plus subs (no doubt including a lot of dead ones). If you just need to check the icons, would a copy of Page Info -> Media work better? That seems to have all the specific favicon URLs, including data: ones.

Anyway, here's both versions, plus a bonus "export as opml".

Comment 7

[Brian R. Bondy [:bbondy]]

awesome thanks, I'll write a small command line script to just load all those pages in tabs and see if that reproduces.

Comment 8

[bomfog]

Attachment: Google "export" result

Comment 9

[bomfog]

Note: Those icon urls are what I got on win7 at home. If they might differ from what I'd get served on vista at work, let me know and I can remote desktop in and get the result from that system.

Comment 12

[Brian R. Bondy [:bbondy]]

Attachment: Patch with confirmed crash fix

The problem was that the contained image buffer (BMP in our case) was NULL for size decodes since it doesn't actually create an image buffer.
The container ICO needed was assuming that the buffer existed since the data processed was high enough for it to be created.

This is fixed now and I also added a couple of extra error checks that weren't absolutely needed, but are better to check to be safe.

I'm not sure if size decodes only happen when image.mem.decodeondraw is true, but I could only reproduce when image.mem.decodeondraw is true.
The default for me was image.mem.decodeondraw = false on Windows.

Thanks to those who gave me the reference images and Brian Carpenter for letting me know how to reproduce with image.mem.decodeondraw.

I'll run this through the try server now by the way.

*** Questions about high priority task:
I think this should probably be pushed directly to mozilla-central instead of mozilla-inbound?
I should wait for a review as usual right?

Comment 13

[Brian R. Bondy [:bbondy]]

Attachment: Patch with confirmed crash fix

Last patch was based on my patch queue, rebased it to directly mozilla-central tip.

Comment 14

[Brian R. Bondy [:bbondy]]

http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=4752ecff1796

Comment 15

[IU]

By the way, this is also crashing on Google Maps, if image.mem.decodeondraw is enabled.

By the way, why does this depend on bug 600556, given that it caused the regression.  Shouldn't this instead be blocking bug 600556?

Comment 16

[Brian R. Bondy [:bbondy]]

> this is also crashing on Google Maps

Should be fixed as well from the fix above which is waiting on review to be pushed.

> By the way, why does this depend on bug 600556

This patch should be pushed after the patch in 600556 (which is already pushed), so I think depends on Bug 600556 is correct.  Maybe I'm wrong but I generally use depends on when a patch must come after the bug it depends on.

Comment 17

[Joe Drew (not getting mail)]

Comment on attachment 556344 [details] [diff] [review]
Patch with confirmed crash fix

Review of attachment 556344 [details] [diff] [review]:
-----------------------------------------------------------------

Size decodes can happen without decodeondraw set to true, but I think they're less common.

In any case, this looks great.

Comment 18

[Brian R. Bondy [:bbondy]]

Pushed to mozilla-inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/d97f7df807fc

Comment 19

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/mozilla-central/rev/d97f7df807fc

Comment 20

[d.a.]

I'm still seeing this crash using the latest nightly built of: http://hg.mozilla.org/mozilla-central/rev/005bce677a00

Anytime I try to view any article on Huffington Post I get this crash, and if I disable decodeondraw no crash will occur. I'm running on Mac OS X 10.6.

Crash report: https://crash-stats.mozilla.com/report/index/f492935c-4470-4c51-94c7-e236e2110831

Comment 21

[d.a.]

Disregard my last comment, the changeset of the nightly build was before the patch was pushed.

--

I thought that the nightly builds started building at 3am PDT with the latest m-c push?

Comment 22

[Brian R. Bondy [:bbondy]]

Ya the crash location is at the old place in code which was fixed, so the build you were using doesn't have the fix included.