Closed Bug 1323723 Opened 7 years ago Closed 7 years ago

RTCPeerConnection: stop sending media when applying a second SDP offer

Categories

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

50 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: mparisdiaz, Assigned: drno)

References

(Depends on 1 open bug)

Details

Attachments

(6 files, 1 obsolete file)

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID: 20161209095719

Steps to reproduce:

Having the next 2 SDP negotiations:

Negotiation 1 - LOCAL SDP OFFER
v=0
o=mozilla...THIS_IS_SDPARTA-50.1.0 2543689248089967985 0 IN IP4 0.0.0.0
s=-
t=0 0
a=fingerprint:sha-256 26:09:35:29:4A:CC:DB:53:08:B9:82:F7:FF:21:A9:FD:A5:7B:DF:58:68:56:4E:3A:55:9D:9F:29:01:20:8A:A1
a=group:BUNDLE sdparta_0 sdparta_1
a=ice-options:trickle
a=msid-semantic:WMS *
m=audio 9 UDP/TLS/RTP/SAVPF 109 9 0 8
c=IN IP4 0.0.0.0
a=recvonly
a=extmap:1 urn:ietf:params:rtp-hdrext:ssrc-audio-level
a=fmtp:109 maxplaybackrate=48000;stereo=1;useinbandfec=1
a=ice-pwd:104efa9005ab9a78ae3436942cdd11e6
a=ice-ufrag:3d69bb57
a=mid:sdparta_0
a=rtcp-mux
a=rtpmap:109 opus/48000/2
a=rtpmap:9 G722/8000/1
a=rtpmap:0 PCMU/8000
a=rtpmap:8 PCMA/8000
a=setup:actpass
a=ssrc:2072412225 cname:{1a9156b4-1a09-4d51-9fba-316325af0062}
m=video 9 UDP/TLS/RTP/SAVPF 120 126 97
c=IN IP4 0.0.0.0
a=sendrecv
a=fmtp:126 profile-level-id=42e01f;level-asymmetry-allowed=1;packetization-mode=1
a=fmtp:97 profile-level-id=42e01f;level-asymmetry-allowed=1
a=fmtp:120 max-fs=12288;max-fr=60
a=ice-pwd:104efa9005ab9a78ae3436942cdd11e6
a=ice-ufrag:3d69bb57
a=mid:sdparta_1
a=msid:{fac09d71-a62d-464c-bc01-e3b2f564580e} {e13d494d-9bfc-4fb1-a6b5-e656bd82c1e0}
a=rtcp-fb:120 nack
a=rtcp-fb:120 nack pli
a=rtcp-fb:120 ccm fir
a=rtcp-fb:120 goog-remb
a=rtcp-fb:126 nack
a=rtcp-fb:126 nack pli
a=rtcp-fb:126 ccm fir
a=rtcp-fb:126 goog-remb
a=rtcp-fb:97 nack
a=rtcp-fb:97 nack pli
a=rtcp-fb:97 ccm fir
a=rtcp-fb:97 goog-remb
a=rtcp-mux
a=rtpmap:120 VP8/90000
a=rtpmap:126 H264/90000
a=rtpmap:97 H264/90000
a=setup:actpass
a=ssrc:251820637 cname:{1a9156b4-1a09-4d51-9fba-316325af0062}


Negotiation 1 - REMOTE SDP ANSWER
v=0
o=- 3690792591 3690792591 IN IP4 0.0.0.0
s=VM76da225fd9594c06b60d05a7bee52203
t=0 0
a=ice-options:trickle
a=msid-semantic:WMS *
a=group:BUNDLE sdparta_0 sdparta_1
a=msid-semantic:WMS *
a=ice-lite
m=audio 13330 UDP/TLS/RTP/SAVPF 109 0
c=IN IP4 172.22.2.175
a=inactive
a=mid:sdparta_0
a=rtcp:13330 IN IP4 172.22.2.175
a=rtpmap:109 opus/48000/2
a=rtpmap:0 PCMU/8000
a=fmtp:109 maxplaybackrate=48000;stereo=1;useinbandfec=1
a=rtcp-mux
a=setup:active
a=ice-ufrag:HLBq
a=ice-pwd:ru2tvQBt7TMLAYKIUQoVCQ
a=fingerprint:sha-256 07:94:59:74:19:9C:57:D6:0A:9A:7A:7B:CE:0B:4B:A1:31:80:72:C2:EF:6C:61:4A:EA:71:9E:50:28:1D:35:82
a=candidate:1 1 UDP 2013266431 fe80::897:43ff:fe24:2b3a 13849 typ host
a=candidate:2 1 UDP 2013266431 54.88.69.222 13330 typ host
a=candidate:1 2 UDP 2013266430 fe80::897:43ff:fe24:2b3a 13036 typ host
a=candidate:2 2 UDP 2013266430 54.88.69.222 10568 typ host
m=video 13330 UDP/TLS/RTP/SAVPF 120
c=IN IP4 172.22.2.175
b=AS:2000
a=recvonly
a=mid:sdparta_1
a=rtcp:13330 IN IP4 172.22.2.175
a=rtpmap:120 VP8/90000
a=rtcp-fb:120 nack
a=rtcp-fb:120 nack pli
a=rtcp-fb:120 ccm fir
a=rtcp-fb:120 goog-remb
a=fmtp:120 max-fs=12288;max-fr=60
a=rtcp-mux
a=setup:active
a=ice-ufrag:HLBq
a=ice-pwd:ru2tvQBt7TMLAYKIUQoVCQ
a=fingerprint:sha-256 07:94:59:74:19:9C:57:D6:0A:9A:7A:7B:CE:0B:4B:A1:31:80:72:C2:EF:6C:61:4A:EA:71:9E:50:28:1D:35:82
a=candidate:1 1 UDP 2013266431 fe80::897:43ff:fe24:2b3a 13849 typ host
a=candidate:2 1 UDP 2013266431 54.88.69.222 13330 typ host
a=candidate:1 2 UDP 2013266430 fe80::897:43ff:fe24:2b3a 13036 typ host
a=candidate:2 2 UDP 2013266430 54.88.69.222 10568 typ host


Negotiation 2 - REMOTE SDP OFFER
v=0
o=- 3690792591 3690792592 IN IP4 0.0.0.0
s=VM76da225fd9594c06b60d05a7bee52203
c=IN IP4 0.0.0.0
t=0 0
a=group:BUNDLE sdparta_0 sdparta_1
a=msid-semantic:WMS *
a=ice-lite
m=audio 1 UDP/TLS/RTP/SAVPF 109 0
a=rtpmap:109 opus/48000/2
a=rtpmap:0 PCMU/8000
a=fmtp:109 maxplaybackrate=48000;stereo=1;useinbandfec=1
a=rtcp:9 IN IP4 0.0.0.0
a=rtcp-mux
a=recvonly
a=mid:sdparta_0
a=ice-ufrag:HLBq
a=ice-pwd:ru2tvQBt7TMLAYKIUQoVCQ
a=candidate:1 1 UDP 2013266431 fe80::897:43ff:fe24:2b3a 13849 typ host
a=candidate:2 1 UDP 2013266431 54.88.69.222 13330 typ host
a=candidate:1 2 UDP 2013266430 fe80::897:43ff:fe24:2b3a 13036 typ host
a=candidate:2 2 UDP 2013266430 54.88.69.222 10568 typ host
a=fingerprint:sha-256 07:94:59:74:19:9C:57:D6:0A:9A:7A:7B:CE:0B:4B:A1:31:80:72:C2:EF:6C:61:4A:EA:71:9E:50:28:1D:35:82
m=video 1 UDP/TLS/RTP/SAVPF 120
b=AS:2000
a=rtpmap:120 VP8/90000
a=fmtp:120 max-fs=12288;max-fr=60
a=rtcp:9 IN IP4 0.0.0.0
a=rtcp-mux
a=sendrecv
a=msid:{ac3bf23b-3a5f-4277-a185-07a1f1519bec} {41a81f09-51ec-4931-b0d7-1c8ae72ff4ed}
a=ssrc:1116397011 cname:user3472025491@host-b2c6baed
a=mid:sdparta_1
a=rtcp-fb:120 nack
a=rtcp-fb:120 nack pli
a=rtcp-fb:120 goog-remb
a=rtcp-fb:120 ccm fir
a=ice-ufrag:HLBq
a=ice-pwd:ru2tvQBt7TMLAYKIUQoVCQ
a=candidate:1 1 UDP 2013266431 fe80::897:43ff:fe24:2b3a 13849 typ host
a=candidate:2 1 UDP 2013266431 54.88.69.222 13330 typ host
a=candidate:1 2 UDP 2013266430 fe80::897:43ff:fe24:2b3a 13036 typ host
a=candidate:2 2 UDP 2013266430 54.88.69.222 10568 typ host
a=fingerprint:sha-256 07:94:59:74:19:9C:57:D6:0A:9A:7A:7B:CE:0B:4B:A1:31:80:72:C2:EF:6C:61:4A:EA:71:9E:50:28:1D:35:82


Negotiation 2 - LOCAL SDP ANSWER
v=0
o=mozilla...THIS_IS_SDPARTA-50.1.0 2543689248089967985 1 IN IP4 0.0.0.0
s=-
t=0 0
a=fingerprint:sha-256 26:09:35:29:4A:CC:DB:53:08:B9:82:F7:FF:21:A9:FD:A5:7B:DF:58:68:56:4E:3A:55:9D:9F:29:01:20:8A:A1
a=group:BUNDLE sdparta_1
a=ice-options:trickle
a=msid-semantic:WMS *
m=audio 0 UDP/TLS/RTP/SAVPF 0
c=IN IP4 0.0.0.0
a=inactive
a=rtpmap:0 PCMU/8000
m=video 9 UDP/TLS/RTP/SAVPF 120
c=IN IP4 0.0.0.0
a=sendrecv
a=fmtp:120 max-fs=12288;max-fr=60
a=ice-pwd:104efa9005ab9a78ae3436942cdd11e6
a=ice-ufrag:3d69bb57
a=mid:sdparta_1
a=msid:{fac09d71-a62d-464c-bc01-e3b2f564580e} {e13d494d-9bfc-4fb1-a6b5-e656bd82c1e0}
a=rtcp-fb:120 nack
a=rtcp-fb:120 nack pli
a=rtcp-fb:120 ccm fir
a=rtcp-fb:120 goog-remb
a=rtcp-mux
a=rtpmap:120 VP8/90000
a=setup:active
a=ssrc:251820637 cname:{1a9156b4-1a09-4d51-9fba-316325af0062}




Actual results:

After "Negotiation 2" Firefox stops sending video and I do not see any error in console.

I am attaching a file with the Firefox log.


Expected results:

Firefox should continue sending video and should start receiving video.
Component: Untriaged → WebRTC: Networking
Product: Firefox → Core
Thanks for the ICE log. But I think what we need in this case is a log file from the signaling layer. Could please provide us with the NSPR logging described here https://wiki.mozilla.org/Media/WebRTC/Logging#Signaling_.28SDP_offer.2Fanswer_handling.29
Whiteboard: [needinfo 2016/12/16 to reporter]
Forgot to actually set the NeedInfo to the reporter.
Flags: needinfo?(mparisdiaz)
Attached file NSPR error log
Nils,
first of all thanks for the pointer, I didn't know it ;).

I have attached the requested log and here you have the standard output of firefox:

Timecard created 1482148724.563581

 Timestamp   | Delta       | Event                          | File                         | Function               
======================================================================================================================
    0.000424 |    0.000424 | Constructor Completed          | PeerConnectionImpl.cpp:344   | PeerConnectionImpl     
    0.003457 |    0.003033 | Initializing PC Ctx            | PeerConnectionImpl.cpp:672   | Initialize             
    0.020280 |    0.016823 | Create Offer                   | PeerConnectionImpl.cpp:1515  | CreateOffer            
    0.023471 |    0.003191 | Set Local Description          | PeerConnectionImpl.cpp:1681  | SetLocalDescription    
    0.049872 |    0.026401 | Ice gathering state: gathering | PeerConnectionImpl.cpp:3301  | IceGatheringStateChange
    0.722054 |    0.672182 | Set Remote Description         | PeerConnectionImpl.cpp:2029  | SetRemoteDescription   
    0.784972 |    0.062918 | Ice state: checking            | PeerConnectionImpl.cpp:3249  | IceConnectionStateChange
    0.915665 |    0.130693 | Ice state: connected           | PeerConnectionImpl.cpp:3252  | IceConnectionStateChange
   13.444991 |   12.529326 | Ice gathering state: complete  | PeerConnectionImpl.cpp:3304  | IceGatheringStateChange
   17.889691 |    4.444700 | Set Remote Description         | PeerConnectionImpl.cpp:2029  | SetRemoteDescription   
   17.898593 |    0.008902 | Create Answer                  | PeerConnectionImpl.cpp:1572  | CreateAnswer           
   17.899576 |    0.000983 | Set Local Description          | PeerConnectionImpl.cpp:1681  | SetLocalDescription    
   17.939186 |    0.039610 | Ice gathering state: gathering | PeerConnectionImpl.cpp:3301  | IceGatheringStateChange
   17.939678 |    0.000492 | Ice state: checking            | PeerConnectionImpl.cpp:3249  | IceConnectionStateChange
   18.049821 |    0.110143 | Ice state: connected           | PeerConnectionImpl.cpp:3252  | IceConnectionStateChange
   31.836831 |   13.787010 | Ice gathering state: complete  | PeerConnectionImpl.cpp:3304  | IceGatheringStateChange
   46.037341 |   14.200510 | Destructor Invoked             | PeerConnectionImpl.cpp:358   | ~PeerConnectionImpl
Flags: needinfo?(mparisdiaz)
Thanks for the log file. That helped me to get an idea what is going on here.

The problem has multiple aspects to it:
- the renegotiation appears to restart ICE - not sure if that is intended or not
- the Negotiation 2 - REMOTE SDP OFFER is missing the a=setup attribute, that appears to be a bug on your implementation
- Firefox answer includes the a=setup attribute which switches the DTLS client server roles, which is actually not allowed
- but the logs show that Firefox DTLS implementation is actually still configured as server and therefore waits for the other side to send the client hello which I guess never happens
Status: UNCONFIRMED → NEW
backlog: --- → webrtc/webaudio+
Rank: 25
Ever confirmed: true
Priority: -- → P2
Whiteboard: [needinfo 2016/12/16 to reporter]
Attachment #8820074 - Flags: review?(docfaraday)
@bwc: mostly looking for feedback initially if you think these are the right places to add these checks. Once we have the right places we should add some unit tests.
Assignee: nobody → drno
Miguel once the try run here https://treeherder.mozilla.org/#/jobs?repo=try&revision=b13147742c6f has finished building you should be able to download the build for your platform and test it against your Kurento server. It should reject the re-offer from the Kurento server with the missing setup attribute.
Comment on attachment 8820074 [details]
Bug 1323723: added setup attribute checks.

https://reviewboard.mozilla.org/r/99600/#review100028

LGTM (I think byron is on PTO this week; so you can decide if you want to take my r+)
Attachment #8820074 - Flags: review+
Attached file nspr_setup_fixed.log
Hello Nils,
thanks for the great support!!

You are right and I missed the detail of a=setup attribute, good point!!
After fixing that and adding a=setup attribute properly, it continues not working.
Is this caused because of the ICE restart? (what is NOT intended).
I have attached a new log of this new case.

In addition, I have tried to verify if the patch you did for Firefox detects the a=setup in the former case, but I didn't find a way to download and install firefox from the link you passed.
I would like to contribute verifying that, so could you please tell me how to download the binary associated to your patch?
Miguel,
my first patch was not good. I started a new try run for my new patch. You can download the Linux build for example from here https://archive.mozilla.org/pub/firefox/try-builds/drno@ohlmeier.org-2321f7cc8da018b378c90051598163bc3e2c275a/try-linux/
You can either navigate from the directory up and then down into the other OS's. Or you can click on the green 'B' on this treeherder page https://treeherder.mozilla.org/#/jobs?repo=try&revision=2321f7cc8da0 and then click on the link in the lower left corner next to the word 'Build'.
Would be great to know if my patch properly rejects the re-offer with the missing 'a=setup' from your initial setup!

I looked at your new log file and I think it is actually not an ICE restart, but for some reason Fx behaves as if there is no pre-existing ICE transport. I need to check in more details and probably ask you for more log files with a different log string.
Miguel,
I'm pretty sure I now understand what the problem is: 
Because in the initial negotiation the first m-line gets set to inactive by your server Firefox establishes the ICE transport kind of bound to the second m-line which has real RTP traffic on it.
But when your server send the re-offer it actually set the first m-line to recvonly. And even though Firefox then sends it back as inactive again my guess is that this causes internally the bundled transport to "move" to the first m-line.
Nils,
after testing the nightly associated to your patch the missed a=setup attr is detected:
2016-12-21 11:04:52.410568 UTC - 1211201408[7f3246b6c5c0]: [main|PeerConnectionImpl] PeerConnectionImpl.cpp:2162: SetRemoteDescription: pc = 09d5ed92b2588c71, error = Missing setup attribute in m-line 0

Great work!!

In relation to the "ICE problem", I support your assumption because if the first m-line is sendrecv in the second offer sent by Kurento, it works.
What could we do to fix this issue?
  - Do you think if it is a bug in Firefox?
  - Do you think if Kurento is doing something wrong?
Flags: needinfo?(drno)
Comment on attachment 8820074 [details]
Bug 1323723: added setup attribute checks.

https://reviewboard.mozilla.org/r/99600/#review101906

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:927
(Diff revision 3)
> +  if (mCurrentLocalDescription) {
> +    if (mCurrentLocalDescription->GetMediaSectionCount() > mlineIndex) {
> +      auto& msection = mCurrentLocalDescription->GetMediaSection(mlineIndex);
> +      if (!mSdpHelper.MsectionIsDisabled(msection) &&
> +          msection.GetAttributeList().GetSetup().mRole != role) {
> +        JSEP_SET_ERROR("Remote side attempted to switch DTLS roles");
> +        return NS_ERROR_FAILURE;
> +      }
> +    }
> +  }

Hmm. Won't the a=setup in the current local description be actpass if we were the initial offerer? That will never be returned by DetermineAnswererSetupRole.
Attachment #8820074 - Flags: review?(docfaraday)
Attachment #8820074 - Attachment is obsolete: true
@bwc the new patch is only to require the presence of the setup attribute (which would have prevented the initial problem). I'll open a separate bug for adding more code to fix and enforce the setup roles per https://tools.ietf.org/html/draft-ietf-mmusic-dtls-sdp-15

Miguel the ICE transport thing is a Firefox issue. I'll probably open a new bug for that as well.
Flags: needinfo?(drno)
Comment on attachment 8823964 [details]
Bug 1323723: enforce a=setup in SDP offers.

https://reviewboard.mozilla.org/r/102442/#review102964

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:1718
(Diff revision 1)
> -                     "\"holdconn\" at level "
> +                       "\"holdconn\" at level "
> -                     << i);
> +                       << i);
> -      return NS_ERROR_INVALID_ARG;
> +        return NS_ERROR_INVALID_ARG;
> -    }
> +      }
> +    } else {
> +      JSEP_SET_ERROR("Missing setup attribute in m-line " << i);

"m-section" I guess.
Attachment #8823964 - Flags: review?(docfaraday) → review+
See Also: → 1329028
(In reply to Nils Ohlmeier [:drno] from comment #18)
> @bwc the new patch is only to require the presence of the setup attribute
> (which would have prevented the initial problem). I'll open a separate bug
> for adding more code to fix and enforce the setup roles per
> https://tools.ietf.org/html/draft-ietf-mmusic-dtls-sdp-15

See bug 1329028
Depends on: 1332183
(In reply to Nils Ohlmeier [:drno] from comment #18)
> Miguel the ICE transport thing is a Firefox issue. I'll probably open a new
> bug for that as well.

I created bug 1332183 for the ICE transport issue. Once we have that landed I would land the small fix here. And the more strict checking of a=setup in bug 1329028 can land later.
Miguel,
can you get me the full offer answer when Kurento includes a=setup in it's re-offer?

So what's happening is:
- Kurento sends the re-offer with the first m-section recvonly
- as Fx has no audio stream for the m-section it rejects it by setting the port to 0 and also removing the mid from the bundle group
- this makes the mid of the video m-section the new bundle master
- therefore new ICE and DTLS need to happen
- but since Kurento omitted the a=setup in it's re-offer Fx just makes up a default a=setup attribute (active in this case), but in fact the DTLS part of Fx is still in the original server mode and waits for Kurento to start the DTLS handshake

So that's why I'm curious what happens if Kurento properly puts a=setup in the re-offer.
Flags: needinfo?(mparisdiaz)
Hello Nils!!
The re-offer is the same that in the report, but adding "a=setup:active" in each m-line.
Hello again Nils,
after the discussion in JSEP, the conclusion [1] is that "a=setup" value MUST be always "actpass" [2].

So, if I am not wrong this should be fixed in Firefox, shouldn't it?

Refs
[1] https://github.com/rtcweb-wg/jsep/issues/448#issuecomment-278495950
[2] http://rtcweb-wg.github.io/jsep/#rfc.section.5.2.1
Flags: needinfo?(drno)
The main work for this will happen in bug 1329028.
Depends on: 1329028
Comment on attachment 8846982 [details]
Bug 1323723: reject setup value actpass in answers.

https://reviewboard.mozilla.org/r/120024/#review122040
Attachment #8846982 - Flags: review?(docfaraday) → review+
Comment on attachment 8823964 [details]
Bug 1323723: enforce a=setup in SDP offers.

https://reviewboard.mozilla.org/r/102442/#review122064

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:1711
(Diff revision 3)
> +    } else {
> +      JSEP_SET_ERROR("Missing setup attribute in m-section at level " << i);
> +      return NS_ERROR_INVALID_ARG;
> +    }

Hang on. In answers a=setup has a default value of "passive", which is fine with the JSEP spec (it recommends "active" though). In offers, the default is "active", which is _not_ ok with the JSEP spec.
Attachment #8823964 - Flags: review+ → review-
Comment on attachment 8846983 [details]
Bug 1323723: update a=setup tests.

https://reviewboard.mozilla.org/r/120026/#review122066

So we need to test the following:

"actpass" in offers is the only thing that is ok; all other values (including missing a=setup) are an error.
"active", "passive", and missing a=setup in answers is ok, everything else is not.
Comment on attachment 8823964 [details]
Bug 1323723: enforce a=setup in SDP offers.

https://reviewboard.mozilla.org/r/102442/#review122232
Attachment #8823964 - Flags: review?(docfaraday) → review+
Comment on attachment 8846983 [details]
Bug 1323723: update a=setup tests.

https://reviewboard.mozilla.org/r/120026/#review122236

::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp:5063
(Diff revision 3)
>  }
>  
>  // In this test we will change the answer SDP's a=setup value
>  // from active to passive.  This will make both sides do
>  // active and should not connect.
> +// TODO jsep unittest can't verify that this does not connect?!

If this worries you, you could inspect the JsepTransports on each side and verify that they both think they're active.
Attachment #8846983 - Flags: review?(docfaraday) → review+
Great to see that you have fixed it.
Thank you very much!! :D
Flags: needinfo?(mparisdiaz)
Miguel,

it is probably worth pointing out that the patches from this bug might not be enough actually fix the whole issue. These were just the easy checks. I'm still working in bug 1329028 on adding more checks.
Flags: needinfo?(drno)
Great!!
If you need help from my side to test newest versions of Firefox with you patch, please let me know, I will be happy to do it ;).
Depends on: 1367049
You need to log in before you can comment on or make changes to this bug.