1194385 - Add new test cases to document current nICEr socket behavior

Status: RESOLVED FIXED

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

Description

[Nils Ohlmeier [:drno]]

Calling nr_socket_buffered_stun_close(), which is used for TCP TURN and ICE, only sets the mCondition to NS_BASE_STREAM_CLOSED here https://dxr.mozilla.org/mozilla-central/source/media/mtransport/nr_socket_prsock.cpp#733

This has at least two flaws:
- you can still call nr_socket_sendto() on that socket and it will happily pretend to have written the data
- it actually does not close the TCP socket with a FIN, as nICEr never calls PR_Close()

We should probably make the detached callback https://dxr.mozilla.org/mozilla-central/source/media/mtransport/nr_socket_prsock.cpp#285 call an optional callback to change internal state of sockets (e.g. set https://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nICEr/src/stun/nr_socket_buffered_stun.c#57 to a value which gets checked in read() and sendto()).

Having the infrastructure for something like this in place is going to help a lot with ICE consent freshness.

Comment 1

[Nils Ohlmeier [:drno]]

Attachment: MozReview Request: Bug 1194385: new unit tests which demonstrate the current behavior. r=bwc

Bug 1194385: unit test to demonstrate the problem

Comment 2

[Nils Ohlmeier [:drno]]

Comment on attachment 8647655 [details]
MozReview Request: Bug 1194385: new unit tests which demonstrate the current behavior. r=bwc

Bug 1194385: new unit tests which demonstrate the current behavior. r?ekr

Comment 3

[Nils Ohlmeier [:drno]]

Apparently the expected behavior is the PR_Close() only gets called from nr_socket_destroy(). Which indeed properly closes the network connection, as shown in the new test cases.

I still don't get why we have an API which does not do, what you would expect from its name: nr_socket_close. Would it make sense to rename this API call to nr_socket_detach() to reflect what it actually does?

Comment 4

[Eric Rescorla (:ekr)]

I think you're conflating two issues:

(In reply to Nils Ohlmeier [:drno] from comment #3)
> Apparently the expected behavior is the PR_Close() only gets called from
> nr_socket_destroy(). Which indeed properly closes the network connection, as
> shown in the new test cases.
> 
> I still don't get why we have an API which does not do, what you would
> expect from its name: nr_socket_close. Would it make sense to rename this
> API call to nr_socket_detach() to reflect what it actually does?

Of course not. The nr_socket API is a preexisting API and nr_socket_prsock
is *one* implementation. However, it looks like nr_socket_prsock.cpp is not
correctly implementing its semantics. If you want to *carefully* move the
PR_Close() to OnDetached() that would probably be fine.

With that said, it's not really clear to me if this is a significant issue:
why are there pieces of code which do nr_socket_close() without nr_socket_destroy()
more or less immediately thereafter?

Comment 5

[Eric Rescorla (:ekr)]

s/why are/are/

Comment 6

[Nils Ohlmeier [:drno]]

The confusion came from me trying to use nr_socket_close() in a new unit test (see attached code) and expecting the socket to really get closed. It is confusing that the current API implementation does not what an unfamiliar developer would expect from the API function names.

That being said I don't think that there is currently any code which actually wants to close a connection and still hold on to the socket. So right now we don't need to change anything.

This topic might become more relevant once we implement ICE consent freshness as we then might need ways to mark a connection as dead.

Comment 7

[Nils Ohlmeier [:drno]]

Comment on attachment 8647655 [details]
MozReview Request: Bug 1194385: new unit tests which demonstrate the current behavior. r=bwc

Bug 1194385: new unit tests which demonstrate the current behavior. r?ekr

Comment 8

[Byron Campen [:bwc]]

Comment on attachment 8647655 [details]
MozReview Request: Bug 1194385: new unit tests which demonstrate the current behavior. r=bwc

https://reviewboard.mozilla.org/r/16027/#review14701

::: media/mtransport/test/multi_tcp_socket_unittest.cpp:266
(Diff revision 2)
> +  void RecvData_sf(nr_socket *sent_to, size_t expected_len, int expected_err) {

Suggest calling this RecvDataFailed_s.

::: media/mtransport/test/multi_tcp_socket_unittest.cpp:275
(Diff revision 2)
> +    printf("Expecting receive failure %d on %s\n", expected_err,
> +      addr_to.as_string);
> +    r=nr_socket_recvfrom(sent_to, received_data, expected_len+1,
> +                         &retlen, 0, &retaddr);
> +    ASSERT_EQ(expected_err, r);

You can stream this onto the ASSERT_EQ like:

ASSERT_EQ(expected_err, r) << "Expected receive failure " << expected_err << " on " << addr_to.as_string;

::: media/mtransport/test/multi_tcp_socket_unittest.cpp:346
(Diff revision 2)
> +  /* We have to detroy as only that calls PR_Close() */

s/detroy/destroy/

::: media/mtransport/test/multi_tcp_socket_unittest.cpp:353
(Diff revision 2)
> +

Extra ws.

::: media/mtransport/test/multi_tcp_socket_unittest.cpp:366
(Diff revision 2)
> +  /* We have to detroy as only that calls PR_Close() */

Same typo.

::: media/mtransport/test/multi_tcp_socket_unittest.cpp:373
(Diff revision 2)
> +

Same ws.

Comment 9

[Nils Ohlmeier [:drno]]

Comment on attachment 8647655 [details]
MozReview Request: Bug 1194385: new unit tests which demonstrate the current behavior. r=bwc

Bug 1194385: new unit tests which demonstrate the current behavior. r=bwc

Comment 10

[Nils Ohlmeier [:drno]]

Try run in mozreview is green.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/33c3681bfa65

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/33c3681bfa65