1206465 - Remove ice_ctx from TestStunTcpServer

Status: RESOLVED FIXED

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

Description

[Nils Ohlmeier [:drno]]

The TestStunTcpServer initializes an ice_ctx with TCP disabled, which gets written into the nICEr central shared registry. This results in single TCP test execution failing and currently the tests in ice_unittest having dependencies on each other.

Comment 1

[Nils Ohlmeier [:drno]]

Attachment: MozReview Request: Bug 1206465: removed ice_ctx from TestStunTcpServer. r?bwc

Bug 1206465: removed ice_ctx from TestStunTcpServer. r?ekr,r?bwc

Comment 2

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/19787/#review17855

There are some simplifications I'd like to see here.

::: media/mtransport/test/stunserver.cpp:394
(Diff revision 1)
> +  nr_socket *send_sock;
> +

Couldn't we just do if (!sock) {sock = send_sock_;}, and avoid the if/else below, and the extra variable?

::: media/mtransport/test/stunserver.cpp:591
(Diff revision 1)
> +void TestStunTcpServer::readable_cb(NR_SOCKET s, int how, void *cb_arg) {

I wonder how much we could simplify this by doing the following:

1) Adding virtual nr_socket\* GetSocket(NR_SOCKET s) to TestStunServer.
2) Making |connections\_| be a non-static member of TestStunTcpServer, as a std::map<NR_SOCKET, nr_socket\*> (so we can override GetSocket).
3) Unifying the two implementations of readable_cb, since the only real difference is now encapsulated in GetSocket()
4) Simplify (or eliminate) PreProcess since we don't need to have a conditional sock param.
5) Eliminate StunTcpReadOperation

::: media/mtransport/test/stunserver.cpp:592
(Diff revision 1)
> +  StunTcpReadOperation* ops = static_cast<StunTcpReadOperation*>(cb_arg);
> +  TestStunServer* server = ops->server_;

Inconsistent \*

::: media/mtransport/test/stunserver.cpp:595
(Diff revision 1)
> +  nr_socket* sock = static_cast<nr_socket *>(static_cast<nr_socket_wrapped *>(wrapped_sock->obj)->sock_);

Why do we need the static_cast<nr_socket\*> here? sock\_ is already an nr_socket\*. Also, inconsistent \*

::: media/mtransport/test/stunserver.cpp:596
(Diff revision 1)
> +  char message[4096];

Let's give this a constant a name, since it is used in two places now.

Comment 3

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/19787/#review17867

Comment 4

[Nils Ohlmeier [:drno]]

Comment on attachment 8663362 [details]
MozReview Request: Bug 1206465: removed ice_ctx from TestStunTcpServer. r?bwc

Bug 1206465: removed ice_ctx from TestStunTcpServer. r?bwc

Comment 5

[Nils Ohlmeier [:drno]]

https://reviewboard.mozilla.org/r/19787/#review17855

> I wonder how much we could simplify this by doing the following:
> 
> 1) Adding virtual nr_socket\* GetSocket(NR_SOCKET s) to TestStunServer.
> 2) Making |connections\_| be a non-static member of TestStunTcpServer, as a std::map<NR_SOCKET, nr_socket\*> (so we can override GetSocket).
> 3) Unifying the two implementations of readable_cb, since the only real difference is now encapsulated in GetSocket()
> 4) Simplify (or eliminate) PreProcess since we don't need to have a conditional sock param.
> 5) Eliminate StunTcpReadOperation

Thanks, that was a good idea.

Comment 6

[Nils Ohlmeier [:drno]]

Comment on attachment 8663362 [details]
MozReview Request: Bug 1206465: removed ice_ctx from TestStunTcpServer. r?bwc

Bug 1206465: removed ice_ctx from TestStunTcpServer. r?bwc

Comment 7

[Nils Ohlmeier [:drno]]

Comment on attachment 8663362 [details]
MozReview Request: Bug 1206465: removed ice_ctx from TestStunTcpServer. r?bwc

Bug 1206465: removed ice_ctx from TestStunTcpServer. r?ekr,r?bwc

Comment 8

[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 9

[Byron Campen [:bwc]]

It looks like you incorporated feedback, but the two subsequent pushes have undone all of the work.

Comment 10

[Nils Ohlmeier [:drno]]

Comment on attachment 8663362 [details]
MozReview Request: Bug 1206465: removed ice_ctx from TestStunTcpServer. r?bwc

Bug 1206465: removed ice_ctx from TestStunTcpServer. r?bwc

Comment 11

[Nils Ohlmeier [:drno]]

(In reply to Byron Campen [:bwc] from comment #9)
> It looks like you incorporated feedback, but the two subsequent pushes have
> undone all of the work.

Yea that subsequent push didn't go as intended.

Comment 12

[Byron Campen [:bwc]]

Did you want me to try to review it as it stands now, or did you want to fix the push first?

Comment 13

[Nils Ohlmeier [:drno]]

(In reply to Byron Campen [:bwc] from comment #12)
> Did you want me to try to review it as it stands now, or did you want to fix
> the push first?

The push from comment #10 should have restored the good version from comment #6. So yes, please go ahead a review :-)

Comment 14

[Byron Campen [:bwc]]

Comment on attachment 8663362 [details]
MozReview Request: Bug 1206465: removed ice_ctx from TestStunTcpServer. r?bwc

https://reviewboard.mozilla.org/r/19789/#review18435

Comment 15

[Pulsebot]

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

Comment 16

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

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