Closed Bug 1033066 Opened 5 years ago Closed 5 years ago

Large OOM coming from mozilla::AudioSegment::WriteTo

Categories

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

33 Branch
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla35
Tracking Status
firefox32 --- wontfix
firefox33 + fixed
firefox34 --- fixed
firefox35 --- fixed

People

(Reporter: mats, Assigned: jesup)

References

Details

(Keywords: crash)

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is 
report bp-e1050f6c-7076-48b0-af95-a95f52140624.
=============================================================

OOM Allocation Size:  4294930432

The number looks like a result from an integer overflow.
http://hg.mozilla.org/mozilla-central/annotate/f78e532e8a10/content/media/AudioSegment.cpp#l163

  uint32_t outBufferLength = GetDuration() * outputChannels;
  buf.SetLength(outBufferLength);

GetDuration() returns TrackTicks which is typedef int64_t.
Could it have been negative? or just very very large?

Perhaps this needs to be a fallible allocation?
FWIW, I sampled 13 version 33.0a1 crashes with this signature - all 13 have AudioSegment::WriteTo on the stack

(In reply to Mats Palmgren (:mats) from comment #0)
> report bp-e1050f6c-7076-48b0-af95-a95f52140624.

0	xul.dll	NS_ABORT_OOM(unsigned int)	xpcom/base/nsDebugImpl.cpp
1	xul.dll	nsTArray_base<nsTArrayInfallibleAllocator,nsTArray_CopyWithMemutils>::EnsureCapacity(unsigned int,unsigned int)	xpcom/glue/nsTArray-inl.h
2	xul.dll	nsTArray_base<nsTArrayInfallibleAllocator,nsTArray_CopyWithMemutils>::InsertSlotsAt(unsigned int,unsigned int,unsigned int,unsigned int)	xpcom/glue/nsTArray-inl.h
3	xul.dll	nsTArray_Impl<float,nsTArrayInfallibleAllocator>::InsertElementsAt(unsigned int,unsigned int)	obj-firefox/dist/include/nsTArray.h
4	xul.dll	nsTArray_Impl<float,nsTArrayInfallibleAllocator>::SetLength(unsigned int)	obj-firefox/dist/include/nsTArray.h
5	xul.dll	mozilla::AudioSegment::WriteTo(unsigned __int64,mozilla::AudioStream *,mozilla::AudioMixer *)	content/media/AudioSegment.cpp
6	xul.dll	mozilla::MediaStreamGraphImpl::PlayAudio(mozilla::MediaStream *,__int64,__int64)	content/media/MediaStreamGraph.cpp
7	xul.dll	mozilla::MediaStreamGraphImpl::RunThread()	content/media/MediaStreamGraph.cpp
8	xul.dll	mozilla::`anonymous namespace'::MediaStreamGraphInitThreadRunnable::Run()	content/media/MediaStreamGraph.cpp
Keywords: crashreportid
[Tracking Requested - why for this release]:
This is the #5 top crash in 32.0 (and 32.0.1) release. I checked a sample of reports from 32.0.1 and it's all AudioSegment stacks as well.

While it's lower in rank in beta right now, that can be due to different user populations on the channels as well as beta being dominated by graphics issues right now.

For more reports and stats, see https://crash-stats.mozilla.com/report/list?signature=OOM+%7C+large+%7C+NS_ABORT_OOM%28unsigned+int%29+%7C+nsTArray_base%3CnsTArrayInfallibleAllocator%2C+nsTArray_CopyWithMemutils%3E%3A%3AEnsureCapacity%28unsigned+int%2C+unsigned+int%29+%7C+nsTArray_base%3CnsTArrayInfallibleAllocator%2C+nsTArray_CopyWithMemutils%3E%3A%3AInsertSlotsAt%28unsigned%2E%2E%2E

OOMAllocationSize is always over 4294920000 Bytes, which very much supports what comment #0 says.
Maire, given that Paul is out atm, can you find someone to take this?

Even if it's just 0.4% of 33 Beta crashes right now, it's a feature that probably isn't used too much yet and this doesn't sound like a nice case.
Flags: needinfo?(mreavy)
Duplicate of this bug: 1070656
Assignee: nobody → rjesup
Flags: needinfo?(mreavy)
Summary: crash in OOM | large | NS_ABORT_OOM(unsigned int) | nsTArray_base<nsTArrayInfallibleAllocator, nsTArray_CopyWithMemutils>::EnsureCapacity(unsigned int, unsigned int) | nsTArray_base<nsTArrayInfallibleAllocator, nsTArray_CopyWithMemutils>::InsertSlotsAt(.. → Large OOM coming from mozilla::AudioSegment::WriteTo
Comment on attachment 8494995 [details] [diff] [review]
Never let AudioSegments underflow mDuration and cause OOM allocation

Original assertion still needs to be dealt with, but it should never be allowed to OOM us
Attachment #8494995 - Flags: review?(karlt)
Comment on attachment 8494995 [details] [diff] [review]
Never let AudioSegments underflow mDuration and cause OOM allocation

r+ on the AudioSegment::WriteTo() change, but please leave AudioChunk::SliceTo() as is or just upgrade the NS_ASSERTION to a MOZ_ASSERT.

SliceTo() has only two callers, both of which check that aStart < aEnd.
Attachment #8494995 - Flags: review?(karlt) → review+
Randell, could you land this patch soon and request for uplift to aurora & beta? Thanks
Flags: needinfo?(rjesup)
I'll be landing this today.
Flags: needinfo?(rjesup)
https://hg.mozilla.org/integration/mozilla-inbound/rev/808aa539aad1
Whiteboard: [leave-open][webrtc-uplift]
Target Milestone: --- → mozilla35
Comment on attachment 8494995 [details] [diff] [review]
Never let AudioSegments underflow mDuration and cause OOM allocation

Approval Request Comment
[Feature/regressing bug #]: Not sure

[User impact if declined]: random crashes - seems to be tied to certain ?Facebook? games; may also happen more on lower-end stressed hardware or a buggy device.  Never seen (or at least never noticed) it in automation.

[Describe test coverage new/current, TBPL]: none other than it will now assert on debug builds which may help catch it.

[Risks and why]: no risk - it's a pure wallpaper-over-bad-allocation.  It will likely lead to an audio underrun (which happen for other reasons) and minor/momentary glitch/pop.

[String/UUID change made/needed]: none
Attachment #8494995 - Flags: approval-mozilla-beta?
Attachment #8494995 - Flags: approval-mozilla-aurora?
(In reply to Randell Jesup [:jesup] from comment #11)
> Approval Request Comment

Note that this is the #4 crash with 2% of all 32.0.3 crashes right now, so it would be really good to have this.
Comment on attachment 8494995 [details] [diff] [review]
Never let AudioSegments underflow mDuration and cause OOM allocation

Of course, I was just waiting for the patch to land in m-c.
Attachment #8494995 - Flags: approval-mozilla-beta?
Attachment #8494995 - Flags: approval-mozilla-beta+
Attachment #8494995 - Flags: approval-mozilla-aurora?
Attachment #8494995 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/f764e665b39c
https://hg.mozilla.org/releases/mozilla-beta/rev/82f4086ba2c7

Calling Fx33 and Fx34 fixed for tracking purposes. Assuming there's follow-up work to be done still, maybe we're better off moving it to a new bug?
See Also: → 1074420
Since this landed and uplifted, I see no reports that match it - the reports for newer builds are other bugs (appear to be OOMs opening files (URLClassifier DB, etc).
Flags: needinfo?(rjesup)
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [leave-open][webrtc-uplift] → [webrtc-uplift]
Whiteboard: [webrtc-uplift]
Duplicate of this bug: 1098010
You need to log in before you can comment on or make changes to this bug.