1762563 - Crash in [@ cubeb_coreaudio::backend::CoreStreamData::setup]

Status: RESOLVED FIXED

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

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

7 crashes from 2+ machines with macOS 12, oldest reported build ID is 100.0a1 20220326213356. Is this a regression from bug 1760774 which added the code directly executed before?

Crash report: https://crash-stats.mozilla.org/report/index/8d2394bd-c38c-430e-9f39-7fa120220331

MOZ_CRASH Reason: ```assertion failed: (left != right)
left: `0`,
right: `0````

Top 10 frames of crashing thread:

0 XUL RustMozCrash mozglue/static/rust/wrappers.cpp:18
1 XUL mozglue_static::panic_hook mozglue/static/rust/lib.rs:91
2 XUL core::ops::function::Fn::call library/core/src/ops/function.rs:70
3 XUL std::panicking::rust_panic_with_hook library/std/src/panicking.rs:610
4 XUL std::panicking::begin_panic_handler::{{closure}} library/std/src/panicking.rs:502
5 XUL std::sys_common::backtrace::__rust_end_short_backtrace library/std/src/sys_common/backtrace.rs:139
6 XUL rust_begin_unwind library/std/src/panicking.rs:498
7 XUL core::panicking::panic_fmt library/core/src/panicking.rs:116
8 XUL core::panicking::assert_failed_inner library/core/src/panicking.rs:197
9 XUL core::panicking::assert_failed library/core/src/panicking.rs:154

Comment 1

[Matthew Gregan [:kinetik]]

I assume we're hitting this assert as a result of one of the get_device_transport_type calls added to should_use_aggregate_device in bug 1760774 (importing https://github.com/mozilla/cubeb-coreaudio-rs/pull/153) passing a self.input_device.id or self.output_device.id with a value of kAudioObjectUnknown (0). Perhaps we can just remove the assert from get_device_transport_type - it seems fine to query kAudioObjectUnknown, it'll just fail with kAudioHardwareBadObjectError and treat it as a non-aggregate device.

Paul, does that make sense to you?

Comment 2

[Paul Adenot (:padenot) (off until 2022-08-15)]

It's probably the best solution yes, thanks. I'm having a hard time figuring out what assert failed without tracing everything manually...

Comment 4

[Barret Rennie [:barret] (they/them)]

:padenot, can you provide a patch for beta?

Comment 5

[Paul Adenot (:padenot) (off until 2022-08-15)]

Yes, likely tomorrow when it's reviewed upstream.

Comment 6

[Paul Adenot (:padenot) (off until 2022-08-15)]

Anything with WASAPI is windows, unrelated to this bug, please file something else is needed I'll look into it.

Comment 7

[Paul Adenot (:padenot) (off until 2022-08-15)]

Merging in 1764574, not hard to uplift I think.

Comment 8

[Release mgmt bot [:suhaib / :marco/ :calixte]]

Changing the priority to P1 as the bug is tracked by a release manager for the current beta.
See Triage for Bugzilla for more information.
If you disagree, please discuss with a release manager.

Comment 9

[Paul Adenot (:padenot) (off until 2022-08-15)]

Attachment: Bug 1762563 - Update cubeb-coreaudio-rs to 39f7e69 on beta to fix an assert crash. r?#cubeb-reviewers

Comment 10

[Paul Adenot (:padenot) (off until 2022-08-15)]

Comment on attachment 9272283 [details]
Bug 1762563 - Update cubeb-coreaudio-rs to 39f7e69 on beta to fix an assert crash. r?#cubeb-reviewers

Beta/Release Uplift Approval Request

• User impact if declined: Firefox crashes when an audio output device is removed.
• 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 essentially reverts part of the code to what it currently is in release.
• String changes made/needed:

Comment 11

[Dianna Smith [:diannaS]]

:padenot this hasn't landed yet, is this still something we want to get into b8?

Comment 12

[Paul Adenot (:padenot) (off until 2022-08-15)]

(In reply to Dianna Smith [:diannaS] from comment #11)

:padenot this hasn't landed yet, is this still something we want to get into b8?

Certainly yes, it's not risky and fixes a crash.

Comment 13

[Dianna Smith [:diannaS]]

Comment on attachment 9272283 [details]
Bug 1762563 - Update cubeb-coreaudio-rs to 39f7e69 on beta to fix an assert crash. r?#cubeb-reviewers

Approved fro 100.0b8

Comment 14

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/42b02ee5e1af

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

There are also crash reports for v101 with this signature. Should those get a new bug?

Comment 16

[Paul Adenot (:padenot) (off until 2022-08-15)]

(In reply to Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout) from comment #15)

There are also crash reports for v101 with this signature. Should those get a new bug?

Yes please.

Comment 17

[Paul Adenot (:padenot) (off until 2022-08-15)]

It's only a single crash, that is different, since we landed the patches in https://bugzilla.mozilla.org/show_bug.cgi?id=1764574: https://crash-stats.mozilla.org/signature/?product=Firefox&version=101.0a1&platform=Mac%20OS%20X&build_id=%3E%3D20220414092955&signature=cubeb_coreaudio%3A%3Abackend%3A%3ACoreStreamData%3A%3Asetup&date=%3E%3D2022-03-20T00%3A00%3A00.000Z&date=%3C2022-04-20T23%3A59%3A00.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=install_time&_columns=startup_crash&_sort=-date&page=1. I'll get a fix for that.

Comment 18

[Paul Adenot (:padenot) (off until 2022-08-15)]

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

(In reply to Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout) from comment #15)

There are also crash reports for v101 with this signature. Should those get a new bug?

Yes please.

I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1765969 and attached the patch there.