789960 - crash in nsLayoutUtils::HasAnimationsForCompositor

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Scoobidiver (away)]

I can reproduce in Aurora and Nightly.

Signature 	PL_DHashTableOperate | nsINode::GetProperty(unsigned short, nsIAtom*, unsigned int*) More Reports Search
UUID	e4703667-c5f4-4ec6-b3e9-048a22120910
Date Processed	2012-09-10 16:01:35
Uptime	99
Last Crash	1.7 minutes before submission
Install Age	3.7 hours since version was first installed.
Install Time	2012-09-10 12:19:26
Product	Firefox
Version	17.0a2
Build ID	20120909042009
Release Channel	aurora
OS	Windows NT
OS Version	6.1.7601 Service Pack 1
Build Architecture	x86
Build Architecture Info	GenuineIntel family 6 model 23 stepping 10
Crash Reason	EXCEPTION_STACK_OVERFLOW
Crash Address	0x69daeb93
App Notes 	
AdapterVendorID: 0x8086, AdapterDeviceID: 0x2a42, AdapterSubsysID: 02961025, AdapterDriverVersion: 8.15.10.2555
D2D? D2D+ DWrite? DWrite+ D3D10 Layers? D3D10 Layers+ 
Processor Notes 	This dump is too long and has triggered the automatic truncation routine
EMCheckCompatibility	True
Adapter Vendor ID	0x8086
Adapter Device ID	0x2a42
Total Virtual Memory	4294836224
Available Virtual Memory	3886981120
System Memory Use Percentage	60
Available Page File	5862342656
Available Physical Memory	1662828544

Frame 	Module 	Signature 	Source
0 	xul.dll 	PL_DHashTableOperate 	obj-firefox/xpcom/build/pldhash.cpp:566
1 	xul.dll 	nsINode::GetProperty 	content/base/src/nsINode.cpp:147
2 	xul.dll 	nsINode::GetProperty 	obj-firefox/dist/include/nsINode.h:575
3 	xul.dll 	nsLayoutUtils::HasAnimationsForCompositor 	layout/base/nsLayoutUtils.cpp:127
4 	xul.dll 	nsIFrame::Preserves3D 	layout/generic/nsFrame.cpp:1016
5 	xul.dll 	mozilla::css::CommonElementAnimationData::CanAnimatePropertyOnCompositor 	layout/style/AnimationCommon.cpp:271
6 	xul.dll 	ElementAnimations::CanPerformOnCompositorThread 	layout/style/nsAnimationManager.cpp:345
7 	xul.dll 	nsLayoutUtils::HasAnimationsForCompositor 	layout/base/nsLayoutUtils.cpp:130
8 	xul.dll 	nsIFrame::Preserves3D 	layout/generic/nsFrame.cpp:1016
9 	xul.dll 	mozilla::css::CommonElementAnimationData::CanAnimatePropertyOnCompositor 	layout/style/AnimationCommon.cpp:271
...

More reports at:
https://crash-stats.mozilla.com/report/list?signature=ApplyOverflowClipping
https://crash-stats.mozilla.com/report/list?signature=PL_DHashTableOperate+|+nsINode%3A%3AGetProperty%28unsigned+short%2C+nsIAtom*%2C+unsigned+int*%29
https://crash-stats.mozilla.com/report/list?signature=nsIFrame%3A%3APreserves3D%28%29
https://crash-stats.mozilla.com/report/list?signature=nsIFrame%3A%3AIsTransformed%28%29
https://crash-stats.mozilla.com/report/list?signature=nsLayoutUtils%3A%3AHasAnimationsForCompositor%28nsIContent*%2C+nsCSSProperty%29

Comment 1

[Alice0775 White]

Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/c14e2d5f17de
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120813105346
Crash:
http://hg.mozilla.org/mozilla-central/rev/75cdb3f932c6
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120813110945
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c14e2d5f17de&tochange=75cdb3f932c6



Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/b753e1dce89f
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120812205544
Crash:
http://hg.mozilla.org/integration/mozilla-inbound/rev/94e4dbce3b94
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120812215445
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b753e1dce89f&tochange=94e4dbce3b94

Comment 2

[David Zbarsky (:dzbarsky)]

That regression range isn't that useful.  The issue is in the stack trace - Preserves3D calls IsTransformed, which checks HasAnimationsForCompositor, which calls CommonElementAnimationData::CanAnimatePropertyOnCompositor, which ends up checking Preserves3D.

Comment 3

[David Zbarsky (:dzbarsky)]

Attachment: Patch

Comment 4

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

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

Review of attachment 659829 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsIFrame.h
@@ +1239,5 @@
>     * Returns true if this frame is transformed (e.g. has CSS or SVG transforms)
>     * or if its parent is an SVG frame that has children-only transforms (e.g.
>     * an SVG viewBox attribute).
>     */
> +   bool Is2DTransformed() const;

But this could return true when there are 3D transforms, so Is2DTransformed is misleading.

I think most of the time people should call an IsTransformed method that returns true when there are async transform animations. So let's keep calling that one IsTransformed. I think in Preserves3D/Perserves3DChildren we could just check GetStyleDisplay()->HasTransform() instead of calling IsTransformed() --- I think IsSVGTransformed shouldn't affect Preserves3D. Then we don't need another method.

@@ +1243,5 @@
> +   bool Is2DTransformed() const;
> +
> +  /**
> +   * Returns true if this frame is 2D-transformed or if it has async transform
> +   * animations.

This comment is wrong. It's Is2DTransform that returns true when there are async transform animations.

Comment 5

[David Zbarsky (:dzbarsky)]

Attachment: Patch

This also fixed the crash.  Try results will be at https://tbpl.mozilla.org/?tree=Try&rev=1cc92de0c52b

Comment 6

[David Zbarsky (:dzbarsky)]

Green on try.
https://hg.mozilla.org/integration/mozilla-inbound/rev/5949120c6062

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5949120c6062

Comment 8

[Alice0775 White]

Aurora17.0a2 ?

Comment 9

[Virgil Dicu [:virgil] [QA]]

http://codepen.io/juliangarnier/fulldetails/idhuG (same demo as in the URL) has been reported on IRC that also crashes the browser in 17 with this signature.

As this bug still affects F17, shouldn't it be ported to beta as well?

Comment 10

[Lukas Blakk [:lsblakk] use ?needinfo]

David - any reason not to nominate this for uplift?  If not, please go ahead with a risk assessment and nom.

Comment 11

[David Zbarsky (:dzbarsky)]

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

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: Can cause infinite loops on some pages
Testing completed (on m-c, etc.): Fixes the loops, does not break tests or regress Off main thread animations
Risk to taking this patch (and alternatives if risky): Low risk
String or UUID changes made by this patch: None

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/73bfa98a91df

Comment 13

[u279076]

Scoobidiver or Alice, can you check if this is reproducible for you in the latest Firefox 17 Beta and Firefox 18 Aurora?

Comment 14

[Alice0775 White]

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #13)
> Scoobidiver or Alice, can you check if this is reproducible for you in the
> latest Firefox 17 Beta and Firefox 18 Aurora?

I cannot reproduce the crash anymore in Firefox17beta and Aurora18.0a2.
http://hg.mozilla.org/releases/mozilla-beta/rev/486335dcb4a2
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20121031065642
http://hg.mozilla.org/releases/mozilla-aurora/rev/c8f3d5623567
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/18.0 Firefox/18.0 ID:20121106042013

Comment 15

[u279076]

Thank you very much, Alice.