Closed Bug 1435673 Opened 2 years ago Closed 2 years ago

Crash in libsystem_pthread.dylib@0x1530

Categories

(Core :: WebRTC: Audio/Video, defect, P1, critical)

58 Branch
Unspecified
macOS
defect

Tracking

()

VERIFIED FIXED
mozilla60
Tracking Status
firefox-esr52 --- unaffected
firefox58 --- unaffected
firefox59 --- unaffected
firefox60 + verified

People

(Reporter: pehrsons, Assigned: pehrsons)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(4 files, 1 obsolete file)

This bug was filed from the Socorro interface and is
report bp-530705c1-1ef9-4d94-8063-7f50a0180201.
=============================================================

Top 10 frames of crashing thread:

0 libsystem_pthread.dylib libsystem_pthread.dylib@0x1530 
1 XUL mozilla::SourceMediaStream::EndTrack xpcom/threads/Mutex.h:65
2 XUL mozilla::MediaEngineWebRTCMicrophoneSource::Deallocate dom/media/webrtc/MediaEngineWebRTCAudio.cpp:603
3 XUL mozilla::GetUserMediaTask::Run dom/media/MediaManager.cpp:1008
4 XUL nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1040
5 XUL NS_ProcessNextEvent xpcom/threads/nsThreadUtils.cpp:517
6 XUL mozilla::ipc::MessagePumpForNonMainThreads::Run ipc/glue/MessagePump.cpp:364
7 XUL MessageLoop::Run ipc/chromium/src/base/message_loop.cc:326
8 XUL base::Thread::ThreadMain ipc/chromium/src/base/thread.cc:181
9 XUL ThreadFunc ipc/chromium/src/base/platform_thread_posix.cc:38

=============================================================

This is a new regression from bug 1299515 as it made us no longer allow double-calling Deallocate().
Duplicate of this bug: 1435619
Crash Signature: [@ libsystem_pthread.dylib@0x1530] → [@ libsystem_pthread.dylib@0x1530] [@ RtlEnterCriticalSection | mozilla::SourceMediaStream::EndTrack]
Ok, I understand this better now thanks to the Windows stacks.

An application asks for audio+video, audio is allocated, video fails to allocate so we deallocate audio.

Bug 1299515 changed the allocation flow from creating a track in Start() and ending it in Stop(), to creating it in SetTrack (after Allocate()) and ending it in Deallocate().

In this case SetTrack() doesn't get called but we have no added checks for that case in Deallocate().\

I'll have a patch up soon.
Comment on attachment 8948331 [details]
Bug 1435673 - Fix calling Deallocate() without SetTrack().

https://reviewboard.mozilla.org/r/217810/#review223574

::: commit-message-84151:1
(Diff revision 1)
> +Bug 1435673 - Fix calling Deallocate() without SetTrack(). r?padenot

Indicate here why it is now possible, for example the situation you mentionned in the bug.
Attachment #8948331 - Flags: review?(padenot)
Comment on attachment 8948332 [details]
Bug 1435673 - Inline Alloc/FreeChannel and fix access per threading model.

https://reviewboard.mozilla.org/r/217812/#review223576

::: dom/media/webrtc/MediaEngineWebRTC.h:551
(Diff revision 1)
>    void SetPassThrough(bool aPassThrough);
>  
>    // Owning thread only.
>    RefPtr<WebRTCAudioDataListener> mListener;
>  
> -  // Note: shared across all microphone sources
> +  // Note: shared across all microphone sources. Owning thread only.

"Owning thread" can mean anything. "MediaManager" thread is more precise.
Attachment #8948332 - Flags: review?(padenot) → review+
Comment on attachment 8948333 [details]
Bug 1435673 - Clean up WebRTCAudioDataListener in Deallocate().

https://reviewboard.mozilla.org/r/217814/#review223578

::: commit-message-ef7e7:1
(Diff revision 1)
> +Bug 1435673 - Clean up WebRTCAudioDataListener in Deallocate(). r?padenot

Say why it's now okay to do that. It's probably not ok.
Attachment #8948333 - Flags: review?(padenot)
Comment on attachment 8948334 [details]
Bug 1435673 - Do some cleanup that was meant to happen earlier.

https://reviewboard.mozilla.org/r/217816/#review223582
Attachment #8948334 - Flags: review?(padenot) → review+
Comment on attachment 8948335 [details]
Bug 1435673 - Strengthen some MediaEngineWebRTCMicrophoneSource asserts.

https://reviewboard.mozilla.org/r/217818/#review223584
Attachment #8948335 - Flags: review?(padenot) → review+
Comment on attachment 8948332 [details]
Bug 1435673 - Inline Alloc/FreeChannel and fix access per threading model.

https://reviewboard.mozilla.org/r/217812/#review223576

> "Owning thread" can mean anything. "MediaManager" thread is more precise.

I'm not changing all the other comments mentioning owning thread here.

And while owning thread can mean any thread that's IMO fine. I don't care which thread it is as long as it is the one and same throughout.
Comment on attachment 8948333 [details]
Bug 1435673 - Clean up WebRTCAudioDataListener in Deallocate().

https://reviewboard.mozilla.org/r/217814/#review223578

> Say why it's now okay to do that. It's probably not ok.

Yeah this was not ok. I'll be removing this patch altogether.
(In reply to Andreas Pehrson [:pehrsons] from comment #13)
> Comment on attachment 8948332 [details]
> Bug 1435673 - Inline Alloc/FreeChannel and fix access per threading model.
> 
> https://reviewboard.mozilla.org/r/217812/#review223576
> 
> > "Owning thread" can mean anything. "MediaManager" thread is more precise.
> 
> I'm not changing all the other comments mentioning owning thread here.
> 
> And while owning thread can mean any thread that's IMO fine. I don't care
> which thread it is as long as it is the one and same throughout.

Ok.
Duplicate of this bug: 1435752
Duplicate of this bug: 1435926
Crash Signature: [@ libsystem_pthread.dylib@0x1530] [@ RtlEnterCriticalSection | mozilla::SourceMediaStream::EndTrack] → [@ libsystem_pthread.dylib@0x1530] [@ RtlEnterCriticalSection | mozilla::SourceMediaStream::EndTrack] [@ pthread_mutex_lock | mozilla::detail::MutexImpl::lock | mozilla::SourceMediaStream::EndTrack ] [@ mozilla::SourceMediaStream::EndTrack ]
Attachment #8948333 - Attachment is obsolete: true
Comment on attachment 8948331 [details]
Bug 1435673 - Fix calling Deallocate() without SetTrack().

https://reviewboard.mozilla.org/r/217810/#review223874
Attachment #8948331 - Flags: review?(padenot) → review+
Pushed by pehrsons@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/1914d7b0ec8f
Fix calling Deallocate() without SetTrack(). r=padenot
https://hg.mozilla.org/integration/autoland/rev/a46387ed0d66
Inline Alloc/FreeChannel and fix access per threading model. r=padenot
https://hg.mozilla.org/integration/autoland/rev/a77670d18077
Do some cleanup that was meant to happen earlier. r=padenot
https://hg.mozilla.org/integration/autoland/rev/f1b6dc89e5d9
Strengthen some MediaEngineWebRTCMicrophoneSource asserts. r=padenot
No more crashes in nightly 60 since the patches landed.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.