1373836 - Crash in OOM | large | NS_ABORT_OOM | nsBaseHashtable<T>::Put | mozilla::layers::CompositorAnimationStorage::SetAnimatedValue

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is 
report bp-d3e9a7c3-f844-429b-a8f1-8ad8e0170616.
=============================================================


Crashing Thread (25), Name: Compositor
Frame 	Module 	Signature 	Source
0 	xul.dll 	NS_ABORT_OOM(unsigned int) 	xpcom/base/nsDebugImpl.cpp:610
1 	xul.dll 	nsBaseHashtable<nsUint64HashKey, nsAutoPtr<mozilla::layers::AnimatedValue>, mozilla::layers::AnimatedValue*>::Put(unsigned __int64 const&, mozilla::layers::AnimatedValue* const&) 	obj-firefox/dist/include/nsBaseHashtable.h:139
2 	xul.dll 	mozilla::layers::CompositorAnimationStorage::SetAnimatedValue(unsigned __int64, float const&) 	gfx/layers/AnimationHelper.cpp:81
3 	xul.dll 	mozilla::layers::ApplyAnimatedValue 	gfx/layers/composite/AsyncCompositionManager.cpp:598
4 	xul.dll 	<lambda_f7495f0d0b221bd0a3a66b28ca0eff19>::operator() 	gfx/layers/composite/AsyncCompositionManager.cpp:685
5 	xul.dll 	mozilla::layers::ForEachNode<mozilla::layers::ForwardIterator, mozilla::layers::Layer*, <lambda_f7495f0d0b221bd0a3a66b28ca0eff19>, <lambda_8cacc904cbe36b7ec82f7aea5f3f70f4> > 	gfx/layers/TreeTraversal.h:137
6 	xul.dll 	mozilla::layers::ForEachNode<mozilla::layers::ForwardIterator, mozilla::layers::Layer*, <lambda_f7495f0d0b221bd0a3a66b28ca0eff19>, <lambda_8cacc904cbe36b7ec82f7aea5f3f70f4> > 	gfx/layers/TreeTraversal.h:142
7 	xul.dll 	mozilla::layers::ForEachNode<mozilla::layers::ForwardIterator, mozilla::layers::Layer*, <lambda_f7495f0d0b221bd0a3a66b28ca0eff19>, <lambda_8cacc904cbe36b7ec82f7aea5f3f70f4> > 	gfx/layers/TreeTraversal.h:142
8 	xul.dll 	mozilla::layers::AsyncCompositionManager::TransformShadowTree(mozilla::TimeStamp, mozilla::BaseTimeDuration<mozilla::TimeDurationValueCalculator>, mozilla::layers::AsyncCompositionManager::TransformsToSkip) 	gfx/layers/composite/AsyncCompositionManager.cpp:1390
9 	xul.dll 	mozilla::layers::CompositorBridgeParent::CompositeToTarget(mozilla::gfx::DrawTarget*, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const*) 	gfx/layers/ipc/CompositorBridgeParent.cpp:992
10 	xul.dll 	mozilla::layers::CompositorVsyncScheduler::Composite(mozilla::TimeStamp) 	gfx/layers/ipc/CompositorVsyncScheduler.cpp:258
11 	xul.dll 	mozilla::detail::RunnableMethodImpl<SoftwareDisplay* const, void ( SoftwareDisplay::*)(mozilla::TimeStamp), 1, 1, mozilla::TimeStamp>::Run() 	obj-firefox/dist/include/nsThreadUtils.h:1133
12 	xul.dll 	MessageLoop::RunTask(already_AddRefed<mozilla::Runnable>) 	ipc/chromium/src/base/message_loop.cc:361
13 	xul.dll 	MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask&&) 	ipc/chromium/src/base/message_loop.cc:369
14 	xul.dll 	MessageLoop::DoWork() 	ipc/chromium/src/base/message_loop.cc:444
15 	xul.dll 	base::MessagePumpForUI::DoRunLoop() 	ipc/chromium/src/base/message_pump_win.cc:212
16 	xul.dll 	base::MessagePumpWin::RunWithDispatcher(base::MessagePump::Delegate*, base::MessagePumpWin::Dispatcher*) 	ipc/chromium/src/base/message_pump_win.cc:56
17 	xul.dll 	base::MessagePumpWin::Run(base::MessagePump::Delegate*) 	ipc/chromium/src/base/message_pump_win.h:80
18 	xul.dll 	MessageLoop::RunHandler() 	ipc/chromium/src/base/message_loop.cc:231
19 	xul.dll 	MessageLoop::Run() 	ipc/chromium/src/base/message_loop.cc:211
20 	xul.dll 	base::Thread::ThreadMain() 	ipc/chromium/src/base/thread.cc:180
21 	xul.dll 	`anonymous namespace'::ThreadFunc 	ipc/chromium/src/base/platform_thread_win.cc:28
22 	kernel32.dll 	BaseThreadInitThunk 	
23 	mozglue.dll 	patched_BaseThreadInitThunk 	mozglue/build/WindowsDllBlocklist.cpp:809
24 	ntdll.dll 	__RtlUserThreadStart 	
25 	ntdll.dll 	_RtlUserThreadStart 	

crash reports with this signature are first showing up in firefox 55.0a1 20170316030211 and subsequent builds. so far they come from 32bit & 64bit versions of firefox on windows and occur in the gpu process 75% of the time. 
parts of the crashing codepath have last been touched with bug 1337889, so i'll tentatively mark this bug here as blocking it.

Comment 1

[Peter Chang[:pchang]]

I will take a look this issue.

Comment 2

[Peter Chang[:pchang]]

For memory usage, assign one fixed id per layer and query entries before insertion should help the memory usage. I just submit a try to see how 'fixed id per layer' works or not.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b5f619a789a80fea5ffa7382aae5014c596d2fe

Comment 3

[Peter Chang[:pchang]]

Submit another try to fix the try failures.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f91d5f616aa82c3327d4f7d54a93869920669a6f

Comment 4

[Peter Chang[:pchang]]

Attachment: Bug 1373836 - Generate fixed animation id per layer if animations exist,

In gecko, it's possible to generate lots of animation ids per layer if animations are changed.
It also introduces lots of memory allocation/deallocation in CompositorAnimationStorage(HashTable).
Generate fixed animations id per layer should help the memory usage and reduce CPU time for memory allocation.

Review commit: https://reviewboard.mozilla.org/r/153298/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/153298/

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Copying my question from IRC in case you missed it there:

Are we currently clearing the CompositorAnimationStorage for the non-WR case at all? I was looking for where we do it and I don't see anything.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

(For posterity, Peter replied with a pointer to http://searchfox.org/mozilla-central/rev/152c0296f8a10f81185ee88dfb4114ec3882b4c6/gfx/layers/composite/AsyncCompositionManager.cpp#1402 which is what I had missed).

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8882202 [details]
Bug 1373836 - Generate fixed animation id per layer if animations exist,

https://reviewboard.mozilla.org/r/153298/#review158572

r=me with typos fixed

::: gfx/layers/ipc/LayerTransactionParent.cpp:534
(Diff revision 1)
> +  // Clean up the Animations by id in the CompositorAnimationSotrage
> +  // if there is no active aniamtions on the layer

s/Sotrage/Storage/
s/aniamtions/animations/

Comment 8

[Peter Chang[:pchang]]

Comment on attachment 8882202 [details]
Bug 1373836 - Generate fixed animation id per layer if animations exist,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/153298/diff/1-2/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5aa966279356
Generate fixed animation id per layer if animations exist, r=kats

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5aa966279356

Comment 11

[Sotaro Ikeda [:sotaro]]

This bug could easily cause oom with omta like Bug 1376896 comment 0.

Comment 12

[Peter Chang[:pchang]]

I will wait for awhile and ask for uplift.

Comment 13

[Peter Chang[:pchang]]

Comment on attachment 8882202 [details]
Bug 1373836 - Generate fixed animation id per layer if animations exist,

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1337889
[User impact if declined]: might cause OOM when users browser pages with animations
[Is this code covered by automated tests?]: passed try and land in central for a while
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: None
[Why is the change risky/not risky?]: It just reduced the chance to consume memory for animation pages
[String changes made/needed]: None

Comment 14

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 8882202 [details]
Bug 1373836 - Generate fixed animation id per layer if animations exist,

avoid oom crashes from animations, beta55+

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/47395fea9c84