Closed Bug 639721 Opened 14 years ago Closed 14 years ago

Data race on nsBuiltinDecoderReader::mInfo

Categories

(Core :: Audio/Video, defect)

Product:
Core
Component:
Audio/Video
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla5

People

(Reporter: cajbir, Assigned: cajbir)

Details

Attachments

(1 file, 2 obsolete files)

Fix
14.81 KB, patch
cajbir
: review+
Details | Diff | Splinter Review 
Split out form bug 638807 comment 3:

FTR (mostly so I don't lose this info), immediately prior to the
above-discussed
race, two other races were reported.  They look closely related.  Comments/
analysis/"this is the same problem"/"this isn't the same problem" welcomed.

The "while holding N locks" feature is intended to be helpful, but has been
present in the tool for a whole 6 hours now :-) so don't take it as gospel.

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

thread announcement (not an error)

Thread #27 was created
   at 0xB2DB6CE: clone
(/build/buildd/eglibc-2.11.1/misc/../sysdeps/unix/sysv/linux/x86_64/clone.S:77)
   by 0x4E37172: pthread_create@@GLIBC_2.2.5
(/build/buildd/eglibc-2.11.1/nptl/../nptl/sysdeps/pthread/createthread.c:75)
   by 0x4C2CA4A: pthread_create_WRK
(/home/sewardj/VgTRUNK/hgdev/helgrind/hg_intercepts.c:257)
   by 0x4C2CB5E: pthread_create@*
(/home/sewardj/VgTRUNK/hgdev/helgrind/hg_intercepts.c:288)
   by 0x7A44D83: _PR_CreateThread (nsprpub/pr/src/pthreads/ptthread.c:432)
   by 0x7A44FD4: PR_CreateThread (nsprpub/pr/src/pthreads/ptthread.c:515)
   by 0x63A19EA: nsThread::Init() (xpcom/threads/nsThread.cpp:355)
   by 0x63A29AD: nsThreadManager::NewThread(unsigned int, nsIThread**)
(xpcom/threads/nsThreadManager.cpp:249)
   by 0x6361A80: NS_NewThread_P(nsIThread**, nsIRunnable*)
(ff-opt/xpcom/build/nsThreadUtils.cpp:74)
   by 0x5CB1E8D: nsBuiltinDecoder::StartStateMachineThread()
(content/media/nsBuiltinDecoder.cpp:237)
   by 0x5CB2124: nsBuiltinDecoder::Load(nsMediaStream*, nsIStreamListener**,
nsMediaDecoder*) (content/media/nsBuiltinDecoder.cpp:227)
   by 0x5AD2340: nsHTMLMediaElement::InitializeDecoderForChannel(nsIChannel*,
nsIStreamListener**) (content/html/content/src/nsHTMLMediaElement.cpp:1863)

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

Possible data race during write of size 1 at 0x152a8284 by thread #27 while
holding 1 lock
   at 0x5CC91C6: nsOggReader::ReadMetadata()
(content/media/ogg/nsOggReader.cpp:305)
   by 0x5CB382C: nsBuiltinDecoderStateMachine::LoadMetadata()
(content/media/nsBuiltinDecoderStateMachine.cpp:1521)
   by 0x5CC5B15: nsOggDecoderStateMachine::LoadMetadata()
(content/media/ogg/nsOggDecoderStateMachine.cpp:51)
   by 0x5CB581C: nsBuiltinDecoderStateMachine::Run()
(content/media/nsBuiltinDecoderStateMachine.cpp:980)
   by 0x63A142C: nsThread::ProcessNextEvent(int, int*)
(xpcom/threads/nsThread.cpp:642)
   by 0x63618C9: NS_ProcessNextEvent_P(nsIThread*, int)
(ff-opt/xpcom/build/nsThreadUtils.cpp:250)
   by 0x63A1B3C: nsThread::ThreadFunc(void*) (xpcom/threads/nsThread.cpp:281)
   by 0x7A44A1C: _pt_root (nsprpub/pr/src/pthreads/ptthread.c:189)
   by 0x4C2CBE7: mythread_wrapper
(/home/sewardj/VgTRUNK/hgdev/helgrind/hg_intercepts.c:221)
   by 0x4E369C9: start_thread
(/build/buildd/eglibc-2.11.1/nptl/pthread_create.c:300)
   by 0xB2DB70C: clone
(/build/buildd/eglibc-2.11.1/misc/../sysdeps/unix/sysv/linux/x86_64/clone.S:112)

 This conflicts with a previous read of size 1 by thread #1 while holding 1
lock
   at 0x5CB44C8: nsBuiltinDecoderStateMachine::HaveNextFrameData() const
(content/media/nsBuiltinDecoderStateMachine.cpp:192)
   by 0x5CB4561: nsBuiltinDecoderStateMachine::GetNextFrameStatus()
(content/media/nsBuiltinDecoderStateMachine.cpp:750)
   by 0x5CB1514: nsBuiltinDecoder::UpdateReadyStateForData()
(content/media/nsBuiltinDecoder.cpp:639)
   by 0x5CB1A10: nsBuiltinDecoder::NotifyDownloadEnded(unsigned int)
(content/media/nsBuiltinDecoder.cpp:593)
   by 0x5CB0D6A: DataEnded::Run() (content/media/nsMediaStream.cpp:738)
   by 0x63A142C: nsThread::ProcessNextEvent(int, int*)
(xpcom/threads/nsThread.cpp:642)
   by 0x63618C9: NS_ProcessNextEvent_P(nsIThread*, int)
(ff-opt/xpcom/build/nsThreadUtils.cpp:250)
   by 0x625AC52: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*)
(ipc/glue/MessagePump.cpp:110)

 Address 0x152a8284 is 324 bytes inside a block of size 616 alloc'd
   at 0x4C27B08: malloc
(/home/sewardj/VgTRUNK/hgdev/coregrind/m_replacemalloc/vg_replace_malloc.c:236)
   by 0x7410251: moz_xmalloc (memory/mozalloc/mozalloc.cpp:98)
   by 0x5CC5B90:
nsOggDecoderStateMachine::nsOggDecoderStateMachine(nsBuiltinDecoder*)
(ff-opt/content/media/ogg/../../../dist/include/mozilla/mozalloc.h:229)
   by 0x5CC5A5D: nsOggDecoder::CreateStateMachine()
(content/media/ogg/nsOggDecoder.cpp:46)
   by 0x5CB206E: nsBuiltinDecoder::Load(nsMediaStream*, nsIStreamListener**,
nsMediaDecoder*) (content/media/nsBuiltinDecoder.cpp:209)
   by 0x5AD2340: nsHTMLMediaElement::InitializeDecoderForChannel(nsIChannel*,
nsIStreamListener**) (content/html/content/src/nsHTMLMediaElement.cpp:1863)
   by 0x5AD2453:
nsHTMLMediaElement::MediaLoadListener::OnStartRequest(nsIRequest*,
nsISupports*) (content/html/content/src/nsHTMLMediaElement.cpp:304)
   by 0x565C313: nsBaseChannel::OnStartRequest(nsIRequest*, nsISupports*)
(netwerk/base/src/nsBaseChannel.cpp:712)
   by 0x5664513: nsInputStreamPump::OnStateStart()
(netwerk/base/src/nsInputStreamPump.cpp:441)
   by 0x56649B4: nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*)
(netwerk/base/src/nsInputStreamPump.cpp:397)
   by 0x6389809: nsInputStreamReadyEvent::Run()
(xpcom/io/nsStreamUtils.cpp:112)
   by 0x63A142C: nsThread::ProcessNextEvent(int, int*)
(xpcom/threads/nsThread.cpp:642)

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

Possible data race during write of size 1 at 0x152a8285 by thread #27 while
holding 1 lock
   at 0x5CC91D5: nsOggReader::ReadMetadata()
(content/media/ogg/nsOggReader.cpp:306)
   by 0x5CB382C: nsBuiltinDecoderStateMachine::LoadMetadata()
(content/media/nsBuiltinDecoderStateMachine.cpp:1521)
   by 0x5CC5B15: nsOggDecoderStateMachine::LoadMetadata()
(content/media/ogg/nsOggDecoderStateMachine.cpp:51)
   by 0x5CB581C: nsBuiltinDecoderStateMachine::Run()
(content/media/nsBuiltinDecoderStateMachine.cpp:980)
   by 0x63A142C: nsThread::ProcessNextEvent(int, int*)
(xpcom/threads/nsThread.cpp:642)
   by 0x63618C9: NS_ProcessNextEvent_P(nsIThread*, int)
(ff-opt/xpcom/build/nsThreadUtils.cpp:250)
   by 0x63A1B3C: nsThread::ThreadFunc(void*) (xpcom/threads/nsThread.cpp:281)
   by 0x7A44A1C: _pt_root (nsprpub/pr/src/pthreads/ptthread.c:189)
   by 0x4C2CBE7: mythread_wrapper
(/home/sewardj/VgTRUNK/hgdev/helgrind/hg_intercepts.c:221)
   by 0x4E369C9: start_thread
(/build/buildd/eglibc-2.11.1/nptl/pthread_create.c:300)
   by 0xB2DB70C: clone
(/build/buildd/eglibc-2.11.1/misc/../sysdeps/unix/sysv/linux/x86_64/clone.S:112)

 This conflicts with a previous read of size 1 by thread #1 while holding 1
lock
   at 0x5CB44DD: nsBuiltinDecoderStateMachine::HaveNextFrameData() const
(content/media/nsBuiltinDecoderStateMachine.cpp:192)
   by 0x5CB4561: nsBuiltinDecoderStateMachine::GetNextFrameStatus()
(content/media/nsBuiltinDecoderStateMachine.cpp:750)
   by 0x5CB1514: nsBuiltinDecoder::UpdateReadyStateForData()
(content/media/nsBuiltinDecoder.cpp:639)
   by 0x5CB1A10: nsBuiltinDecoder::NotifyDownloadEnded(unsigned int)
(content/media/nsBuiltinDecoder.cpp:593)
   by 0x5CB0D6A: DataEnded::Run() (content/media/nsMediaStream.cpp:738)
   by 0x63A142C: nsThread::ProcessNextEvent(int, int*)
(xpcom/threads/nsThread.cpp:642)
   by 0x63618C9: NS_ProcessNextEvent_P(nsIThread*, int)
(ff-opt/xpcom/build/nsThreadUtils.cpp:250)
   by 0x625AC52: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*)
(ipc/glue/MessagePump.cpp:110)

 Address 0x152a8285 is 325 bytes inside a block of size 616 alloc'd
   at 0x4C27B08: malloc
(/home/sewardj/VgTRUNK/hgdev/coregrind/m_replacemalloc/vg_replace_malloc.c:236)
   by 0x7410251: moz_xmalloc (memory/mozalloc/mozalloc.cpp:98)
   by 0x5CC5B90:
nsOggDecoderStateMachine::nsOggDecoderStateMachine(nsBuiltinDecoder*)
(ff-opt/content/media/ogg/../../../dist/include/mozilla/mozalloc.h:229)
   by 0x5CC5A5D: nsOggDecoder::CreateStateMachine()
(content/media/ogg/nsOggDecoder.cpp:46)
   by 0x5CB206E: nsBuiltinDecoder::Load(nsMediaStream*, nsIStreamListener**,
nsMediaDecoder*) (content/media/nsBuiltinDecoder.cpp:209)
   by 0x5AD2340: nsHTMLMediaElement::InitializeDecoderForChannel(nsIChannel*,
nsIStreamListener**) (content/html/content/src/nsHTMLMediaElement.cpp:1863)
   by 0x5AD2453:
nsHTMLMediaElement::MediaLoadListener::OnStartRequest(nsIRequest*,
nsISupports*) (content/html/content/src/nsHTMLMediaElement.cpp:304)
   by 0x565C313: nsBaseChannel::OnStartRequest(nsIRequest*, nsISupports*)
(netwerk/base/src/nsBaseChannel.cpp:712)
   by 0x5664513: nsInputStreamPump::OnStateStart()
(netwerk/base/src/nsInputStreamPump.cpp:441)
   by 0x56649B4: nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*)
(netwerk/base/src/nsInputStreamPump.cpp:397)
   by 0x6389809: nsInputStreamReadyEvent::Run()
(xpcom/io/nsStreamUtils.cpp:112)
   by 0x63A142C: nsThread::ProcessNextEvent(int, int*)
(xpcom/threads/nsThread.cpp:642)
From bug 638807 comment 4:

The issue in comment 1 appears to be different. There is an mInfo structure
owned by nsOggReader. This is written to in the state machine thread and
guarded by a monitor owned by nsOggReader (through its base class
nsBuiltinDecoderReader).

It's read from the main thread in nsOggDecoderStateMachine::HaveNextFrameData.
This is guarded by a lock on the nsDecoder's monitor. This monitor and
nsOggReader's monitor are different monitors.

The mInfo in nsBuiltinDecoderReader shouldn't be accessed without taking its
lock.
Assignee: nobody → chris.double
Status: NEW → ASSIGNED
Attached patch Possible fix (obsolete) — — Splinter Review 
This changes mInfo so that nsBuiltinDecoderStateMachine keeps it's own version of it that is protected by it's monitor. nsBuiltinDecoderReader has it's version protected by its monitor. Since the structure is populated during metadata read, and not updated afterwards, I think keeping the two copies should be ok.
Attachment #518259 - Flags: review?(kinetik)
     res = mReader->ReadMetadata();
+    mReader->GetInfo(&info);

Why not pass &info to ReadMetadata and have that fill it?  Then there's no need for GetInfo.
I did it this way so that implementers of backends didn't have to do the assign to the passed in info structure in each backend. I'll change it to your method though since it does make the interface clearer.
Attached patch Fix (obsolete) — — Splinter Review 
Address review comment
Attachment #518259 - Attachment is obsolete: true
Attachment #518259 - Flags: review?(kinetik)
Attachment #519532 - Flags: review?(kinetik)
Attachment #519532 - Flags: review?(kinetik) → review+
Whiteboard: [needs landing post ff4]
Attached patch Fix — — Splinter Review 
rebase, carry forward r+
Attachment #519532 - Attachment is obsolete: true
Attachment #521399 - Flags: review+
http://hg.mozilla.org/mozilla-central/rev/5915aecd1db7
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing post ff4]
Target Milestone: --- → mozilla2.2
Can someone confirm if this is fixed?
From a couple of test runs with Helgrind, I can no longer detect the
race, so I guess it is fixed.  If it reappears I'll post again.
Marking VERIFIED based on comment 9.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: