New issue
Advanced search Search tips

Issue 802938 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 799030



Sign in to add a comment

Crash when processing a WebRTC RtpReceiver with no streams

Project Member Reported by steveanton@chromium.org, Jan 17 2018

Issue description

Latest 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.
 

Comment 1 by hta@chromium.org, Jan 17 2018

The 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.

Comment 2 by hta@chromium.org, 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.

Comment 3 by hbos@chromium.org, Jan 17 2018

Cc: hbos@chromium.org
Labels: -Pri-3 Pri-2
Owner: hta@chromium.org
Status: Assigned (was: Untriaged)
hta@ did you say you were looking at this? Changing priority (since it's an SDP-induced crash) and reassigning, feel free to object.

Comment 4 by hbos@chromium.org, Jan 17 2018

Or if this has to do with  https://crbug.com/769743  maybe I should take a look?

Comment 5 by hta@chromium.org, Jan 17 2018

Cc: steveanton@chromium.org
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.

Comment 6 by hbos@chromium.org, Jan 17 2018

Owner: hbos@chromium.org
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).

Comment 8 by hbos@chromium.org, Jan 18 2018

Blocking: 799030

Comment 9 by hbos@chromium.org, Jan 18 2018

OK thanks then this is something that needs to be fixed before Unified Plan, not something that broke.

Comment 10 by hta@chromium.org, Jan 29 2018

Labels: -Pri-2 Pri-3
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.

Comment 11 by hbos@chromium.org, 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.

Comment 12 by hbos@chromium.org, 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".

Comment 13 by fi...@appear.in, 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
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.
Project Member

Comment 15 by bugdroid1@chromium.org, 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

Comment 16 by hta@chromium.org, Feb 2 2018

Status: Fixed (was: Assigned)

Sign in to add a comment