318 bytes, text/html
14.80 KB, text/plain
27.01 KB, text/plain
7.14 KB, patch
|Details | Diff | Splinter Review|
Created attachment 632428 [details] stack trace (breakpad + minidump_stackwalk) bp-7d2d7035-f989-48ae-bbc8-5efd72120612
Why doesn't Breakpad make it out of CoreGraphics? Bug 650239?
Only crashes if the testcase is the first thing loaded in the session?
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.
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
Nope, it translates to: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=87e3ea12ed5d&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.
TenFourFox is not vulnerable to this crash. We don't use CoreUI at all because we support 10.4.
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.
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).
Created attachment 644057 [details] [diff] [review] Cap "area" drawn by CUIDraw() This OK, Markus?
For what it's worth, here's a tryserver build of my patch from comment #30: http://firstname.lastname@example.org/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.
> 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.
(Leaving open per comment #36) Also, should this have a test?
(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.
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
Comment on attachment 644057 [details] [diff] [review] Cap "area" drawn by CUIDraw() low risk, approving across all branches.
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
I'm willing to work on the in-testsuite request if someone can provide me some mentorship.
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