Closed Bug 1185198 Opened 5 years ago Closed 5 years ago

ICE TCP active candidate need to use port 9

Categories

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

41 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed
Blocking Flags:

People

(Reporter: drno, Assigned: drno)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
backlog: --- → webRTC+
Rank: 15
Priority: -- → P1
Bug 1185198: use port 9 for TCP active candidates. r?bwc
Attachment #8637047 - Flags: review?(docfaraday)
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.
Attachment #8637047 - Flags: review?(docfaraday) → review+
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
Attachment #8637047 - Attachment description: MozReview Request: Bug 1185198: use port 9 for TCP active candidates. r?bwc → MozReview Request: Bug 1185198: use port 9 for TCP active candidates. r=bwc
Attachment #8637047 - Flags: review+ → review?(docfaraday)
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.
Attachment #8637047 - Flags: review?(docfaraday) → review+
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
Attachment #8637047 - Flags: review+ → review?(docfaraday)
Comment on attachment 8637047 [details]
MozReview Request: Bug 1185198: use port 9 for TCP active candidates. r=bwc

Carrying forward r+=bwc
Attachment #8637047 - Flags: review?(docfaraday) → review+
Try run looks good
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/72b38c5bcf73
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.