Closed
Bug 682568
Opened 13 years ago
Closed 13 years ago
Fix ICO crash with decodeondraw = true and BMPs with alpha bitmasks
Categories
(Core :: Graphics: ImageLib, defect)
Core
Graphics: ImageLib
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.
"
Assignee | ||
Comment 1•13 years ago
|
||
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?)
Assignee | ||
Comment 3•13 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•13 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).
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•13 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.
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 | ||
Comment 12•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
Assignee | ||
Updated•13 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•13 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•13 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•13 years ago
|
Crash Signature: [@ mozilla::imagelib::nsICODecoder::WriteInternal ]
[@ mozilla::imagelib::nsICODecoder::WriteInternal(char const*, unsigned int) ]
Comment 17•13 years ago
|
||
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•13 years ago
|
||
Pushed to mozilla-inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/d97f7df807fc
Comment 19•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
Comment 20•13 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•13 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•13 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.
You need to log in
before you can comment on or make changes to this bug.
Description
•