1185198 - ICE TCP active candidate need to use port 9

Status: RESOLVED FIXED

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

Description

[Nils Ohlmeier [:drno]]

Currently the ICE TCP candidates for 'tcptype active' have the real port number in them. According to the spec: https://tools.ietf.org/html/rfc6544#section-4.5 they need to have 9 in them instead.

Comment 1

[Nils Ohlmeier [:drno]]

Attachment: MozReview Request: Bug 1185198: use port 9 for TCP active candidates. r=bwc

Bug 1185198: use port 9 for TCP active candidates. r?bwc

Comment 2

[Byron Campen [:bwc]]

Comment on attachment 8637047 [details]
MozReview Request: Bug 1185198: use port 9 for TCP active candidates. r=bwc

https://reviewboard.mozilla.org/r/13779/#review12407

::: media/mtransport/test/ice_unittest.cpp:1713
(Diff revision 1)
> +  ASSERT_TRUE(StreamHasMatchingCandidate(0, "tcptype passive"));
> +  ASSERT_TRUE(StreamHasMatchingCandidate(0, "tcptype so"));

Let's also check that " 9 " is not present on these.

::: media/mtransport/third_party/nICEr/src/ice/ice_candidate.c:932
(Diff revision 1)
> +    if (cand->base.protocol==IPPROTO_TCP && cand->tcp_type==TCP_TYPE_ACTIVE)
> +      // https://tools.ietf.org/html/rfc6544#section-4.5
> +      port=9;

Putting a comment here without braces is hard to read, although it should still do the right thing. Suggest either adding braces, or putting the comment above the if.

::: media/mtransport/test/ice_unittest.cpp:1689
(Diff revision 1)
> +  ASSERT_TRUE(StreamHasMatchingCandidate(0, "tcptype passive"));
> +  ASSERT_TRUE(StreamHasMatchingCandidate(0, "tcptype so"));

Let's also check that " 9 " is not present on these.

Comment 3

[Nils Ohlmeier [:drno]]

Comment on attachment 8637047 [details]
MozReview Request: Bug 1185198: use port 9 for TCP active candidates. r=bwc

Bug 1185198: use port 9 for TCP active candidates. r=bwc

Comment 4

[Byron Campen [:bwc]]

Comment on attachment 8637047 [details]
MozReview Request: Bug 1185198: use port 9 for TCP active candidates. r=bwc

https://reviewboard.mozilla.org/r/13779/#review12525

::: media/mtransport/third_party/nICEr/src/ice/ice_candidate.c:932
(Diff revision 2)
> +    // https://tools.ietf.org/html/rfc6544#section-4.5

Gah, completely missed this the first time, but it would be good to make this a c-style comment. Doesn't need another round.

Comment 5

[Nils Ohlmeier [:drno]]

Comment on attachment 8637047 [details]
MozReview Request: Bug 1185198: use port 9 for TCP active candidates. r=bwc

Bug 1185198: use port 9 for TCP active candidates. r=bwc

Comment 6

[Nils Ohlmeier [:drno]]

Comment on attachment 8637047 [details]
MozReview Request: Bug 1185198: use port 9 for TCP active candidates. r=bwc

Carrying forward r+=bwc

Comment 7

[Nils Ohlmeier [:drno]]

Try run looks good

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/72b38c5bcf73

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/72b38c5bcf73