1220441 - ice unit tests TestStun[Tcp]ServerTrickle are at least unreliable

Status: RESOLVED FIXED

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

Description

[Nils Ohlmeier [:drno]]

I'm not quite sure what the TestStun[Tcp]ServerTrickle tests are suppose to prove.

We configure the fake STUN server to throw away incoming packets...
  TestStunServer::GetInstance(AF_INET)->SetActive(false);
We start gathering.
  Gather(0);
Now we assert that the round trip time between our stun client and the fake stun server is bigger then the execution time between the return of the previous statement and the next assert?!?!
  ASSERT_FALSE(StreamHasMatchingCandidate(0, "192.0.2.1"));
Now we let the fake stun server answer requests...
  TestStunServer::GetInstance(AF_INET)->SetActive(true);
  WaitForGather();
  ASSERT_TRUE(StreamHasMatchingCandidate(0, "192.0.2.1"));

I guess the point is suppose to be that the stun client keeps retransmitting its requests until it gets a response (and we hopefully don't time out).

Problem is: for TCP there are no retransmits. So if the fake STUN server was set to inactive at the time of receiving the first request it drops the request and then will never send a response!

For UDP one possible improvement could be to tell the STUN server how many requests to throw away before answering, if that is what we want to test here. For TCP I could only see us delaying the answer here so that the gathering will take time until it finishes.

Comment 1

[Nils Ohlmeier [:drno]]

Byron: any thoughts/ideas?

Comment 2

[Byron Campen [:bwc]]

I think your analysis is correct, and this this test makes no sense for TCP.

Comment 3

[Nils Ohlmeier [:drno]]

Attachment: MozReview Request: Bug 1220441: Improve gather trickle ice unit tests. r?bwc,mjf

Bug 1220441: Improve gather trickle ice unit tests

Comment 4

[Nils Ohlmeier [:drno]]

Comment on attachment 8682205 [details]
MozReview Request: Bug 1220441: Improve gather trickle ice unit tests. r?bwc,mjf

Bug 1220441: Improve gather trickle ice unit tests. r?bwc,mjf

Comment 5

[Nils Ohlmeier [:drno]]

Just looking for one review. So whoever can get this done quicker... :-)

Comment 6

[Michael Froman [:mjf]]

Comment on attachment 8682205 [details]
MozReview Request: Bug 1220441: Improve gather trickle ice unit tests. r?bwc,mjf

https://reviewboard.mozilla.org/r/23989/#review21583

::: media/mtransport/test/stunserver.cpp:479
(Diff revision 2)
>      server->Process(reinterpret_cast<const uint8_t *>(message), message_len,

Looks like this can now be called even if !server->active.  Is this intended?

Comment 7

[Nils Ohlmeier [:drno]]

Comment on attachment 8682205 [details]
MozReview Request: Bug 1220441: Improve gather trickle ice unit tests. r?bwc,mjf

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23989/diff/2-3/

Comment 8

[Byron Campen [:bwc]]

Comment on attachment 8682205 [details]
MozReview Request: Bug 1220441: Improve gather trickle ice unit tests. r?bwc,mjf

https://reviewboard.mozilla.org/r/23989/#review21893

Comment 9

[Nils Ohlmeier [:drno]]

Try run is all green.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/27066ece22a9

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/27066ece22a9

Comment 12

[Michael Froman [:mjf]]

Comment on attachment 8682205 [details]
MozReview Request: Bug 1220441: Improve gather trickle ice unit tests. r?bwc,mjf

https://reviewboard.mozilla.org/r/23989/#review30909