640405 - Data race on sa_stream::bytes_written (in libsydneyaudio)

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Julian Seward [:jseward]]

TEST_PATH=content/media/test/test_playback.html

using Helgrinding setup described in bug 551155 comment 13 and later
(ignore preceding junk)

Looks like possibly incorrect locking (two different locks?) when
accessing sa_stream::bytes_written (in sydney_audio_alsa.c) leading to
report shown below.

first access is a write:

sydney_audio_alsa.c:273:  s->bytes_written += nbytes;

second is a read:

sydney_audio_alsa.c:335     *pos = s->bytes_written;

both accessing threads allegedly holding one lock, so presumably it's
not the same lock, else obviously there would be no race.

Am happy to try out suggested patches.

-----------------------------------------------

Possible data race during read of size 8 at 0x184d2568 by thread #32 while holding 1 lock
   at 0x5CE7449: sa_stream_get_position (media/libsydneyaudio/src/sydney_audio_alsa.c:335)
   by 0x5CADFC5: nsAudioStreamLocal::GetSampleOffset() (content/media/nsAudioStream.cpp:574)
   by 0x5CAD9F3: nsAudioStreamLocal::GetPosition() (content/media/nsAudioStream.cpp:556)
   by 0x5CA7034: nsBuiltinDecoderStateMachine::GetAudioClock() (content/media/nsBuiltinDecoderStateMachine.cpp:1301)
   by 0x5CA92D9: nsBuiltinDecoderStateMachine::AdvanceFrame() (content/media/nsBuiltinDecoderStateMachine.cpp:1331)
   by 0x5CA9E64: nsBuiltinDecoderStateMachine::Run() (content/media/nsBuiltinDecoderStateMachine.cpp:1050)
   by 0x6336FEC: nsThread::ProcessNextEvent(int, int*) (xpcom/threads/nsThread.cpp:642)
   by 0x62F6B19: NS_ProcessNextEvent_P(nsIThread*, int) (ff-opt/xpcom/build/nsThreadUtils.cpp:250)
   by 0x63376FC: nsThread::ThreadFunc(void*) (xpcom/threads/nsThread.cpp:281)
   by 0x79ABACC: _pt_root (nsprpub/pr/src/pthreads/ptthread.c:189)
   by 0x4C2CBE7: mythread_wrapper (/home/sewardj/VgTRUNK/hgde2/helgrind/hg_intercepts.c:221)
   by 0x4E369C9: start_thread (/build/buildd/eglibc-2.11.1/nptl/pthread_create.c:300)

 This conflicts with a previous write of size 8 by thread #34 while holding 1 lock
   at 0x5CE75E5: sa_stream_write (media/libsydneyaudio/src/sydney_audio_alsa.c:273)
   by 0x5CAE1E4: nsAudioStreamLocal::Write(void const*, unsigned int, int) (content/media/nsAudioStream.cpp:475)
   by 0x5CA7B7E: nsBuiltinDecoderStateMachine::PlayFromAudioQueue(unsigned long, unsigned int) (content/media/nsBuiltinDecoderStateMachine.cpp:611)
   by 0x5CA86E9: nsBuiltinDecoderStateMachine::AudioLoop() (content/media/nsBuiltinDecoderStateMachine.cpp:476)
   by 0x5CAA892: nsRunnableMethodImpl<void (nsBuiltinDecoderStateMachine::*)(), true>::Run() (ff-opt/content/media/../../dist/include/nsThreadUtils.h:345)
   by 0x6336FEC: nsThread::ProcessNextEvent(int, int*) (xpcom/threads/nsThread.cpp:642)
   by 0x62F6B19: NS_ProcessNextEvent_P(nsIThread*, int) (ff-opt/xpcom/build/nsThreadUtils.cpp:250)
   by 0x63376FC: nsThread::ThreadFunc(void*) (xpcom/threads/nsThread.cpp:281)

 Address 0x184d2568 is 8 bytes inside a block of size 40 alloc'd
   at 0x4C27B08: malloc (/home/sewardj/VgTRUNK/hgde2/coregrind/m_replacemalloc/vg_replace_malloc.c:236)
   by 0x5CE731D: sa_stream_create_pcm (media/libsydneyaudio/src/sydney_audio_alsa.c:109)
   by 0x5CAE3C2: nsAudioStreamLocal::Init(int, int, nsAudioStream::SampleFormat) (content/media/nsAudioStream.cpp:371)
   by 0x5CA7AA1: nsBuiltinDecoderStateMachine::StartPlayback() (content/media/nsBuiltinDecoderStateMachine.cpp:693)
   by 0x5CAA4E0: nsBuiltinDecoderStateMachine::Run() (content/media/nsBuiltinDecoderStateMachine.cpp:1038)
   by 0x6336FEC: nsThread::ProcessNextEvent(int, int*) (xpcom/threads/nsThread.cpp:642)
   by 0x62F6B19: NS_ProcessNextEvent_P(nsIThread*, int) (ff-opt/xpcom/build/nsThreadUtils.cpp:250)
   by 0x63376FC: nsThread::ThreadFunc(void*) (xpcom/threads/nsThread.cpp:281)
   by 0x79ABACC: _pt_root (nsprpub/pr/src/pthreads/ptthread.c:189)
   by 0x4C2CBE7: mythread_wrapper (/home/sewardj/VgTRUNK/hgde2/helgrind/hg_intercepts.c:221)
   by 0x4E369C9: start_thread (/build/buildd/eglibc-2.11.1/nptl/pthread_create.c:300)
   by 0xB24270C: clone (/build/buildd/eglibc-2.11.1/misc/../sysdeps/unix/sysv/linux/x86_64/clone.S:112)

Comment 1

[Matthew Gregan [:kinetik]]

Thanks.  This is sort-of a known problem, but it looks like the bugs discussing it have been closed.  We're also not using the ALSA API in a thread safe way as far as I can tell.  The plan is to fix this by replacing libsydneyaudio with a new audio abstraction (bug 623444).

The locks in questions are:
nsAudioStream::GetPosition called with decoder monitor held.
nsAudioStream::Write called with the audio monitor held.

Unless I'm missing a subtle threading/code generation aspect of this (and assuming an x86-like memory model), it seems like the racing reader thread will only ever see the old or new value of bytes_written, which is not a problem.

Comment 2

[Julian Seward [:jseward]]

(In reply to comment #1)
> Unless I'm missing a subtle threading/code generation aspect of this (and
> assuming an x86-like memory model),

As a general point, the ARM memory model -- which, with the advent of
multicore ARM A9s, we will be exposed to -- allows more reordering
than the x86 model.  The x86 model ensures that writes by one core/cpu
are observed by other cores/cpus in the order in which they were
performed.  The ARM model doesn't guarantee that, so that writes by
one cpu may appear in a different order in a different cpu.  It's
pretty similar to the POWER/PowerPC memory model.

Comment 3

[Julian Seward [:jseward]]

(In reply to comment #1)
> Thanks.  This is sort-of a known problem, [...]

Thanks for the clarification.  It's good at least to know that
Helgrind is finding real stuff, and not merely producing noise.

Comment 4

[Julian Seward [:jseward]]

(In reply to comment #1)
> The plan is to fix this by replacing libsydneyaudio with a
> new audio abstraction (bug 623444).

Should we close this as WONTFIX, then?

Also, is there any approximate timeframe for the new abstraction?
I looked at 623444 but it's not clear from that.

Comment 5

[Matthew Gregan [:kinetik]]

Hopefully Firefox 5.  I'm working on it now.

Comment 6

[Matthew Gregan [:kinetik]]

Fixed by bug 623444.