media/webrtc/trunk/src/modules/video_render/main/source/incoming_video_stream.cc IncomingVideoStream::IncomingVideoStreamProcess reads incoming_render_thread_ without holding a lock at line 287. By contrast, IncomingVideoStream::Stop() reads and writes it only after doing 'thread_critsect_.Enter()'. How this can screw up is with the following interleaving: T1 (in ::IncomingVideoStreamProcess()) vs T2 (::Stop()) T2:241 thread_critsect_.Enter(); T2:242 if (incoming_render_thread_) { T1:287 if (incoming_render_thread_ == NULL) { /*NOT TAKEN*/ T1:290 } T1 now believes we are not in a terminating state T2:243 ThreadWrapper* thread = incoming_render_thread_; T2:244 incoming_render_thread_ = NULL; T2 sets it to 'terminating' state If T1 now accesses incoming_render_thread_ again, we will segfault. Line 292 is: thread_critsect_.Enter(); If this was pushed upwards to cover the test at line 287 I think we'd be safe.

The relevant complaint from Helgrind. The 4th and 5th stacks are the main race report. Note how the writer holds 3 locks but the reader holds none. ---------------------------------------------------------------- Lock at 0x1F4367D8 was first observed at 0x4A0A0C9: pthread_mutex_init (hg_intercepts.c:429) by 0x8061802: webrtc::CriticalSectionPosix::CriticalSectionPosix() (critical_section_posix.cc:26) by 0x8061749: webrtc::CriticalSectionWrapper::CreateCriticalSection() (critical_section.cc:24) by 0x807F847: webrtc::IncomingVideoStream::IncomingVideoStream(int, unsigned int) (incoming_video_stream.cc:39) by 0x8082109: webrtc::ModuleVideoRenderImpl::AddIncomingRenderStream(unsigned int, unsigned int, float, float, float, float) (video_render_impl.cc:470 ) by 0x80A82BC: webrtc::ViERenderer::Init(unsigned int, float, float, float, float) (vie_renderer.cc:120) by 0x80A841D: webrtc::ViERenderer::CreateViERenderer(int, int, webrtc::VideoRender&, webrtc::ViERenderManager&, unsigned int, float, float, float, flo at) (vie_renderer.cc:31) by 0x80A899C: webrtc::ViERenderManager::AddRenderStream(int, void*, unsigned int, float, float, float, float) (vie_render_manager.cc:142) by 0x808F6F7: webrtc::ViERenderImpl::AddRenderer(int, webrtc::RawVideoType, webrtc::ExternalRenderer*) (vie_render_impl.cc:384) by 0x74CD429: mozilla::MediaEngineWebRTCVideoSource::Start(mozilla::SourceMediaStream*, int) (MediaEngineWebRTCVideo.cpp:269) by 0x7344A86: mozilla::MediaOperationRunnable::Run() (MediaManager.h:224) by 0x7C74686: nsThread::ProcessNextEvent(bool, bool*) (nsThread.cpp:627) Lock at 0x1F436848 was first observed at 0x4A0A0C9: pthread_mutex_init (hg_intercepts.c:429) by 0x8061802: webrtc::CriticalSectionPosix::CriticalSectionPosix() (critical_section_posix.cc:26) by 0x8061749: webrtc::CriticalSectionWrapper::CreateCriticalSection() (critical_section.cc:24) by 0x807F850: webrtc::IncomingVideoStream::IncomingVideoStream(int, unsigned int) (incoming_video_stream.cc:40) by 0x8082109: webrtc::ModuleVideoRenderImpl::AddIncomingRenderStream(unsigned int, unsigned int, float, float, float, float) (video_render_impl.cc:470) by 0x80A82BC: webrtc::ViERenderer::Init(unsigned int, float, float, float, float) (vie_renderer.cc:120) by 0x80A841D: webrtc::ViERenderer::CreateViERenderer(int, int, webrtc::VideoRender&, webrtc::ViERenderManager&, unsigned int, float, float, float, float) (vie_renderer.cc:31) by 0x80A899C: webrtc::ViERenderManager::AddRenderStream(int, void*, unsigned int, float, float, float, float) (vie_render_manager.cc:142) by 0x808F6F7: webrtc::ViERenderImpl::AddRenderer(int, webrtc::RawVideoType, webrtc::ExternalRenderer*) (vie_render_impl.cc:384) by 0x74CD429: mozilla::MediaEngineWebRTCVideoSource::Start(mozilla::SourceMediaStream*, int) (MediaEngineWebRTCVideo.cpp:269) by 0x7344A86: mozilla::MediaOperationRunnable::Run() (MediaManager.h:224) by 0x7C74686: nsThread::ProcessNextEvent(bool, bool*) (nsThread.cpp:627) Lock at 0x263391E8 was first observed at 0x4A0A0C9: pthread_mutex_init (hg_intercepts.c:429) by 0x8061802: webrtc::CriticalSectionPosix::CriticalSectionPosix() (critical_section_posix.cc:26) by 0x8061749: webrtc::CriticalSectionWrapper::CreateCriticalSection() (critical_section.cc:24) by 0x8082437: webrtc::ModuleVideoRenderImpl::ModuleVideoRenderImpl(int, webrtc::VideoRenderType, void*, bool) (video_render_impl.cc:97) by 0x8082516: webrtc::VideoRender::CreateVideoRender(int, void*, bool, webrtc::VideoRenderType) (video_render_impl.cc:80) by 0x80A8A37: webrtc::ViERenderManager::AddRenderStream(int, void*, unsigned int, float, float, float, float) (vie_render_manager.cc:128) by 0x808F6F7: webrtc::ViERenderImpl::AddRenderer(int, webrtc::RawVideoType, webrtc::ExternalRenderer*) (vie_render_impl.cc:384) by 0x74CD429: mozilla::MediaEngineWebRTCVideoSource::Start(mozilla::SourceMediaStream*, int) (MediaEngineWebRTCVideo.cpp:269) by 0x7344A86: mozilla::MediaOperationRunnable::Run() (MediaManager.h:224) by 0x7C74686: nsThread::ProcessNextEvent(bool, bool*) (nsThread.cpp:627) by 0x7C3B1D0: NS_ProcessNextEvent_P(nsIThread*, bool) (nsThreadUtils.cpp:237) by 0x7C751F9: nsThread::ThreadFunc(void*) (nsThread.cpp:265) Possible data race during read of size 8 at 0x261A8BB8 by thread #67 Locks held: none at 0x80803FC: webrtc::IncomingVideoStream::IncomingVideoStreamProcess() (incoming_video_stream.cc:287) by 0x8080828: webrtc::IncomingVideoStream::IncomingVideoStreamThreadFun(void*) (incoming_video_stream.cc:282) by 0x8069EDE: webrtc::ThreadPosix::Run() (thread_posix.cc:361) by 0x8069FB6: StartThread (thread_posix.cc:72) by 0x4A09F6C: mythread_wrapper (hg_intercepts.c:219) by 0x30E2C07D13: start_thread (in /usr/lib64/libpthread-2.15.so) by 0x30E28F167C: clone (in /usr/lib64/libc-2.15.so) This conflicts with a previous write of size 8 by thread #45 Locks held: 3, at addresses 0x1F4367D8 0x1F436848 0x263391E8 at 0x80800E1: webrtc::IncomingVideoStream::Stop() (incoming_video_stream.cc:244) by 0x8081DED: webrtc::ModuleVideoRenderImpl::StopRender(unsigned int) (video_render_impl.cc:653) by 0x80A8206: webrtc::ViERenderer::StopRender() (vie_renderer.cc:50) by 0x808F013: webrtc::ViERenderImpl::StopRender(int) (vie_render_impl.cc:258) by 0x74CD6B4: mozilla::MediaEngineWebRTCVideoSource::Stop(mozilla::SourceMediaStream*, int) (MediaEngineWebRTCVideo.cpp:311) by 0x7344B16: mozilla::MediaOperationRunnable::Run() (MediaManager.h:247) by 0x7C74686: nsThread::ProcessNextEvent(bool, bool*) (nsThread.cpp:627) by 0x7C3B1D0: NS_ProcessNextEvent_P(nsIThread*, bool) (nsThreadUtils.cpp:237) Address 0x261A8BB8 is 40 bytes inside a block of size 328 alloc'd at 0x4A072A1: operator new(unsigned long) (vg_replace_malloc.c:298) by 0x80820F8: webrtc::ModuleVideoRenderImpl::AddIncomingRenderStream(unsigned int, unsigned int, float, float, float, float) (video_render_impl.cc:470) by 0x80A82BC: webrtc::ViERenderer::Init(unsigned int, float, float, float, float) (vie_renderer.cc:120) by 0x80A841D: webrtc::ViERenderer::CreateViERenderer(int, int, webrtc::VideoRender&, webrtc::ViERenderManager&, unsigned int, float, float, float, float) (vie_renderer.cc:31) by 0x80A899C: webrtc::ViERenderManager::AddRenderStream(int, void*, unsigned int, float, float, float, float) (vie_render_manager.cc:142) by 0x808F6F7: webrtc::ViERenderImpl::AddRenderer(int, webrtc::RawVideoType, webrtc::ExternalRenderer*) (vie_render_impl.cc:384) by 0x74CD429: mozilla::MediaEngineWebRTCVideoSource::Start(mozilla::SourceMediaStream*, int) (MediaEngineWebRTCVideo.cpp:269) by 0x7344A86: mozilla::MediaOperationRunnable::Run() (MediaManager.h:224) by 0x7C74686: nsThread::ProcessNextEvent(bool, bool*) (nsThread.cpp:627) by 0x7C3B1D0: NS_ProcessNextEvent_P(nsIThread*, bool) (nsThreadUtils.cpp:237) by 0x7C751F9: nsThread::ThreadFunc(void*) (nsThread.cpp:265) by 0x505E156: _pt_root (ptthread.c:158) ----------------------------------------------------------------

Upstream issue with webrtc.org trunk; we need to file an issue (and verify it's not fixed).

I need to generate a patch for this

Comment on attachment 721670 [details] [diff] [review] move critsect to cover thread existence test (upstream issue 1465) Review of attachment 721670 [details] [diff] [review]: ----------------------------------------------------------------- Don't forget to file that upstream issue.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ff35a246bb0 Already filed (note the comment on the patch - Issue 1465)

Backed out for now while we investigate bug 848966. I'll re-land whatever comes up clean. https://hg.mozilla.org/integration/mozilla-inbound/rev/3cb82d9f3f9c

https://hg.mozilla.org/integration/mozilla-inbound/rev/6cab296a4940 Windows PGO M2 is looking green.