Closed Bug 1194385 Opened 6 years ago Closed 6 years ago

Add new test cases to document current nICEr socket behavior

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed
Blocking Flags:

People

(Reporter: drno, Assigned: drno)

Details

Attachments

(1 file)

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 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
Attachment #8647655 - Attachment description: MozReview Request: Bug 1194385: unit test to demonstrate the problem → MozReview Request: Bug 1194385: new unit tests which demonstrate the current behavior. r?ekr
Attachment #8647655 - Flags: review?(ekr)
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?
Flags: needinfo?(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?
Flags: needinfo?(ekr)
s/why are/are/
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 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
Attachment #8647655 - Flags: review?(docfaraday)
Attachment #8647655 - Flags: review?(docfaraday) → review+
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.
Assignee: nobody → drno
backlog: --- → webrtc/webaudio+
Rank: 15
Priority: -- → P1
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
Attachment #8647655 - Attachment description: MozReview Request: Bug 1194385: new unit tests which demonstrate the current behavior. r?ekr → MozReview Request: Bug 1194385: new unit tests which demonstrate the current behavior. r=bwc
Attachment #8647655 - Flags: review?(ekr)
Try run in mozreview is green.
Keywords: checkin-needed
Summary: nr_socket_buffered_stun_close (used for ICE TCP) is broken → Add new test cases to document current nICEr socket behavior
https://hg.mozilla.org/mozilla-central/rev/33c3681bfa65
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.