1399978 - Crash in mozalloc_abort | abort | cubeb_pulse::capi::capi_init

Status: RESOLVED FIXED

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

Description

[Andrew McCreight [:mccr8]]

This bug was filed from the Socorro interface and is 
report bp-3338f41c-5b84-4e50-a71c-f0d660170913.
=============================================================

This is the number 3 top crash on Linux Nightly for 9-13, but the crashes are only from a couple of different installations. These all are Rust panics, on the assert "assertion failed: ctx.default_sink_info.is_some()". While there is a surge of these crashes on 9-13, I also see a few in the last week on a build from 8-17.

Comment 1

[Maire Reavy [:mreavy]]

Dan -- I believe this is your area.  Can you take this?

Comment 2

[u480271]

What happened that we suddenly get that failure?!

Comment 3

[u480271]

This assert is firing because it's likely the machine doesn't have any pulse audio sinks.

I reproduced the behaviour on my local machine by removing all sinks:
$ pacmd unload-module 6 { Disable ALSA }
$ pacmd unload-module 14 { Disable ensure sink }
$ pacmd list-sinks
0 sink(s) available.
$ ./test_devices
Running main() from gtest_main.cc
[==========] Running 2 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 2 tests from cubeb
[ RUN      ] cubeb.destroy_default_collection
test_devices: /home/djg/Mozilla/cubeb/src/cubeb_pulse.c:633: pulse_init: Assertion `ctx->default_sink_info' failed.
[1]    32214 abort (core dumped)  ./test_devices

This is the same assert in the C version of cubeb's pulse audio backend.

I've reviewed the rust version and it's safe to remove the assert.  Rust forces handling this case and cubeb will return errors instead of dummy values or crashing.

Comment 4

[u480271]

Attachment: Bug 1399978 - Update cubeb-pulse-rs to git commit 2e22e53.

Review commit: https://reviewboard.mozilla.org/r/183070/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/183070/

Comment 5

[Matthew Gregan [:kinetik]]

Comment on attachment 8911673 [details]
Bug 1399978 - Update cubeb-pulse-rs to git commit 2e22e53.

https://reviewboard.mozilla.org/r/183070/#review188262

Comment 6

[u480271]

Comment on attachment 8911673 [details]
Bug 1399978 - Update cubeb-pulse-rs to git commit 2e22e53.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183070/diff/1-2/

Comment 7

[u480271]

Incorporated the fixes from upstream cubeb-pulse-rs.

Comment 8

[u480271]

try - https://treeherder.mozilla.org/#/jobs?repo=try&revision=496146977ee6&selectedJob=133256336

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4d35361e1aec
Update cubeb-pulse-rs to git commit 2e22e53. r=kinetik

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/4d35361e1aec

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Is cubeb-pulse-rs Nightly-only at the moment?

Comment 12

[u480271]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #11)
> Is cubeb-pulse-rs Nightly-only at the moment?

Yes: http://searchfox.org/mozilla-central/source/media/libcubeb/src/cubeb.c#169

But it issue exists in the C version of cubeb_pulse. Do we want to uplift cubeb to all the old versions?

Comment 13

[u480271]

Matthew, should we apply the cubeb_pulse.c fixes as a patch in gecko?

Comment 14

[Matthew Gregan [:kinetik]]

(In reply to Dan Glastonbury :kamidphish from comment #13)
> Matthew, should we apply the cubeb_pulse.c fixes as a patch in gecko?

Given the same assert has been present in cubeb_pulse for a long time, it's weird this was suddenly a top crash now.  But it seems worth uplifting.  Fixes a crash and seems fairly safe.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Is there a different signature for the C version of the crashes? Because all the reports I'm seeing on crash-stats are from Nightly builds only (57a1/58a1).

Comment 16

[u480271]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #15)
> Is there a different signature for the C version of the crashes? Because all
> the reports I'm seeing on crash-stats are from Nightly builds only
> (57a1/58a1).

The rust version is supposed to be a direct port of the C version. Looking at the stack we hit the assert and pulse audio doesn't have any output devices. But I can't find a corresponding signature for pulse_init. That means it wasn't being reported correctly in the C version or there's a subtle difference in the rust version.

There are these mysterious crashes[1] (top 3) that are reported inside libpulsecommon.so and start from cubeb_init. There's something a bit odd with the stacks that there are no frames for pulse_init or below. Perhaps these are the equivalent crash?

[1] https://crash-stats.mozilla.com/search/?signature=~pulse&platform=Linux&date=%3E%3D2017-03-29T01%3A11%3A26.000Z&date=%3C2017-07-12T01%3A11%3A00.000Z&_sort=-date&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#facet-signature

Comment 17

[u480271]

Actually, here's the same assert happening in the C version of cubeb_pulse. (Took me a while to work out how to use the search interface)

https://crash-stats.mozilla.com/search/?proto_signature=~pulse_init&reason=%3DSIGABRT&platform=Linux&date=%3E%3D2017-03-29T05%3A49%3A10.000Z&date=%3C2017-06-12T05%3A49%3A00.000Z&_sort=-date&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#facet-signature

I found reports for 57 too. I'd like to nominate that we uplift the fix to the C code to all relevant firefox versions. (Should we open a new bug for that?)

Comment 18

[Ryan VanderMeulen [:RyanVM]]

If it's the same root problem, I think attaching a branch patch here and requesting approval on it would be fine.

Comment 19

[u480271]

Attachment: Port cubeb_pulse.c fix for startup assert.

Ported pulse_audio only fix to stop cubeb asserting if the machine is
configured without any audio sinks.

MozReview-Commit-ID: ATaAfnJ6Yfa

Comment 20

[u480271]

Attachment: Port cubeb_pulse.c fix for startup assert.

Ported pulse_audio only fix to stop cubeb asserting if the machine is
configured without any audio sinks.

MozReview-Commit-ID: ATaAfnJ6Yfa

Comment 21

[u480271]

Comment on attachment 8916470 [details] [diff] [review]
Port cubeb_pulse.c fix for startup assert.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1399978
[User impact if declined]: FF Startup crash if PulseAudio has no output sinks.
[Is this code covered by automated tests?]: Y
[Has the fix been verified in Nightly?]: Y
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: No
[Why is the change risky/not risky?]: This patch removes an unnecessary assertion and improves handling the case which caused the assertion to fire. It is a small amount of code that is easy to inspect to verify in addition to local and automated testing.
[String changes made/needed]: None

Comment 22

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8916470 [details] [diff] [review]
Port cubeb_pulse.c fix for startup assert.

Fixes a new crash in 57, beta57+

Comment 23

[Ryan VanderMeulen [:RyanVM]]

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

Comment 24

[Andrei Vaida [:avaida]]

(In reply to Dan Glastonbury :kamidphish from comment #21)
> [Is this code covered by automated tests?]: Y
> [Has the fix been verified in Nightly?]: Y
> [Needs manual test from QE? If yes, steps to reproduce]: 

Setting qe-verify- based on Dan's assessment on manual testing needs and the fact that this fix has automated coverage.