916187 - Add a fake STUN server to unit tests

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect)

Description

[Eric Rescorla (:ekr)]

Add a fake STUN server to the C++ unit tests to allow for controllable results

Comment 1

[Eric Rescorla (:ekr)]

Attachment: Part 2. Cleanup of trickle candidates and a test STUN server

Comment 2

[Byron Campen [:bwc]]

Attachment: Part 2. Cleanup of trickle candidates and a test STUN server

Comment 3

[Byron Campen [:bwc]]

Made cpplint stop complaining about a few things, and allow the listen port to be configured.

Comment 4

[Eric Rescorla (:ekr)]

Comment on attachment 804590 [details] [diff] [review]
Part 2. Cleanup of trickle candidates and a test STUN server

Review of attachment 804590 [details] [diff] [review]:
-----------------------------------------------------------------

This all lgtm

::: media/mtransport/test/stunserver.cpp
@@ +178,5 @@
> +
> +// Instance static.
> +// Note: Calling Create() at static init time is not going to be safe, since
> +// we have no reason to expect this will be initted to a nullptr yet.
> +TestStunServer* TestStunServer::instance;

This is true, but generally we try to avoid C++ static initialization.

I think this comment is unnecessary.

Comment 5

[Byron Campen [:bwc]]

Attachment: Using the test stun server, write some tests to exercise the checking logic written in .

Comment 6

[Byron Campen [:bwc]]

Attachment: Using the test stun server, write some tests to exercise the checking logic written in .

Comment 7

[Eric Rescorla (:ekr)]

Comment on attachment 804646 [details] [diff] [review]
Using the test stun server, write some tests to exercise the checking logic written in .

Review of attachment 804646 [details] [diff] [review]:
-----------------------------------------------------------------

I would refactor these a bit for common code as below.

::: media/mtransport/test/ice_unittest.cpp
@@ +700,5 @@
>    Gather();
>    peer_->ParseCandidate(0, kBogusIceCandidate);
>  }
>  
> +TEST_F(IceGatherTest, TestStunServerReturnsWildcardRelayAddr) {

This is srflx, not relay, right?

@@ +704,5 @@
> +TEST_F(IceGatherTest, TestStunServerReturnsWildcardRelayAddr) {
> +  TestStunServer::GetInstance()->SetActive(true);
> +  TestStunServer::GetInstance()->SetResponseAddr("0.0.0.0", 3333);
> +  // Sets an additional stun server
> +  peer_->SetStunServer(TestStunServer::GetInstance()->addr(),

Can we refactor these three lines into a fxn?

@@ +705,5 @@
> +  TestStunServer::GetInstance()->SetActive(true);
> +  TestStunServer::GetInstance()->SetResponseAddr("0.0.0.0", 3333);
> +  // Sets an additional stun server
> +  peer_->SetStunServer(TestStunServer::GetInstance()->addr(),
> +                TestStunServer::GetInstance()->port());

Indentation.

@@ +709,5 @@
> +                TestStunServer::GetInstance()->port());
> +  Gather();
> +  std::vector<std::string> candidates = peer_->GetCandidates(0);
> +  for (size_t c = 0; c < candidates.size(); ++c) {
> +    ASSERT_EQ(std::string::npos, candidates[c].find("0.0.0.0"));

Does it make sense to factor this type of test out.

@@ +718,5 @@
> +  TestStunServer::GetInstance()->SetActive(true);
> +  TestStunServer::GetInstance()->SetResponseAddr("10.0.0.133", 0);
> +  // Sets an additional stun server
> +  peer_->SetStunServer(TestStunServer::GetInstance()->addr(),
> +                TestStunServer::GetInstance()->port());

Indent

Comment 8

[Byron Campen [:bwc]]

Attachment: Using the test stun server, write some tests to exercise the checking logic written in .

Comment 9

[Byron Campen [:bwc]]

Attachment: Using the test stun server, write some tests to exercise the checking logic written in .

Comment 10

[Byron Campen [:bwc]]

bzexport is stripping out part of the commit message; this is to test logic written in 908740.

Comment 11

[Eric Rescorla (:ekr)]

Comment on attachment 804590 [details] [diff] [review]
Part 2. Cleanup of trickle candidates and a test STUN server

Review of attachment 804590 [details] [diff] [review]:
-----------------------------------------------------------------

This all lgtm

::: media/mtransport/test/stunserver.cpp
@@ +178,5 @@
> +
> +// Instance static.
> +// Note: Calling Create() at static init time is not going to be safe, since
> +// we have no reason to expect this will be initted to a nullptr yet.
> +TestStunServer* TestStunServer::instance;

This is true, but generally we try to avoid C++ static initialization.

I think this comment is unnecessary.

Comment 12

[Adam Roach [:abr]]

Comment on attachment 804590 [details] [diff] [review]
Part 2. Cleanup of trickle candidates and a test STUN server

Review of attachment 804590 [details] [diff] [review]:
-----------------------------------------------------------------

This patch has a few minor resource management issues, but enough that I'd like to take another look at it after they've been repaired.

Note that any changes to the build system (e.g. moz.build) need review by a build peer. Given that the current changes to moz.build are no-ops, we probably just want to revert that file rather than asking for a build peer review.

::: media/mtransport/test/ice_unittest.cpp
@@ +417,5 @@
>  
>  class IceGatherTest : public ::testing::Test {
>   public:
>    void SetUp() {
> +    TestStunServer::GetInstance()->Reset();

Change this to dispatch Reset to the STS time, so as to allow canceling of a delayed response (see the change I call out for TestStunServer::Reset()). If a previous test case failed before the previous response, we don't want it's delayed response to cause the next one to fail.

@@ +692,5 @@
>      size_t sp1 = candidates[i].find(' ');
>      ASSERT_EQ(0, candidates[i].compare(sp1+1, 1, "1", 1));
>    }
>  }
>  

This ws-only change will make the patch more likely to rot versus the rest of the tree. I would advise reverting it.

::: media/mtransport/test/moz.build
@@ +6,5 @@
>  
>  MODULE = 'test_mtransport'
>  
>  if CONFIG['OS_TARGET'] != 'WINNT' and CONFIG['MOZ_WIDGET_TOOLKIT'] != 'gonk':
> +#    LIBRARY_NAME = 'transport_tests'

Why is this commented out?

@@ +7,5 @@
>  MODULE = 'test_mtransport'
>  
>  if CONFIG['OS_TARGET'] != 'WINNT' and CONFIG['MOZ_WIDGET_TOOLKIT'] != 'gonk':
> +#    LIBRARY_NAME = 'transport_tests'
> +        

Trailing WS

@@ +9,5 @@
>  if CONFIG['OS_TARGET'] != 'WINNT' and CONFIG['MOZ_WIDGET_TOOLKIT'] != 'gonk':
> +#    LIBRARY_NAME = 'transport_tests'
> +        
> +    CPP_SOURCES = [
> +#        'stunserver.cpp'

Same as above.

@@ +11,5 @@
> +        
> +    CPP_SOURCES = [
> +#        'stunserver.cpp'
> +    ]
> +    

Trailing WS

::: media/mtransport/test/stunserver.cpp
@@ +183,5 @@
> +uint16_t TestStunServer::instance_port = 3478;
> +
> +TestStunServer::~TestStunServer() {
> +  // TODO([e-mail]): Put this on the right thread.
> +  nr_socket_destroy(&listen_sock_);

I think we're leaking resources here.

> nr_socket_destroy(&send_sock_);
> nr_stun_server_ctx_destroy(&stun_server_);

Don't we need to disarm the NR_ASYNC_WAIT here also?

@@ +253,5 @@
> +    return nullptr;
> +  }
> +
> +  // Cache the address and port.
> +  char addr_string[100];

s/100/INET6_ADDRSTRLEN/

@@ +257,5 @@
> +  char addr_string[100];
> +  r = nr_transport_addr_get_addrstring(&addrs[0].addr, addr_string,
> +                                       sizeof(addr_string));
> +  if (r)
> +    return nullptr;

No logging?

@@ +266,5 @@
> +  NR_ASYNC_WAIT(fd, NR_ASYNC_WAIT_READ, &TestStunServer::readable_cb, server.get());
> +
> +  return server.forget();
> +}
> +

I think we might want a comment here indicating that this must be called on the STS thread, at least the first time it's called.

@@ +387,5 @@
> +
> +  return NS_OK;
> +}
> +
> +  nsresult TestStunServer::SetResponseAddr(const std::string& addr,

Unindent.

@@ +399,5 @@
> +    return NS_ERROR_FAILURE;
> +
> +  return SetResponseAddr(&addr2);
> +}
> +

Add a comment that this must run on the STS thread.

@@ +403,5 @@
> +
> +void TestStunServer::Reset() {
> +  delay_ms_ = 0;
> +  delete response_addr_;
> +  response_addr_ = nullptr;

We really don't want a previous test case's delayed response to land on us and mess us up.

> if (timer_handle_) {
>   NR_async_timer_cancel(&timer_handle_);
>   timer_handle_ = nullptr;
> }

::: media/mtransport/test/stunserver.h
@@ +24,5 @@
> +  // Note: None of these are safe to call at static init, or at static deinit.
> +  // They are also not threadsafe.
> +  // |ConfigurePort| will only have an effect if called before the first call
> +  // to |GetInstance| (possibly following a |ShutdownInstance| call)
> +  static void ConfigurePort(uint16_t port);

ConfigurePort() does not appear to be implemented. Implement or remove the declaration.

@@ +45,5 @@
> +
> + private:
> +  TestStunServer()
> +      : listen_sock_(nullptr),
> +        send_sock_(nullptr),

stun_server_(nullptr),

Comment 13

[Adam Roach [:abr]]

Comment on attachment 804590 [details] [diff] [review]
Part 2. Cleanup of trickle candidates and a test STUN server

Also, please rename this patch when you upload the new version. Having two patches, with one called "part 2" and the other one without a part number, is confusing enough even if "part 2" were the second patch.

The fact that you're supposed to apply "part 2" first makes the situation particularly egregious.

Comment 14

[Adam Roach [:abr]]

Comment on attachment 804761 [details] [diff] [review]
Using the test stun server, write some tests to exercise the checking logic written in .

Review of attachment 804761 [details] [diff] [review]:
-----------------------------------------------------------------

r- for stuff in here that belongs in the other patch.

::: media/mtransport/databuffer.h
@@ +10,5 @@
>  #define databuffer_h__
>  #include <algorithm>
>  #include <mozilla/Scoped.h>
>  #include <m_cpp_utils.h>
> +#include <nsISupportsImpl.h>

Why is this needed? There's been a big push to minimize the include graph recently. Given that this is the only change in this file, I have a hard time believing this change is needed here. If you need this in some downstream file, please include it there (preferring .c/.cpp strongly over .h)

Also, if this is needed for the earlier patch, please make sure that the inclusion I mention is moved to that earlier patch rather than this one.

::: media/mtransport/test/ice_unittest.cpp
@@ +30,5 @@
>  #include "mtransport_test_utils.h"
>  #include "runnable_utils.h"
>  #include "stunserver.h"
> +// TODO([e-mail]): Big fat hack since the build system doesn't give
> +// us a clean way to add object files to a single executable.

Move this comment to the earlier patch, please.

@@ +431,5 @@
>      ASSERT_TRUE_WAIT(peer_->gathering_complete(), 10000);
>    }
> +
> +  void SetFakeStunServerResponse(const std::string& fake_addr,
> +                                 uint16_t fake_port) {

The fact that this has the side effect of also activating the use of the fake STUN server makes the tests somewhat hard to read. I recommend renaming to something like "UseFakeStunServerWithResponseAddress(...)"

@@ +439,5 @@
> +                         TestStunServer::GetInstance()->port());
> +  }
> +
> +  bool StreamHasMatchingCandidate(unsigned int stream,
> +                                  const std::string& match) {

We want to either make sure we prevent "1.2.3.4" from matching "121.2.3.47", or put bright flashing lights around this function to point out that it is a simple substring match, caveat emptor.

@@ +721,5 @@
>  TEST_F(IceGatherTest, TestBogusCandidate) {
>    Gather();
>    peer_->ParseCandidate(0, kBogusIceCandidate);
>  }
>  

I would start out here with a positive test to ensure things are working as expected (e.g., test with 192.0.2.1:54321 and double check that the stream has that as a relayed candidate).

@@ +729,5 @@
> +  ASSERT_FALSE(StreamHasMatchingCandidate(0, "0.0.0.0"));
> +}
> +
> +TEST_F(IceGatherTest, TestStunServerReturnsPort0) {
> +  SetFakeStunServerResponse("10.0.0.133", 0);

Use an RFC 5737 address rather than an RFC 1918 address here, please. If we decide to filter 1918 addresses at some future date, I don't want the behavior here to be impacted.

::: media/mtransport/test/moz.build
@@ +6,5 @@
>  
>  MODULE = 'test_mtransport'
>  
>  if CONFIG['OS_TARGET'] != 'WINNT' and CONFIG['MOZ_WIDGET_TOOLKIT'] != 'gonk':
> +

In aggregate, I think this patch plus the previous one results in a ws-only change. Please just remove the moz.build file from both patches.

Comment 15

[Byron Campen [:bwc]]

Attachment: Part 1. Cleanup of trickle candidates and a test STUN server

Comment 16

[Byron Campen [:bwc]]

Comment on attachment 806317 [details] [diff] [review]
Part 1. Cleanup of trickle candidates and a test STUN server

Review of attachment 806317 [details] [diff] [review]:
-----------------------------------------------------------------

I took a crack at fixing up the patch per abr's feedback, but noticed a few things myself.

::: media/mtransport/test/stunserver.cpp
@@ +170,5 @@
> +  int r = nr_socket_create_int(wrapped.forget(), &nr_socket_wrapped_vtbl, outp);
> +  if (!r)
> +    return r;
> +
> +  wrapped.forget();

We're already calling wrapped.forget() above. Did we mean to call

nr_socket_create_int(wrapped.get(),... ?

@@ +182,5 @@
> +TestStunServer* TestStunServer::instance;
> +uint16_t TestStunServer::instance_port = 3478;
> +
> +TestStunServer::~TestStunServer() {
> +  // TODO([e-mail]): Put this on the right thread.

Since we're calling ShutdownInstance() from the STS thread (in ice_unittest.cpp), this is taken care of, unless we want to add a member so this class to handle the dispatch, right?

@@ +211,5 @@
> +  nr_local_addr addrs[100];
> +  int addr_ct;
> +  int r;
> +
> +  NR_reg_init(NR_REG_MODE_LOCAL);

Duplicate call? (see line 207)

Comment 17

[Byron Campen [:bwc]]

Attachment: Part 2. Using the test stun server, write some tests to exercise the checking logic written in 908740.

Comment 18

[Byron Campen [:bwc]]

Comment on attachment 804761 [details] [diff] [review]
Using the test stun server, write some tests to exercise the checking logic written in .

Review of attachment 804761 [details] [diff] [review]:
-----------------------------------------------------------------

Just a comment about the unrelated include I picked up.

::: media/mtransport/databuffer.h
@@ +10,5 @@
>  #define databuffer_h__
>  #include <algorithm>
>  #include <mozilla/Scoped.h>
>  #include <m_cpp_utils.h>
> +#include <nsISupportsImpl.h>

This is just a violation of "include what you use" that I tripped over while trying to get the build system to build stunserver.o standalone, and link it into ice_unittest. (this header would not compile unless its includer picked up this header file first)

Comment 19

[Adam Roach [:abr]]

(In reply to Byron Campen [:bwc] from comment #18)
> Comment on attachment 804761 [details] [diff] [review]
> Using the test stun server, write some tests to exercise the checking logic
> written in .
> 
> Review of attachment 804761 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just a comment about the unrelated include I picked up.
> 
> ::: media/mtransport/databuffer.h
> @@ +10,5 @@
> >  #define databuffer_h__
> >  #include <algorithm>
> >  #include <mozilla/Scoped.h>
> >  #include <m_cpp_utils.h>
> > +#include <nsISupportsImpl.h>
> 
> This is just a violation of "include what you use" that I tripped over while
> trying to get the build system to build stunserver.o standalone, and link it
> into ice_unittest. (this header would not compile unless its includer picked
> up this header file first)

Okay, sure. That makes sense.

Comment 20

[Byron Campen [:bwc]]

Attachment: Part 1. Cleanup of trickle candidates and a test STUN server

Comment 21

[Byron Campen [:bwc]]

Attachment: Part 2. Using the test stun server, write some tests to exercise the checking logic written in 908740.

Comment 22

[Byron Campen [:bwc]]

Attachment: Part 3. Double checking for leaks in new test stun server code, and fixing other things as I see them.

Comment 23

[Byron Campen [:bwc]]

Attachment: Part 1. Cleanup of trickle candidates and a test STUN server

Comment 24

[Adam Roach [:abr]]

Comment on attachment 806861 [details] [diff] [review]
Part 1. Cleanup of trickle candidates and a test STUN server

Review of attachment 806861 [details] [diff] [review]:
-----------------------------------------------------------------

Looks mostly good. r+, assuming the following three comments are addressed before checkin.

::: media/mtransport/test/stunserver.cpp
@@ +263,5 @@
> +    return nullptr;
> +  }
> +
> +  // Cache the address and port.
> +  char addr_string[INET6_ADDRSTRLEN];

I think we still need to add 1 here to accommodate a \0.

@@ +267,5 @@
> +  char addr_string[INET6_ADDRSTRLEN];
> +  r = nr_transport_addr_get_addrstring(&addrs[0].addr, addr_string,
> +                                       sizeof(addr_string));
> +  if (r)
> +    return nullptr;

Still no logging?

@@ +280,5 @@
> +
> +void TestStunServer::ConfigurePort(uint16_t port) {
> +  instance_port = port;
> +}
> +

Still need a comment here talking about thread issues.

Comment 25

[Adam Roach [:abr]]

Comment on attachment 806838 [details] [diff] [review]
Part 2. Using the test stun server, write some tests to exercise the checking logic written in 908740.

Review of attachment 806838 [details] [diff] [review]:
-----------------------------------------------------------------

looks good.

Comment 26

[Adam Roach [:abr]]

Comment on attachment 806854 [details] [diff] [review]
Part 3. Double checking for leaks in new test stun server code, and fixing other things as I see them.

Review of attachment 806854 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm

Comment 27

[Byron Campen [:bwc]]

(In reply to Adam Roach [:abr] from comment #24)
> Comment on attachment 806861 [details] [diff] [review]
> Part 1. Cleanup of trickle candidates and a test STUN server
> 
> Review of attachment 806861 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks mostly good. r+, assuming the following three comments are addressed
> before checkin.
> 
> ::: media/mtransport/test/stunserver.cpp
> @@ +263,5 @@
> > +    return nullptr;
> > +  }
> > +
> > +  // Cache the address and port.
> > +  char addr_string[INET6_ADDRSTRLEN];
> 
> I think we still need to add 1 here to accommodate a \0.
> 

     I double-checked this, and it seems to include room for a null terminator already (== 46).

> @@ +267,5 @@
> > +  char addr_string[INET6_ADDRSTRLEN];
> > +  r = nr_transport_addr_get_addrstring(&addrs[0].addr, addr_string,
> > +                                       sizeof(addr_string));
> > +  if (r)
> > +    return nullptr;
> 
> Still no logging?
>

Yeah, sure. We seem to be logging all the other errors.
 
> @@ +280,5 @@
> > +
> > +void TestStunServer::ConfigurePort(uint16_t port) {
> > +  instance_port = port;
> > +}
> > +
> 
> Still need a comment here talking about thread issues.

We have a comment about this function in the header file, so I think we should be good, right?

Comment 28

[Byron Campen [:bwc]]

Attachment: Part 1. Cleanup of trickle candidates and a test STUN server

Comment 29

[Byron Campen [:bwc]]

Try push for patches with r+=abr:

https://tbpl.mozilla.org/?tree=Try&rev=905f88fc05f9

Comment 30

[Adam Roach [:abr]]

(In reply to Byron Campen [:bwc] from comment #27)
>      I double-checked this, and it seems to include room for a null
> terminator already (== 46).

Thanks for checking. Sounds good.

> > Still no logging?
> >
> 
> Yeah, sure. We seem to be logging all the other errors.

OTOH, I guess this is just a test driver, so I'm not going to sweat it.

> > Still need a comment here talking about thread issues.
> 
> We have a comment about this function in the header file, so I think we
> should be good, right?

Sorry, I don't see it. I see a comment about ConfigurePort and SetResponseAddr, but not GetInstance. What am I missing?

Comment 31

[Byron Campen [:bwc]]

> OTOH, I guess this is just a test driver, so I'm not going to sweat it.
> 
> > > Still need a comment here talking about thread issues.
> > 
> > We have a comment about this function in the header file, so I think we
> > should be good, right?
> 
> Sorry, I don't see it. I see a comment about ConfigurePort and
> SetResponseAddr, but not GetInstance. What am I missing?

It seems like there is not really any limitation on where you call GetInstance from the first time, as long as you make subsequent calls (like SetResponseAddr) from the same thread, unless I'm mistaken. (In fact, you might be ok as long as you ensure that calls are serialized; does nICEr use thread local storage?) Conventionally, we use the STS thread for stuff involving nICEr, but this test doohickey doesn't really care, right? I can add a note to this effect.

Comment 32

[Byron Campen [:bwc]]

Attachment: Part 1. Cleanup of trickle candidates and a test STUN server

Comment 33

[Adam Roach [:abr]]

Comment on attachment 807240 [details] [diff] [review]
Part 1. Cleanup of trickle candidates and a test STUN server

Review of attachment 807240 [details] [diff] [review]:
-----------------------------------------------------------------

Okay, the new comments look reasonable.

Comment 34

[Eric Rescorla (:ekr)]

(In reply to Byron Campen [:bwc] from comment #31)
> > OTOH, I guess this is just a test driver, so I'm not going to sweat it.
> > 
> > > > Still need a comment here talking about thread issues.
> > > 
> > > We have a comment about this function in the header file, so I think we
> > > should be good, right?
> > 
> > Sorry, I don't see it. I see a comment about ConfigurePort and
> > SetResponseAddr, but not GetInstance. What am I missing?
> 
> It seems like there is not really any limitation on where you call
> GetInstance from the first time, as long as you make subsequent calls (like
> SetResponseAddr) from the same thread, unless I'm mistaken. (In fact, you
> might be ok as long as you ensure that calls are serialized; does nICEr use
> thread local storage?) Conventionally, we use the STS thread for stuff
> involving nICEr, but this test doohickey doesn't really care, right? I can
> add a note to this effect.

nICEr does not use TLS. The convention is that it must be run on one thread
at a time. However, it's generally safe-ish to do your setup work on main thread
provided that you have some synchronization primitive between that and
actually running stuff on the STS thread.

Comment 35

[Adam Roach [:abr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6948b7730d67
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce4aadef1cc5
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7a1c35bce2a

Comment 36

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6948b7730d67
https://hg.mozilla.org/mozilla-central/rev/ce4aadef1cc5
https://hg.mozilla.org/mozilla-central/rev/f7a1c35bce2a