812776 - crash in nsCSSRendering::PrepareBackgroundLayer @ nsIFrame::GetStyleVisibility

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Scoobidiver (away)]

With that stack trace, it first showed up in 19.0a1/20121107 and is #21 top crasher in 19.0a1. The regression window is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f9c2c266e7aa&tochange=e587aa26326e
It might be a regression from bug 798964.

Signature 	nsIFrame::GetStyleVisibility() More Reports Search
UUID	3b999546-e411-419e-a590-ae0512121116
Date Processed	2012-11-16 07:36:22
Uptime	1634
Last Crash	1.7 weeks before submission
Install Age	13.0 hours since version was first installed.
Install Time	2012-11-15 17:34:35
Product	Firefox
Version	19.0a1
Build ID	20121115030705
Release Channel	nightly
OS	Windows NT
OS Version	6.1.7601 Service Pack 1
Build Architecture	x86
Build Architecture Info	AuthenticAMD family 16 model 4 stepping 3
Crash Reason	EXCEPTION_ACCESS_VIOLATION_READ
Crash Address	0xfffffffff0de8037
App Notes 	
AdapterVendorID: 0x10de, AdapterDeviceID: 0x0ca3, AdapterSubsysID: 00000000, AdapterDriverVersion: 8.16.11.9107
D3D10 Layers? D3D10 Layers- D3D9 Layers? D3D9 Layers- 
Processor Notes 	This dump is too long and has triggered the automatic truncation routine
EMCheckCompatibility	True
Adapter Vendor ID	0x10de
Adapter Device ID	0x0ca3
Total Virtual Memory	2147352576
Available Virtual Memory	1732386816
System Memory Use Percentage	36
Available Page File	3159334912
Available Physical Memory	1356132352

Frame 	Module 	Signature 	Source
0 	xul.dll 	nsIFrame::GetStyleVisibility 	layout/style/nsStyleStructList.h:61
1 	xul.dll 	nsCSSRendering::PrepareBackgroundLayer 	layout/base/nsCSSRendering.cpp:2837
2 	xul.dll 	nsDisplayBackground::GetBounds 	layout/base/nsDisplayList.cpp:2160
3 	xul.dll 	nsOverflowClipWrapper::WrapList 	layout/generic/nsFrame.cpp:1659
4 	xul.dll 	nsDisplayWrapper::WrapListsInPlace 	layout/base/nsDisplayList.cpp:2725
5 	xul.dll 	nsIFrame::OverflowClip 	layout/generic/nsFrame.cpp:1714
6 	xul.dll 	nsGfxScrollFrameInner::BuildDisplayList 	layout/generic/nsGfxScrollFrame.cpp:2166
7 	xul.dll 	nsHTMLScrollFrame::BuildDisplayList 	layout/generic/nsGfxScrollFrame.h:374
8 	xul.dll 	nsIFrame::BuildDisplayListForChild 	layout/generic/nsFrame.cpp:2288
9 	xul.dll 	nsBlockFrame::BuildDisplayList 	layout/generic/nsBlockFrame.cpp:6188
10 	xul.dll 	nsIFrame::BuildDisplayListForStackingContext 	layout/generic/nsFrame.cpp:1938
11 	xul.dll 	nsIFrame::BuildDisplayListForChild 	layout/generic/nsFrame.cpp:2262
12 	xul.dll 	nsBlockFrame::BuildDisplayList 	layout/generic/nsBlockFrame.cpp:6188
...

More reports at:
https://crash-stats.mozilla.com/report/list?signature=nsIFrame%3A%3AGetStyleVisibility%28%29

Comment 1

[Mats Palmgren (inactive)]

> It might be a regression from bug 798964.

Yeah, seems likely.  Frame #2 above is in code added in that bug.
http://hg.mozilla.org/mozilla-central/annotate/a761bfc192b5/layout/base/nsDisplayList.cpp#l2160

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think the problem is that we're calling methods on InlineBackgroundData (via nsCSSRendering::ComputeBackgroundPositioningArea/PrepareBackgroundLayer, via nsDisplayBackgroundImage::GetBounds) which set up cached frame pointers, and later we delete those frames, or at least mBlockFrame. Then a later call to ComputeBackgroundPositioningArea tries to use the cached mBlockFrame and dies.

Before, we tried to prevent this sort of thing by calling InlineBackgroundData::Reset after painting (via nsCSSRendering::DidPaint), because we "knew" that PrepareBackgroundLayer was only called during painting and was always followed by a call to DidPaint before any frames could be destroyed. That worked but that invariant is too fragile to rely on anymore.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://bugzilla.mozilla.org/show_bug.cgi?id=812776

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #4)
> https://bugzilla.mozilla.org/show_bug.cgi?id=812776

I meant https://hg.mozilla.org/integration/mozilla-inbound/rev/36fcf82a515b

Comment 6

[Robert Kaiser]

roc, this is marked as not affecting 18, but you nominated for there, does that mean it actually is affected and we should uplift?

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/36fcf82a515b

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 683014 [details] [diff] [review]
fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 798964
User impact if declined: Some amount of crashes due to regression in bug 798964 which just landed in FF18
Testing completed (on m-c, etc.): none really
Risk to taking this patch (and alternatives if risky): This is low risk. It's easy to see that we just call gInlineBGData->Reset() strictly more often, which is completely safe.
String or UUID changes made by this patch: none

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Robert Kaiser (:[e-mail]) from comment #6)
> roc, this is marked as not affecting 18, but you nominated for there, does
> that mean it actually is affected and we should uplift?

Yes. It only just started affecting FF18 because we landed bug 798964 there (which we had to do)

Comment 10

[Alex Keybl [:akeybl]]

Comment on attachment 683014 [details] [diff] [review]
fix

[Triage Comment]
While this isn't a top crash, it is a new crash regression. We're early enough in the cycle to land a fix of this nature. Approving for Beta 18.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d44ff5554e98

Comment 12

[Ioana (away)]

This crash is still reproducing on Firefox 18 beta 1 - 20121121075611:
https://crash-stats.mozilla.com/report/list?query_search=signature&query_type=contains&reason_type=contains&range_value=4&range_unit=weeks&hang_type=any&process_type=any&signature=nsIFrame%3A%3AGetStyleVisibility%28%29


Are there any chances this fix didn't get in that build?

Comment 13

[Scoobidiver (away)]

(In reply to Ioana Budnar [QA] from comment #12)
> This crash is still reproducing on Firefox 18 beta 1 - 20121121075611
It's indeed #14 top browser crasher in this build.

> Are there any chances this fix didn't get in that build?
The patch landed in 18.0b1: http://hg.mozilla.org/releases/mozilla-beta/pushloghtml?startdate=2012-11-20&enddate=2012-11-22

Comment 14

[Scoobidiver (away)]

(In reply to Ioana Budnar [QA] from comment #12) 
> Are there any chances this fix didn't get in that build?
Bug 798964 that caused this regression hasn't landed yet in 18.0 Beta so it could be the reason why there are still crashes despite the fix.

Comment 15

[Ioana (away)]

There are still a lot of new crashes with this signature and bug 798964 is not tracked for Fx18:
https://crash-stats.mozilla.com/report/list?query_search=signature&query_type=contains&reason_type=contains&range_value=4&range_unit=weeks&hang_type=any&process_type=any&signature=nsIFrame%3A%3AGetStyleVisibility%28%29.

Robert, please let me know if there is any other way QA can verify that this bug is fixed.

Comment 16

[Scoobidiver (away)]

(In reply to Scoobidiver from comment #13)
> > Are there any chances this fix didn't get in that build?
> The patch landed in 18.0b1:
I was wrong. The patch landed in 18.0b2. See http://hg.mozilla.org/releases/mozilla-beta/graph/117384?revcount=480

(In reply to Ioana Budnar [QA] from comment #15)
> There are still a lot of new crashes with this signature
I see only seven crashes in 18.0b2.

Comment 17

[Ioana (away)]

This crash is still ocurring - there are 12 crashes on 19 beta by now.

Comment 18

[u279076]

Scoobidiver, any thoughts on comment 17?

Comment 19

[Scoobidiver (away)]

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #18)
> Scoobidiver, any thoughts on comment 17?
Remaining crashes with that signature are unrelated to this bug as their stack trace contains AnyTablePartHasBorderOrBackground and not nsCSSRendering::PrepareBackgroundLayer. In addition, it's a very low volume.

Comment 20

[u279076]

Marking this verified based on comment 19. Please add the verifyme keyword if there's something you'd like QA to check further.