Closed Bug 1427009 Opened 2 years ago Closed 2 years ago

Crash in mozalloc_abort | abort | webrtc::StreamId::Set

Categories

(Core :: WebRTC, defect, critical)

58 Branch
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox-esr52 --- unaffected
firefox57 --- unaffected
firefox58 --- wontfix
firefox59 --- fixed

People

(Reporter: philipp, Assigned: mjf)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file)

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."
Flags: needinfo?(mfroman)
Might be related to issues with screensharing in Hangouts, but that has different symptoms
Flags: needinfo?(na-g)
Flags: needinfo?(dminor)
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.
Flags: needinfo?(docfaraday)
This crash has shown up over in bug 1355486.
See Also: → 1355486
(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.
Flags: needinfo?(na-g)
I'll have a look.
Assignee: nobody → dminor
Flags: needinfo?(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.
See Also: → 1429571
Pushed by mfroman@nostrum.com:
https://hg.mozilla.org/integration/autoland/rev/c8ad0b964a65
limit accepted mid length to 16 chars. r=bwc
https://hg.mozilla.org/mozilla-central/rev/c8ad0b964a65
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
low volume crash, we've built the last beta for 58, wontfix.
You need to log in before you can comment on or make changes to this bug.