Closed Bug 1370562 Opened 7 years ago Closed 5 years ago

PeerConnection stops functioning properly after receiving disabled m= line in SDP without a=inactive attribute

Categories

(Core :: WebRTC: Signaling, defect, P2)

Product:
Core
Component:
WebRTC: Signaling
Version:
53 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla67
Tracking Flags:
Tracking Status
firefox67 --- fixed

People

(Reporter: akvakh, Assigned: bwc)

Details

(Whiteboard: [need info reporter 2017-06-06])

Attachments

(2 files)

Bug 1370562: Don't try to validate attributes on disabled m-sections in answers.
59 bytes, text/x-review-board-request
drno
: review+
Details
Bug 1370562: Don't try to validate attributes on disabled m-sections in answers. r?mjf
47 bytes, text/x-phabricator-request
Details | Review 
User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:53.0) Gecko/20100101 Firefox/53.0
Build ID: 20170518000419

Steps to reproduce:

We have established PeerConnection.
We receive SDP answer from remote peer that has following line

m=video 0 RTP/SAVPF 104

But doesn't have 

a=inactive 

attribute for it. It's completely valid according to RFC 3264:

8.2 Removing a Media Stream

   Existing media streams are removed by creating a new SDP with the
   port number for that stream set to zero.  The stream description MAY
   omit all attributes present previously, and MAY list just a single
   media format.
   A stream that is offered with a port of zero MUST be marked with port
   zero in the answer.  Like the offer, the answer MAY omit all
   attributes present previously, and MAY list just a single media
   format from amongst those in the offer.

After we finish that negotiation, we decide to add new track and call addTrack() on this PeerConnection.




Actual results:

It doesn't result in onnegotiationneeded or any error.


Expected results:

onnegotiationneede should be called, as it does if m= line has a=inactive attribute after it.
Component: Untriaged → WebRTC: Networking
Product: Firefox → Core
Component: WebRTC: Networking → WebRTC: Signaling
Are there any other m sections in the SDP which cause the PeerConnection to connect?
Or are you trying to establish a transport with an disable m-section, so the transport is ready when you decide to add the track?

Can you provide us with a copy of about:webrtc on the Firefox side right after the failure?
In optimal case a signaling log https://wiki.mozilla.org/Media/WebRTC/Logging#Signaling_.28SDP_offer.2Fanswer_handling.29 would probably help even more.
Flags: needinfo?(akvakh)
Whiteboard: [need info reporter 2017-06-06]
Here you can download logs that we managed to collect:
https://drive.google.com/file/d/0B5zFc3Uq6-03WXBUdGhQLWFDVDA/view?usp=sharing

The archive contains 3 folders:
1) Adding video track (screen sharing)
2) Removing a track
3) Adding video track once more

Please note that 
a=sendrecv 
in remote SDP is added by about:webrtc automatically.

While id displays 
m=video 0 UDP/TLS/RTP/SAVPF 120
c=IN IP4 0.0.0.0
a=sendrecv
we only send 
m=video 0 UDP/TLS/RTP/SAVPF 120
c=IN IP4 0.0.0.0
to the browser
Flags: needinfo?(akvakh)
Nils, could you follow up with these logs?
Flags: needinfo?(drno)
Yeah, the behavior here is probably totally different with the transceivers changesets.
Sorry for dropping the ball on this one. Andrey could you please check with Firefox 59, currently in Beta, if this is still a problem?
Flags: needinfo?(drno) → needinfo?(akvakh)
I still have the same problem with Firefox 59.

I'm using SIP.js and calling 'session.hold()' reproduces the problem reliably.

The offered SDP (by firefox) is this one:

v=0
o=mozilla...THIS_IS_SDPARTA-59.0.1 4269060407824622734 1 IN IP4 0.0.0.0
s=-
t=0 0
a=sendonly
a=fingerprint:sha-256 08:5A:A3:87:98:4C:61:53:A2:42:00:39:C2:65:C3:7B:F2:01:30:E5:D6:0F:9D:3C:CE:F4:FD:A0:9B:6D:B8:6A
a=ice-options:trickle
a=msid-semantic:WMS *
m=audio 9 RTP/SAVPF 9 0 8 109 101
c=IN IP4 0.0.0.0
a=sendonly
a=extmap:1 urn:ietf:params:rtp-hdrext:ssrc-audio-level
a=extmap:2 urn:ietf:params:rtp-hdrext:sdes:mid
a=fmtp:109 maxplaybackrate=48000;stereo=1;useinbandfec=1
a=fmtp:101 0-15
a=ice-pwd:c850e4005580ee14bca957173a9854a6
a=ice-ufrag:59832b1c
a=msid:{e8328e8a-0dbe-4ea1-bde0-63686e4356aa} {a779a9c7-5444-4b39-87d1-793347a54be9}
a=rtcp-mux
a=rtpmap:9 G722/8000/1
a=rtpmap:0 PCMU/8000
a=rtpmap:8 PCMA/8000
a=rtpmap:109 opus/48000/2
a=rtpmap:101 telephone-event/8000
a=setup:actpass
a=ssrc:2092307956 cname:{132ce8e3-ad73-4413-89da-fcd7ed9f82fe}
m=video 0 RTP/SAVPF 120
c=IN IP4 0.0.0.0
a=inactive
a=rtpmap:120 VP8/90000

The answer provides this SDP:

v=0
o=root 12014 12015 IN IP4 172.30.200.158
s=session
c=IN IP4 172.30.200.158
t=0 0
m=audio 32200 RTP/SAVPF 9 8 0
a=rtpmap:9 G722/8000
a=rtpmap:8 PCMA/8000
a=rtpmap:0 PCMU/8000
a=ptime:20
a=recvonly
a=rtcp:32200
a=rtcp-mux
a=setup:passive
a=fingerprint:sha-1 C6:2F:37:A1:4F:D9:0A:EA:68:B1:45:41:06:7D:59:D1:CD:27:99:43
a=ice-ufrag:SGWfW3sj
a=ice-pwd:uTNT3q6LrwuuUVhbsEIEl1ALN8
a=candidate:iaCbhDmVlksTpWyU 1 UDP 2130706431 172.30.200.158 32200 typ host
m=video 0 RTP/SAVPF 120

It fails with this message:

DOMException {
  ​message: "Answer tried to set recv when offer did not set send"
  ​name: "InvalidSessionDescriptionError"
}

Glancing at the source code I see that the error comes from /media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:

if (!offerMsection.IsSending() && answerMsection.IsReceiving()) {
    JSEP_SET_ERROR("Answer tried to set recv when offer did not set send");
    return NS_ERROR_INVALID_ARG;
}

Presumably, I guess that to comply with the RFC, the SDP parsing code should consider the direction to be Direction::kInactive when the port is 0, but now it considers that because a=inactive is not there, the direction is Direction::kSendrecv.
I forgot to say that my user agent string is:

    Mozilla/5.0 (X11; Linux x86_64; rv:59.0) Gecko/20100101 Firefox/59.0

I also found a workaround in my client JS code, which consists simply on adding an "a=inactive" line after the "m=video 0" line. If I do that, then it works as expected and Firefox doesn't complain anymore.
(In reply to Steve Frécinaux from comment #6)

> Presumably, I guess that to comply with the RFC, the SDP parsing code should
> consider the direction to be Direction::kInactive when the port is 0, but
> now it considers that because a=inactive is not there, the direction is
> Direction::kSendrecv.

So, when the direction attribute is absent, it takes a default value of sendrecv, which is probably why Firefox is sad. We really ought to ignore the direction on rejected m-sections though. Should be an easy fix.
Assignee: nobody → docfaraday
Comment on attachment 8962844 [details]
Bug 1370562: Don't try to validate attributes on disabled m-sections in answers.

https://reviewboard.mozilla.org/r/231670/#review237230

LGTM
Attachment #8962844 - Flags: review?(drno) → review+
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Rank: 15
Priority: -- → P2
Hello,

I'm currently unable to test but I will ask colleagues to have a try. I'll keep you posted.
Flags: needinfo?(code)
Check back on this.
Flags: needinfo?(docfaraday)
Hello,

It turns out the page crashes when answering the call (we get the oops page and the console is wiped out even when "persist logs" is checked. No idea if it comes from this patch or from something else though.
Can you go to about:crashes and submit/link this crash report?
Flags: needinfo?(code)
(In reply to Steve Frécinaux from comment #14)
> It turns out the page crashes when answering the call (we get the oops page
> and the console is wiped out even when "persist logs" is checked. No idea if
> it comes from this patch or from something else though.

(In reply to Byron Campen [:bwc] from comment #15)
> Can you go to about:crashes and submit/link this crash report?

I've checked about:crashes, and I found a crash report(https://crash-stats.mozilla.com/report/index/6b2abba4-0fa4-47f8-b746-d59f60180409), but it doesn't seem relevant to me.

Then, I check the console, and I found some error log.
The console isn't draggable, so I took a screenshot. https://i.imgur.com/o0iLRHC.png

> ###!!! [Parent][MessageChannel] Error: (msgtype=0x16008C,name=PBrowser::Msg_UpdateNativeWindowHandle) Channel error: cannot send/recv
> 
> ###!!! [Parent][MessageChannel] Error: (msgtype=0x16007F,name=PBrowser::Msg_Destroy) Channel error: cannot send/rec
(In reply to Byron Campen [:bwc] from comment #15)
> Can you go to about:crashes and submit/link this crash report?

As you might have noticed, Sinyeol who did the actual test answered above. The build above didn't work for me because I use linux 64 bits and the build you linked to was 32 bits. I couldn't find my way in the CI webpage to find the link to a 64 bits version unfortunately.
Flags: needinfo?(code)
Yeah, that crash report was from a build from 2016...
Is there any way I could get access to the app you're developing, so I could try to reproduce this crash?
Flags: needinfo?(soundlake)
Hello,

Sorry for the lack of feedback.

Unfortunately we have no way to do that at the moment, sorry :-( Our dev platform is rather heavyweight and the live product is installed on premises.

I'll tell you when/if we have a public instance (it is in the plans but it might take a few months...)
Flags: needinfo?(docfaraday)

Ok, I think I will rebase this and land it. I doubt this is directly triggering any kind of crash.

Flags: needinfo?(soundlake)
Flags: needinfo?(akvakh)
Pushed by bcampen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5c8a828c886f
Don't try to validate attributes on disabled m-sections in answers. r=mjf

https://hg.mozilla.org/mozilla-central/rev/5c8a828c886f

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox67: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: