Crash when processing a WebRTC RtpReceiver with no streams |
|||||||
Issue descriptionLatest WebRTC and Chromium development build. Running the third test here (Unified Plan can connect to itself) crashes: https://chromium-review.googlesource.com/c/chromium/src/+/852272/3/third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-sdpSemantics.html#50 Digging around, it looks like this crash is caused by the underlying WebRTC RtpReceiver on the caller not having any streams. This happens in Unified Plan mode because streams are only set when the answer indicates send capability, but since there is no track on the callee the answer is recvonly. Note that this doesn't happen in Plan B mode because RtpReceivers are always created with one stream. Crash is from this DCHECK: https://codesearch.chromium.org/chromium/src/third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp?l=1577&rcl=d70f20cdd0fd8936bba68adb49e9e2a9c4be4753 The reason there is no track is the loop above iterates through each stream associated with the RtpReceiver, adding tracks that are part of that stream. But in this case we have an RtpReceiver with a track but no streams, so the track does not get added in that loop.
,
Jan 17 2018
But answer SDP (which should have no tracks) contains: a=ssrc-group:FID 3831019057 1217356593 a=ssrc:3831019057 cname:7EMatyaDJcMsFi1e a=ssrc:3831019057 msid:110c7070-6892-42db-a3dd-0dd6e85c0fa5 a42eba05-ead2-4ecf-81ad-c58452042c87 a=ssrc:3831019057 mslabel:110c7070-6892-42db-a3dd-0dd6e85c0fa5 a=ssrc:3831019057 label:a42eba05-ead2-4ecf-81ad-c58452042c87 a=ssrc:1217356593 cname:7EMatyaDJcMsFi1e a=ssrc:1217356593 msid:110c7070-6892-42db-a3dd-0dd6e85c0fa5 a42eba05-ead2-4ecf-81ad-c58452042c87 a=ssrc:1217356593 mslabel:110c7070-6892-42db-a3dd-0dd6e85c0fa5 a=ssrc:1217356593 label:a42eba05-ead2-4ecf-81ad-c58452042c87 This is Plan B format, and indicates a track is present. Twice.
,
Jan 17 2018
hta@ did you say you were looking at this? Changing priority (since it's an SDP-induced crash) and reassigning, feel free to object.
,
Jan 17 2018
Or if this has to do with https://crbug.com/769743 maybe I should take a look?
,
Jan 17 2018
If it's a crasher and it's in this part of the code, you should definitely take a look. We shouldn't crash on incoming SDP no matter what the SDP is. If webrtc sends up something we can't handle, we should be able to tell Steve what's wrong with it.
,
Jan 17 2018
,
Jan 17 2018
To clarify, this scenario does not occur when Plan B semantics are selected regardless of SDP because the PeerConnection always creates RtpReceivers with one stream. Of course this could change when adding support for zero streams (marked with the dash -). With Unified Plan semantics, streams are not set until a track is added, i.e. when the transceiver direction changes to indicate a receiving track. That's how this crash is reproduced with the Unified Plan test case (since the callee does not have a track to send, the direction is negotiated recvonly in the answer).
,
Jan 18 2018
,
Jan 18 2018
OK thanks then this is something that needs to be fixed before Unified Plan, not something that broke.
,
Jan 29 2018
Hm. Reading the WebRTC spec, it seems that: 1) a track is created when the receiver is created, not when a remote track is associated with it. https://w3c.github.io/webrtc-pc/#dom-rtcrtpreceiver 2) the ontrack event is fired when the track is associated, not when it's created. https://w3c.github.io/webrtc-pc/#process-remote-track-addition 3) a track's id is mutable - otherwise the logic makes no sense. I started trying to make a receiver's track mutable. But according to this, I need to make a track's id mutable.
,
Jan 29 2018
When does a track's ID change? I thought it was chosen at creation (when receiver is created) and was then immutable.
,
Jan 29 2018
See also https://github.com/w3c/webrtc-pc/issues/1718 for discussion about possible duplicate IDs, which may relate to the "otherwise the logic makes no sense".
,
Jan 29 2018
track ids are not mutable -- unless you want to reopen https://github.com/w3c/webrtc-pc/issues/1128 which already is the most-commented on issue
,
Jan 29 2018
Yeah, the track ID is not mutable and there's ongoing discussion on the WebRTC and JSEP issue trackers about what to make sense of it.
,
Feb 2 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/50b1fbedd5c4a7e5c0470e7f0d9b0d476a2502d6 commit 50b1fbedd5c4a7e5c0470e7f0d9b0d476a2502d6 Author: Harald Alvestrand <hta@chromium.org> Date: Fri Feb 02 16:05:29 2018 Allow RTCRtpReceiver to have a track without streams. Also add tests on what happens when Unified Plan talks to Plan B in some combinations. Bug: 799030 , 802938 Change-Id: I8d17fd42221c5e8f3d34bfc93d6bfe4c99c6ddd1 Reviewed-on: https://chromium-review.googlesource.com/852272 Commit-Queue: Harald Alvestrand <hta@chromium.org> Reviewed-by: Henrik Boström <hbos@chromium.org> Reviewed-by: Guido Urdaneta <guidou@chromium.org> Cr-Commit-Position: refs/heads/master@{#534064} [modify] https://crrev.com/50b1fbedd5c4a7e5c0470e7f0d9b0d476a2502d6/third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-createOffer-promise.html [add] https://crrev.com/50b1fbedd5c4a7e5c0470e7f0d9b0d476a2502d6/third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-sdpSemantics.html [modify] https://crrev.com/50b1fbedd5c4a7e5c0470e7f0d9b0d476a2502d6/third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp
,
Feb 2 2018
|
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by hta@chromium.org
, Jan 17 2018The crash happens when processing a SetRemoteDescription(Answer). The stack trace is: [1:1:0116/203101.974388:FATAL:RTCPeerConnection.cpp(1577)] Check failed: track. #0 0x7f951e43eacd base::debug::StackTrace::StackTrace() #1 0x7f951e43cf0c base::debug::StackTrace::StackTrace() #2 0x7f951e4c468a logging::LogMessage::~LogMessage() #3 0x7f95131567f6 blink::RTCPeerConnection::DidAddRemoteTrack() #4 0x7f952231ebe8 content::RTCPeerConnectionHandler::OnAddRemoteTrack() #5 0x7f952233474f content::RTCPeerConnectionHandler::WebRtcSetRemoteDescriptionObserverImpl::OnSetRemoteDescriptionComplete() which is this code: for (auto& receiver_state : states.receiver_states) { if (ReceiverWasAdded(receiver_state)) { handler_->OnAddRemoteTrack(receiver_state.receiver, std::move(receiver_state.track_ref), std::move(receiver_state.stream_refs)); } } Seems fixable.