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)
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)
59 bytes,
text/x-review-board-request
|
mozbugz
:
review+
jcristau
:
approval-mozilla-beta+
|
Details |
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)
Assignee | ||
Comment 1•7 years ago
|
||
This allocates 32 * 608 bytes = 19456 bytes.. Likely to late to uplift to 55 unfortunately....
Flags: needinfo?(jyavenard)
Assignee | ||
Updated•7 years ago
|
tracking-firefox55:
--- → ?
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → jyavenard
Comment hidden (mozreview-request) |
Comment 4•7 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Assignee | ||
Comment 6•7 years ago
|
||
mozreview-review-reply |
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)
Pushed by jyavenard@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/bcae431edab2 Reduce memory usage. r=gerald
Assignee | ||
Comment 8•7 years ago
|
||
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 9•7 years ago
|
||
Comment on attachment 8889933 [details] Bug 1384026 - Reduce memory usage. oom regression fix, beta55+
Attachment #8889933 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 10•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/850ba6548180
Comment 11•7 years ago
|
||
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•7 years ago
|
||
bugherder |
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.
Description
•