Closed Bug 1084589 Opened 9 years ago Closed 9 years ago

Crash on Yosemite when changing system appearance

Categories

(Core :: Widget: Cocoa, defect)

Product:
Core
Component:
Widget: Cocoa
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla36
Tracking Flags:
Tracking Status
firefox34 + verified
firefox35 --- verified
firefox36 --- verified

People

(Reporter: zpao, Assigned: smichaud)

References

Details

(Keywords: crash, topcrash-mac)

Crash Data

Attachments

(2 files)

lldb all-thread stack trace
50.03 KB, text/plain
Details
Fix
1012 bytes, patch
Gijs
: review+
Gavin
: approval-mozilla-aurora+
Gavin
: approval-mozilla-beta+
Details | Diff | Splinter Review 
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.
Group: core-security
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.
Keywords: topcrash-mac
Severity: normal → critical
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 <mstange@themasta.com>
	Thu Aug 28 02:15:27 2014 +0200 (at Thu Aug 28 02:15:27 2014 +0200)
Blocks: 1055018
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.
Attached patch Fix — — Splinter Review 
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+
Attachment #8507267 - Flags: approval-mozilla-beta+
Attachment #8507267 - Flags: approval-mozilla-aurora+
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.
Group: core-security
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.
https://hg.mozilla.org/mozilla-central/rev/2ae5cd9d4dca
Status: NEW → RESOLVED
Closed: 9 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?
Flags: needinfo?(smichaud)
> 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.
Flags: needinfo?(smichaud)
I've opened bug 1085475 on what I discussed in comment #26.
Flags: qe-verify+
QA Contact: catalin.varga
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
Status: RESOLVED → VERIFIED
status-firefox34: fixed → verified
status-firefox35: fixed → verified
status-firefox36: fixed → verified
You need to log in before you can comment on or make changes to this bug.