Data race on nsBuiltinDecoderReader::mInfo

VERIFIED FIXED in mozilla5

Status

()

VERIFIED FIXED
8 years ago
8 years ago

People

(Reporter: cajbir, Assigned: cajbir)

Tracking

Trunk
mozilla5
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

8 years ago
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)
(Assignee)

Comment 1

8 years ago
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)

Updated

8 years ago
Assignee: nobody → chris.double
Status: NEW → ASSIGNED
(Assignee)

Comment 2

8 years ago
Created attachment 518259 [details] [diff] [review]
Possible fix

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.
(Assignee)

Comment 4

8 years ago
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.
(Assignee)

Comment 5

8 years ago
Created attachment 519532 [details] [diff] [review]
Fix

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+
(Assignee)

Updated

8 years ago
Whiteboard: [needs landing post ff4]
(Assignee)

Comment 6

8 years ago
Created attachment 521399 [details] [diff] [review]
Fix

rebase, carry forward r+
Attachment #519532 - Attachment is obsolete: true
Attachment #521399 - Flags: review+
(Assignee)

Comment 7

8 years ago
http://hg.mozilla.org/mozilla-central/rev/5915aecd1db7
Status: ASSIGNED → RESOLVED
Last Resolved: 8 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.