Last Comment Bug 682568 - Fix ICO crash with decodeondraw = true and BMPs with alpha bitmasks
: Fix ICO crash with decodeondraw = true and BMPs with alpha bitmasks
: crash, regression
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla9
Assigned To: Brian R. Bondy [:bbondy]
: Milan Sreckovic [:milan]
: 682584 682617 (view as bug list)
Depends on: 600556
Blocks: 573583
  Show dependency treegraph
Reported: 2011-08-27 09:03 PDT by Brian R. Bondy [:bbondy]
Modified: 2011-08-31 21:36 PDT (History)
12 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Possible fix for crash (1.35 KB, patch)
2011-08-27 09:10 PDT, Brian R. Bondy [:bbondy]
no flags Details | Diff | Splinter Review
Page Info -> Media copy/paste (22.74 KB, text/plain)
2011-08-27 14:07 PDT, bomfog
no flags Details
Google subscriptions copy/paste (130.00 KB, text/plain)
2011-08-27 14:11 PDT, bomfog
no flags Details
Google "export" result (270.08 KB, text/xml)
2011-08-27 14:13 PDT, bomfog
no flags Details
Patch with confirmed crash fix (6.99 KB, patch)
2011-08-27 19:05 PDT, Brian R. Bondy [:bbondy]
no flags Details | Diff | Splinter Review
Patch with confirmed crash fix (5.80 KB, patch)
2011-08-27 19:22 PDT, Brian R. Bondy [:bbondy]
joe: review+
Details | Diff | Splinter Review

Description Brian R. Bondy [:bbondy] 2011-08-27 09:03:47 PDT
Originally reported by inside Bug 600556:

On a mozilla-inbound nightly, Vista OS:

(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) ]

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] 2011-08-27 09:10:41 PDT
Created attachment 556268 [details] [diff] [review]
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 2011-08-27 11:48:40 PDT
I narrowed it down some. I had Google reader (all items, "list" display vs expanded) open in a background tab, with GoogleReaderPlus 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] 2011-08-27 12:13:19 PDT
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] 2011-08-27 12:42:50 PDT
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 2011-08-27 14:07:26 PDT
Created attachment 556303 [details]
Page Info -> Media copy/paste
Comment 6 bomfog 2011-08-27 14:11:54 PDT
Created attachment 556306 [details]
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] 2011-08-27 14:13:26 PDT
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 2011-08-27 14:13:57 PDT
Created attachment 556307 [details]
Google "export" result
Comment 9 bomfog 2011-08-27 15:05:47 PDT
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 10 Brian R. Bondy [:bbondy] 2011-08-27 15:23:22 PDT
*** Bug 682617 has been marked as a duplicate of this bug. ***
Comment 11 Brian R. Bondy [:bbondy] 2011-08-27 16:16:47 PDT
*** Bug 682584 has been marked as a duplicate of this bug. ***
Comment 12 Brian R. Bondy [:bbondy] 2011-08-27 19:05:49 PDT
Created attachment 556342 [details] [diff] [review]
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] 2011-08-27 19:22:55 PDT
Created attachment 556344 [details] [diff] [review]
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] 2011-08-27 19:32:39 PDT
Comment 15 IU 2011-08-29 07:37:42 PDT
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] 2011-08-29 07:41:44 PDT
> 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) 2011-08-29 13:09:35 PDT
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] 2011-08-29 22:19:02 PDT
Pushed to mozilla-inbound:
Comment 19 Marco Bonardo [::mak] 2011-08-31 01:55:40 PDT
Comment 20 d.a. 2011-08-31 07:01:29 PDT
I'm still seeing this crash using the latest nightly built of:

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:
Comment 21 d.a. 2011-08-31 07:07:52 PDT
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] 2011-08-31 07:09:31 PDT
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.

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