444260 - Wide <xul:button> causes nsNativeThemeCocoa::DrawCellWithScaling to autorelease a freed object

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect, P1)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

Version: trunk debug build on Leopard

Loading the testcase causes a malloc error "can't allocate region" (perhaps bug 435223), and then:

objc[20992]: FREED(id): message autorelease sent to freed object=0x1ca0afe0

The malloc error doesn't bother me too much, but touching freed objects is pretty bad.  In fact, with MallocScribble enabled, Firefox dereferences 0x55555575 instead of triggering the error message above.

Comment 1

[Steven Michaud [:smichaud] (Retired)]

Attachment: Gdb trace of crash (with debug symbols)

Here's a gdb trace of this crash, made with an opt 1.9.0-branch build
containing debug symbols.  It has both console messages and a stack
trace.

I'll be working on this.

Comment 2

[Steven Michaud [:smichaud] (Retired)]

Attachment: Trivial fix (somewhat ugly)

Here's a trivial patch that fixes the crash -- it refuses to draw when
CGBitmapContextCreate() fails (when it returns NULL).

But the impossibly large button does get drawn on other platforms
(e.g. Windows and Linux), so I suppose we should try to work some kind
of fallback into the drawing code, so that it will still "work" even
when it's called with parameters that are far too large.

Since I'm not very familiar with this code, I'd prefer to leave this
to others.

Josh? :-)

Comment 3

[Steven Michaud [:smichaud] (Retired)]

Attachment: Less ugly fix (still pretty trivial)

On second thought, maybe I _can_ do better.

Here's a slightly less trivial patch that's considerably less ugly.
You still see errors in the console (which I think is entirely
appropriate), but the button does get drawn (and in the same way as on
Windows and Linux).

Comment 4

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 328698 [details] [diff] [review]
Less ugly fix (still pretty trivial)

This patch is (I think) superceded by my current patch for bug 444864
(attachment 329570 [details] [diff] [review]).

Comment 5

[Steven Michaud [:smichaud] (Retired)]

The autorelease-after-deletion problem is an Apple bug.  See bug 449111
comment #5.

Comment 6

[Daniel Veditz [:dveditz]]

Does that mean we can't fix it? Can we work around it?
Is there an Apple bug reference for the problem?

Comment 7

[Steven Michaud [:smichaud] (Retired)]

We can work around the problem.  The patch for bug 444864 does so.

Comment 8

[Steven Michaud [:smichaud] (Retired)]

Fixed by patch for bug 444864, which was just landed on mozilla-central.

Comment 9

[Steven Michaud [:smichaud] (Retired)]

Reopened because I've backed out my patch for bug 444864 (which
probably caused some reftest failures).

Comment 10

[Steven Michaud [:smichaud] (Retired)]

Fixed by my new patch for bug 444864, which was just landed on mozilla-central.

Comment 11

[Jesse Ruderman]

Crashtest is in (bug 444864 comment 26).

Comment 12

[Daniel Veditz [:dveditz]]

I assume this isn't needed for Firefox 2 because it's cocoa? I'm that's incorrect please nominate for blocking 1.8.1.x

Comment 13

[Steven Michaud [:smichaud] (Retired)]

This bug (bug 444260), bug 444864 and bug 449111 only effect (happen on)
Firefox 3.X -- not Firefox 2.

Comment 14

[Steven Michaud [:smichaud] (Retired)]

Fixed on the 1.9.0 branch by the patch for bug 444864.

Comment 15

[Marcia Knous [:marcia]]

verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b2pre) Gecko/20081013 Minefield/3.1b2pre. I verified by using the testcase in Comment 0.

Comment 16

[Al Billings [:abillings - ex-MoCo]]

Verified for 1.9.0.4 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.4pre) Gecko/2008102104 GranParadiso/3.0.4pre.