1207451 - Remove TCP framing as an option from ICE TCP

Status: RESOLVED FIXED

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

Comment 1

[Nils Ohlmeier [:drno]]

Attachment: MozReview Request: Bug 1207451: removed framing from multi_tcp API. r=bwc

Bug 1207451: removed framing from multi_tcp API. r?bwc

Comment 2

[Byron Campen [:bwc]]

Comment on attachment 8667643 [details]
MozReview Request: Bug 1207451: removed framing from multi_tcp API. r=bwc

https://reviewboard.mozilla.org/r/20793/#review18729

::: media/mtransport/test/multi_tcp_socket_unittest.cpp:203
(Diff revision 1)
> -  void SendData_s(nr_socket *from, nr_socket *to, const char *data,
> +  void SendDataT_s(nr_socket *from, nr_transport_addr *to, const char *data,

Maybe call this SendDataToAddress_s?

::: media/mtransport/test/multi_tcp_socket_unittest.cpp:222
(Diff revision 1)
> +  void SendDataS_s(nr_socket *from, nr_socket *to, const char *data,

Maybe call this SendDataToSocket_s?

::: media/mtransport/test/multi_tcp_socket_unittest.cpp:238
(Diff revision 1)
> -  void RecvData_s(nr_socket *expected_from, nr_socket *sent_to,
> +  void RecvDataT_s(nr_transport_addr *expected_from, nr_socket *sent_to,

Maybe call this RecvDataFromAddress_s?

::: media/mtransport/test/multi_tcp_socket_unittest.cpp:271
(Diff revision 1)
> +  void RecvDataS_s(nr_socket *expected_from, nr_socket *sent_to,

Maybe call this RecvDataFromSocket_s?

::: media/mtransport/test/multi_tcp_socket_unittest.cpp:458
(Diff revision 1)
> +  /* 500 error reply expected from the STUN server */
> +  const char stunReply[] = {
> +    '\x01', '\x11', '\x00', '\x14', '\x21', '\x12', '\xa4', '\x42',
> +    '\x00', '\x00', '\x00', '\x00', '\x00', '\x00', '\x0c', '\x00',
> +    '\x00', '\x00', '\x00', '\x00', '\x00', '\x09', '\x00', '\x10',
> +    '\x00', '\x00', '\x05', '\x00', '\x53', '\x65', '\x72', '\x76',
> +    '\x65', '\x72', '\x20', '\x45', '\x72', '\x72', '\x6f', '\x72'
> +  };

Some changes to the test STUN server will break this test. Could we instead verify that some data (any data really) made it back?

Comment 3

[Nils Ohlmeier [:drno]]

Comment on attachment 8667643 [details]
MozReview Request: Bug 1207451: removed framing from multi_tcp API. r=bwc

Bug 1207451: removed framing from multi_tcp API. r?bwc

Comment 4

[Byron Campen [:bwc]]

Comment on attachment 8667643 [details]
MozReview Request: Bug 1207451: removed framing from multi_tcp API. r=bwc

https://reviewboard.mozilla.org/r/20793/#review18747

::: media/mtransport/test/multi_tcp_socket_unittest.cpp:267
(Diff revisions 1 - 2)
> +    if (expected_data) {
> -    r=memcmp(expected_data, received_data, retlen);
> +      r=memcmp(expected_data, received_data, retlen);
> -    ASSERT_EQ(0, r);
> +      ASSERT_EQ(0, r);
> -  }
> +    }

This should probably only execute if |expected_len| is non-zero.

::: media/mtransport/test/multi_tcp_socket_unittest.cpp:273
(Diff revisions 1 - 2)
> +  void RecvData(nr_transport_addr *expected_from, nr_socket *sent_to) {
> +    ASSERT_TRUE_WAIT(IsReadable(), 1000);
> +    test_utils->sts_target()->Dispatch(
> +            WrapRunnable(
> +                this, &MultiTcpSocketTest::RecvDataFromAddress_s,
> +                expected_from, sent_to, nullptr, 0),
> +            NS_DISPATCH_SYNC);
> +  }

If you want, feel free to use default args instead of overloading. I'm not too particular either way.

Comment 5

[Nils Ohlmeier [:drno]]

Comment on attachment 8667643 [details]
MozReview Request: Bug 1207451: removed framing from multi_tcp API. r=bwc

Bug 1207451: removed framing from multi_tcp API. r=bwc

Comment 6

[Nils Ohlmeier [:drno]]

Try run appears green.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d164679f91f5

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d164679f91f5