Closed Bug 682568 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: ImageLib, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla9

People

(Reporter: bbondy, Assigned: bbondy)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(4 files, 2 obsolete files)

Originally reported by bomfog@gmail.com 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.
"
Attached patch Possible fix for crash (obsolete) — Splinter Review
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.
Attachment #556268 - Flags: review?(joe)
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?)
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.
OS: Windows Vista → All
Hardware: x86_64 → All
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).
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".
awesome thanks, I'll write a small command line script to just load all those pages in tabs and see if that reproduces.
Attached file Google "export" result
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.
Duplicate of this bug: 682617
Duplicate of this bug: 682584
Attached patch Patch with confirmed crash fix (obsolete) — Splinter Review
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?
Attachment #556268 - Attachment is obsolete: true
Attachment #556268 - Flags: review?(joe)
Attachment #556342 - Flags: review?(joe)
Last patch was based on my patch queue, rebased it to directly mozilla-central tip.
Attachment #556342 - Attachment is obsolete: true
Attachment #556342 - Flags: review?(joe)
Attachment #556344 - Flags: review?(joe)
Blocks: 573583
Summary: Investigate ICO crash and create reftest, reference image, and fix → Fix ICO crash with decodeondraw = true and BMPs with alpha bitmasks
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?
Keywords: regression
> 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.
Crash Signature: [@ mozilla::imagelib::nsICODecoder::WriteInternal ] [@ mozilla::imagelib::nsICODecoder::WriteInternal(char const*, unsigned int) ]
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.
Attachment #556344 - Flags: review?(joe) → review+
http://hg.mozilla.org/mozilla-central/rev/d97f7df807fc
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
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
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?
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.
Keywords: crash
You need to log in before you can comment on or make changes to this bug.