Closed Bug 1384026 Opened 7 years ago Closed 7 years ago

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

Categories

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

56 Branch
All
Windows
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- unaffected
firefox55 + fixed
firefox56 --- fixed

People

(Reporter: philipp, Assigned: jya)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file)

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
Flags: needinfo?(jyavenard)
This allocates 32 * 608 bytes = 19456 bytes..

Likely to late to uplift to 55 unfortunately....
Flags: needinfo?(jyavenard)
Assignee: nobody → jyavenard
recent regression in beta, let's try getting this fixed in 55.0b13
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!)
Attachment #8889933 - Flags: review?(gsquelart) → review+
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 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
Attachment #8889933 - Flags: approval-mozilla-beta?
Comment on attachment 8889933 [details]
Bug 1384026 - Reduce memory usage.

oom regression fix, beta55+
Attachment #8889933 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
https://hg.mozilla.org/mozilla-central/rev/bcae431edab2
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.