Closed Bug 1543622 Opened 8 months ago Closed Last month

Crash in [@ mozilla::MediaPipelineReceiveAudio::PipelineListener::NotifyPullImpl]

Categories

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

65 Branch
Unspecified
Windows
defect

Tracking

()

RESOLVED FIXED
mozilla72
Tracking Status
firefox-esr68 --- wontfix
firefox70 --- wontfix
firefox71 --- fixed
firefox72 --- fixed

People

(Reporter: pehrsons, Assigned: dminor)

References

(Regression)

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file)

This bug is for crash report bp-5e627f49-f8ff-4289-ab07-345430190411.

Top 10 frames of crashing thread:

0 xul.dll void mozilla::MediaPipelineReceiveAudio::PipelineListener::NotifyPullImpl media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1655
1 xul.dll bool mozilla::SourceMediaStream::PullNewData dom/media/MediaStreamGraph.cpp:2588
2 xul.dll void mozilla::MediaStreamGraphImpl::UpdateGraph dom/media/MediaStreamGraph.cpp:1196
3 xul.dll mozilla::MediaStreamGraphImpl::OneIteration dom/media/MediaStreamGraph.cpp:1395
4 xul.dll long mozilla::AudioCallbackDriver::DataCallback dom/media/GraphDriver.cpp:892
5 xul.dll passthrough_resampler<float>::fill media/libcubeb/src/cubeb_resampler.cpp:76
6 xul.dll static long `anonymous namespace'::refill media/libcubeb/src/cubeb_wasapi.cpp:477
7 xul.dll static bool `anonymous namespace'::refill_callback_duplex media/libcubeb/src/cubeb_wasapi.cpp
8 xul.dll static unsigned int `anonymous namespace'::wasapi_stream_render_loop media/libcubeb/src/cubeb_wasapi.cpp
9 ucrtbase.dll o_strcat_s 

DivideByZero in code added in [1], aka bug 901633. It's likely that something else triggered this later on. According to crash-stats it started in 65, and with this being a pull of a receive-stream, I suspect webrtc.org and the 64 update, bug 1376873. Supposedly AudioSessionConduit::GetAudioFrame() could have changed, in a way such that there is a new condition in which it gives no error but too few samples. Marking the webrtc.org 64 update as the regressor for now.

[1] https://searchfox.org/mozilla-central/diff/db0807f658c4a57abaf27dd9fcb4e8ef380c8aaa/media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp#1373

Making this P2 since it is low frequency.

Priority: -- → P2

While this has been noted in Windows, we're seeing this on all platforms -- Windows, Mac, and Linux, running Version 70.0

I can get a crash nearly every time at this spot from this website:

https://cb.virtualairwaves.com/

This WebRTC site works fine on Chrome and Safari.

Thank you for letting us know, I can get a crash consistently while changing between channels on that site. I'll have a look.

Assignee: nobody → dminor
Status: NEW → ASSIGNED

In the crash I'm looking at now, we're getting samplesLength = 71, samplesPer10ms = 441, so we're getting less than a full frame of data, leading to us calculating a channelCount of zero which leads to the divide by zero and crash.

Shouldn't channel count always be a minimum of "1" even if you should pick up a short frame?

The number of channels is available in mAudioFrame in GetAudioFrame so
there is no reason to calculate it after the fact in MediaPipeline.

Pushed by dminor@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/25bf8e097e60
Make number of channels out param of GetAudioFrame; r=pehrsons
Status: ASSIGNED → RESOLVED
Closed: Last month
Resolution: --- → FIXED
Target Milestone: --- → mozilla72

Doesn't look like the volume is high enough to worry about ESR68 uplift, but did you want to nominate this for Beta?

Flags: needinfo?(dminor)

I'd love to see this fixed ASAP so we can get our users on Firefox.

Comment on attachment 9104901 [details]
Bug 1543622 - Make number of channels out param of GetAudioFrame; r=pehrsons!

Beta/Release Uplift Approval Request

  • User impact if declined: Crashes with WebRTC audio.
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This is a low risk change because it is grabbing the channel count directly from the audio frame rather than relying upon calculating it from other parameters, so very little code has changed.
  • String changes made/needed: None
Flags: needinfo?(dminor)
Attachment #9104901 - Flags: approval-mozilla-beta?

Comment on attachment 9104901 [details]
Bug 1543622 - Make number of channels out param of GetAudioFrame; r=pehrsons!

Low risk crash fix for a non-frequent crash but we are still in early betas, uplift approved for 71 beta 6, thanks.

Attachment #9104901 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Duplicate of this bug: 1591521

Tested in Firefox 71 beta. It's fixed! Thanks a lot.

You need to log in before you can comment on or make changes to this bug.