Closed Bug 1427009 Opened 2 years ago Closed 2 years ago
Crash in mozalloc
_abort | abort | webrtc::Stream Id::Set
59 bytes, text/x-review-board-request
This bug was filed from the Socorro interface and is report bp-f5709f5d-714a-403f-ba65-980250171211. ============================================================= Top 10 frames of crashing thread: 0 firefox mozalloc_abort memory/mozalloc/mozalloc_abort.cpp:33 1 firefox abort memory/mozalloc/mozalloc_abort.cpp:80 2 libxul.so rtc::FatalMessage::~FatalMessage media/webrtc/trunk/webrtc/base/checks.cc:109 3 libxul.so webrtc::StreamId::Set media/webrtc/trunk/webrtc/common_types.cc:32 4 libxul.so webrtc::RTPSender::SetMId media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/rtp_sender.cc:227 5 libxul.so webrtc::internal::VideoSendStreamImpl::VideoSendStreamImpl media/webrtc/trunk/webrtc/video/video_send_stream.cc:830 6 libxul.so webrtc::internal::VideoSendStream::ConstructionTask::Run media/webrtc/trunk/webrtc/video/video_send_stream.cc:478 7 libxul.so rtc::TaskQueue::OnWakeup media/webrtc/trunk/webrtc/base/task_queue_libevent.cc:300 8 libxul.so event_process_active_single_queue.isra.55 9 libxul.so event_base_loop ============================================================= these content crashes in webrtc code are newly showing up in firefox 58 - on osx & linux only, so with quite a low frequency overall. the comment to this particular report says "The crash happends when I trie to share my screen in a RTC call using Circuit by Unify. This functionality have been working in Firefox for a long time."
Might be related to issues with screensharing in Hangouts, but that has different symptoms
libxul.so webrtc::StreamId::Set media/webrtc/trunk/webrtc/common_types.cc:32 is RTC_CHECK_LE(size, kMaxSize), so presumably we're aborting when trying to set an MId that is too long.
JsepSessionImpl sets JsepTransport->mTransportId from the SDP MID attr (or generates one if the MID attr is missing) here: https://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp#1130-1136 Either we're getting a MID that is longer than 16 chars from SDP (which I'm pretty sure is legal), or we're generating one from an sdp section greater than 99 (which seems somewhat unlikely). The MID rtp header extension is based on StreamId which is limited to 16 chars to fit into a one-byte RTP header extension. I don't believe two-byte rtp header extensions are implemented in the webrtc.org code. Byron, can you confirm that an MID longer than 16 chars from SDP is legal? Any thoughts on how we might work around getting a MID from SDP that is longer than 16 chars? We could add code to simply limit the length to the allowable 16 chars down in rtp_sender. This would eliminate the abort, but might not result in a working call.
Flags: needinfo?(mfroman) → needinfo?(docfaraday)
The base SDP MID spec says that mid can be any length. Looks like we'll need to add a check to reject SDP with too long of a mid. I guess we should also consider shortening the mids we choose, to avoid the need to limit the number of m-sections.
(In reply to Byron Campen [:bwc] from comment #4) > The base SDP MID spec says that mid can be any length. Looks like we'll need > to add a check to reject SDP with too long of a mid. This shouldn't be too difficult and should be fairly easy to test. > I guess we should also > consider shortening the mids we choose, to avoid the need to limit the > number of m-sections. This is definitely doable.
I'll have a look.
Assignee: nobody → dminor
I already have most of this fixed, I'll take it - sorry I didn't mark it Friday night.
Assignee: dminor → mfroman
Comment on attachment 8940940 [details] Bug 1427009 - limit accepted mid length to 16 chars. https://reviewboard.mozilla.org/r/211218/#review217192 ::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:1134 (Diff revision 1) > - os << "level_" << msection.GetLevel() << "(no mid)"; > + os << "no_mid_lvl_" << msection.GetLevel(); > + // This works providing we don't have an msection level higher than 99999. > + // We need to fit inside the 16 character mid limitation that results from > + // not having two-byte rtp header extensions support in webrtc.org yet. > transport->mTransportId = os.str(); > } > + // This assert can go away when webrtc.org supports 2-byte rtp header exts. > + MOZ_ASSERT(transport->mTransportId.length() <= 16); Huh. So we're using mTransportId to set the MID later on in webrtc.org now. That's surprising. We shouldn't be setting a MID at all if there wasn't one in the SDP, right?
Attachment #8940940 - Flags: review?(docfaraday)
Comment on attachment 8940940 [details] Bug 1427009 - limit accepted mid length to 16 chars. https://reviewboard.mozilla.org/r/211218/#review217194 r+, but we need a follow-up to stop configuring MIDs that we just made up
Attachment #8940940 - Flags: review+
Comment on attachment 8940940 [details] Bug 1427009 - limit accepted mid length to 16 chars. https://reviewboard.mozilla.org/r/211218/#review217192 > Huh. So we're using mTransportId to set the MID later on in webrtc.org now. That's surprising. We shouldn't be setting a MID at all if there wasn't one in the SDP, right? Do you have a better solution in mind than using mTransportId? I originally used it when I worked on Bug 1402495 because it was available in MediaPipelineFactory when we're creating Video or Audio conduits. I'm open to doing a more correct fix if there is a better way.
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/c8ad0b964a65 limit accepted mid length to 16 chars. r=bwc
You need to log in before you can comment on or make changes to this bug.