ALSA crash in AudioStream::Shutdown when seeking during playback

RESOLVED FIXED in mozilla33

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: padenot, Assigned: jwwang)

Tracking

(Blocks 1 bug)

unspecified
mozilla33
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(1 attachment)

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
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.
Blocks: 948269
Crash Signature: [@ cubeb_stream_destroy]
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.
Flags: needinfo?(kinetik)
Duplicate of this bug: 1037331
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.)
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.
Assignee: nobody → jwwang
Status: NEW → ASSIGNED
Attachment #8458395 - Flags: review?(kinetik)
Attachment #8458395 - Flags: review?(kinetik) → review+
(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!
Flags: needinfo?(kinetik)
Duplicate of this bug: 1040436
Crash Signature: [@ cubeb_stream_destroy] → [@ libc-2.19.so@0x33d67 ] [@ cubeb_stream_destroy]
No longer blocks: 1040436
https://hg.mozilla.org/mozilla-central/rev/3a0e9a83d8c5
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Blocks: 1041385
You need to log in before you can comment on or make changes to this bug.