877085 - crash in nsGfxScrollFrameInner::GetNondisappearingScrollbarWidth @ nsNativeThemeCocoa::GetMinimumWidgetSize

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Scoobidiver (away)]

It first showed up in 24.0a1/20130528. The regression range is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a39263b0c896&tochange=c190422547ed
It's likely a regression from bug 869314.

Signature 	nsNativeThemeCocoa::GetMinimumWidgetSize(nsRenderingContext*, nsIFrame*, unsigned char, nsIntSize*, bool*) More Reports Search
UUID	bc24da13-f131-465a-9a24-c8f7a2130529
Date Processed	2013-05-29 02:05:03
Uptime	27706
Last Crash	4.0 days before submission
Install Age	7.7 hours since version was first installed.
Install Time	2013-05-28 17:34:24
Product	Firefox
Version	24.0a1
Build ID	20130528030942
Release Channel	nightly
OS	Mac OS X
OS Version	10.8.2 12C3103
Build Architecture	amd64
Build Architecture Info	family 6 model 58 stepping 9
Crash Reason	EXC_BAD_ACCESS / KERN_INVALID_ADDRESS
Crash Address	0x0
App Notes 	
AdapterVendorID: 0x8086, AdapterDeviceID: 0x 166GL Layers? GL Context? GL Context+ GL Layers+ 
Processor Notes 	sp-processor06_phx1_mozilla_com_16597:2012; exploitability tool: ERROR: unable to analyze dump
EMCheckCompatibility	True
Adapter Vendor ID	0x8086
Adapter Device ID	0x 166

Frame 	Module 	Signature 	Source
0 	XUL 	nsNativeThemeCocoa::GetMinimumWidgetSize 	widget/cocoa/nsNativeThemeCocoa.mm:1783
1 	libobjc.A.dylib 	lookUpMethod 	
2 	XUL 	nsGfxScrollFrameInner::GetNondisappearingScrollbarWidth 	layout/generic/nsGfxScrollFrame.cpp:954
3 	XUL 	nsNativeTheme::IsWidgetStyled 	widget/xpwidgets/nsNativeTheme.cpp:330
4 	XUL 	_ZThn112_N17nsHTMLScrollFrame32GetNondisappearingScrollbarWidthEP13nsPresContext 	layout/generic/nsGfxScrollFrame.h:493
5 	XUL 	nsComboboxControlFrame::GetIntrinsicWidth 	layout/forms/nsComboboxControlFrame.cpp:742
6 	XUL 	nsLayoutUtils::IntrinsicForContainer 	layout/base/nsLayoutUtils.cpp:2727

More reports at:
https://crash-stats.mozilla.com/report/list?signature=nsNativeThemeCocoa%3A%3AGetMinimumWidgetSize%28nsRenderingContext*%2C+nsIFrame*%2C+unsigned+char%2C+nsIntSize*%2C+bool*%29

Comment 1

[Stephen A Pohl [:spohl] (OOO until 9/9)]

Attachment: Patch

GetParentScrollbarFrame crashes when aFrame is NULL. We don't need to have a valid frame to retrieve the scrollbar width for non-disappearing scrollbars. This patch avoids any dependency on aFrame.

Comment 2

[Steven Michaud [:smichaud] (Retired)]

> We don't need to have a valid frame to retrieve the scrollbar width
> for non-disappearing scrollbars.

Why is this?  

If it's because non-disappearing scrollbars are currently never
"small", is that a good enough reason?

Comment 3

[Stephen A Pohl [:spohl] (OOO until 9/9)]

(In reply to Steven Michaud from comment #2)
> > We don't need to have a valid frame to retrieve the scrollbar width
> > for non-disappearing scrollbars.
> 
> Why is this?  
> 
> If it's because non-disappearing scrollbars are currently never
> "small", is that a good enough reason?

We're interested in finding out what the width of the traditional scrollbars was in order to display a dropmarker in non-native styled comboboxes. As far as I can tell, these comboboxes always had the regular (not small) scrollbar width.

Comment 4

[Steven Michaud [:smichaud] (Retired)]

I'd feel more comfortable if, for non-disappearing scrollbars, you defaulted to kThemeMetricScrollBarWidth if aFrame is NULL.  You should also add an XXX comment saying you're not sure it's necessary to check scrollbarFrame->StyleDisplay()->mAppearance at all in this case, and why.

Comment 5

[Stephen A Pohl [:spohl] (OOO until 9/9)]

Attachment: Patch

Comment 6

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 755480 [details] [diff] [review]
Patch

Looks good to me.

Another question is why aFrame is sometimes NULL in this code, where it (apparently) never was before.  *That* may be the real bug.  But I wouldn't pursue it unless these crashes are reproducible.

Comment 7

[Stephen A Pohl [:spohl] (OOO until 9/9)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7220b0615ba6

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7220b0615ba6

Comment 9

[Stephen A Pohl [:spohl] (OOO until 9/9)]

Scoobidiver, could you verify that this crash signature disappears in the coming days? Once confirmed I'm going to uplift bug 869314 and this bug here to aurora simultaneously. Thanks!

Comment 10

[Stephen A Pohl [:spohl] (OOO until 9/9)]

If I'm reading the crash stats correctly I think this crash has disappeared.

Comment 11

[Scoobidiver (away)]

(In reply to Stephen Pohl [:spohl] from comment #9)
> Scoobidiver, could you verify that this crash signature disappears in the
> coming days?
There have been no crashes since 24.0a1/20130602. Crashes in 24.0a1/20130531 and 20130601 (post-patch) are in Nightly UX.

Comment 12

[Stephen A Pohl [:spohl] (OOO until 9/9)]

I'll land this on aurora with the combined patch in bug 869314, once the aurora approval has gone through.

Comment 13

[Robert Strong (they/them - no direct email)]

Landed in a combined patch in bug 869314

Comment 14

[Robert Strong (they/them - no direct email)]

(In reply to Robert Strong [:rstrong] (do not email) from comment #13)
> Landed in a combined patch in bug 869314
on mozilla-aurora

Comment 15

[u279076]

(In reply to Scoobidiver from comment #11)
> There have been no crashes since 24.0a1/20130602.

Could you please check if it's a similar story for Firefox 23?

Comment 16

[Scoobidiver (away)]

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #15)
> Could you please check if it's a similar story for Firefox 23?
There have never been crashes in 23.0a2 because the patch of bug 869314 landed in Aurora with this bug's fix.