1629839 - Crash in [@ cubeb_coreaudio::backend::buffer_manager::BufferManager::push_silent_data]

Status: RESOLVED FIXED

(Core :: Audio/Video: cubeb, defect, P1)

Description

[Alex Chronopoulos [:achronop]]

This bug is for crash report bp-3d1796a9-b704-4049-abed-1ff470200413.

Top 10 frames of crashing thread:

0 XUL RustMozCrash mozglue/static/rust/wrappers.cpp:17
1 XUL mozglue_static::panic_hook mozglue/static/rust/lib.rs:89
2 XUL core::ops::function::Fn::call src/libcore/ops/function.rs:72
3 XUL std::panicking::rust_panic_with_hook src/libstd/panicking.rs:475
4 XUL rust_begin_unwind src/libstd/panicking.rs:375
5 XUL core::panicking::panic_fmt src/libcore/panicking.rs:84
6 XUL core::panicking::panic src/libcore/panicking.rs:51
7 XUL cubeb_coreaudio::backend::buffer_manager::BufferManager::push_silent_data third_party/rust/cubeb-coreaudio/src/backend/buffer_manager.rs
8 XUL cubeb_coreaudio::backend::audiounit_output_callback third_party/rust/cubeb-coreaudio/src/backend/mod.rs:615
9 CoreAudio CoreAudio@0xca1e 

Comment 1

[Paul Adenot (:padenot)]

Crashing here https://doc.rust-lang.org/src/core/slice/mod.rs.html#1081.

Looks like we're requesting more input data than INPUT_BUFFER_CAPACITY. I think this can happen with the following setup:

Input stereo at 192kHz (for example on an external professional sound card, this is pretty common), output at 44.1kHz, IO buffer size at 512 frames (because we're doing a WebRTC call with AEC on)

If we don't have input data, we need to push:

>>> (192000 / 44100.)  * 512 * @
4458.231292517007

frames, which is bigger than out limit of 4096.

Bumping this twofold in size would make it work in this scenario, but one could have even more channels than stereo (I think we support it the days).

Chunmin, do we support more than stereo input?

Comment 2

[C.M.Chang[:chunmin]]

(In reply to Paul Adenot (:padenot) from comment #1)

Chunmin, do we support more than stereo input?

cubeb-coreaudio doesn't check the input settings inside so we follow the general cubeb policy here: The valid input channel range is from 1 to 8(included).

Crashing here https://doc.rust-lang.org/src/core/slice/mod.rs.html#1081.

Looks like we're requesting more input data than INPUT_BUFFER_CAPACITY. I think this can happen with the following setup:

Input stereo at 192kHz (for example on an external professional sound card, this is pretty common), output at 44.1kHz, IO buffer size at 512 frames (because we're doing a WebRTC call with AEC on)

If we don't have input data, we need to push:

>>> (192000 / 44100.)  * 512 * @
4458.231292517007

frames, which is bigger than out limit of 4096.

An alternative approach is appending the silence data in the LinearInputBuffer, which can happen when the needed_samples is greater than the samples we have in the buffer, so we don't need to worry about the size of BufferManager. The LinearInputBuffer is a Vec<T> so its size can grow as it needs until the process is out of memory. What do you think?

Comment 3

[C.M.Chang[:chunmin]]

Bump the priority to P1 since it could cause difficulties to use WebRTC, which is the top priority in this time.

Comment 4

[Paul Adenot (:padenot)]

Up for review upstream: https://github.com/ChunMinChang/cubeb-coreaudio-rs/pull/86

Comment 5

[C.M.Chang[:chunmin]]

Attachment: GitHub Pull Request

The fix will be imported via bug 1631448

Comment 6

[C.M.Chang[:chunmin]]

The fix is imported in bug 1631448

Comment 7

[Paul Adenot (:padenot)]

Attachment: Bug 1629839 - Uplift three patches from cubeb-coreaudio-rs to 76. r?chunmin

This also fixes 1628411 on 76.

Patches below (sha from upstream:
https://github.com/ChunMinChang/cubeb-coreaudio-rs/)

commit e8b8a052d8ce716c6a6f846d3a6fd4deea2d5ed8
Author: Paul Adenot <[e-mail]>
Date: Thu Apr 16 18:13:34 2020 +0200

Don't really push silence to the ring buffer (#86)

commit b49264bfce977b5ab3e84fb246235845f6720a6b
Author: Chun-Min Chang <[e-mail]>
Date: Thu Apr 9 13:29:24 2020 -0700

Remove unknown devices when querying devices in scope (#81)

BMO 1628411 shows we might get some devices whose `AudioObjectID` is
`kAudioObjectUnknown`. Not sure if it's normal but we can simply remove
them from the scoped-device list when it happens.

commit 27cf89ef920d1968c06e45f2a7903725d6b635cd
Author: Paul Adenot <[e-mail]>
Date: Thu Apr 9 20:22:50 2020 +0200

Always pass all of the buffered input in the resampler (#79)

Comment 8

[Paul Adenot (:padenot)]

Attachment: Bug 1629839 - mach vendor rust. r?chunmin

Depends on D72420

Comment 9

[Paul Adenot (:padenot)]

Attachment: uplift-76.patch

Ryan, this contain both fixes (bug 1628411 and bug 1629839).

Comment 10

[Paul Adenot (:padenot)]

Comment on attachment 9143163 [details] [diff] [review]
uplift-76.patch

Beta/Release Uplift Approval Request

• User impact if declined: This fixes two crashes that occur on 76, on macOS: bug 1628411 and bug 1629839.

The first crash occurs when users have a changed their audio preferences from the default to something else with either a high sample-rate or a high-channel count (or both). This doesn't necessarily need special hardware. The second one occur with setups we don't know about, so we side-step an assert and skip a device when doing a specific operation on the list of device present on the machine.

• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• 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 has baked on Nightly for some time, and is the minimum amount of code possible to fix those without drifting away too much from what has been proven to work in Nightly. I've built a beta locally and re-did the scenario I did when fixing bug 1629839.

(The other one is even less risky and we don't know how to test, it likely require special hardware/software/configuration).

• String changes made/needed:

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9143163 [details] [diff] [review]
uplift-76.patch

Fixes some cubeb crashes. Approved for 76.0rc1.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2401b08c64c0
https://hg.mozilla.org/releases/mozilla-beta/rev/39428a673db2