589069 - valgrind - Possible data race during read of size 8 (nsBuiltinDecoder::MetadataLoaded():333 vs nsBuiltinDecoder::NotifyBytesConsumed(long):561)

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Comment 1

[Matthew Gregan [:kinetik]]

Found running test_playback under Helgrind:

==4691== Possible data race during read of size 8 at 0x1f1c28d0 by thread #1
==4691==    at 0x5772F18: nsBuiltinDecoder::MetadataLoaded() (nsBuiltinDecoder.c
pp:333)
==4691==    by 0x57736A0: nsRunnableMethodImpl<void (nsBuiltinDecoder::*)(), tru
e>::Run() (nsThreadUtils.h:347)
==4691==    by 0x5DA2B50: nsThread::ProcessNextEvent(int, int*) (nsThread.cpp:54
7)
==4691==    by 0x5D66FD1: NS_ProcessNextEvent_P(nsIThread*, int) (nsThreadUtils.
cpp:250)
==4691==    by 0x5C9D496: mozilla::ipc::MessagePump::Run(base::MessagePump::Dele
gate*) (MessagePump.cpp:110)
==4691==    by 0x5DDEB8E: MessageLoop::RunInternal() (message_loop.cc:219)
==4691==    by 0x5DDEB9A: MessageLoop::RunHandler() (message_loop.cc:202)
==4691==    by 0x5DDEC09: MessageLoop::Run() (message_loop.cc:176)
==4691==    by 0x5BD885D: nsBaseAppShell::Run() (nsBaseAppShell.cpp:175)
==4691==    by 0x5A6A772: nsAppStartup::Run() (nsAppStartup.cpp:191)
==4691==    by 0x51B47B0: XRE_main (nsAppRunner.cpp:3659)
==4691==    by 0x400E4F: main (nsBrowserApp.cpp:158)
==4691==  This conflicts with a previous write of size 8 by thread #114
==4691==    at 0x577244F: nsBuiltinDecoder::NotifyBytesConsumed(long) (nsBuiltinDecoder.cpp:561)
==4691==    by 0x57884CE: webm_read(void*, unsigned long, void*) (nsWebMReader.cpp:87)
==4691==    by 0x5788E62: io_read (nestegg.c:488)
==4691==    by 0x5789024: read_uint (nestegg.c:602)
==4691==    by 0x578908A: read_int (nestegg.c:622)
==4691==    by 0x57895C8: read_block (nestegg.c:1198)
==4691==    by 0x578A28B: nestegg_read_packet (nestegg.c:1854)
==4691==    by 0x5787A6D: nsWebMReader::NextPacket(nsWebMReader::TrackType) (nsWebMReader.cpp:494)


NotifyBytesLoaded holds the decoder monitor.  MetadataLoaded does not hold the decoder monitor until after using mDecoderPosition (used at line 333, monitor taken at line 342).

Comment 2

[Matthew Gregan [:kinetik]]

Attachment: patch v0

Take the decoder lock slightly earlier.  mDecoderPosition is already documented as needing the decoder lock held to use.

Comment 3

[Karl Tomlinson (:karlt)]

http://hg.mozilla.org/mozilla-central/rev/810ff22372ac