1358896 - Crash in audiounit_setup_stream

Status: RESOLVED FIXED

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

Description

[[:philipp]]

+++ 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.

Comment 1

[Blake Wu [:bwu][:blakewu]]

chunmin, 
Can you have a look?

Comment 2

[C.M.Chang[:chunmin]]

sure.

Comment 3

[Nathan Froyd [:froydnj]]

[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.

Comment 4

[C.M.Chang[:chunmin]]

(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

Comment 5

[C.M.Chang[:chunmin]]

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

Comment 6

[Paul Adenot (: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.

Comment 7

[C.M.Chang[:chunmin]]

(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

Comment 8

[Gerry Chang [:gchang]]

Track 54+ as this crash impacts Beta54 from b1 to b4.

Comment 9

[Marcia Knous [:marcia]]

The comments mention Netflix - have we tried to reproduce it?

Comment 10

[C.M.Chang[:chunmin]]

(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

Comment 11

[C.M.Chang[:chunmin]]

(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.

Comment 12

[[:philipp]]

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.

Comment 13

[Alex Chronopoulos [:achronop]]

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?

Comment 14

[[:philipp]]

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

Comment 15

[Alex Chronopoulos [:achronop]]

Right, thanks! For some reason I thought I had set the timeframe. Sorry for the inconvenience.

Comment 16

[Randell Jesup [:jesup] (needinfo me)]

We need an uplift to 54, right?

Note padenot is on PTO through end-of-month

Comment 17

[Matthew Gregan [:kinetik]]

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?

Comment 18

[C.M.Chang[:chunmin]]

There is no crash after https://github.com/kinetiknz/cubeb/pull/288 is merged into gecko. You can delete the debugging pull: https://github.com/kinetiknz/cubeb/pull/296 for now.

[0] https://crash-stats.mozilla.com/signature/?proto_signature=~audiounit_setup_stream&version=55.0a1&signature=audiounit_setup_stream&date=%3E%3D2017-04-27T08%3A30%3A00.000Z&date=%3C2017-05-11T03%3A56%3A00.000Z#summary

Comment 19

[Matthew Gregan [:kinetik]]

Attachment: bug1358896_v0.patch

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

Comment 20

[Matthew Gregan [:kinetik]]

Note that this patch needs to be applied on top of the uplift in bug 1358868.

Comment 21

[Alex Chronopoulos [:achronop]]

Comment on attachment 8866628 [details] [diff] [review]
bug1358896_v0.patch

Looks good, thanks for taking care.

Comment 22

[Matthew Gregan [:kinetik]]

Attachment: bug1358896_v1.patch

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.

Comment 23

[Gerry Chang [:gchang]]

Comment on attachment 8867027 [details] [diff] [review]
bug1358896_v1.patch

Fix a crash. Beta54+. Should be in 54 beta 8.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b891a2a9af3d

Comment 25

[C.M.Chang[:chunmin]]

- I am going to close it because there is no report in the last month.
- Re-assign to :kinetik for his contribution.

Comment 26

[C.M.Chang[:chunmin]]

No crash report for this in the last month:
https://crash-stats.mozilla.com/signature/?proto_signature=~audiounit_setup_stream&version=55.0a1&signature=audiounit_setup_stream&date=%3E%3D2017-04-25T07%3A14%3A08.000Z&date=%3C2017-05-25T07%3A14%3A08.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=install_time&_sort=-date&page=1#reports

Comment 27

[Andrei Vaida [:avaida]]

(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.