1037423 - ALSA crash in AudioStream::Shutdown when seeking during playback

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Paul Adenot (:padenot)]

This is a split-off of bug 1032266. It thought that I had a patch for it, but I misunderstood the reporter's problem, so I'm going to summarize the problem he reported here and morph the other bug.

STR:
0. Make sure you use the Alsa cubeb backend (Linux uses pulse by default). --disable-pulse in the mozconfig. alsa is used by default on some BSD and on Linux B2G emulator.
1. Open http://hpr.dogphilosophy.net/test/opus.opus
2. Seek to any position a few times while the file is playing
3. It should crash with an assert() on alsa_stream_destroy

The value of stm->state is RUNNING, there.

Assertion failed: (stm && (stm->state == INACTIVE || stm->state == ERROR)), function alsa_stream_destroy, file media/libcubeb/src/cubeb_alsa.c, line 870.

Program received signal SIGABRT, Aborted.
[Switching to Thread 81e60ec00 (LWP 100717 Media Audio)]
thr_kill () at thr_kill.S:3
3       thr_kill.S: No such file or directory.
(gdb) bt
#0  thr_kill () at thr_kill.S:3
#1  0x00000008012d5579 in abort () at lib/libc/stdlib/abort.c:65
#2  0x00000008012b60b1 in __assert (func=0x8074a9809 <.L__func__.alsa_stream_destroy> "alsa_stream_destroy", file=0xfffff80061a58000 <Address 0xfffff80061a58000 out of bounds>, line=1638236160, failedexpr=<optimized out>) at lib/libc/gen/assert.c:54
#3  0x0000000806316f80 in alsa_stream_destroy (stm=0x821b67580) at media/libcubeb/src/cubeb_alsa.c:870
#4  0x0000000806315903 in cubeb_stream_destroy (stream=0x821b67580) at media/libcubeb/src/cubeb.c:219
#5  0x000000080519c155 in nsAutoRefTraits<cubeb_stream>::Release (aStream=0x821b67580) at content/media/AudioStream.h:25
#6  0x000000080519c090 in nsAutoRefBase<cubeb_stream>::SafeRelease (this=0x821c08e60) at xpcom/base/nsAutoRef.h:667
#7  0x000000080519433f in nsAutoRef<cubeb_stream>::reset (this=0x821c08e60) at xpcom/base/nsAutoRef.h:197
#8  0x000000080517d0b0 in mozilla::AudioStream::Shutdown (this=0x821c08d00) at content/media/AudioStream.cpp:844
#9  0x000000080517c901 in mozilla::AudioSink::Cleanup (this=0x823332580) at content/media/AudioSink.cpp:225
#10 0x000000080517bb48 in mozilla::AudioSink::AudioLoop (this=0x823332580) at content/media/AudioSink.cpp:183

Comment 1

[Jan Beich]

It's possible to reproduce with official Nightly package on PulseAudio-enabled system (such as Ubuntu). To do so find and rename libpulse.so.0 under /usr/lib/ which would make libcubeb fallback to ALSA.

Comment 2

[JW Wang [:jwwang] [:jw_wang]]

AudioStream::Cancel() will break the blocking operation of AudioStream::Drain(). Then it is possible to reach AudioStream::Shutdown() which will call alsa_stream_destroy() even before an ALSA stream switches from RUNNING to DRAINING. And since AudioStream::Cancel() changes |mState| to ERRORED, AudioStream::Shutdown() will not call Pause() which calls alsa_stream_stop() to turn stream state into INACTIVE.

I think the fix in bug 1032266 has a problem. If we allow DRAINING in alsa_stream_destroy(), there will be a race between [1] and [2] where [1] tries to destroy the condition variable and [2] tries to broadcast the condition variable.

[1] http://hg.mozilla.org/mozilla-central/file/7f9db2379b3f/media/libcubeb/src/cubeb_alsa.c#l883
[2] http://hg.mozilla.org/mozilla-central/file/7f9db2379b3f/media/libcubeb/src/cubeb_alsa.c#l398

I think it is better to call alsa_stream_stop() to put an ALSA stream in a stable state before destroying it.

Comment 4

[JW Wang [:jwwang] [:jw_wang]]

Btw, it is easy to reproduce this bug by running mochitest-1 on B2G Desktop Linux Op. (You have to edit content/media/test/mochitest.ini to enable tests on B2G Desktop.)

Comment 5

[JW Wang [:jwwang] [:jw_wang]]

Attachment: 1037423_fix_alsa_stream_destroy-v1.patch

See comment 2 for the root cause.

Force stop the cubeb stream to ensure stable state before destroying the stream. Also fix some race conditions in AudioStream.

Try: https://tbpl.mozilla.org/?tree=Try&rev=4f706e6fb4c0
Looks most green.

Comment 6

[Matthew Gregan [:kinetik]]

(In reply to JW Wang [:jwwang] from comment #2)
> AudioStream::Cancel() will break the blocking operation of
> AudioStream::Drain(). Then it is possible to reach AudioStream::Shutdown()
> which will call alsa_stream_destroy() even before an ALSA stream switches
> from RUNNING to DRAINING. And since AudioStream::Cancel() changes |mState|
> to ERRORED, AudioStream::Shutdown() will not call Pause() which calls
> alsa_stream_stop() to turn stream state into INACTIVE.
> 
> I think the fix in bug 1032266 has a problem. If we allow DRAINING in
> alsa_stream_destroy(), there will be a race between [1] and [2] where [1]
> tries to destroy the condition variable and [2] tries to broadcast the
> condition variable.
> 
> [1]
> http://hg.mozilla.org/mozilla-central/file/7f9db2379b3f/media/libcubeb/src/
> cubeb_alsa.c#l883
> [2]
> http://hg.mozilla.org/mozilla-central/file/7f9db2379b3f/media/libcubeb/src/
> cubeb_alsa.c#l398
> 
> I think it is better to call alsa_stream_stop() to put an ALSA stream in a
> stable state before destroying it.

Agreed, good catch!

Comment 7

[JW Wang [:jwwang] [:jw_wang]]

try on B2G: https://tbpl.mozilla.org/?tree=Try&rev=0c59c9432a45
try on Android: https://tbpl.mozilla.org/?tree=Try&rev=6ab85dcbc1e6
try on other desktop platforms: https://tbpl.mozilla.org/?tree=Try&rev=4f706e6fb4c0

Looks most green.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3a0e9a83d8c5

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3a0e9a83d8c5