Last Comment Bug 764176 - Crash with gradient, hugeness, "-moz-appearance: statusbar"
: Crash with gradient, hugeness, "-moz-appearance: statusbar"
Status: VERIFIED FIXED
[advisory-tracking+] STR in comment #...
: crash, regression, sec-critical, sec-vector, testcase
Product: Core
Classification: Components
Component: Widget: Cocoa (show other bugs)
: Trunk
: x86_64 Mac OS X
: -- critical (vote)
: mozilla17
Assigned To: Steven Michaud [:smichaud] (Retired)
: Anthony Hughes (:ashughes) [GFX][QA][Mentor]
: Markus Stange [:mstange]
Mentors:
Depends on:
Blocks: randomstyles 668195
  Show dependency treegraph
 
Reported: 2012-06-12 14:58 PDT by Jesse Ruderman
Modified: 2012-10-21 22:14 PDT (History)
13 users (show)
ryanvm: in‑testsuite?
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
+
verified
+
verified
verified
15+
verified


Attachments
testcase (crashes Firefox when loaded) (318 bytes, text/html)
2012-06-12 14:58 PDT, Jesse Ruderman
no flags Details
stack trace (gdb) (14.80 KB, text/plain)
2012-06-12 14:58 PDT, Jesse Ruderman
no flags Details
stack trace (breakpad + minidump_stackwalk) (27.01 KB, text/plain)
2012-06-12 14:58 PDT, Jesse Ruderman
no flags Details
Cap "area" drawn by CUIDraw() (7.14 KB, patch)
2012-07-19 16:05 PDT, Steven Michaud [:smichaud] (Retired)
mstange: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
lukasblakk+bugs: approval‑mozilla‑esr10+
Details | Diff | Splinter Review

Description Jesse Ruderman 2012-06-12 14:58:02 PDT
Created attachment 632425 [details]
testcase (crashes Firefox when loaded)
Comment 1 Jesse Ruderman 2012-06-12 14:58:23 PDT
Created attachment 632427 [details]
stack trace (gdb)
Comment 2 Jesse Ruderman 2012-06-12 14:58:57 PDT
Created attachment 632428 [details]
stack trace (breakpad + minidump_stackwalk)

bp-7d2d7035-f989-48ae-bbc8-5efd72120612
Comment 3 Jesse Ruderman 2012-06-12 15:02:57 PDT
Why doesn't Breakpad make it out of CoreGraphics?  Bug 650239?
Comment 4 Jesse Ruderman 2012-06-12 15:08:38 PDT
Only crashes if the testcase is the first thing loaded in the session?
Comment 5 Steven Michaud [:smichaud] (Retired) 2012-06-13 09:01:04 PDT
Anyone interested in looking for a regression range?
Comment 6 Steven Michaud [:smichaud] (Retired) 2012-06-13 09:02:25 PDT
I wonder if this only happens with certain versions of OS X.
Comment 7 Steven Michaud [:smichaud] (Retired) 2012-06-13 09:12:58 PDT
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) 2012-06-13 09:15:57 PDT
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) 2012-06-13 09:16:40 PDT
Without better STR (or a better testcase), I'd say this bug isn't reproducible.
Comment 10 Steven Michaud [:smichaud] (Retired) 2012-06-13 09:32:11 PDT
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) 2012-06-13 09:48:12 PDT
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) 2012-06-13 10:21:27 PDT
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) 2012-06-13 10:23:34 PDT
Nope, it translates to:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=87e3ea12ed5d&tochange=04dfb49d3a3d
Comment 14 Joe Drew (not getting mail) 2012-06-13 10:59:50 PDT
Markus may have ideas.
Comment 15 Vladimir Vukicevic [:vlad] [:vladv] 2012-06-13 11:02:53 PDT
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) 2012-06-13 12:57:58 PDT
> 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 Ted Mielczarek [:ted.mielczarek] 2012-06-13 13:19:01 PDT
(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) 2012-06-13 13:42:01 PDT
> 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] 2012-06-14 06:48:14 PDT
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) 2012-06-15 07:40:23 PDT
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) 2012-06-15 08:21:22 PDT
(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] 2012-06-15 11:23:53 PDT
(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] 2012-06-21 13:16:45 PDT
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) 2012-06-21 13:56:19 PDT
Yes.  The problem is figuring out what size to cap at :-(
Comment 25 David Bolter [:davidb] 2012-06-28 13:52:36 PDT
Should we be filing a radr bug? (I'm probably confused)
Comment 26 Steven Michaud [:smichaud] (Retired) 2012-06-28 14:08:08 PDT
We don't (yet) have enough information for a bug report to Apple.
Comment 27 David Bolter [:davidb] 2012-07-12 13:08:30 PDT
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) 2012-07-12 15:42:59 PDT
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) 2012-07-19 15:21:18 PDT
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) 2012-07-19 16:05:14 PDT
Created attachment 644057 [details] [diff] [review]
Cap "area" drawn by CUIDraw()

This OK, Markus?
Comment 31 Steven Michaud [:smichaud] (Retired) 2012-07-20 12:41:15 PDT
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.
Comment 32 Alex Keybl [:akeybl] 2012-07-26 17:06:27 PDT
(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) 2012-07-26 18:42:57 PDT
Alternately - Benoit, do you want to steal the review from Markus?
Comment 34 Markus Stange [:mstange] 2012-07-27 07:17:37 PDT
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) 2012-07-30 13:27:51 PDT
> 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) 2012-08-01 08:33:56 PDT
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] 2012-08-01 14:56:10 PDT
https://hg.mozilla.org/mozilla-central/rev/970f2d233615
Comment 38 Ryan VanderMeulen [:RyanVM] 2012-08-01 14:56:47 PDT
(Leaving open per comment #36)

Also, should this have a test?
Comment 39 Steven Michaud [:smichaud] (Retired) 2012-08-01 15:05:59 PDT
(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 2012-08-02 16:30:57 PDT
Please nominate for branch uplift, including ESR10.
Comment 41 Steven Michaud [:smichaud] (Retired) 2012-08-03 08:24:21 PDT
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 2012-08-03 11:26:34 PDT
Comment on attachment 644057 [details] [diff] [review]
Cap "area" drawn by CUIDraw()

low risk, approving across all branches.
Comment 44 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-14 15:16:57 PDT
I'm willing to work on the in-testsuite request if someone can provide me some mentorship.
Comment 45 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-21 15:48:11 PDT
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

Note You need to log in before you can comment on or make changes to this bug.