Bugzilla

Assignee

Comment 1

•

6 years ago

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]

Assignee

Comment 2

•

6 years ago

Forgot to actually set the NeedInfo to the reporter.

Flags: needinfo?(mparisdiaz)

Reporter

Comment 3

•

6 years ago

Attached file NSPR error log — Details

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)

Assignee

Comment 4

•

6 years ago

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

Assignee

Updated

•

6 years ago

Whiteboard: [needinfo 2016/12/16 to reporter]

Comment hidden (mozreview-request)

Review commit: https://reviewboard.mozilla.org/r/99600/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/99600/

Assignee

Updated

•

6 years ago

Attachment #8820074 - Flags: review?(docfaraday)

Assignee

Comment 6

•

6 years ago

@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

Comment hidden (mozreview-request)

Comment on attachment 8820074 [details]
Bug 1323723: added setup attribute checks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/99600/diff/1-2/

Randell Jesup [:jesup] (needinfo me)

Assignee

Comment 8

•

6 years ago

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 9

•

6 years ago

mozreview-review

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+

Reporter

Comment 10

•

6 years ago

Attached file nspr_setup_fixed.log — Details

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?

Comment hidden (mozreview-request)

Comment on attachment 8820074 [details]
Bug 1323723: added setup attribute checks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/99600/diff/2-3/

Assignee

Comment 12

•

6 years ago

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.

Assignee

Comment 13

•

6 years ago

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.

Reporter

Comment 14

•

6 years ago

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 15

•

6 years ago

mozreview-review

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)

Reporter

Comment 16

•

6 years ago

Related discussions:
https://github.com/rtcweb-wg/jsep/issues/448
https://groups.google.com/forum/#!msg/discuss-webrtc/DfpIMwvUfeM/VgCtnJ0cEgAJ

Comment hidden (mozreview-request)

Review commit: https://reviewboard.mozilla.org/r/102442/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/102442/

Assignee

Updated

•

6 years ago

Attachment #8820074 - Attachment is obsolete: true

Assignee

Comment 18

•

6 years ago

@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 19

•

6 years ago

mozreview-review

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+

Comment hidden (mozreview-request)

Comment on attachment 8823964 [details]
Bug 1323723: enforce a=setup in SDP offers.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102442/diff/1-2/

Assignee

Updated

•

6 years ago

Comment 21

•

6 years ago

(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

Assignee

Updated

•

6 years ago

Depends on: 1332183

Assignee

Comment 22

•

6 years ago

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

Assignee

Comment 23

•

6 years ago

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.

Assignee

Updated

•

6 years ago

Flags: needinfo?(mparisdiaz)

Reporter

Comment 24

•

6 years ago

Hello Nils!!
The re-offer is the same that in the report, but adding "a=setup:active" in each m-line.

Randell Jesup [:jesup] (needinfo me)

Reporter

Comment 25

•

6 years ago

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

Updated

•

6 years ago

Flags: needinfo?(drno)

Assignee

Comment 26

•

6 years ago

The main work for this will happen in bug 1329028.

Depends on: 1329028

Comment hidden (mozreview-request)

Review commit: https://reviewboard.mozilla.org/r/120024/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/120024/

Comment hidden (mozreview-request)

Review commit: https://reviewboard.mozilla.org/r/120026/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/120026/

Comment hidden (mozreview-request)

Comment on attachment 8823964 [details]
Bug 1323723: enforce a=setup in SDP offers.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102442/diff/2-3/

Comment hidden (mozreview-request)

Comment on attachment 8846983 [details]
Bug 1323723: update a=setup tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120026/diff/1-2/

Comment 31

•

6 years ago

mozreview-review

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 32

•

6 years ago

mozreview-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 33

•

6 years ago

mozreview-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 hidden (mozreview-request)

Comment on attachment 8823964 [details]
Bug 1323723: enforce a=setup in SDP offers.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102442/diff/3-4/

Comment hidden (mozreview-request)

Comment on attachment 8846982 [details]
Bug 1323723: reject setup value actpass in answers.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120024/diff/1-2/

Comment hidden (mozreview-request)

Comment on attachment 8846983 [details]
Bug 1323723: update a=setup tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120026/diff/2-3/

Comment 37

•

6 years ago

mozreview-review

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 38

•

6 years ago

mozreview-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+

Comment hidden (mozreview-request)

Comment on attachment 8846983 [details]
Bug 1323723: update a=setup tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120026/diff/3-4/

Pulsebot

Comment 40

•

6 years ago

Pushed by drno@ohlmeier.org:
https://hg.mozilla.org/integration/autoland/rev/e5fc51a36f97
enforce a=setup in SDP offers. r=bwc
https://hg.mozilla.org/integration/autoland/rev/36223085d3d5
reject setup value actpass in answers. r=bwc
https://hg.mozilla.org/integration/autoland/rev/b954d11db347
update a=setup tests. r=bwc

Carsten Book [:Tomcat]

Comment 41

•

6 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/e5fc51a36f97
https://hg.mozilla.org/mozilla-central/rev/36223085d3d5
https://hg.mozilla.org/mozilla-central/rev/b954d11db347

Status: NEW → RESOLVED

Closed: 6 years ago

status-firefox55: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla55

Reporter

Comment 42

•

6 years ago

Great to see that you have fixed it.
Thank you very much!! :D

Flags: needinfo?(mparisdiaz)

Assignee

Comment 43

•

6 years ago

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)