Closed Bug 1387525 Opened 7 years ago Closed 7 years ago

Crash in webrtc::NetEqImpl::InsertPacketInternal

Categories

(Core :: WebRTC, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: philipp, Assigned: dminor)

References

(Blocks 1 open bug)

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is 
report bp-136e1645-191c-448e-a363-a9a1c0170804.
=============================================================
Crashing Thread (12), Name: Socket Thread
Frame 	Module 	Signature 	Source
0 	xul.dll 	webrtc::NetEqImpl::InsertPacketInternal(webrtc::WebRtcRTPHeader const&, rtc::ArrayView<unsigned char const >, unsigned int) 	media/webrtc/trunk/webrtc/modules/audio_coding/neteq/neteq_impl.cc:694
1 	xul.dll 	webrtc::NetEqImpl::InsertPacket(webrtc::WebRtcRTPHeader const&, rtc::ArrayView<unsigned char const >, unsigned int) 	media/webrtc/trunk/webrtc/modules/audio_coding/neteq/neteq_impl.cc:141
2 	xul.dll 	webrtc::acm2::AcmReceiver::InsertPacket(webrtc::WebRtcRTPHeader const&, rtc::ArrayView<unsigned char const >) 	media/webrtc/trunk/webrtc/modules/audio_coding/acm2/acm_receiver.cc:107
3 	xul.dll 	webrtc::`anonymous namespace'::AudioCodingModuleImpl::IncomingPacket 	media/webrtc/trunk/webrtc/modules/audio_coding/acm2/audio_coding_module.cc:1096
4 	xul.dll 	webrtc::voe::Channel::OnReceivedPayloadData(unsigned char const*, unsigned __int64, webrtc::WebRtcRTPHeader const*) 	media/webrtc/trunk/webrtc/voice_engine/channel.cc:580
5 	xul.dll 	webrtc::RTPReceiverAudio::ParseAudioCodecSpecific(webrtc::WebRtcRTPHeader*, unsigned char const*, unsigned __int64, webrtc::AudioPayload const&, bool) 	media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/rtp_receiver_audio.cc:304
6 	xul.dll 	webrtc::RTPReceiverAudio::ParseRtpPacket(webrtc::WebRtcRTPHeader*, webrtc::PayloadUnion const&, bool, unsigned char const*, unsigned __int64, __int64, bool) 	media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/rtp_receiver_audio.cc:154
7 	xul.dll 	webrtc::RtpReceiverImpl::IncomingRtpPacket(webrtc::RTPHeader const&, unsigned char const*, unsigned __int64, webrtc::PayloadUnion, bool) 	media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/rtp_receiver_impl.cc:186
8 	xul.dll 	webrtc::voe::Channel::ReceivePacket(unsigned char const*, unsigned __int64, webrtc::RTPHeader const&, bool) 	media/webrtc/trunk/webrtc/voice_engine/channel.cc:1661
9 	xul.dll 	webrtc::voe::Channel::ReceivedRTPPacket(unsigned char const*, unsigned __int64, webrtc::PacketTime const&) 	media/webrtc/trunk/webrtc/voice_engine/channel.cc:1643
10 	xul.dll 	webrtc::VoENetworkImpl::ReceivedRTPPacket(int, void const*, unsigned __int64, webrtc::PacketTime const&) 	media/webrtc/trunk/webrtc/voice_engine/voe_network_impl.cc:87
11 	xul.dll 	webrtc::VoENetworkImpl::ReceivedRTPPacket(int, void const*, unsigned __int64) 	media/webrtc/trunk/webrtc/voice_engine/voe_network_impl.cc:63
12 	xul.dll 	mozilla::WebrtcAudioConduit::ReceivedRTPPacket(void const*, int, unsigned int) 	media/webrtc/signaling/src/media-conduit/AudioConduit.cpp:793
13 	xul.dll 	mozilla::MediaPipeline::RtpPacketReceived(mozilla::TransportLayer*, unsigned char const*, unsigned __int64) 	media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1124
14 	xul.dll 	mozilla::MediaPipeline::PacketReceived(mozilla::TransportLayer*, unsigned char const*, unsigned __int64) 	media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:1234
15 	xul.dll 	sigslot::signal3<mozilla::TransportLayer*, unsigned char const*, unsigned __int64, sigslot::single_threaded>::operator()(mozilla::TransportLayer*, unsigned char const*, unsigned __int64) 	media/mtransport/sigslot.h:2486
16 	xul.dll 	mozilla::TransportLayerIce::IcePacketReceived(mozilla::NrIceMediaStream*, int, unsigned char const*, int) 	media/mtransport/transportlayerice.cpp:224
17 	xul.dll 	mozilla::NrIceCtx::msg_recvd(void*, nr_ice_peer_ctx_*, nr_ice_media_stream_*, int, unsigned char*, int) 	media/mtransport/nricectx.cpp:380
18 	xul.dll 	nr_ice_peer_ctx_deliver_packet_maybe 	media/mtransport/third_party/nICEr/src/ice/ice_peer_ctx.c:837
19 	xul.dll 	nr_ice_ctx_deliver_packet 	media/mtransport/third_party/nICEr/src/ice/ice_ctx.c:944
20 	xul.dll 	nr_ice_socket_readable_cb 	media/mtransport/third_party/nICEr/src/ice/ice_socket.c:191
21 	xul.dll 	mozilla::NrUdpSocketIpc::recv_callback_s(RefPtr<mozilla::nr_udp_message>) 	media/mtransport/nr_socket_prsock.cpp:1666
22 	xul.dll 	mozilla::runnable_args_memfn<RefPtr<mozilla::NrUdpSocketIpc>, void ( mozilla::NrUdpSocketIpc::*)(RefPtr<mozilla::nr_udp_message>), RefPtr<mozilla::nr_udp_message> >::Run() 	media/mtransport/runnable_utils.h:172
23 	xul.dll 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp:1446
24 	xul.dll 	NS_ProcessNextEvent(nsIThread*, bool) 	xpcom/threads/nsThreadUtils.cpp:480
25 	xul.dll 	mozilla::net::nsSocketTransportService::Run() 	netwerk/base/nsSocketTransportService2.cpp:976
26 	xul.dll 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp:1446
27 	xul.dll 	NS_ProcessNextEvent(nsIThread*, bool) 	xpcom/threads/nsThreadUtils.cpp:480
28 	xul.dll 	mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp:369
29 	xul.dll 	MessageLoop::RunHandler() 	ipc/chromium/src/base/message_loop.cc:319
30 	xul.dll 	MessageLoop::Run() 	ipc/chromium/src/base/message_loop.cc:299
31 	xul.dll 	nsThread::ThreadFunc(void*) 	xpcom/threads/nsThread.cpp:506
32 	nss3.dll 	PR_NativeRunThread 	nsprpub/pr/src/threads/combined/pruthr.c:397
33 	nss3.dll 	pr_root 	nsprpub/pr/src/md/windows/w95thred.c:95
34 	ucrtbase.dll 	o__realloc_base 	
35 	kernel32.dll 	BaseThreadInitThunk 	
36 	ntdll.dll 	RtlUserThreadStart

this crash is cross platform and first appeared on nightly 56.0a1 build 20170617030206. some user comments indicate that they were using FireRTC when the crash happened.
Rank: 15
Priority: -- → P1
unassigned -> p2
Priority: P1 → P2
firefox crashed in firefox 56.0b2 (64-bit) beta  once joining webrtc conference. 
https://crash-stats.mozilla.com/report/index/06e5f867-0722-4c39-9cff-b2eeb0170815
Assignee: nobody → dminor
Status: NEW → ASSIGNED
Priority: P2 → P1
I had to disable one of the unit tests because it also assumes it will always get a valid decoder [1]. That would be a good place to start if we want to either start supporting more payload types or remove things from the list of types we claim to support.

[1] http://searchfox.org/mozilla-central/rev/e5b13e6224dbe3182050cf442608c4cb6a8c5c55/media/webrtc/trunk/gtest/moz.build#194
Attachment #8897395 - Flags: review?(rjesup)
Comment on attachment 8897395 [details] [diff] [review]
Add check that we have a valid decoder

Review of attachment 8897395 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/webrtc/trunk/webrtc/modules/audio_coding/neteq/neteq_impl.cc
@@ +691,5 @@
>      AudioDecoder* decoder = decoder_database_->GetDecoder(main_payload_type);
>      RTC_DCHECK(decoder);  // Should always get a valid object, since we have
>                            // already checked that the payload types are known.
> +    // The above assumption is not true for Mozilla webrtc.org builds.
> +    if (!decoder) {

This will crash out because of the RTC_DCHECK in debug builds.  We should probably either remove it and the comment (or modify the comment), or use a MOZILLA ifdef.  In this case, I think relaxing the check to what we have is better than an ifdef.  If so, remove the DCHECK, and modify the comments to describe how it works now.
Attachment #8897395 - Flags: review?(rjesup) → review+
Pushed by dminor@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/89f3908b2c51
Fix crash in webrtc::NetEqImpl::InsertPacketInternal; r=jesup
https://hg.mozilla.org/mozilla-central/rev/89f3908b2c51
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
(In reply to Wes Kocher (:KWierso) from comment #6)
> https://hg.mozilla.org/mozilla-central/rev/89f3908b2c51

will the modification be merged into firefox 56?
Please request Beta approval on this when you get a chance.
Flags: needinfo?(dminor)
Comment on attachment 8897395 [details] [diff] [review]
Add check that we have a valid decoder

Approval Request Comment
[Feature/Bug causing the regression]:
This is in upstream webrtc.org code. They support codecs that we don't use and so make an assumption about always getting a valid decoder that isn't applicable for our build.
[User impact if declined]:
Crashes if an unsupported rtp type is used.
[Is this code covered by automated tests?]:
No, our tests currently only cover supported RTP payload types (or we would have seen this crash in automation.)
[Has the fix been verified in Nightly?]:
It will be in tonight's nightly.
[Needs manual test from QE? If yes, steps to reproduce]:
No. 
[List of other uplifts needed for the feature/fix]:
None.
[Is the change risky?]:
No.
[Why is the change risky/not risky?]:
This only replaces a debug assert with a null check.
[String changes made/needed]:
None.
Flags: needinfo?(dminor)
Attachment #8897395 - Flags: approval-mozilla-beta?
Comment on attachment 8897395 [details] [diff] [review]
Add check that we have a valid decoder

Fix a crash related to webrtc. Beta56+.
Attachment #8897395 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
We should upstream this fix.
You need to log in before you can comment on or make changes to this bug.