764176 - Crash with gradient, hugeness, "-moz-appearance: statusbar"

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

Comment 1

[Jesse Ruderman]

Attachment: stack trace (gdb)

Comment 2

[Jesse Ruderman]

Attachment: stack trace (breakpad + minidump_stackwalk)

bp-7d2d7035-f989-48ae-bbc8-5efd72120612

Comment 3

[Jesse Ruderman]

Why doesn't Breakpad make it out of CoreGraphics?  Bug 650239?

Comment 4

[Jesse Ruderman]

Only crashes if the testcase is the first thing loaded in the session?

Comment 5

[Steven Michaud [:smichaud] (Retired)]

Anyone interested in looking for a regression range?

Comment 6

[Steven Michaud [:smichaud] (Retired)]

I wonder if this only happens with certain versions of OS X.

Comment 7

[Steven Michaud [:smichaud] (Retired)]

This bug is *very* hard to reproduce.

Loading the testcase first thing in the session doesn't help.

Running ".../firefox [testcaseurl]" or ".../firefox -foreground [testcaseurl]" from the commandline doesn't help.

Just about the only way I can repro a crash is if one has (accidentally) already happened, and Firefox on restart opens to the same page.

Comment 8

[Steven Michaud [:smichaud] (Retired)]

Even making this bug's testcase my home page doesn't help.

I've been testing with the same mozilla-central nightly Jesse used -- yesterday's.

Comment 9

[Steven Michaud [:smichaud] (Retired)]

Without better STR (or a better testcase), I'd say this bug isn't reproducible.

Comment 10

[Steven Michaud [:smichaud] (Retired)]

Got it, I think:

Maximize your window before loading the testcase.  I think the window has to be a certain size (or larger) to reproduce the crashes.

Comment 11

[Steven Michaud [:smichaud] (Retired)]

With my STR from comment #10, I've found that these crashes only happen on OS X 10.7.

I've also found a regression range -- somewhere between FF 7.0.1 and 8.0.  Later I'll narrow this down to two mozilla-central nightlies.

Comment 12

[Steven Michaud [:smichaud] (Retired)]

Here's the mozilla-central regression range:

firefox-2011-08-09-03-07-51-mozilla-central
firefox-2011-08-10-03-07-38-mozilla-central

Which translates to:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=08327218cb8b&tochange=04dfb49d3a3d

Comment 13

[Steven Michaud [:smichaud] (Retired)]

Nope, it translates to:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=87e3ea12ed5d&tochange=04dfb49d3a3d

Comment 14

[Joe Drew (not getting mail)]

Markus may have ideas.

Comment 15

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

93c610640952	Markus Stange — bug 668195 - Use CoreUI to draw window chrome and toolbars. r=josh

That switched us to using the CUI APIs for doing the drawing of this stuff, so whatever APIs we were using before seemed to not have this problem.

The comment in the (current) code says:

  // kCUIWidgetWindowFrame draws a complete window frame with both title bar
  // and bottom bar. We only want the bottom bar, so we extend the draw rect
  // upwards to make space for the title bar, and then we clip it away.
  CGRect drawRect = inBoxRect;
  const int extendUpwards = 40;
  drawRect.origin.y -= extendUpwards;
  drawRect.size.height += extendUpwards;

So in theory with a fullscreen window we could get origin.y being negative.  This is fine from a graphics perspective, and all the CG APIs in the stack should handle it just fine, but who knows if some optimization is being triggered that causes a problem here.

I'd suggest trying to reproduce the bug with a window that's not quite mazimized -- maybe 50 pixels shorter than maximum, but flush up against the bottom of the display leaving space open at the top.

Comment 16

[Steven Michaud [:smichaud] (Retired)]

> 93c610640952 Markus Stange — bug 668195 - Use CoreUI to draw window chrome and toolbars. r=josh

I've confirmed this by building with (http://hg.mozilla.org/mozilla-central/rev/93c610640952) at tip (we crash) and with the previously landed patch (http://hg.mozilla.org/mozilla-central/rev/820ac76cec76) at tip (we don't crash).

Comment 17

[(not currently active) Ted Mielczarek]

(In reply to Jesse Ruderman from comment #3)
> Why doesn't Breakpad make it out of CoreGraphics?  Bug 650239?

Pretty much. We don't have symbols for it, so we don't know how to get out of it properly.

Comment 18

[Steven Michaud [:smichaud] (Retired)]

> drawRect.origin.y -= extendUpwards;
> drawRect.size.height += extendUpwards;

Commenting out these two lines (in current code) doesn't stop the crashes happening.

Comment 19

[Cameron Kaiser [:spectre]]

TenFourFox is not vulnerable to this crash. We don't use CoreUI at all because we support 10.4.

Comment 20

[Steven Michaud [:smichaud] (Retired)]

Markus, what sort of documentation (or sample code) for the CoreUI did you work from writing your patch for bug 668195?  Or did you have any?

When I finally get back to this, the first thing I'll need to do is double-check that you're using it correctly (by reverse-engineering how (say) Safari uses it).

Comment 21

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #20)

Never mind.  I re-read bug 668195 comment #0 more carefully and now see what you were working from :-)

But from what you say in comment #0 you may not have done much reverse-engineering on 10.7.  So it's possible the CoreUI stuff behaves slightly differently there.

I'll do more checking once I get back to this.

Comment 22

[Markus Stange [:mstange]]

(In reply to Steven Michaud from comment #21)
> So it's possible the CoreUI stuff behaves
> slightly differently there.

Sure, that's definitely possible. But maybe CoreUI window rendering just doesn't anticipate extreme sizes since windows usually aren't that large. I think we should just refuse to draw anything if the draw rect exceeds a certain size.

Comment 23

[Daniel Veditz [:dveditz]]

That was certainly our fix for various image problems -- just cap the size of what we'll deal with.

Comment 24

[Steven Michaud [:smichaud] (Retired)]

Yes.  The problem is figuring out what size to cap at :-(

Comment 25

[David Bolter [:davidb] (NeedInfo me for attention)]

Should we be filing a radr bug? (I'm probably confused)

Comment 26

[Steven Michaud [:smichaud] (Retired)]

We don't (yet) have enough information for a bug report to Apple.

Comment 27

[David Bolter [:davidb] (NeedInfo me for attention)]

Steven can we get a mitigation into 15 and 16? Can we cap to the biggest reasonable resolution (times 2 maybe?)

Comment 28

[Steven Michaud [:smichaud] (Retired)]

I'll try to hack something out next week.

We can't know exactly where the cap(s) should be without extensive testing (which I won't have time for).  But reasonable guesses should allow me to write a bandaid fix.

Comment 29

[Steven Michaud [:smichaud] (Retired)]

I can no longer reproduce these crashes in current trunk code.  So I'm going to write a patch that bails if we'd be asking CUIDraw to draw a rectangle whose "area" (width * height) is larger than an arbitrary value.

We've already been doing this kind of thing for several years in nsNativeThemeCocoa.mm's DrawCellWithScaling() and RenderTransformedHIThemeControl(), without any reports of problems.  So I'll just borrow the value we used there (BITMAP_MAX_AREA == 500000).

Comment 30

[Steven Michaud [:smichaud] (Retired)]

Attachment: Cap "area" drawn by CUIDraw()

This OK, Markus?

Comment 31

[Steven Michaud [:smichaud] (Retired)]

For what it's worth, here's a tryserver build of my patch from comment #30:

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-0ac139f6e412/try-macosx64/firefox-17.0a1.en-US.mac.dmg

Please test with it if you're still able to reproduce this bug in current trunk code.

Comment 32

[Alex Keybl [:akeybl]]

(In reply to Steven Michaud from comment #30)
> Created attachment 644057 [details] [diff] [review]
> Cap "area" drawn by CUIDraw()
> 
> This OK, Markus?

We're looking to land this in FF15 if deemed low risk. Markus - would you mind reviewing?

Comment 33

[Joe Drew (not getting mail)]

Alternately - Benoit, do you want to steal the review from Markus?

Comment 34

[Markus Stange [:mstange]]

Comment on attachment 644057 [details] [diff] [review]
Cap "area" drawn by CUIDraw()

Looks good, but I don't know if it will prevent these crashes completely. For example, I could imagine that we would still crash when drawing a small area (smaller than the cap that this patch imposes) if the transform applied to the element is scaled up by a huge factor. Note that this patch only checks the pre-transform paint size.

Comment 35

[Steven Michaud [:smichaud] (Retired)]

> I could imagine that we would still crash when drawing a small area
> (smaller than the cap that this patch imposes) if the transform
> applied to the element is scaled up by a huge factor

Which transform?  The one belonging to the CGContextRef passed into
CUIDraw() (accessible using CGContextGetCTM())?

If so, it'd be easy enough for me to use it to scale the area before
checking its size.

Comment 36

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 644057 [details] [diff] [review]
Cap "area" drawn by CUIDraw()

Landed on mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/970f2d233615

Since this is a bit urgent, let's take my current patch now.  We can alter it later once Markus responds.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/970f2d233615

Comment 38

[Ryan VanderMeulen [:RyanVM]]

(Leaving open per comment #36)

Also, should this have a test?

Comment 39

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #38)

The testcase for this bug no longer works in current code.  Until we get one that does, I don't think it'll be possible to write a test.

Comment 40

[Lukas Blakk [:lsblakk] use ?needinfo]

Please nominate for branch uplift, including ESR10.

Comment 41

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 644057 [details] [diff] [review]
Cap "area" drawn by CUIDraw()

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Probable Apple bug triggered by patch for bug 668195 (which uses CoreUI APIs directly)
User impact if declined: Possible exposure to a security exploit
Testing completed (on m-c, etc.): Minimal testing on m-c
Risk to taking this patch (and alternatives if risky): Very low risk (analogous to patch for bug 464589, which has been in tree for five years without causing trouble)
String or UUID changes made by this patch: none

Comment 42

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 644057 [details] [diff] [review]
Cap "area" drawn by CUIDraw()

low risk, approving across all branches.

Comment 43

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 644057 [details] [diff] [review]
Cap "area" drawn by CUIDraw()

Landed on branches:
http://hg.mozilla.org/releases/mozilla-aurora/rev/cf0d8079a6d8
http://hg.mozilla.org/releases/mozilla-beta/rev/8971a306e188
http://hg.mozilla.org/releases/mozilla-esr10/rev/f78a0fd268cf

Comment 44

[u279076]

I'm willing to work on the in-testsuite request if someone can provide me some mentorship.

Comment 45

[u279076]

Confirmed testcase crashes 2012-06-12 Firefox 16.0a1 Nightly Debug.

Does not crash:
2012-08-02 Firefox 17.0a1 Nightly Debug
2012-08-04 Firefox 16.0a2 Aurora Debug
2012-08-04 Firefox 15.0 Beta Debug
2012-08-04 Firefox 10.0.7 ESR Debug