User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/56.0.2924.87 Safari/537.36 Steps to reproduce: Using MediaStream, MediaRecorder and AudioContext to get access to a user's camera and microphone to record him talking crashes randomly with FF 51. Actual results: Firefox crashes: https://crash-stats.mozilla.com/report/index/b1433abc-8452-49c4-8560-82c052170202 Expected results: Firefox not crashing

This bug seems related to https://crash-stats.mozilla.com/signature/?product=Firefox&version=51.0.1&signature=OOM%20%7C%20small&date=%3E%3D2017-01-27T09%3A26%3A59.000Z&date=%3C2017-02-03T09%3A26%3A59.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_sort=-date&page=1#bugzilla Maybe duplicated to some other bug and the fact that my traceback mentions using "mozilla::SourceMediaStream::ResampleAudioToGraphSampleRate(mozilla::SourceMediaStream::TrackData*, mozilla::MediaSegment*)" might be non-related.

I've created a POC @ https://rafaelsierra.github.io/poc-bugzilla-1336367/

The following diff prevents Firefox from allocating too much memory: https://github.com/rafaelsierra/poc-bugzilla-1336367/pull/1/files In short, it pops the audio track from the source media stream and uses it to create a new audio track with volume control and attach it back to the source media stream.

If you think this is the right component, can you please take a look at what Rafael created in comment 3? If not please suggest a proper component. Thanks

The proposed diff mentioned at comment #3 does not prevent the same issue from happening when "mediaRecorder.stop()" is called. Calling "mediaRecorder.stop()" will cause a plugin process[1] to increase in size the same way as without the diff. Happens only on FF < 53, FF 53 and 54 does not have memory issues. [1] - Command line: /usr/lib/firefox/plugin-container -greomni /usr/lib/firefox/omni.ja -appomni /usr/lib/firefox/browser/omni.ja -appdir /usr/lib/firefox/browser 8260 true tab

To prevent Firefox (or the plugin) from taking all the memory while using MediaRecorder, it is required to call MediaStreamTrack.stop() for each track inside MediaRecorder.stream, only calling MediaRecorder.stop() is not enough.

Looks like a MediaRecorder issue.

I've observed something like this a few times (and can confirm the bug with the proof of concept page), but never had the chance to dig deeper. It appears that we stop consuming data as we stop() the MediaRecorder, but the tracks are still live and keep putting data into the TrackEncoder buffers. If we identify what fixed it in 53 we can probably take that out and make a small uplift to 52. First we should check whether 52 is affected. Some 53 bugs were uplifted to 52.

No plan to have dot release for 51. Mark 51 won't fix.

This is a P1 bug without an assignee. P1 are bugs which are being worked on for the current release cycle/iteration/sprint. If the bug is not assigned by Monday, 28 August, the bug's priority will be reset to '--'.

I'm still seeing issues when running the test case here. I see massive memory usage, which I expect could lead to the oom mentioned. - The issue is profile specific for me. Doesn't happen on a clean profile, but I have others that reliably repro this. I do not know what yet in the profiles is causing this. - The issue does not appear to be fixed, I see ballooning memory usage on current nightly. I'm gonna grab and take a look at this further.

Looks like the memory bloat aspect of this only happens when running with e10s disabled, which is the key difference I'm seeing between profiles. I'm not sure why this is the case, the problem appears to caused by buffering raw video frames, which I would have anticipated would take place with e10s on or off. I'll see if I can nail down why there's a difference, but I think the real problem is described below. The raw frame buffering is caused by the POC not plumbing audio all the way through. This results in volumeGainNode outputting 0 channel silence. That said, I think this is a bug and we should be getting at least one channel, but I'm no expert on Web Audio (Bug 916392 looks related to this). Since the data has no channels, the audio encoder is not initialized, which gates initialization of the video encoder, which results in buffering the raw frames until an init happens (when one stops the recorder). Here is a fiddle of the original POC, but with the required line (`mediaStreamAudioSource.connect(volumeGainNode);`) added in a comment: https://jsfiddle.net/SingingTree/2crc63d2/ Uncommenting this line resolves the issue for me. Talking points: - We want to make sure we have audio and video data to init our encoders and be able to write a correct header. However, this failure case is difficult to diagnose and pretty nasty in terms of eating large amounts of memory. I think if nothing else it would be useful to add debug code to better detect these issues (telemetry could be useful too). Bug 1376134 is a related problem and shows that we could benefit with some more tools around detecting these faults. - We could force an init if we have not received data for a certain amount of time. Then up/down mix as required when data starts coming in. This isn't ideal, but currently deferring the init is significantly worse in my mind due to memory blow out and sync issues (right now it looks like we end up writing all our audio data after the video data, so the output is mangled). - We could have WebAudio nodes output non zero channel counts for silence. That way the recorder could be changed to init correctly even when being fed silence. However, I'm not familiar with what would be required to do this. :padenot do you have any insight into this? :jib, :pehrsons, do you have thoughts?

There's a 30s timeout already that I think we should consider lowering for audio, see [1]. Is one second reasonable? That would be three video frames worth of buffering for most cameras. Noteworthy that there should, unlike video frames, always be audio present. I think we can also change the default behavior of a timeout from causing an error in the MediaRecorder, to init the encoder with our best guess of the number of channels. Probably stereo. I believe we have code that will mix to the initial number of channels whenever it changes, so the changes to code to init with a default should be simple. I think it could also make sense to file a spec issue on what to do when the number of channels in the input to the recorder changes mid-recording; and what to do if the number of channels is not known initially. I don't see the spec mentioning any details like this. [1] http://searchfox.org/mozilla-central/rev/00fa5dacedb925022f53d025121f1a919508e7ce/dom/media/encoder/TrackEncoder.cpp#85

FWIW I think the video timeout should remain at 30s. Unless we look into doing the same default-init step for it as well. It should be able to handle resolution changes now so we could probably do that.

I don't have much to add here besides what Andreas says, except it would be nice if we understood why we can't reproduce in e10s. Getting rid of the bloat problem is the goal, even though this sounds like an edge-case (I'd love a reduced jsfiddle or something that showed the extent of the problem, to understand how much of an edge case it is).

Reduced JS fiddle: https://jsfiddle.net/SingingTree/tb7pzusy/

Mass change P1->P2 to align with new Mozilla triage process

Comment on attachment 8907951 [details] Bug 1336367 - Significantly lower TrackEncoder timeout, make best effort init audio encoder on timeout. https://reviewboard.mozilla.org/r/179618/#review184882

Comment on attachment 8907952 [details] Bug 1336367 - Move AudioTrackEncoder Segment init logic into new method. https://reviewboard.mozilla.org/r/179620/#review184890

Comment on attachment 8907954 [details] Bug 1336367 - Rename TestTrackEncoder to TestAudioTrackEncoder. https://reviewboard.mozilla.org/r/179624/#review184898 Heh, I am doing the same in bug 1296531. https://reviewboard.mozilla.org/r/142312/diff/#index_header https://reviewboard.mozilla.org/r/142314/diff/#index_header Feel free to do it now though, I can rebase.

Comment on attachment 8907953 [details] Bug 1336367 - Add gtest for new AudioTrackEncoder init method and behaviour. https://reviewboard.mozilla.org/r/179622/#review184896 ::: dom/media/gtest/TestTrackEncoder.cpp:44 (Diff revision 1) > +static AudioSegment > +CreateTestSegment() > +{ > + RefPtr<SharedBuffer> dummyBuffer = SharedBuffer::Create(2); > + AutoTArray<const int16_t*, 1> channels; > + const int16_t* channelData = static_cast<const int16_t*>(dummyBuffer->Data()); > + channels.AppendElement(channelData); > + > + AudioSegment testSegment; > + testSegment.AppendFrames( > + dummyBuffer.forget(), channels, 1 /* #samples */, PRINCIPAL_HANDLE_NONE); > + return testSegment; > +} For improved portability, this could take the number of channels and number of samples as arguments. That said, I'm adding a segment generator in bug 1296531 [1] that can be customized, so feel free to land this now and I'll replace it when time comes. [1] https://reviewboard.mozilla.org/r/142320/diff/3#index_header

Comment on attachment 8907955 [details] Bug 1336367 - Fix unified build issues from previous changes. https://reviewboard.mozilla.org/r/179626/#review184908 Good catch!

Cheers for quick review! I'll leave things as is and hope that I'm not creating too much work for bug 1296531 merges ^_^

Bumping tracking as I believe this has continued to exist. Re e10s: I've been looking deeper and it looks like we are still eating memory and there's no particular mystery here. It may mitigate the issue in that the memory load is spread to a different process so we don't OOM as easily before the audio encoder default init happens. Aside from that it was me not driving the tools to diagnose the memory bloat well, and with some deeper digging it is clear that it's still happening.

Pushed by bvandyk@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ef7b496eda73 Significantly lower TrackEncoder timeout, make best effort init audio encoder on timeout. r=pehrsons https://hg.mozilla.org/integration/autoland/rev/b843181a1be7 Move AudioTrackEncoder Segment init logic into new method. r=pehrsons https://hg.mozilla.org/integration/autoland/rev/9438b1661ccd Add gtest for new AudioTrackEncoder init method and behaviour. r=pehrsons https://hg.mozilla.org/integration/autoland/rev/41efa509e924 Rename TestTrackEncoder to TestAudioTrackEncoder. r=pehrsons https://hg.mozilla.org/integration/autoland/rev/4cdf5e40be84 Fix unified build issues from previous changes. r=pehrsons

https://hg.mozilla.org/mozilla-central/rev/ef7b496eda73 https://hg.mozilla.org/mozilla-central/rev/b843181a1be7 https://hg.mozilla.org/mozilla-central/rev/9438b1661ccd https://hg.mozilla.org/mozilla-central/rev/41efa509e924 https://hg.mozilla.org/mozilla-central/rev/4cdf5e40be84

clearing NI