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

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

Comment 1

[Steven Michaud [:smichaud] (Retired)]

Attachment: 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.

Comment 2

[Steven Michaud [:smichaud] (Retired)]

This happens on 10.5 and 10.6, but not on 10.4.

Comment 3

[Steven Michaud [:smichaud] (Retired)]

Attachment: 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:[e-mail]-bugzilla464589/[e-mail]

Comment 4

[Marcia Knous [:marcia]]

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

[Markus Stange [:mstange]]

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.

Comment 6

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev1 (follow Markus' suggestions)

A new tryserver build will follow shortly.

Comment 7

[Steven Michaud [:smichaud] (Retired)]

Here's a tryserver build made with my rev1 patch:

https://build.mozilla.org/tryserver-builds/2008-11-19_08:[e-mail]-bugzilla464589-rev1/[e-mail]

Comment 8

[Steven Michaud [:smichaud] (Retired)]

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.

Comment 9

[Steven Michaud [:smichaud] (Retired)]

Attachment: 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.

Comment 10

[Steven Michaud [:smichaud] (Retired)]

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.

Comment 11

[Steven Michaud [:smichaud] (Retired)]

Marking FIXED, since this has been landed on trunk.

Comment 12

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 351023 [details] [diff] [review]
Crashtest

Landed on mozilla-central.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

Comment 14

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #13)

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

Comment 15

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 348985 [details] [diff] [review]
Fix rev1 (follow Markus' suggestions)

Put this on radar for 1.9.0.6.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+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.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Has it been baking enough? It looks like everything is green. Can we get an approval?

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 348985 [details] [diff] [review]
Fix rev1 (follow Markus' suggestions)

a191=beltzner

Comment 19

[Markus Stange [:mstange]]

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

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

Comment 21

[Daniel Veditz [:dveditz]]

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

Comment 22

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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

Comment 23

[Daniel Veditz [:dveditz]]

Comment on attachment 348985 [details] [diff] [review]
Fix rev1 (follow Markus' suggestions)

Approved for 1.9.0.7, a=dveditz for release-drivers.

Comment 24

[Steven Michaud [:smichaud] (Retired)]

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

Comment 25

[Samuel Sidler (old account; do not CC)]

Steven, is there a reason you didn't checkin the crash test?

Comment 26

[Steven Michaud [:smichaud] (Retired)]

Nope.  Just forgot.  I'll do it tomorrow.

Comment 27

[Steven Michaud [:smichaud] (Retired)]

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

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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