[10.5 up] Crash [@ nsNativeThemeCocoa::DrawButton] [@ img_data_lock] with nested <option>

VERIFIED FIXED in mozilla1.9.2a1

Status

()

Core
Widget: Cocoa
--
critical
VERIFIED FIXED
10 years ago
7 years ago

People

(Reporter: Jesse Ruderman, Assigned: smichaud)

Tracking

(Blocks: 2 bugs, 4 keywords)

Trunk
mozilla1.9.2a1
All
Mac OS X
crash, testcase, verified1.9.0.7, verified1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.8.1.x -
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: post 1.8-branch, crash signature)

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

10 years ago
Created attachment 347890 [details]
testcase (crashes Firefox when loaded)
(Assignee)

Comment 1

10 years ago
Created attachment 347976 [details]
Gdb stack trace (with debug symbols)

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

10 years ago
Assignee: joshmoz → smichaud
(Assignee)

Comment 2

10 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

10 years ago
Created attachment 348240 [details] [diff] [review]
Fix

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

10 years ago
Created attachment 348985 [details] [diff] [review]
Fix rev1 (follow Markus' suggestions)

A new tryserver build will follow shortly.
Attachment #348985 - Flags: review?(mstange)
(Assignee)

Updated

10 years ago
Attachment #348240 - Attachment is obsolete: true
Attachment #348240 - Flags: review?(mstange)
Attachment #348985 - Flags: review?(mstange) → review+
(Assignee)

Updated

10 years ago
Attachment #348985 - Flags: review?(joshmoz)

Updated

10 years ago
Attachment #348985 - Flags: review?(joshmoz) → review+
(Assignee)

Updated

10 years ago
Attachment #348985 - Flags: superreview?(roc)
Attachment #348985 - Flags: superreview?(roc) → superreview+
(Assignee)

Comment 8

10 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

10 years ago
Created attachment 351023 [details] [diff] [review]
Crashtest

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)

Updated

10 years ago
Attachment #351023 - Flags: review?(joshmoz) → review+
(Assignee)

Updated

10 years ago
Attachment #351023 - Flags: superreview?(roc)
(Assignee)

Comment 10

10 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

10 years ago
Marking FIXED, since this has been landed on trunk.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Assignee)

Comment 12

10 years ago
Comment on attachment 351023 [details] [diff] [review]
Crashtest

Landed on mozilla-central.
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

10 years ago
(In reply to comment #13)

Sorry.  I usually do that, but this time I forgot :-(
(Assignee)

Comment 15

10 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?
Whiteboard: [needs 1.9.1 approval/baking]

Updated

10 years ago
Flags: in-testsuite+
Flags: blocking1.9.1?
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
Has it been baking enough? It looks like everything is green. Can we get an approval?
Attachment #348985 - Flags: approval1.9.1? → approval1.9.1+
Comment on attachment 348985 [details] [diff] [review]
Fix rev1 (follow Markus' suggestions)

a191=beltzner
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
Flags: blocking1.9.1?
Keywords: fixed1.9.1
Whiteboard: [needs 1.9.1 approval/baking]
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
Attachment #348985 - Flags: approval1.9.0.6? → approval1.9.0.7?
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)?
Whiteboard: [needs 1.9.1 baking - 01/16]
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+
Whiteboard: [needs 1.9.1 baking - 01/16]
(Assignee)

Comment 24

10 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
Steven, is there a reason you didn't checkin the crash test?
(Assignee)

Comment 26

10 years ago
Nope.  Just forgot.  I'll do it tomorrow.
(Assignee)

Comment 27

10 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
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
Flags: wanted1.8.1.x-
Whiteboard: post 1.8-branch
Crash Signature: [@ nsNativeThemeCocoa::DrawButton] [@ img_data_lock]
You need to log in before you can comment on or make changes to this bug.