Closed Bug 1084589 Opened 6 years ago Closed 6 years ago
Crash on Yosemite when changing system appearance
See https://crash-stats.mozilla.com/report/index/99229836-fb5b-4b8b-99e8-9255c2141017 I believe I might have triggered this while toggling the system appearance or toggling the transparency effect. Either of those would likely cause us to handle some event and need to redraw the window, which makes sense from the stack.
Do let us know if you find a way to reproduce this.
And confirmed. We crash on changing the system appearance. Reproduced twice since, once in a new profile.
Summary: Crash on Yosemite, maybe due to changing system theme → Crash on Yosemite when changing system appearance
There are *lots* of these, all in the Yosemite release (which appeared yesterday). This must be an Apple bug. Also, one of these crash addresses is in part an ASCII string (0x707474 == 'ptt'): https://crash-stats.mozilla.com/report/index/9a270cdf-c53c-40c0-95df-cc2c12141017 So on general principles I'm marking this security sensitive. I may unmark it once I've had a closer look, if I decide it's really not.
These aren't just in nightlies. I see them back to FF 32.0.2, and probably would also happen with earlier versions of Firefox. More evidence that this is an Apple bug. I also see them back to Yosemite build 14A386a -- which if I remember right is GM Candidate 1. So that's probably when the bug was introduced. There are lots more crashes on the Yosemite release, but that's probably because far more people are using it.
> We crash on changing the system appearance. Please give STR for one or two variants of this, just for the record.
I expect these crashes also happen in other apps/browsers. Please check.
I can repro these crashes very easily in today's m-c nightly. But not at all so far in FF 33. The 33 branch and earlier crashes may be unrelated, or only distantly related. Will look for regression ranges on the 34 branch and up. Markus and Gijs, I expect one or more of your Yosemite appearance fixes have triggered these crashes.
For the record, here are the ways I've crashed, while today's m-c nightly is running: 1) In the General system pref panel, Change the Appearance from Blue to Graphite or vice versa 2) In the Accessibility system pref panel, check or uncheck Increase contrast.
Needless to say, while this bug remains unresolved we should *not* be recommending that people use non-release versions of Firefox on Yosemite.
I expect these will soon become the #1 Mac topcrasher.
Crash Signature: libobjc.A.dylib@0x10dd → [@ libobjc.A.dylib@0x10dd ] [@ libobjc.A.dylib@0x10e9 ]
Here's the regression range on trunk (mozilla-central): firefox-2014-08-28-03-02-06-mozilla-central firefox-2014-08-29-03-02-04-mozilla-central http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=3be45b58fc47&tochange=d697d649c765
Here's the regression range on aurora: firefox-2014-09-02-00-40-02-mozilla-aurora firefox-2014-09-03-00-40-02-mozilla-aurora This is where aurora changed from the 33 branch to the 34 branch. So the trunk patch that triggered these crashes rode the trains -- it didn't get explicitly uplifted to the aurora branch.
(In reply to Steven Michaud from comment #12) > Here's the regression range on aurora: > > firefox-2014-09-02-00-40-02-mozilla-aurora > firefox-2014-09-03-00-40-02-mozilla-aurora > > This is where aurora changed from the 33 branch to the 34 branch. So the > trunk patch that triggered these crashes rode the trains -- it didn't get > explicitly uplifted to the aurora branch. The original regression range was before the branch point, so that makes sense. Isn't it just going to be one of these patches: 396db9a7dcea Markus Stange — Bug 1058713 - Don't attempt to create a zero-sized CGContext for titlebar buttons. r=smichaud badea3d50106 Markus Stange — Bug 1059278 - Style the sidebar in the Firefox main browser window with Yosemite behind-window vibrancy. r=Gijs ad26257c3e0d Markus Stange — Bug 1051522 - Create NSVisualEffectViews for vibrant window regions. r=smichaud d4bf1bdbf7ec Markus Stange — Bug 1051522 - Add -moz-appearance values -moz-mac-vibrancy-light and -moz-mac-vibrancy-dark for the behind-window vibrancy effect on 10.10. r=roc 965564003cda Markus Stange — Bug 1051522 - Mark nsChildView coordinate conversion functions const. r=smichaud 0eb0a00238ed Markus Stange — Bug 1055018 - Draw CoreUI widgets through -[NSAppearance _drawInRect:context:options:] on 10.10 in order to pick up the 10.10 look. r=smichaud ? I would have thought that we should be able to catch this in the debugger to see what we're passing to Cocoa that makes it choke...
I'm looking at exactly those :-) Will get an lldb stack trace later.
This is the patch that triggered these crashes: http://hg.mozilla.org/mozilla-central/rev/0eb0a00238ed Bug 1055018 - Draw CoreUI widgets through -[NSAppearance _drawInRect:context:options:] on 10.10 in order to pick up the 10.10 look. r=smichaud author Markus Stange <email@example.com> Thu Aug 28 02:15:27 2014 +0200 (at Thu Aug 28 02:15:27 2014 +0200)
So the crashes happen either here: https://hg.mozilla.org/mozilla-central/annotate/51892b39597a/widget/cocoa/nsNativeThemeCocoa.mm#l883 or here: https://hg.mozilla.org/mozilla-central/annotate/51892b39597a/widget/cocoa/nsNativeThemeCocoa.mm#l888 It kind of looks like 'appearance' is invalid. Which if true would definitely be an Apple bug. I'll play around with my interpose library.
> It kind of looks like 'appearance' is invalid. Which if true would definitely be an Apple bug. It's not. It's our bug, and requires only a one word change. Back in a bit.
Yes, these crashes happen when 'appearance' becomes invalid. And it's entirely our fault -- the code as it now stands assumes that what's returned by -[NSAppearance appearanceNamed:] can never change. But that's not true -- it changes whenever the user changes the global appearance.
Assignee: nobody → smichaud
Attachment #8507267 - Flags: review?(gijskruitbosch+bugs)
Attachment #8507267 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8507267 [details] [diff] [review] Fix Landed on mozilla-inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/2ae5cd9d4dca
Comment on attachment 8507267 [details] [diff] [review] Fix Landed on aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/65006dd2ec2b
Comment on attachment 8507267 [details] [diff] [review] Fix Landed on beta: https://hg.mozilla.org/releases/mozilla-beta/rev/3a24d0c65745
Thanks for the quick fix Steven! Does this still need the cloak of security? Sounds like it was totally us and nothing exploitable.
> Thanks for the quick fix Steven! You're quite welcome. Thanks for bringing this to our attention :-) > Does this still need the cloak of security? I'm not really sure. It *is* a use-after-free, which are in principle supposedly exploitable. And there's that weird crash address (or supposed crash address) with an ASCII string in it (from comment #3). But I don't think there's a way for an evil web page (say) to trigger one of these crashes while Firefox is running. And though if you could persuade someone to download something and run it (while FF was still running) it'd be trivially easy to trigger one of these crashes, there are lots of worse things that could happen in that scenario. So there's room for doubt, and I'll leave the decision to someone who knows more about security than I do.
>> Does this still need the cloak of security? > ... > But I don't think there's a way for an evil web page (say) to > trigger one of these crashes while Firefox is running. > ... I think this is the key, and makes this bug non-exploitable. In any case, these crashes happen so often, and are so easily reproducible, that I don't think the STR can be considered a secret. So I'm opening up this bug. That'll make it easier for people to see it, and to dup the inevitable duplicate reports to this one.
Note to myself: There are also crashes here on Yosemite, probably with a similar cause: http://hg.mozilla.org/releases/mozilla-beta/annotate/7a12de89326b/widget/cocoa/VibrancyManager.mm#l159 In other words, we probably need to create the EffectViewClass more than just once.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
(In reply to Steven Michaud from comment #26) > Note to myself: > > There are also crashes here on Yosemite, probably with a similar cause: > http://hg.mozilla.org/releases/mozilla-beta/annotate/7a12de89326b/widget/ > cocoa/VibrancyManager.mm#l159 > > In other words, we probably need to create the EffectViewClass more than > just once. Is bug 1085136 really that rather than a direct dupe of this one?
> Is bug 1085136 really that rather than a direct dupe of this one? No. Once I've investigated further I'll open a new bug. I just wanted to make sure not to forget.
These crash signatures are still showing up for Firefox 34 in Beta, including the latest, 34.0b5 -- but now less than 100 crashes total in the last week over all versions for Firefox. Catalin maybe you can verify this as fixed from the STR rather than from crash-stats.
I filed duplicate bug 1084589 but can no longer reproduce this bug with the steps given in that bug.
> These crash signatures are still showing up for Firefox 34 in Beta, including the latest, 34.0b5 This bug's crash "signature" is very generic -- two different offsets in objc_msgSend, which is used in calling almost every Objective-C "method". What you're seeing is almost certainly not this bug. I see a bunch of crashes involving plugin IPC (on the 34 and 35 branches). I'll open a bug on them (if one hasn't already been opened).
I've opened bug 1093330. It's unrelated. I mention it because it's (probably) the cause of the crashes you've been seeing, Liz.
Verified as fixed using the following environment: FF 34.06 Build Id:20141103144234 FF 35 Build Id:20141104004002 FF 36 Build Id:20141104030202 OS: Mac OS X 10.10
You need to log in before you can comment on or make changes to this bug.