1384026 - Crash in OOM | small with mp4_demuxer::H264::ExtractExtraData

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is 
report bp-1d98e1de-8bfa-48cb-ab84-37be40170725.
=============================================================
Crashing Thread (26), Name: MediaPlayback #1
Frame 	Module 	Signature 	Source
0 	mozglue.dll 	mozalloc_abort(char const* const) 	memory/mozalloc/mozalloc_abort.cpp:33
1 	mozglue.dll 	mozalloc_handle_oom(unsigned int) 	memory/mozalloc/mozalloc_oom.cpp:54
2 	mozglue.dll 	moz_xmalloc 	memory/mozalloc/mozalloc.cpp:85
3 	xul.dll 	nsTArray_base<nsTArrayInfallibleAllocator, nsTArray_CopyWithMemutils>::EnsureCapacity<nsTArrayInfallibleAllocator>(unsigned int, unsigned int) 	obj-firefox/dist/include/nsTArray-inl.h:136
4 	xul.dll 	mp4_demuxer::H264::ExtractExtraData(mozilla::MediaRawData const*) 	media/libstagefright/binding/H264.cpp:849
5 	xul.dll 	mozilla::H264Converter::CheckForSPSChange(mozilla::MediaRawData*) 	dom/media/platforms/wrappers/H264Converter.cpp:396
6 	xul.dll 	mozilla::H264Converter::Decode(mozilla::MediaRawData*) 	dom/media/platforms/wrappers/H264Converter.cpp:98
7 	xul.dll 	mozilla::MediaFormatReader::DecoderFactory::Wrapper::Decode(mozilla::MediaRawData*) 	dom/media/MediaFormatReader.cpp:547
8 	xul.dll 	mozilla::MediaFormatReader::DecodeDemuxedSamples(mozilla::TrackInfo::TrackType, mozilla::MediaRawData*) 	dom/media/MediaFormatReader.cpp:1975
9 	xul.dll 	mozilla::MediaFormatReader::HandleDemuxedSamples(mozilla::TrackInfo::TrackType, mozilla::AbstractMediaDecoder::AutoNotifyDecoded&) 	dom/media/MediaFormatReader.cpp:2077
10 	xul.dll 	mozilla::MediaFormatReader::Update(mozilla::TrackInfo::TrackType) 	dom/media/MediaFormatReader.cpp:2425
11 	xul.dll 	mozilla::detail::RunnableMethodImpl<mozilla::MediaFormatReader* const, void ( mozilla::MediaFormatReader::*)(mozilla::TrackInfo::TrackType), 1, 0, mozilla::TrackInfo::TrackType>::Run() 	obj-firefox/dist/include/nsThreadUtils.h:1133
12 	xul.dll 	mozilla::AutoTaskDispatcher::TaskGroupRunnable::Run() 	obj-firefox/dist/include/mozilla/TaskDispatcher.h:205
13 	xul.dll 	mozilla::TaskQueue::Runner::Run() 	xpcom/threads/TaskQueue.cpp:240
14 	xul.dll 	nsThreadPool::Run() 	xpcom/threads/nsThreadPool.cpp:225
15 	xul.dll 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp:1418
16 	xul.dll 	NS_ProcessNextEvent(nsIThread*, bool) 	xpcom/threads/nsThreadUtils.cpp:475
17 	xul.dll 	mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp:368
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 	nsThread::ThreadFunc(void*) 	xpcom/threads/nsThread.cpp:501
21 	nss3.dll 	_PR_NativeRunThread 	nsprpub/pr/src/threads/combined/pruthr.c:397
22 	nss3.dll 	pr_root 	nsprpub/pr/src/md/windows/w95thred.c:95
23 	ucrtbase.dll 	_o__CIpow 	
24 	kernel32.dll 	BaseThreadInitThunk 	
25 	mozglue.dll 	patched_BaseThreadInitThunk 	mozglue/build/WindowsDllBlocklist.cpp:809
26 	ntdll.dll 	__RtlUserThreadStart 	
27 	ntdll.dll 	_RtlUserThreadStart

OOM|small crashes with this pattern are regressing in 55.0b11 after bug 1374774 was uplifted:
https://crash-stats.mozilla.com/signature/?product=Firefox&proto_signature=~mp4_demuxer%3A%3AH264%3A%3AExtractExtraData&signature=OOM%20|%20small&date=%3E%3D2017-06-25T08%3A21%3A01.000Z#graphs

Comment 1

[Jean-Yves Avenard [:jya]]

This allocates 32 * 608 bytes = 19456 bytes..

Likely to late to uplift to 55 unfortunately....

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1384026 - Reduce memory usage.

We dynamically allocate the memory we need and abort if OOM.

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

Comment 3

[Julien Cristau [:jcristau]]

recent regression in beta, let's try getting this fixed in 55.0b13

Comment 4

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8889933 [details]
Bug 1384026 - Reduce memory usage.

https://reviewboard.mozilla.org/r/160990/#review166474

r+ after this:

::: media/libstagefright/binding/H264.cpp:850
(Diff revision 1)
>    }
>  
>    ByteReader reader(aSample->Data(), sampleSize);
>  
> -  nsTArray<SPSData> SPSTable(MAX_SPS_COUNT);
> -  SPSTable.SetLength(MAX_SPS_COUNT);
> +  nsTArray<SPSData> SPSTable;
> +  size_t size = sizeof(SPSData);

You're not using this `size` variable anywhere! -- Guessing it was part of an earlier solution? :-)
So it should be removed (can't believe it would compile anyway: clang errors when I don't use a variable!)

Comment 5

[Jean-Yves Avenard [:jya]]

Comment on attachment 8889933 [details]
Bug 1384026 - Reduce memory usage.

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

Comment 6

[Jean-Yves Avenard [:jya]]

Comment on attachment 8889933 [details]
Bug 1384026 - Reduce memory usage.

https://reviewboard.mozilla.org/r/160990/#review166474

> You're not using this `size` variable anywhere! -- Guessing it was part of an earlier solution? :-)
> So it should be removed (can't believe it would compile anyway: clang errors when I don't use a variable!)

doh... (I had put that to check on the size of the structure)

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bcae431edab2
Reduce memory usage. r=gerald

Comment 8

[Jean-Yves Avenard [:jya]]

Comment on attachment 8889933 [details]
Bug 1384026 - Reduce memory usage.

Approval Request Comment
[Feature/Bug causing the regression]:  1374774
[User impact if declined]: OOM on 32 bits device.
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: No, however the change is trivial.
[Needs manual test from QE? If yes, steps to reproduce]: Would be hard to reproduce, the number of crashes is rather small.
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No.
[Why is the change risky/not risky?]: We dynamically allocate the array as we need it rather than always allocatin 32 elements every time. In 99.9% of the case, the array will only contain a single element.
[String changes made/needed]: none

Comment 9

[Julien Cristau [:jcristau]]

Comment on attachment 8889933 [details]
Bug 1384026 - Reduce memory usage.

oom regression fix, beta55+

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/850ba6548180

Comment 11

[(Away)]

This fix probably wasn't necessary, although I guess it doesn't harm anything now that it's in.

A specific stack of OOM|small changing in volume doesn't mean much if total OOM|small isn't changing (which it doesn't appear to be, in this case). The point of failure is just moving around. Machines that crash on this mp4 signature would have just crashed elsewhere before bug 1374774, and after this fix they'll go back to crashing elsewhere. In the future we probably don't need to worry too much about these unless there is some horrible egregious pattern happening.

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bcae431edab2