Closed
Bug 464589
Opened 16 years ago
Closed 16 years ago
[10.5 up] Crash [@ nsNativeThemeCocoa::DrawButton] [@ img_data_lock] with nested <option>
Categories
(Core :: Widget: Cocoa, defect)
Tracking
()
VERIFIED
FIXED
mozilla1.9.2a1
People
(Reporter: jruderman, Assigned: smichaud)
References
Details
(4 keywords, Whiteboard: post 1.8-branch)
Crash Data
Attachments
(4 files, 1 obsolete file)
(deleted),
text/html
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
mstange
:
review+
jaas
:
review+
roc
:
superreview+
beltzner
:
approval1.9.1+
dveditz
:
approval1.9.0.7+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jaas
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•16 years ago
|
||
Here's a gdb stack trace made using an opt trunk build (with debug symbols) whose code was pulled Monday morning (2008-11-10). This crash looks related to the one from bug 460387.
Assignee | ||
Updated•16 years ago
|
Assignee: joshmoz → smichaud
Assignee | ||
Comment 2•16 years ago
|
||
This happens on 10.5 and 10.6, but not on 10.4.
Summary: Crash [@ nsNativeThemeCocoa::DrawButton] [@ img_data_lock] with nested <option> → [10.5 up] Crash [@ nsNativeThemeCocoa::DrawButton] [@ img_data_lock] with nested <option>
Assignee | ||
Comment 3•16 years ago
|
||
Turns out this bug's crash is the same basic problem that caused bug 444864, bug 444260 and bug 449111 -- various Apple methods (including CGBitmapContextCreate(), CGContextDrawImage()) can't deal with objects whose area is too large. So I've extended the solution for those three bugs to cover both this bug and another potential variant of it (in nsNativeThemeCocoa::DrawScrollbar()). In principle, the same value for IMAGE_SCALING_MAX_AREA should work in all three cases. But (to make sure) I tested it again by hand in nsNativeThemeCocoa::DrawButton(), and found that it fits the bill -- it seems neither too large nor too small. Here's a tryserver build made with this patch: https://build.mozilla.org/tryserver-builds/2008-11-14_09:59-smichaud@pobox.com-bugzilla464589/smichaud@pobox.com-bugzilla464589-firefox-try-mac.dmg
Attachment #348240 -
Flags: review?(mstange)
Comment 4•16 years ago
|
||
Jesse's testcase does not crash me using : Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b2pre) Gecko/20081114 Minefield/3.1b2pre.
Comment 5•16 years ago
|
||
Comment on attachment 348240 [details] [diff] [review] Fix >+#define IMAGE_SCALING_MAX_AREA 500000 How about BUFFER_MAX_AREA? In the scrollbar case we don't use the buffer for scaling. >+ if (needsScaling && (drawWidth * drawHeight > IMAGE_SCALING_MAX_AREA)) >+ needsScaling = PR_FALSE; >+ > if (!needsScaling) { if (!needsScaling || (drawWidth * drawHeight > IMAGE_SCALING_MAX_AREA)) { would be simpler. >@@ -1102,24 +1107,28 @@ nsNativeThemeCocoa::DrawScrollbar(CGCont >+ // Fall back to no scaling if the area of our scrollbar (in pixels^2) is too large. >+ if (!drawDirect && (aBoxRect.size.width * aBoxRect.size.height > IMAGE_SCALING_MAX_AREA)) >+ drawDirect = PR_TRUE; > > if (drawDirect) { Same here. And the comment is misleading; in this case the buffer is used as a workaround to make scrollbar drawing honor the current transform.
Assignee | ||
Comment 6•16 years ago
|
||
A new tryserver build will follow shortly.
Attachment #348985 -
Flags: review?(mstange)
Assignee | ||
Updated•16 years ago
|
Attachment #348240 -
Attachment is obsolete: true
Attachment #348240 -
Flags: review?(mstange)
Updated•16 years ago
|
Attachment #348985 -
Flags: review?(mstange) → review+
Assignee | ||
Updated•16 years ago
|
Attachment #348985 -
Flags: review?(joshmoz)
Assignee | ||
Comment 7•16 years ago
|
||
Here's a tryserver build made with my rev1 patch: https://build.mozilla.org/tryserver-builds/2008-11-19_08:53-smichaud@pobox.com-bugzilla464589-rev1/smichaud@pobox.com-bugzilla464589-rev1-firefox-try-mac.dmg
Attachment #348985 -
Flags: review?(joshmoz) → review+
Assignee | ||
Updated•16 years ago
|
Attachment #348985 -
Flags: superreview?(roc)
Attachment #348985 -
Flags: superreview?(roc) → superreview+
Assignee | ||
Comment 8•16 years ago
|
||
Comment on attachment 348985 [details] [diff] [review] Fix rev1 (follow Markus' suggestions) This is a fairly trivial extension of the fix for bug 444864. A crashtest for this bug will follow shortly.
Attachment #348985 -
Flags: approval1.9.1?
Assignee | ||
Comment 9•16 years ago
|
||
Here's a crashtest, made using Jesse's testcase from comment #0. I checked to make sure it passes with this bug's patch (attachment 348985 [review]), but fails without it.
Attachment #351023 -
Flags: review?(joshmoz)
Attachment #351023 -
Flags: review?(joshmoz) → review+
Assignee | ||
Updated•16 years ago
|
Attachment #351023 -
Flags: superreview?(roc)
Assignee | ||
Comment 10•16 years ago
|
||
Comment on attachment 348985 [details] [diff] [review] Fix rev1 (follow Markus' suggestions) Landed on mozilla-central. Awaiting approval to land on the 1.9.1 branch.
Attachment #351023 -
Flags: superreview?(roc) → superreview+
Assignee | ||
Comment 11•16 years ago
|
||
Marking FIXED, since this has been landed on trunk.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 12•16 years ago
|
||
Comment on attachment 351023 [details] [diff] [review] Crashtest Landed on mozilla-central.
Comment 13•16 years ago
|
||
Steven, it would be great if you could post the hg checkin link as easy reference. Thanks. (In reply to comment #10) > (From update of attachment 348985 [details] [diff] [review]) > Landed on mozilla-central. Patch: http://hg.mozilla.org/mozilla-central/rev/ef021e974c04 (In reply to comment #12) > (From update of attachment 351023 [details] [diff] [review]) > Landed on mozilla-central. Crashtest: http://hg.mozilla.org/mozilla-central/rev/3dd567dd1322
Hardware: PC → All
Target Milestone: --- → mozilla1.9.2a1
Assignee | ||
Comment 14•16 years ago
|
||
(In reply to comment #13) Sorry. I usually do that, but this time I forgot :-(
Assignee | ||
Comment 15•16 years ago
|
||
Comment on attachment 348985 [details] [diff] [review] Fix rev1 (follow Markus' suggestions) Put this on radar for 1.9.0.6.
Attachment #348985 -
Flags: approval1.9.0.6?
Updated•16 years ago
|
Whiteboard: [needs 1.9.1 approval/baking]
Updated•16 years ago
|
Flags: in-testsuite+
Updated•16 years ago
|
Flags: blocking1.9.1?
Comment 16•16 years ago
|
||
Opening the given testcase doesn't crash Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20081221 Minefield/3.2a1pre ID:20081221222459 => verified.
Status: RESOLVED → VERIFIED
Comment 17•16 years ago
|
||
Has it been baking enough? It looks like everything is green. Can we get an approval?
Updated•16 years ago
|
Attachment #348985 -
Flags: approval1.9.1? → approval1.9.1+
Comment 18•16 years ago
|
||
Comment on attachment 348985 [details] [diff] [review] Fix rev1 (follow Markus' suggestions) a191=beltzner
Comment 19•16 years ago
|
||
Landed on 1.9.1: patch: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/1084e74c25ce crashtest: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/aee4fa59bd32
Comment 20•16 years ago
|
||
REFTEST TEST-PASS | file:///Volumes/Daten/mozilla/source/mozilla-1.9.1/mozilla/widget/src/cocoa/crashtests/464589-1.html | (LOAD ONLY) Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090103 Shiretoko/3.1b3pre ID:20090103020545
Keywords: fixed1.9.1 → verified1.9.1
Target Milestone: mozilla1.9.2a1 → mozilla1.9.1b3
Updated•16 years ago
|
Attachment #348985 -
Flags: approval1.9.0.6? → approval1.9.0.7?
Comment 21•16 years ago
|
||
Comment on attachment 348985 [details] [diff] [review] Fix rev1 (follow Markus' suggestions) Seems optional for the branch so we don't want to stuff this in last minute. Leaving for more testing in 1.9.1
(In reply to comment #3) > various Apple methods (including > CGBitmapContextCreate(), CGContextDrawImage()) can't deal with objects > whose area is too large. Has anyone ever filed this with Apple (or was this our bug because we were feeding those methods data larger than they size the methods specify they can accept)?
Updated•16 years ago
|
Whiteboard: [needs 1.9.1 baking - 01/16]
Comment 23•16 years ago
|
||
Comment on attachment 348985 [details] [diff] [review] Fix rev1 (follow Markus' suggestions) Approved for 1.9.0.7, a=dveditz for release-drivers.
Attachment #348985 -
Flags: approval1.9.0.7? → approval1.9.0.7+
Updated•16 years ago
|
Whiteboard: [needs 1.9.1 baking - 01/16]
Assignee | ||
Comment 24•16 years ago
|
||
Landed on the 1.9.0 branch: Checking in widget/src/cocoa/nsNativeThemeCocoa.mm; /cvsroot/mozilla/widget/src/cocoa/nsNativeThemeCocoa.mm,v <-- nsNativeThemeCocoa.mm new revision: 1.96; previous revision: 1.95 done
Keywords: fixed1.9.0.7
Comment 25•16 years ago
|
||
Steven, is there a reason you didn't checkin the crash test?
Assignee | ||
Comment 26•16 years ago
|
||
Nope. Just forgot. I'll do it tomorrow.
Assignee | ||
Comment 27•16 years ago
|
||
Crashtest landed on the 1.9.0 branch: cvs commit: Examining widget/src/cocoa/crashtests RCS file: /cvsroot/mozilla/widget/src/cocoa/crashtests/464589-1.html,v done Checking in widget/src/cocoa/crashtests/464589-1.html; /cvsroot/mozilla/widget/src/cocoa/crashtests/464589-1.html,v <-- 464589-1.html initial revision: 1.1 done Checking in widget/src/cocoa/crashtests/crashtests.list; /cvsroot/mozilla/widget/src/cocoa/crashtests/crashtests.list,v <-- crashtests.list new revision: 1.3; previous revision: 1.2 done
Comment 28•16 years ago
|
||
Verified on 1.9.0 branch with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.7pre) Gecko/2009012404 GranParadiso/3.0.7pre
Keywords: fixed1.9.0.7 → verified1.9.0.7
Target Milestone: mozilla1.9.1b3 → mozilla1.9.2a1
Updated•16 years ago
|
Flags: wanted1.8.1.x-
Whiteboard: post 1.8-branch
Updated•13 years ago
|
Crash Signature: [@ nsNativeThemeCocoa::DrawButton]
[@ img_data_lock]
You need to log in
before you can comment on or make changes to this bug.
Description
•