Closed Bug 764176 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: Widget: Cocoa, defect)

x86_64
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla17
Tracking Status
firefox14 --- wontfix
firefox15 + verified
firefox16 + verified
firefox17 --- verified
firefox-esr10 15+ verified

People

(Reporter: jruderman, Assigned: smichaud)

References

Details

(5 keywords, Whiteboard: [advisory-tracking+] STR in comment #10, maybe OS X 10.7 only?)

Crash Data

Attachments

(4 files)

      No description provided.
Attached file stack trace (gdb)
Why doesn't Breakpad make it out of CoreGraphics?  Bug 650239?
Keywords: sec-critical
Only crashes if the testcase is the first thing loaded in the session?
Component: Graphics → Widget: Cocoa
QA Contact: thebes → cocoa
Anyone interested in looking for a regression range?
I wonder if this only happens with certain versions of OS X.
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.
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.
Without better STR (or a better testcase), I'd say this bug isn't reproducible.
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.
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.
Whiteboard: STR in comment #10
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
Markus may have ideas.
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.
> 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).
(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.
> drawRect.origin.y -= extendUpwards;
> drawRect.size.height += extendUpwards;

Commenting out these two lines (in current code) doesn't stop the crashes happening.
Blocks: 668195
TenFourFox is not vulnerable to this crash. We don't use CoreUI at all because we support 10.4.
Assignee: nobody → smichaud
Whiteboard: STR in comment #10 → STR in comment #10, maybe OS X 10.7 only?
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).
(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.
(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.
That was certainly our fix for various image problems -- just cap the size of what we'll deal with.
Keywords: sec-vector
Yes.  The problem is figuring out what size to cap at :-(
Should we be filing a radr bug? (I'm probably confused)
We don't (yet) have enough information for a bug report to Apple.
Steven can we get a mitigation into 15 and 16? Can we cap to the biggest reasonable resolution (times 2 maybe?)
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.
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).
This OK, Markus?
Attachment #644057 - Flags: review?(mstange)
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/smichaud@pobox.com-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.
(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?
Alternately - Benoit, do you want to steal the review from Markus?
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.
Attachment #644057 - Flags: review?(mstange) → review+
> 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 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.
https://hg.mozilla.org/mozilla-central/rev/970f2d233615
Flags: in-testsuite-
Target Milestone: --- → mozilla17
(Leaving open per comment #36)

Also, should this have a test?
Flags: in-testsuite- → in-testsuite?
(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.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Please nominate for branch uplift, including ESR10.
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
Attachment #644057 - Flags: approval-mozilla-esr10?
Attachment #644057 - Flags: approval-mozilla-beta?
Attachment #644057 - Flags: approval-mozilla-aurora?
Comment on attachment 644057 [details] [diff] [review]
Cap "area" drawn by CUIDraw()

low risk, approving across all branches.
Attachment #644057 - Flags: approval-mozilla-esr10?
Attachment #644057 - Flags: approval-mozilla-esr10+
Attachment #644057 - Flags: approval-mozilla-beta?
Attachment #644057 - Flags: approval-mozilla-beta+
Attachment #644057 - Flags: approval-mozilla-aurora?
Attachment #644057 - Flags: approval-mozilla-aurora+
Whiteboard: STR in comment #10, maybe OS X 10.7 only? → [advisory-tracking+] STR in comment #10, maybe OS X 10.7 only?
Whiteboard: [advisory-tracking+] STR in comment #10, maybe OS X 10.7 only? → [advisory-tracking+][qa+] STR in comment #10, maybe OS X 10.7 only?
I'm willing to work on the in-testsuite request if someone can provide me some mentorship.
QA Contact: anthony.s.hughes
Keywords: verifyme
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
Status: RESOLVED → VERIFIED
Keywords: verifyme
Whiteboard: [advisory-tracking+][qa+] STR in comment #10, maybe OS X 10.7 only? → [advisory-tracking+] STR in comment #10, maybe OS X 10.7 only?
Group: core-security
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: