Crash [@ nsNativeThemeCocoa::DrawPushButton] with MathML

RESOLVED FIXED in mozilla1.9.1b2

Status

()

Core
Widget: Cocoa
--
critical
RESOLVED FIXED
9 years ago
2 years ago

People

(Reporter: Jesse Ruderman, Assigned: mstange)

Tracking

(Blocks: 2 bugs, {crash, testcase})

Trunk
mozilla1.9.1b2
x86
Mac OS X
crash, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?], crash signature)

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

9 years ago
Created attachment 343463 [details]
testcase (crashes Firefox when loaded)

Loading the testcase in a debug Firefox build triggers:

firefox-bin[2945] <Error>: Unable to create bitmap delegate device
firefox-bin[2945] <Error>: createBitmapContext: failed to create delegate.
firefox-bin[2945] <Error>: CGContextTranslateCTM: invalid context
objc[2945]: FREED(id): message autorelease sent to freed object=0x1ca26120
Crash [@ _freedHandler]

If MallocScribble is enabled, the "freed object" warning doesn't appear, and the crash occurs [@ objc_msgSend] instead, dereferencing 0x55555575.  Either way, nsNativeThemeCocoa::DrawPushButton is on the stack.

I guess the fix for bug 444864 didn't take care of this :(
(Reporter)

Updated

9 years ago
Whiteboard: [sg:critical?]
Oddly, I don't crash, or see anything in the system console -- either
on the trunk or the 1.9.0 branch.

So far I haven't tested on OS X 10.4.11, though -- only on 10.5.5.
(Reporter)

Comment 2

9 years ago
I saw this on a mac mini and a macbook pro (both intel, 10.5.5, mozilla-central).
This is a recent "regression" -- it starts happening with the
2008-10-14-02-mozilla-central nightly.  It doesn't happen at all on
the 1.9.0 branch (in today's nightly).

Here's the regression range:

http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2008-10-13+00%3A00%3A00&enddate=2008-10-14+05%3A31%3A00

But I can't find anything there that could reasonably have "caused"
this problem.  So whatever it was just probably uncovered an already
existing problem.

Side note:  I *really* miss Bonsai when looking for regression ranges.
It lists each file changed for every commit, which makes it *much*
easier to look for possible causes of regressions.  Descriptions and
bug numbers just aren't enough.
(Assignee)

Comment 5

9 years ago
(In reply to comment #3)
> But I can't find anything there that could reasonably have "caused"
> this problem.

In bug 394892, which is in the regression range, I changed DrawCellWithScaling. It's possible that I broke something there (I think I shuffled some CTM stuff).
Created attachment 343557 [details]
Gdb trace with debug symbols and console output
Assignee: joshmoz → smichaud
Status: NEW → ASSIGNED
(In reply to comment #5)

Thanks, Markus.  I'll look into that.

Oddly, I didn't see the whole regression range when I first looked.
Only about the top third showed up, and the little "loading" symbol
(in the upper right) continued to spin (for at least 10 minutes).  I
figured that was a fluke, and ignored it.  Now I see the whole page.
Go figure.
Yup, Markus, here you blew away my fix for bug 444864:

-  if (xscale == 1.0f && yscale == 1.0f) {
+  if ((!needsBuffer && drawRect.size.width == destRect.size.width &&
+       drawRect.size.height == destRect.size.height) || noBufferOverride) {

I'll try to figure out how to restore my patch and still keep your changes.
Actually comment #8 is wrong.  But the changes to DrawCellWithScaling() must somehow (I think) be responsible.  I'll keep digging.
Created attachment 343608 [details] [diff] [review]
Fix

I've figured out what was happening:

Part of the new code was dividing by zero, which caused
drawRect.size.width to be set to 'nan', which wreaked havoc later on.

So Markus' changes didn't break my patch for bug 444864, and this bug
is (almost) completely unrelated.  But (just to be safe) I tried the
testcases for bug 444864, bug 444260 and bug 449111, and had no
problems.  I even tried these testcases after having changed the code
to make DrawCellWithScaling()'s 'flip' parameter always true -- still
no problems.

I say *almost* completely unrelated because it's the same Apple bug
that gets triggered here as in bug 444260 and bug 449111 (see bug
449111 comment #5).
Attachment #343608 - Flags: review?(mstange)
(Assignee)

Comment 12

9 years ago
So the real problem here is that the destRect's size is zero, which should never happen. Drawing invisible frames doesn't make sense.
I think it would be better to return early in DrawWidgetBackground if the draw rect's width or height are 0. There's no need to even attempt to draw controls that will never end up on the screen.
That way, we could rely on being able to divide by the drawRect's dimensions, which is what I did in bug 394892 in the first place. (It never occured to me that these dimensions could be zero.)

I don't know the display list code, but to me it seems strange that frames with a zero width / height can end up in the display list at all. Maybe we should find out how that happens in this case?
(In reply to comment #12)

> So the real problem here is that the destRect's size is zero, which
> should never happen.

I wondered about that.

> I think it would be better to return early in DrawWidgetBackground
> if the draw rect's width or height are 0. There's no need to even
> attempt to draw controls that will never end up on the screen.

Makes sense.  But we should at least attempt to test this ... and
you're much better equipped to do so than I am :-)

Why don't you try your idea for a patch (or try combining my patch
with it), and test that out?  If everything works, assign this bug to
yourself and carry on.

> I don't know the display list code, but to me it seems strange that
> frames with a zero width / height can end up in the display list at
> all. Maybe we should find out how that happens in this case?

Good idea.  But how about you do it? :-)
(Assignee)

Comment 14

9 years ago
Created attachment 343903 [details] [diff] [review]
fix v2
Assignee: smichaud → mstange
Attachment #343608 - Attachment is obsolete: true
Attachment #343903 - Flags: review?(roc)
Attachment #343608 - Flags: review?(mstange)
Comment on attachment 343903 [details] [diff] [review]
fix v2

Is it possible that the rect was some small number of appunits and got rounded away to zero?
(Assignee)

Comment 16

9 years ago
In this case the frame's rect is in fact 0x0 appunits.
(Assignee)

Comment 17

9 years ago
pushed: http://hg.mozilla.org/mozilla-central/rev/c02be3f51a31

I'm going to create the crashtest now.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b2
(Assignee)

Comment 18

9 years ago
Created attachment 344308 [details] [diff] [review]
crashtest
Attachment #344308 - Flags: review?(roc)
(Assignee)

Comment 19

9 years ago
pushed the crashtest: http://hg.mozilla.org/mozilla-central/rev/d388f6ab98a1

Beta 2 has shipped, can this bug be opened now?
(Reporter)

Updated

9 years ago
Flags: in-testsuite+
Crash Signature: [@ nsNativeThemeCocoa::DrawPushButton]

Updated

2 years ago
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.