Crash in audiounit_setup_stream

RESOLVED FIXED in Firefox 54

Status

()

P1
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: philipp, Assigned: kinetik)

Tracking

({crash, regression})

54 Branch
mozilla55
Unspecified
macOS
crash, regression
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr45 unaffected, firefox-esr52 unaffected, firefox53 unaffected, firefox54+ fixed, firefox55 fixed)

Details

(crash signature)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
+++ This bug was initially created as a clone of Bug #1344580 +++

(In reply to Ting-Yu Chou [:ting] from comment #12)
> I am still seeing audiounit_setup_stream on 54 and 55, and
> audiounit_layout_init on 54.
> 
> https://crash-stats.mozilla.com/signature/
> ?product=Firefox&signature=audiounit_setup_stream&date=%3E%3D2017-04-
> 05T03%3A11%3A01.000Z&date=%3C2017-04-12T03%3A11%3A01.
> 000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_colum
> ns=platform&_columns=reason&_columns=address&_columns=install_time&_sort=-
> date&page=1#aggregations
> 
> https://crash-stats.mozilla.com/signature/
> ?signature=audiounit_layout_init&date=%3E%3D2017-03-29T03%3A12%3A09.
> 000Z&date=%3C2017-04-12T03%3A12%3A09.
> 000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_colum
> ns=platform&_columns=reason&_columns=address&_columns=install_time&_sort=-
> date&page=1#aggregations

(In reply to Chun-Min Chang[:chunmin] from comment #13)
> Thanks for the report. I'll try an ASAN build to test it.

(In reply to Ting-Yu Chou [:ting] from comment #14)
> Just double checked, it seems that audiounit_layout_init is fixed, but not
> audiounit_setup_stream.
chunmin, 
Can you have a look?
Flags: needinfo?(cchang)
(Reporter)

Updated

2 years ago
Summary: Crash in audiounit_layout_init → Crash in audiounit_setup_stream
sure.
Assignee: nobody → cchang
Flags: needinfo?(cchang)
[Tracking Requested - why for this release]: This crash does affect a lot of people on OS X, according to crash-stats, but the people affected by it appear to experience a lot of crashes.
tracking-firefox54: --- → ?
(In reply to Nathan Froyd [:froydnj] from comment #3)
> [Tracking Requested - why for this release]: This crash does affect a lot of
> people on OS X, according to crash-stats, but the people affected by it
> appear to experience a lot of crashes.
I will investigate this today.

https://crash-stats.mozilla.com/signature/?product=Firefox&signature=audiounit_setup_stream&date=%3E%3D2017-04-26T01%3A55%3A12.000Z&date=%3C2017-05-03T01%3A55%3A12.000Z#summary
The crash percentage is very high[0], but I don't know how to reproduce it. They all crash when calling audiounit_layout_init(stm, OUTPUT)[1]

I run a ASAN build to test it today and find two bugs: bug 1361657 and bug 1361669. However, I am not sure if these bugs are related because I don't see the crashes in [1].

It doesn't crash in audiounit_layout_init. It crashes when calling audiounit_layout_init, so I guess "stm" is nulled somewhere. The "stm" will be deleted in audiounit_stream_destroy, but audiounit_stream_destroy shouldn't be called before audiounit_layout_init.

I have no idea how this crash happen. Paul, could you have a look?


[0] https://crash-stats.mozilla.com/signature/?product=Firefox&signature=audiounit_setup_stream&date=%3E%3D2017-04-26T01%3A55%3A12.000Z&date=%3C2017-05-03T01%3A55%3A12.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=install_time&_sort=-date&page=1#summary

[1] https://hg.mozilla.org/releases/mozilla-beta/annotate/8a7f650cff30/media/libcubeb/src/cubeb_audiounit.cpp#l2220
Flags: needinfo?(padenot)
Chun-min, the function that is crashing is doing a lot of dangerous things with memory (array indexing without having checked the value of the index). It also seem to assume a particular limit of channel count of the device (we've fixed that upstream iirc). 

Also the `offsetof` is weird.
Flags: needinfo?(padenot)
(In reply to Paul Adenot (:padenot) from comment #6)
> Chun-min, the function that is crashing is doing a lot of dangerous things
> with memory (array indexing without having checked the value of the index).
> It also seem to assume a particular limit of channel count of the device
> (we've fixed that upstream iirc). 
> 
> Also the `offsetof` is weird.
Yes, you're right! It will crashes when the output device's channel count is more than CHANNEL_MAX. 
However, I was wondering why it doesn't crash in audiounit_convert_channel_layout. Maybe the stack is not explicit enough.

The `offsetof`[0] is used to calculate the size for the require channel layout. If mChannelDescriptions is placed at the end of the struct AudioChannelLayout[1], it should be correct. One apple technical reference uses the same way[2] to calculate the size of the required layout. 

[0] https://en.wikipedia.org/wiki/Offsetof
[1] https://github.com/darlinghq/darling/blob/dd7dfc306daf255fc96ff18a26db40705c0d0622/src/CoreAudio/CoreAudioTypes.h#L356
[2] https://developer.apple.com/library/content/qa/qa1627/_index.html
Track 54+ as this crash impacts Beta54 from b1 to b4.
tracking-firefox54: ? → +
The comments mention Netflix - have we tried to reproduce it?
(In reply to Marcia Knous [:marcia - use ni] from comment #9)
> The comments mention Netflix - have we tried to reproduce it?

I tried but I cannot reproduce it. 

(In reply to Paul Adenot (:padenot) from comment #6)
> Chun-min, the function that is crashing is doing a lot of dangerous things
> with memory (array indexing without having checked the value of the index).
> It also seem to assume a particular limit of channel count of the device
> (we've fixed that upstream iirc). 
> 
> Also the `offsetof` is weird.

I plan to add some assertions around `make_sized_audio_channel_layout` and update cubeb to see if this could be solved by pull 288[0]. If crashes happen again, maybe I need to replace the `offsetof`.

[0] https://github.com/kinetiknz/cubeb/pull/288
(In reply to Chun-Min Chang[:chunmin] from comment #10)
> I plan to add some assertions around `make_sized_audio_channel_layout` and
> update cubeb to see if this could be solved by pull 288[0]. If crashes
> happen again, maybe I need to replace the `offsetof`.

pull 288 is already landed by bug 1359451 in FF55. We should check if we have crashes in FF55.
(Reporter)

Comment 12

2 years ago
i think it isn't crashing frequently enough on nightly to determine an effect with confidence. the last recorded crash was from 55.0a1 build 20170416030209, 10 days before bug 1359451 landed.
Flags: needinfo?(achronop)

Updated

2 years ago
Depends on: 1359451
I am looking at the following crash-stat report and crash does not occur in 55.0* versions. Stops at 54.0b5.

https://crash-stats.mozilla.com/signature/?proto_signature=~audiounit_setup_stream&signature=audiounit_setup_stream&date=%3E%3D2017-05-02T08%3A09%3A00.000Z&date=%3C2017-05-09T08%3A09%3A00.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=install_time&_sort=-version&_sort=-date&page=1

In comment 12 you mention that crash appears 55.0a1 but I cannot find that crash. Am I doing something wrong? Can you please provide the crash stat?
Flags: needinfo?(madperson)
(Reporter)

Comment 14

2 years ago
yeah, there was no crash on 55.0a1 in the past 7 days - if you extend the timeframe, here are all the recorded reports from this version: https://crash-stats.mozilla.com/signature/?proto_signature=~audiounit_setup_stream&version=55.0a1&signature=audiounit_setup_stream&date=%3E%3D2017-02-09T08%3A30%3A04.000Z#reports
Flags: needinfo?(madperson)
Right, thanks! For some reason I thought I had set the timeframe. Sorry for the inconvenience.
We need an uplift to 54, right?

Note padenot is on PTO through end-of-month
Flags: needinfo?(kinetik)
(Assignee)

Comment 17

2 years ago
Do we have a confirmed fix? If so, have we identified which upstream change was the correct fix?

I'm having trouble following the bug and there are still debugging PRs in-flight upstream for this (https://github.com/kinetiknz/cubeb/pull/296).  Is https://github.com/kinetiknz/cubeb/pull/288 sufficient to solve this?
Flags: needinfo?(kinetik) → needinfo?(cchang)
(Assignee)

Comment 19

2 years ago
Posted patch bug1358896_v0.patch (obsolete) — Splinter Review
Approval Request Comment
[Feature/Bug causing the regression]: OS X multi-channel support (bug 1339723 or a later change)
[User impact if declined]: potential crash when starting audio playback or capture
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes (confirmed via crash stats)
[Needs manual test from QE? If yes, steps to reproduce]: n/a
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: n/a
[String changes made/needed]: none
Attachment #8866628 - Flags: review?(achronop)
Attachment #8866628 - Flags: approval-mozilla-beta?
(Assignee)

Comment 20

2 years ago
Note that this patch needs to be applied on top of the uplift in bug 1358868.
(Reporter)

Updated

2 years ago
status-firefox55: ? → fixed
Comment on attachment 8866628 [details] [diff] [review]
bug1358896_v0.patch

Looks good, thanks for taking care.
Flags: needinfo?(achronop)
Attachment #8866628 - Flags: review?(achronop) → review+
(Assignee)

Comment 22

2 years ago
Updated patch to apply against latest revision of the patches in bug 1358868.  No logical change, carrying forward review.  See comment 19 for approval request.
Attachment #8866628 - Attachment is obsolete: true
Attachment #8866628 - Flags: approval-mozilla-beta?
Attachment #8867027 - Flags: review+
Attachment #8867027 - Flags: approval-mozilla-beta?
Comment on attachment 8867027 [details] [diff] [review]
bug1358896_v1.patch

Fix a crash. Beta54+. Should be in 54 beta 8.
Attachment #8867027 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 24

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/b891a2a9af3d
status-firefox54: affected → fixed

Updated

2 years ago
See Also: → bug 1350893
- I am going to close it because there is no report in the last month.
- Re-assign to :kinetik for his contribution.
Assignee: cchang → kinetik
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(In reply to Matthew Gregan [:kinetik] from comment #19)
> [Is this code covered by automated tests?]: no
> [Has the fix been verified in Nightly?]: yes (confirmed via crash stats)
> [Needs manual test from QE? If yes, steps to reproduce]: n/a

Setting qe-verify- based on Matthew's assessment on manual testing needs.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.