The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla9

Status

()

Core
ImageLib
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bbondy, Assigned: bbondy)

Tracking

({crash, regression})

Trunk
mozilla9
crash, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(4 attachments, 2 obsolete attachments)

(Assignee)

Description

6 years ago
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.
"
(Assignee)

Comment 1

6 years ago
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.
Attachment #556268 - Flags: review?(joe)

Comment 2

6 years ago
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?)
(Assignee)

Comment 3

6 years ago
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
(Assignee)

Comment 4

6 years ago
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

6 years ago
Created attachment 556303 [details]
Page Info -> Media copy/paste

Comment 6

6 years ago
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".
(Assignee)

Comment 7

6 years ago
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

6 years ago
Created attachment 556307 [details]
Google "export" result

Comment 9

6 years ago
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.
(Assignee)

Updated

6 years ago
Duplicate of this bug: 682617
(Assignee)

Updated

6 years ago
Duplicate of this bug: 682584
(Assignee)

Comment 12

6 years ago
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?
Attachment #556268 - Attachment is obsolete: true
Attachment #556268 - Flags: review?(joe)
Attachment #556342 - Flags: review?(joe)
(Assignee)

Comment 13

6 years ago
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.
Attachment #556342 - Attachment is obsolete: true
Attachment #556342 - Flags: review?(joe)
Attachment #556344 - Flags: review?(joe)
(Assignee)

Comment 14

6 years ago
http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=4752ecff1796
Blocks: 573583
(Assignee)

Updated

6 years ago
Summary: Investigate ICO crash and create reftest, reference image, and fix → Fix ICO crash with decodeondraw = true and BMPs with alpha bitmasks

Comment 15

6 years ago
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
(Assignee)

Comment 16

6 years ago
> 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.

Updated

6 years ago
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+
(Assignee)

Comment 18

6 years ago
Pushed to mozilla-inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/d97f7df807fc
http://hg.mozilla.org/mozilla-central/rev/d97f7df807fc
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9

Comment 20

6 years ago
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

6 years ago
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?
(Assignee)

Comment 22

6 years ago
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.

Updated

6 years ago
Keywords: crash
You need to log in before you can comment on or make changes to this bug.