571798 - Invalid read [@ snd_config_searcha_hooks] during shutdown

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Jesse Ruderman]

Attachment: valgrind output

This happens sometimes during shutdown.  I don't have steps to reproduce, but I'm pretty sure there's an ogg vorbis file involved.

Comment 1

[Matthew Gregan [:kinetik]]

Looking at the implementation in libasound, snd_pcm_open/snd_pcm_close are not thread-safe.

I can't find any documentation that states what (if any) ALSA functionality *is* thread-safe, so sydneyaudio could be stepping on toes all over the place.

Comment 2

[Matthew Gregan [:kinetik]]

This general problem is probably largely responsible for the random orange in bug 562004, bug 557432, and maybe others.

Comment 3

[Matthew Gregan [:kinetik]]

Adding two locks to sydneyaudio may be sufficient: a library-level lock for snd_pcm_open/snd_pcm_close, and a per-stream lock for everything else.  This would require changing nsBuiltinDecoderStateMachine's logic back to the emulated-non-blocking approach we used to use, otherwise we'll be forced to hold locks around long-running calls to snd_pcm_writei.

Digging around more, mAudioMonitor should already protect most calls into sydneyaudio, so perhaps we only need a lock for the library and any other threading bugs can be attributed to incorrect use of mAudioMonitor (such as bug 560784).

Comment 4

[Matthew Gregan [:kinetik]]

Attachment: patch v0

(In reply to comment #3)
> Digging around more, mAudioMonitor should already protect most calls into
> sydneyaudio, so perhaps we only need a lock for the library and any other
> threading bugs can be attributed to incorrect use of mAudioMonitor (such as bug
> 560784).

The OS X and Win32 sydneyaudio implementations seem to already be thread-safe with respect to concurrent calls to write/get_position, so I think it actually does make sense to make the per-stream stuff thread safe.

This patch does that, but doesn't address the potential problem with blocking snd_pcm_writei calls vs calls to get_position.  The OS X and Win32 backends won't suffer from this problem, so I think I'll need to address it before asking for review.

Comment 5

[Matthew Gregan [:kinetik]]

Attachment: patch v1

Comment 6

[Matthew Gregan [:kinetik]]

Same as the last patch, but addresses the blocking snd_pcm_writei issue.  The key differences are:

Open the pcm device as non-blocking:

   if (snd_pcm_open(&s->output_unit, 
                    "default", 
                    SND_PCM_STREAM_PLAYBACK, 
-                   0) < 0) {
+                   SND_PCM_NONBLOCK) < 0) {

Handle non-blocking writes by dropping the stream lock, waiting for the pcm to become ready, and reacquiring the lock:

+      if (frames == -EAGAIN && snd_pcm_state(s->output_unit) == SND_PCM_STATE_RUNNING) {
+        pthread_mutex_unlock(&s->mutex);
+        snd_pcm_wait(s->output_unit, -1);
+        pthread_mutex_lock(&s->mutex);
+        continue;
+      }

I think this patch is good to go, but needs testing by more people on more machines.  I've tested Fedora 12 with PulseAudio and direct ALSA and it works fine there (under VMWare Fusion 3.1 and VirtualBox 3.2).

Comment 7

[Matthew Gregan [:kinetik]]

Confirmation that it's not safe to call ALSA library functions for the same device concurrently: http://mailman.alsa-project.org/pipermail/alsa-devel/2010-June/028349.html

Unfortunately, I've found a problem with the patch: in my F12/VMWare Fusion 3.1 setup I'm seeing a livelock where snd_pcm_writei returns EAGAIN, snd_pcm_wait returns 1 (ready), and snd_pcm_writei still returns EAGAIN, so the write makes no progress.  I can usually reproduce this within 2-3 runs of 8 parallel sets of test_playback running.

Comment 8

[Matthew Gregan [:kinetik]]

It looks like that hang is fixed in the current git head of alsa-plugins, most likely fixed by the patch discussed in bug 573924 comment 2.

Comment 9

[Julian Seward [:jseward]]

Still seeing this on m-c of two days back
TEST_PATH=content/media/test/test_playback.html

I get the same valgrind errors as Jesse (comment #1).  Native
runs randomly either succeed, terminate with "out of memory" (!)
or terminate with a double free detected by glibc.
This is on Ubuntu 10.04 x86_64.

Comment 10

[Matthew Gregan [:kinetik]]

(In reply to comment #5)
> Created attachment 454312 [details] [diff] [review]
> patch v1

This patch isn't right.  It looks like it's not safe to call snd_pcm_wait on one thread while another thread calls other snd_pcm_* functions.  I think the current patch also breaks drain, since in non-blocking mode the drain will return immediately.  The drain call needs to be modified to wait on the PCM device and check its status until it moves from DRAINING to whatever state it ends up in after draining.

Rather than try to fix everything at once and end up blocked on updating PulseAudio/ALSA on the test machines, I'll attach a patch that fixes this particular bug.

Comment 11

[Matthew Gregan [:kinetik]]

Attachment: patch v2

Comment 12

[Matthew Gregan [:kinetik]]

Comment on attachment 468249 [details] [diff] [review]
patch v2

This adds a library-global lock that's used for creating and destroying streams.  It doesn't solve the general thread-safety problems we have with the current design, but it will prevent a bunch of crashes caused by asserts like the following:

Assertion 'pa_atomic_load(&(s)->_ref) >= 1' failed at pulse/stream.c:253, function pa_stream_unref(). Aborting.

Comment 13

[Chris Pearce [:cpearce (Not reading bugmail)]]

http://hg.mozilla.org/mozilla-central/rev/2ba7ee65686d