1035468 - We need a NAT simulator that can be used for testing ICE

Status: RESOLVED FIXED

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

Comment 1

[Byron Campen [:bwc]]

Attachment: A NAT simulator based on NrSocket.

Comment 2

[Byron Campen [:bwc]]

Attachment: Part 2: Integrate the nat simulator into ice_unittest

Comment 3

[Byron Campen [:bwc]]

Attachment: MozReview Request: bz://1035468/bwc

/r/5551 - Bug 1035468: A NAT simulator based on NrSocket.
/r/5553 - Bug 1035468 - Part 2: Integrate the nat simulator into ice_unittest

Pull down these commits:

hg pull review -r 6ea704b98a7a04f5f9ab50170345c3d41b6ea771

Comment 4

[Byron Campen [:bwc]]

Comment on attachment 8578921 [details]
MozReview Request: bz://1035468/bwc

/r/5551 - Bug 1035468: A NAT simulator based on NrSocket.
/r/5553 - Bug 1035468 - Part 2: Integrate the nat simulator into ice_unittest

Pull down these commits:

hg pull review -r d10b85b5072490350687eec3b5435acad2843421

Comment 5

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=99081c7013c8

Comment 6

[Byron Campen [:bwc]]

Comment on attachment 8578921 [details]
MozReview Request: bz://1035468/bwc

/r/5551 - Bug 1035468 - Part 1: A NAT simulator based on NrSocket.
/r/5553 - Bug 1035468 - Part 2: Integrate the nat simulator into ice_unittest

Pull down these commits:

hg pull review -r 9b24a208f25f448a7ec44c77745a1e2011383eb3

Comment 7

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=31208dd8d5bd

Comment 8

[Byron Campen [:bwc]]

Comment on attachment 8578921 [details]
MozReview Request: bz://1035468/bwc

/r/5551 - Bug 1035468: A NAT simulator based on NrSocket, and integrate into ice_unittest.

Pull down this commit:

hg pull -r 7a6a7162a448eb6fa62036afb98c8b0ed6c046c2 https://reviewboard-hg.mozilla.org/gecko/

Comment 9

[Byron Campen [:bwc]]

Comment on attachment 8578921 [details]
MozReview Request: bz://1035468/bwc

/r/5551 - Bug 1035468: A NAT simulator based on NrSocket, and integrate into ice_unittest.

Pull down this commit:

hg pull -r e21f9a82ceef17ba58d4a600c8f35ee79c1a07ff https://reviewboard-hg.mozilla.org/gecko/

Comment 10

[Byron Campen [:bwc]]

Comment on attachment 8578921 [details]
MozReview Request: bz://1035468/bwc

/r/5551 - Bug 1035468: A NAT simulator based on NrSocket, and integrate into ice_unittest.

Pull down this commit:

hg pull -r 5cf0904b32bfce4ed5db5ba3306bdc3d61988fc2 https://reviewboard-hg.mozilla.org/gecko/

Comment 11

[Byron Campen [:bwc]]

Comment on attachment 8578921 [details]
MozReview Request: bz://1035468/bwc

/r/5551 - Bug 1035468: A NAT simulator based on NrSocket, and integrate into ice_unittest.

Pull down this commit:

hg pull -r 913d9bb868a8a5c5839c28797c5e158385ef41d1 https://reviewboard-hg.mozilla.org/gecko/

Comment 12

[Byron Campen [:bwc]]

Comment on attachment 8578921 [details]
MozReview Request: bz://1035468/bwc

/r/5551 - Bug 1035468: A NAT simulator based on NrSocket, and integrate into ice_unittest.

Pull down this commit:

hg pull -r 725788297d1f34408023832fa25f2f028ad7d727 https://reviewboard-hg.mozilla.org/gecko/

Comment 13

[Eric Rescorla (:ekr)]

Comment on attachment 8578921 [details]
MozReview Request: bz://1035468/bwc

https://reviewboard.mozilla.org/r/5549/#review7957

This review covers the simulator code and the integration with nICEr but not the mtransport tests that use the simulator.

::: media/mtransport/nr_socket_prsock.h:117
(Diff revision 8)
> +  const nr_transport_addr* my_addr() const {

Could this return a const& instead?

::: media/mtransport/nr_socket_prsock.cpp:594
(Diff revision 8)
> -    r_log(LOG_GENERIC, LOG_INFO, "Error in recvfrom");
> +    r_log(LOG_GENERIC, LOG_INFO, "Error in recvfrom %d", (int)PR_GetError());

Nit: ':' after recvfrom

::: media/mtransport/nr_socket_prsock.h:181
(Diff revision 8)
> +      : data(new DataBuffer(static_cast<const uint8_t*>(msg), len)) {

BUG?: This looks like it's missing an initializer for |from| from |addr|

::: media/mtransport/nr_socket_prsock.cpp:1165
(Diff revision 8)
> -int nr_socket_local_create(nr_transport_addr *addr, nr_socket **sockp) {
> +int nr_socket_local_create(void *obj, nr_transport_addr *addr, nr_socket **sockp) {

BUG: Do you use obj anywhere? I don't see it.

::: media/mtransport/test_nr_socket.h:97
(Diff revision 8)
> +#include "nsAutoPtr.h"

Can we convert this to UniquePtr<>

::: media/mtransport/test_nr_socket.h:117
(Diff revision 8)
> +      /** Self explanatory */
> +      NO_NAT,
> +      /** Single external IP/port, no filtering */
> +      FULL_CONE,
> +      /** Single external IP/port, filter by address (but not port) */
> +      ADDRESS_RESTRICTED_CONE,
> +      /** Single external IP/port, filter by address and port */
> +      PORT_RESTRICTED_CONE,
> +      /** One external IP/port per destination, filter by address and port */
> +      SYMMETRIC,
> +      /** Block all UDP traffic */
> +      BLOCK_UDP

This is no longer the preffered nomenclature....

::: media/mtransport/test_nr_socket.h:109
(Diff revision 8)
> +class TestNat : public std::set<TestNrSocket*> {

I'm not so sure about inheriting from std::set here. Suggest composition.

::: media/mtransport/test_nr_socket.h:136
(Diff revision 8)
> +    NatType GetNatType() const {
> +      return nat_type_;
> +    }
> +
> +    unsigned int GetMappingTimeout() const {
> +      return mapping_timeout_;
> +    }
> +
> +    bool GetAllowHairpinning() const {
> +      return allow_hairpinning_;

Convention in mtransort is nat_type(), here and below.

::: media/mtransport/test_nr_socket.h:167
(Diff revision 8)
> +    explicit TestNrSocket(nsRefPtr<TestNat> nat);

Any reason this can't be a pointer or a const ref&

::: media/mtransport/test_nr_socket.h:181
(Diff revision 8)
> +    void SetMappingTimeout(unsigned int timeout_ms);

Prefer concrete types here: uint32_t?

::: media/mtransport/test_nr_socket.h:189
(Diff revision 8)
> +    virtual int sendto(const void *msg, size_t len,
> +                       int flags, nr_transport_addr *to) override;
> +    virtual int recvfrom(void * buf, size_t maxlen,
> +                         size_t *len, int flags,
> +                         nr_transport_addr *from) override;
> +    virtual int connect(nr_transport_addr *addr) override;
> +    virtual int write(const void *msg, size_t len, size_t *written) override;
> +    virtual int read(void* buf, size_t maxlen, size_t *len) override;
> +
> +    virtual int async_wait(int how, NR_async_cb cb, void *cb_arg,
> +                           char *function, int line) override;
> +    virtual int cancel(int how) override;

virtual ... override is now deprecated.

::: media/mtransport/test_nr_socket.h:211
(Diff revision 8)
> +        nsAutoPtr<DataBuffer> buffer_;

UniquePtr<T>

::: media/mtransport/test_nr_socket.h:203
(Diff revision 8)
> +    class UdpPacket {
> +      public:
> +        UdpPacket(const void *msg, size_t len, nr_transport_addr *addr)
> +          : buffer_(new DataBuffer(static_cast<const uint8_t*>(msg), len)) {
> +          nr_transport_addr_copy(&remote_address_, addr);
> +        }
> +
> +        nr_transport_addr remote_address_;
> +        nsAutoPtr<DataBuffer> buffer_;
> +
> +        NS_INLINE_DECL_THREADSAFE_REFCOUNTING(UdpPacket);
> +      private:

can we use nr_udp_message here?

::: media/mtransport/test_nr_socket.h:222
(Diff revision 8)
> +        PRIntervalTime last_used_;

vertical whitespace please.

::: media/mtransport/test_nr_socket.h:222
(Diff revision 8)
> +        PRIntervalTime last_used_;
> +        nsRefPtr<NrSocket> external_socket_;
> +        // For non-symmetric, most of the data here doesn't matter
> +        nr_transport_addr remote_address_;
> +        // If external_socket_ returns E_WOULDBLOCK, we don't want to propagate
> +        // that to the code using the TestNrSocket. We can also perhaps use this
> +        // to help simulate things like latency.
> +        std::list<nsRefPtr<UdpPacket>> send_queue_;
> +

Methods come before variables. Also, can any of these be private.

::: media/mtransport/test_nr_socket.h:232
(Diff revision 8)
> +        int AsyncWait(int how, NR_async_cb cb, void *cb_arg,

I'm eagerly awaiting why this is being exposed.

::: media/mtransport/test_nr_socket.h:256
(Diff revision 8)
> +    static void PortMappingWriteableCallback(void *ext_sock_v,
> +                                                int how,
> +                                                void *test_sock_v);

indent.

::: media/mtransport/test_nr_socket.cpp:102
(Diff revision 8)
> +  nsRefPtr<TestNat> nat(static_cast<TestNat*>(obj));

Does this need to be a nsRefPtr<T>?

::: media/mtransport/test_nr_socket.cpp:119
(Diff revision 8)
> +    // We will release this reference in destroy(), not exactly the normal
> +    // ownership model, but it is what it is.
> +    NrSocketBase* dummy = sock.forget().take();
> +    (void)dummy;

Why do you need the temporary?

::: media/mtransport/third_party/nICEr/src/net/nr_socket.c:140
(Diff revision 8)
> +int nr_socket_factory_create(void *obj,

I'm not sure this is quite the abstraction we want. This fxn should be called nr_socket_factory_create_int() and it should be called by nr_socket_factory_<blah>_create().

::: media/mtransport/third_party/nICEr/src/net/nr_socket.h:76
(Diff revision 8)
> +  int (*create)(void *obj, nr_transport_addr *addr, nr_socket **sockp);

Can we call this create_socket() and have that be the name of the API call too?

::: media/mtransport/third_party/nICEr/src/net/nr_socket.h:77
(Diff revision 8)
> +  void (*destroy)(void **obj);

destroy functions are int in this code. Sorry.

::: media/mtransport/third_party/nICEr/src/ice/ice_ctx.c:240
(Diff revision 8)
> +void nr_ice_ctx_set_socket_factory(nr_ice_ctx *ctx, nr_socket_factory *factory)

Doesn't this leak memory? Suggest that we create the factory the first time we need a socket, to avoid the create/destroy cycle.

::: media/mtransport/third_party/nICEr/src/ice/ice_component.c:326
(Diff revision 8)
> -        if((r=nr_socket_local_create(&addr, &sock))){
> +        if((r=nr_socket_factory_create_socket(ctx->socket_factory, &addr, &sock))){

I know this isn't consistent int his code, but can we make the spacing consistent between these two fxns?

::: media/mtransport/third_party/nICEr/src/ice/ice_ctx.c:73
(Diff revision 8)
> +  nr_socket_local_create,

Suggest having a wrapper function here so you don't need to add a new (ignored) argument to nr_socket_local_create()

::: media/mtransport/test_nr_socket.cpp:144
(Diff revision 8)
> +  for (TestNrSocket* sock : *this) {

|\*this| just made my head explode. please please composition.

::: media/mtransport/test_nr_socket.cpp:196
(Diff revision 8)
> +  : nat_(nat ? nat.get() : new TestNat),

This seems pretty nasty. Can you require this to be explicitly constructed?

::: media/mtransport/test_nr_socket.cpp:245
(Diff revision 8)
> +  if (nat_type_ == TestNat::NO_NAT || IsInSameSubnet(to)) {

Let's rename IsInSameSubnet to something else.

::: media/mtransport/test_nr_socket.cpp:259
(Diff revision 8)
> +      external_socket = port_mappings_.front()->external_socket_;

In this case, the array should be of length 1, right? Maybe an assert here.

::: media/mtransport/test_nr_socket.cpp:306
(Diff revision 8)
> +    r = NrSocket::recvfrom(buf, maxlen, len, flags, from);

UGLY

::: media/mtransport/test_nr_socket.cpp:308
(Diff revision 8)
> +      // We do not use OnIngress() here because we do not want FULL_CONE to
> +      // allow traffic from another private subnet.

I'm not following this. Can you explain.

Note to self, this may become clear later.

::: media/mtransport/test_nr_socket.cpp:376
(Diff revision 8)
> +  port_mapping->last_used_ = PR_IntervalNow();

Do you want to implement a mode where the bindings are only refreshed by outgoing packets.

::: media/mtransport/test_nr_socket.cpp:474
(Diff revision 8)
> +            " (Why did the app not call recvfrom?)",

Should this be a MOZ_ASSERT()?

::: media/mtransport/test_nr_socket.cpp:380
(Diff revision 8)
> +int TestNrSocket::connect(nr_transport_addr *addr) {

Assert that all of these calls are on STS

::: media/mtransport/test_nr_socket.cpp:448
(Diff revision 8)
> +

Extra whitespace.

::: media/mtransport/test_nr_socket.cpp:483
(Diff revision 8)
> +    for (PortMapping* port_mapping : port_mappings_) {

I am concerned that this logic is going to cause double firing. Consider what happens when you have a single mapping. Someone calls async_wait() on it when it's already ready to read. We now dispatch onto the main thread *and* call async_wait() on the mapping.

The end outcome is that we read from the socket in the read callback but it's still set to poll even though only one call to async_wait() has occurred, so the next time a packet comes in, a potentially stale callback is called.

More generally, it seems like you are registering multiple async_waits() in response to a single call because you register for both the internal and external sockets. I think you need some gizmo to intercept that.

::: media/mtransport/test_nr_socket.cpp:508
(Diff revision 8)
> +    port_mapping->external_socket_->cancel(how);

Shouldn't this be wrapped the same way AsyncWait is?

::: media/mtransport/test_nr_socket.cpp:514
(Diff revision 8)
> +  r_log(LOG_GENERIC, LOG_DEBUG, "TestNrSocket %s stop waiting for %s",
> +                                my_addr_.as_string,
> +                                how == NR_ASYNC_WAIT_READ ? "read" : "write");

Fix indent

::: media/mtransport/test_nr_socket.cpp:518
(Diff revision 8)
> +  // Writable callbacks are decoupled except for the TCP case

Why are they decoupled?

::: media/mtransport/test_nr_socket.cpp:526
(Diff revision 8)
> +bool TestNrSocket::IsInSameSubnet(nr_transport_addr *addr) const {

Please rename, since this is same nat, right?

Also, doesn't this actually stop us from simulating two NATs with the same address space?

::: media/mtransport/test_nr_socket.cpp:83
(Diff revision 8)
> +extern "C" {

I notice in this file you seem to have trouble making up your mind about whether \*/& are attached to the type or the variable.

E.g.
 TestNat \*nat = static_cast<TestNat*>(*obj);
 
And.
 for (TestNrSocket\* sock : \*this) {

I know the rest of the code is a bit inconsistent, but probably best to pick one style since this is a new file

::: media/mtransport/test_nr_socket.cpp:568
(Diff revision 8)
> +

Extra whitespace.

::: media/mtransport/test_nr_socket.cpp:587
(Diff revision 8)
> +

Extra whitespace.

::: media/mtransport/test_nr_socket.cpp:607
(Diff revision 8)
> +    // Stop listening on all mapped sockets; we will start listening again
> +    // if the app starts listening to us again.
> +    CancelPortMappingAsyncWait(NR_ASYNC_WAIT_READ);
> +    r_log(LOG_GENERIC, LOG_DEBUG, "TestNrSocket %s ready for read",
> +                                  my_addr_.as_string);

Can this ever be called when poll_flags() doesn't include read. In OnPortMappingReadable() we assert that PR_POLL_READ is set and in TestNrSocket::async_wait() we call NrSocket::async_wait() which sets poll_flags_.

::: media/mtransport/test_nr_socket.cpp:607
(Diff revision 8)
> +    // Stop listening on all mapped sockets; we will start listening again
> +    // if the app starts listening to us again.
> +    CancelPortMappingAsyncWait(NR_ASYNC_WAIT_READ);
> +    r_log(LOG_GENERIC, LOG_DEBUG, "TestNrSocket %s ready for read",
> +                                  my_addr_.as_string);

Can this ever be called when poll_flags() doesn't include read. In OnPortMappingReadable() we assert that PR_POLL_READ is set and in TestNrSocket::async_wait() we call NrSocket::async_wait() which sets poll_flags_.

::: media/mtransport/test_nr_socket.cpp:616
(Diff revision 8)
> +void TestNrSocket::PortMappingWriteableCallback(void *ext_sock_v,
> +                                                int how,
> +                                                void *test_sock_v) {
> +
> +  TestNrSocket *test_socket = static_cast<TestNrSocket*>(test_sock_v);
> +  NrSocket *external_socket = static_cast<NrSocket*>(ext_sock_v);
> +
> +  test_socket->WriteToPortMapping(external_socket);
> +}
> +
> +void TestNrSocket::WriteToPortMapping(NrSocket *external_socket) {
> +
> +  MOZ_ASSERT(!IsTcpConnectionThroughNat());
> +
> +  int r = 0;
> +  for (PortMapping* port_mapping : port_mappings_) {
> +    if (port_mapping->external_socket_ == external_socket) {
> +      // Returns 0 if nothing to send, so loop continues
> +      r = port_mapping->SendFromQueue();
> +      if (r) {
> +        break;
> +      }
> +    }
> +  }
> +
> +  if (r == R_WOULDBLOCK) {
> +    // Re-register for writeable callbacks, since we still have stuff to send
> +    NR_ASYNC_WAIT(external_socket,
> +                  NR_ASYNC_WAIT_WRITE,
> +                  &TestNrSocket::PortMappingWriteableCallback,
> +                  this);
> +  }

This is just for TCP mappings, right? Since for UDP the upper layer should buffer.

::: media/mtransport/test_nr_socket.cpp:631
(Diff revision 8)
> +  for (PortMapping* port_mapping : port_mappings_) {
> +    if (port_mapping->external_socket_ == external_socket) {
> +      // Returns 0 if nothing to send, so loop continues
> +      r = port_mapping->SendFromQueue();
> +      if (r) {
> +        break;
> +      }
> +    }
> +  }

Why aren't you flushing all of the mappings?

::: media/mtransport/test_nr_socket.cpp:665
(Diff revision 8)
> +

Extra whitespace

::: media/mtransport/test_nr_socket.cpp:664
(Diff revision 8)
> +bool TestNrSocket::IsTcpConnectionThroughNat() const {

What are the semantics of this as opposed to a TCP connection not through NAT? Internal?

::: media/mtransport/test_nr_socket.cpp:702
(Diff revision 8)
> +

Whitespace.

::: media/mtransport/test_nr_socket.cpp:710
(Diff revision 8)
> +    case TestNat::FULL_CONE:
> +      switch (remote_address->ip_version) {
> +        case NR_IPV4:
> +          masked_address.u.addr4.sin_addr.s_addr = INADDR_ANY;
> +          break;
> +        case NR_IPV6:
> +          memcpy(&masked_address.u.addr6.sin6_addr,
> +                 &in6addr_any,
> +                 sizeof(struct in6_addr));
> +          break;
> +        default:
> +          MOZ_CRASH("Unknown IP version");
> +      }

This kind of makes me sad. You really shouldn't be reaching in like this. Any reason you can't use the nr_transport_addr APIs?

::: media/mtransport/test_nr_socket.cpp:748
(Diff revision 8)
> +

Whitespace.

::: media/mtransport/test_nr_socket.cpp:1
(Diff revision 8)
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

Is there a reason this isn't in media/mtransport/test

Overall, please do:

void foo() {
  ...
  

With no vertical whitespace after {

::: media/mtransport/test_nr_socket.cpp:750
(Diff revision 8)
> +                         const_cast<nr_transport_addr*>(&remote_address));

Maybe better to just pass a non-const ptr. ugh.

::: media/mtransport/test_nr_socket.cpp:753
(Diff revision 8)
> +int TestNrSocket::PortMapping::SendFromQueue() {
> +
> +  MOZ_ASSERT(remote_address_.protocol != IPPROTO_TCP);

Why is this needed for UDP? Can't you just have the caller retransmit? That's the guarantee above anyway

::: media/mtransport/test_nr_socket.cpp:802
(Diff revision 8)
> +  if (r == R_WOULDBLOCK) {

review Why can't this return R_WOULDBLOCK?

Comment 14

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/5549/#review8145

> This is no longer the preffered nomenclature....

What is?

> can we use nr_udp_message here?

Technically yes, although nr_udp_message::from would actually be pointing at the destination, which would be extremely confusing.

> Is there a reason this isn't in media/mtransport/test
> 
> Overall, please do:
> 
> void foo() {
>   ...
>   
> 
> With no vertical whitespace after {

It is not in media/mtransport/test because I intend to make this usable in the broswer, behind a pref.

> Why do you need the temporary?

This is lifted from here: https://dxr.mozilla.org/mozilla-central/source/media/mtransport/nr_socket_prsock.cpp#1193

It probably wouldn't compile (without warnings) without the temporary.

> Why are they decoupled?

I didn't want to open the possibility that we would get EWOULDBLOCK twice in a row on the same socket, and I also wanted to have a queue for outgoing traffic that could be used to simulate latency.

Comment 15

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/5549/#review8197

> BUG?: This looks like it's missing an initializer for |from| from |addr|

I think this was a half-baked attempt to use nr_udp_message instead of UdpPacket? Will remove.

> BUG: Do you use obj anywhere? I don't see it.

It's there because that's what nr_socket_factory_vtbl takes.

> UGLY

I could use composition here maybe (TestNrSocket would have an internal_socket_ or something).

> I'm not following this. Can you explain.
> 
> Note to self, this may become clear later.

I'm fixing the comment to state that on_ingress() only applies to traffic arriving on an external port.

> Do you want to implement a mode where the bindings are only refreshed by outgoing packets.

That isn't hard. I suppose the default should be to only refresh on outgoing traffic? Not sure what the norm is here.

> In this case, the array should be of length 1, right? Maybe an assert here.

Actually, the array can be any length, it is just that every port mapping has the same external socket unless we're a symmetric NAT.

> I am concerned that this logic is going to cause double firing. Consider what happens when you have a single mapping. Someone calls async_wait() on it when it's already ready to read. We now dispatch onto the main thread *and* call async_wait() on the mapping.
> 
> The end outcome is that we read from the socket in the read callback but it's still set to poll even though only one call to async_wait() has occurred, so the next time a packet comes in, a potentially stale callback is called.
> 
> More generally, it seems like you are registering multiple async_waits() in response to a single call because you register for both the internal and external sockets. I think you need some gizmo to intercept that.

So, TestNrSocket::on_port_mapping_readable calls TestNrSocket::fire_readable_callback, which cancels all of the port mappings, before calling NrSocketBase::fire_callback. So, any time one of the port mappings is ready to read, the async_wait(READ) on everything is canceled. When the TestNrSocket itself becomes readable, TestNrSocket::cancel is called by NrSocketBase::fire_callback, which cancels all the port mappings.

> Please rename, since this is same nat, right?
> 
> Also, doesn't this actually stop us from simulating two NATs with the same address space?

This is a misnomer, I'll be renaming it. Has nothing to do with subnets, really.

> Can this ever be called when poll_flags() doesn't include read. In OnPortMappingReadable() we assert that PR_POLL_READ is set and in TestNrSocket::async_wait() we call NrSocket::async_wait() which sets poll_flags_.

I can make this an assert.

> This is just for TCP mappings, right? Since for UDP the upper layer should buffer.

This is only for UDP. I'm not assuming anything about what the upper layer is doing here.

> Should this be a MOZ_ASSERT()?

Actually, it shouldn't even be logged now that I think about it. If multiple sockets (the TestNrSocket itself or some port mappings) become ready to read at the same time, we can only fire one callback, meaning that something has to go unserviced until async_wait is called again. I think what this means is we can just keep one readable socket, and not make any assertions about it (or add any handling for it) here.

> What are the semantics of this as opposed to a TCP connection not through NAT? Internal?

Yeah, in the case where this is a TCP connection that's entirely behind the NAT, we bypass all of the special NAT logic. I might be able to refactor this to be more clear.

> This kind of makes me sad. You really shouldn't be reaching in like this. Any reason you can't use the nr_transport_addr APIs?

There is no nr_transport_addr API that takes an in6_addr to construct an nr_transport_addr (although the IPV6 work adds one I think). I might be able to revisit this once the IPV6 work lands.

> Maybe better to just pass a non-const ptr. ugh.

Or we could open a ticket to fix nr_transport_addr_copy, and point a comment here at it?

> Why is this needed for UDP? Can't you just have the caller retransmit? That's the guarantee above anyway

I don't want EWOULDBLOCK to result in packet loss. This would complicate the unit-test for TestNrSocket for starters, and probably slow down ice_unittest too.

> review Why can't this return R_WOULDBLOCK?

It could, but then we'd end up in the situation where we'd return R_WOULDBLOCK immediately after firing a writeable callback, which seems like odd behavior. Maybe this matters more to me than it should?

> Doesn't this leak memory? Suggest that we create the factory the first time we need a socket, to avoid the create/destroy cycle.

I think it does leak, I wonder why try never picked up on it. Are you proposing lazy-creating the default the first time we need a socket? I suppose we could do that, but I would still fix the leak anyway.

> Methods come before variables. Also, can any of these be private.

send_queue_ can be private as-is, the rest would require defining a bunch of accessors.

> Why aren't you flushing all of the mappings?

I am, my comment is just misleading. It also returns 0 if stuff was successfully sent. I'll fix the comment.

Comment 16

[Byron Campen [:bwc]]

Attachment: MozReview Request: Bug 1035468: A NAT simulator based on NrSocket, and integrate into ice_unittest.

Bug 1035468: A NAT simulator based on NrSocket, and integrate into ice_unittest.

Comment 17

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/5549/#review8271

> Any reason this can't be a pointer or a const ref&

I made this a pointer.

> send_queue_ can be private as-is, the rest would require defining a bunch of accessors.

I've made send_queue_ private, and moved the functions up. Is that enough, or would you like accessors?

> I notice in this file you seem to have trouble making up your mind about whether \*/& are attached to the type or the variable.
> 
> E.g.
>  TestNat \*nat = static_cast<TestNat*>(*obj);
>  
> And.
>  for (TestNrSocket\* sock : \*this) {
> 
> I know the rest of the code is a bit inconsistent, but probably best to pick one style since this is a new file

I think I caught all of these...

> Suggest having a wrapper function here so you don't need to add a new (ignored) argument to nr_socket_local_create()

I feel like the wrapper would add more obfuscation than clarity, personally.

Comment 18

[Byron Campen [:bwc]]

Comment on attachment 8611583 [details]
MozReview Request: Bug 1035468: A NAT simulator based on NrSocket, and integrate into ice_unittest.

I've closed the issues that I completely fixed, although there are some incomplete and partially complete issues I have questions on.

Comment 19

[Eric Rescorla (:ekr)]

Cullen,

We have a question on NAT binding refreshes. Are they typically refreshed by both outgoing and incoming packets or just outgoing?

Comment 20

[Eric Rescorla (:ekr)]

https://reviewboard.mozilla.org/r/5549/#review8445

Byron, can you advise on the status of this? Do you want a review of this as-is or are you going to resubmit once we resolve the issues in my comments?

Comment 21

[Byron Campen [:bwc]]

(In reply to Eric Rescorla (:ekr) from comment #20)
> https://reviewboard.mozilla.org/r/5549/#review8445
> 
> Byron, can you advise on the status of this? Do you want a review of this
> as-is or are you going to resubmit once we resolve the issues in my comments?

   Let's talk through the issues I left open, so I know what to do with them (or if they're done enough).

Comment 22

[Eric Rescorla (:ekr)]

https://reviewboard.mozilla.org/r/5549/#review8441

> What is?

https://tools.ietf.org/html/rfc4787

> I've made send_queue_ private, and moved the functions up. Is that enough, or would you like accessors?

That seems fine.

> It is not in media/mtransport/test because I intend to make this usable in the broswer, behind a pref.

OK

> That isn't hard. I suppose the default should be to only refresh on outgoing traffic? Not sure what the norm is here.

That's my impression of the standard behavior. I needinfoed fluffy.

> So, TestNrSocket::on_port_mapping_readable calls TestNrSocket::fire_readable_callback, which cancels all of the port mappings, before calling NrSocketBase::fire_callback. So, any time one of the port mappings is ready to read, the async_wait(READ) on everything is canceled. When the TestNrSocket itself becomes readable, TestNrSocket::cancel is called by NrSocketBase::fire_callback, which cancels all the port mappings.

I will re-review to confirm.

> I didn't want to open the possibility that we would get EWOULDBLOCK twice in a row on the same socket, and I also wanted to have a queue for outgoing traffic that could be used to simulate latency.

I don't see a problem with multiple EWOULDBLOCKS (which are rare in any case for UDP).

I would suggest that we remove this for now on the principle of YAGNI

> This is only for UDP. I'm not assuming anything about what the upper layer is doing here.

As I said above, it's the upper layer's job to deal with wouldblock, so I think this code should be removed pending actually having a queue.

> There is no nr_transport_addr API that takes an in6_addr to construct an nr_transport_addr (although the IPV6 work adds one I think). I might be able to revisit this once the IPV6 work lands.

Suggest we land just that piece first rather than reaching in. Alternately, ifdef this IPV6 stuff out.

> Or we could open a ticket to fix nr_transport_addr_copy, and point a comment here at it?

That would be fine.

> I don't want EWOULDBLOCK to result in packet loss. This would complicate the unit-test for TestNrSocket for starters, and probably slow down ice_unittest too.

Please explain? How is this any different from the current state of play where we can get EWOULDBLOCK?

> It could, but then we'd end up in the situation where we'd return R_WOULDBLOCK immediately after firing a writeable callback, which seems like odd behavior. Maybe this matters more to me than it should?

I would hope we would be ok with this.

> I think it does leak, I wonder why try never picked up on it. Are you proposing lazy-creating the default the first time we need a socket? I suppose we could do that, but I would still fix the leak anyway.

Yes, that's what I was suggesting.

Comment 23

[Randell Jesup [:jesup] (needinfo me)]

https://reviewboard.mozilla.org/r/5549/#review8533

> That's my impression of the standard behavior. I needinfoed fluffy.

Refresh-on-outgoing is the norm.  I can't say there are *no* routers that act otherwise, but I haven't run across any (at least for generic automatic pinholes), nor can I see a good reason to use a different trigger.

Comment 24

[Byron Campen [:bwc]]

Comment on attachment 8611583 [details]
MozReview Request: Bug 1035468: A NAT simulator based on NrSocket, and integrate into ice_unittest.

Bug 1035468: A NAT simulator based on NrSocket, and integrate into ice_unittest.

Comment 25

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/5549/#review8571

> https://tools.ietf.org/html/rfc4787

Ok, will adopt the nomenclature there, and refactor to split mapping behavior from filtering behavior.

> I don't see a problem with multiple EWOULDBLOCKS (which are rare in any case for UDP).
> 
> I would suggest that we remove this for now on the principle of YAGNI

Discussed in person. Since we would spin when the internal socket is ready, but the external socket is not, I'll be leaving this stuff in place.

> Yes, that's what I was suggesting.

Since I'm already going to fix the leak, doing lazy-creation isn't going to fix anything, and I feel like it adds complexity.

> Suggest we land just that piece first rather than reaching in. Alternately, ifdef this IPV6 stuff out.

As it happens, this chunk of code interacts poorly with factoring nat_type_ out into orthogonal filtering and mapping types, so I'm removing the whole thing.

Comment 26

[Eric Rescorla (:ekr)]

https://reviewboard.mozilla.org/r/5549/#review8717

This is looking pretty good. Here are comments on the non-unit test code. Doing that next.

::: media/mtransport/test_nr_socket.h:120
(Diff revisions 8 - 10)
>      typedef enum {
> -      /** Self explanatory */
> -      NO_NAT,
> -      /** Single external IP/port, no filtering */
> -      FULL_CONE,
> -      /** Single external IP/port, filter by address (but not port) */
> -      ADDRESS_RESTRICTED_CONE,
> -      /** Single external IP/port, filter by address and port */
> -      PORT_RESTRICTED_CONE,
> -      /** One external IP/port per destination, filter by address and port */
> -      SYMMETRIC,
> -      /** Block all UDP traffic */
> +      /** For mapping, one port is used for all destinations.
> +       *  For filtering, allow any external address/port. */
> +      ENDPOINT_INDEPENDENT,
> +      /** For mapping, one port for each destination address (ignore port).
> +       *  For filtering, allow incoming traffic from addresses that outgoing
> +       *  traffic has been sent to. */
> +      ADDRESS_DEPENDENT,
> +      /** For mapping, one port for each destination address/port.
> +       *  For filtering, allow incoming traffic only from addresses/ports that
> +       *  outgoing traffic has been sent to. */
> +      PORT_DEPENDENT,

Suggest you define enums first.

::: media/mtransport/test_nr_socket.h:132
(Diff revisions 8 - 10)
>      } NatType;

Suggest NatBehavior, since NAT type is now a typle

::: media/mtransport/test_nr_socket.h:121
(Diff revisions 8 - 10)
> -      /** Self explanatory */
> -      NO_NAT,
> -      /** Single external IP/port, no filtering */
> -      FULL_CONE,
> -      /** Single external IP/port, filter by address (but not port) */
> -      ADDRESS_RESTRICTED_CONE,
> -      /** Single external IP/port, filter by address and port */
> -      PORT_RESTRICTED_CONE,
> -      /** One external IP/port per destination, filter by address and port */
> -      SYMMETRIC,
> -      /** Block all UDP traffic */
> +      /** For mapping, one port is used for all destinations.
> +       *  For filtering, allow any external address/port. */
> +      ENDPOINT_INDEPENDENT,
> +      /** For mapping, one port for each destination address (ignore port).
> +       *  For filtering, allow incoming traffic from addresses that outgoing
> +       *  traffic has been sent to. */
> +      ADDRESS_DEPENDENT,
> +      /** For mapping, one port for each destination address/port.
> +       *  For filtering, allow incoming traffic only from addresses/ports that
> +       *  outgoing traffic has been sent to. */
> +      PORT_DEPENDENT,

Can you put whitespace between these so it's clear what attaches to what?

::: media/mtransport/test_nr_socket.h:124
(Diff revisions 8 - 10)
> -      FULL_CONE,
> -      /** Single external IP/port, filter by address (but not port) */
> +      /** For mapping, one port for each destination address (ignore port).
> +       *  For filtering, allow incoming traffic from addresses that outgoing

Instead of "(ignore port)" suggest ("for any port")

::: media/mtransport/test_nr_socket.h:135
(Diff revisions 8 - 10)
> -    void SetNatType(NatType type);
> -    void SetMappingTimeout(unsigned int timeout_ms);
> +    bool is_my_external_address(nr_transport_addr *addr) const;
> +    bool is_my_internal_address(nr_transport_addr *addr) const;

Can these be const &

::: media/mtransport/test_nr_socket.h:157
(Diff revisions 8 - 10)
> +  private:

Whitespace before private.

::: media/mtransport/test_nr_socket.h:140
(Diff revisions 8 - 10)
> -    unsigned int GetMappingTimeout() const {
> -      return mapping_timeout_;
> +    void insert(TestNrSocket *socket) {
> +      sockets_.insert(socket);
>      }
>  
> -    bool GetAllowHairpinning() const {
> -      return allow_hairpinning_;
> +    void erase(TestNrSocket *socket) {
> +      sockets_.erase(socket);

InsertSocket();
EraseSocket();

I want to apologize for not being clear about the conventions: Methods are camelcased except for accessors which are non-camel-cased. Sorry about that.

::: media/mtransport/test_nr_socket.cpp:152
(Diff revisions 8 - 10)
> -  allow_hairpinning_ = allow;
> -
> -  for (TestNrSocket* sock : *this) {
> +    nr_transport_addr addr_in_subnet;
> +    if (sock->getaddr(&addr_in_subnet)) {
> +      MOZ_CRASH("PortMapping::getaddr failed!");

You probably want a different name, since it's not a subnet thing.

::: media/mtransport/test_nr_socket.cpp:181
(Diff revisions 8 - 10)
>  TestNrSocket::~TestNrSocket() {

The invariant here is that this socket can never outlive the NAT, right?

::: media/mtransport/test_nr_socket.cpp:279
(Diff revisions 8 - 10)
> -      ingress_allowed = OnIngress(from);
> +      ingress_allowed = on_ingress(from);

Maybe on_ingress should be called Ingressalowed()

::: media/mtransport/test_nr_socket.cpp:158
(Diff revisions 8 - 10)
> -
> -  for (TestNrSocket* sock : *this) {
> +                               &addr_in_subnet,
> +                               NR_TRANSPORT_ADDR_CMP_MODE_ALL)) {

Why is this CMP_MODE_ALL?

::: media/mtransport/test_nr_socket.cpp:287
(Diff revisions 8 - 10)
> -      // We do not use OnIngress() here because we do not want FULL_CONE to
> -      // allow traffic from another private subnet.
> -      ingress_allowed = (nat_type_ == TestNat::NO_NAT ||
> -                         IsInSameSubnet(from));
> +      // We do not use on_ingress() here because that only handles traffic
> +      // landing on an external port.
> +      ingress_allowed = (!nat_->enabled_ ||
> +                         nat_->is_my_internal_address(from));

How does this handle two NATs with the same internal addresses?

::: media/mtransport/test_nr_socket.cpp:379
(Diff revisions 8 - 10)
> -  PortMapping *port_mapping = CreatePortMapping(addr, external_socket);
> +  PortMapping *port_mapping = create_port_mapping(addr, external_socket);

Would it be better to have just the side effect and then set to port_mappings_.last()?

::: media/mtransport/test_nr_socket.cpp:546
(Diff revisions 8 - 10)
> +    readable_socket_ = external_socket;

Is there any reason to have this be a member variable rather than a callback argument.

::: media/mtransport/test/ice_unittest.cpp:392
(Diff revisions 8 - 10)
> +  }
> +
> +  void SetFilteringType(TestNat::NatType type) {
> +    nat_->filtering_type_ = type;
> +  }
> +
> +  void SetMappingType(TestNat::NatType type) {
> +    nat_->mapping_type_ = type;
> +  }
> +
> +  void SetBlockUdp(bool block) {
> +    nat_->block_udp_ = block;

Can you assert that these can't be called once there are mappings?

::: media/mtransport/test/ice_unittest.cpp:1174
(Diff revisions 8 - 10)
>        // If we enable nat simulation, but still use a real STUN server somewhere
>        // on the internet, we will see failures if there is a real NAT in
>        // addition to our simulated one, particularly if it disallows
>        // hairpinning.
>        UseTestStunServer();

This is just a warning, right? It doesn't do anyting to fix it.

::: media/mtransport/test_nr_socket.h:158
(Diff revisions 8 - 10)
> +    std::set<TestNrSocket*> sockets_;

Please add an empty ctor for sockets\_. Convention here is to initialize everything.

Comment 27

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/5549/#review9035

> This is just a warning, right? It doesn't do anyting to fix it.

It's explaining why a call to |UseTestStunServer| is a consequence of enabling the NAT simulator.

> Why is this CMP_MODE_ALL?

Maybe the function should be renamed. We're asking whether the addr/port/proto matches an internal addr/prot/proto behind the NAT.

> The invariant here is that this socket can never outlive the NAT, right?

Correct.

> How does this handle two NATs with the same internal addresses?

I think this is again a sign that this function needs to be renamed. Maybe MatchesOneOfMyInternalSockets? Not sure what the best name here would be.

> Would it be better to have just the side effect and then set to port_mappings_.last()?

I'll see if that can be done cleanly.

> Is there any reason to have this be a member variable rather than a callback argument.

Talked about this in person, agreed to drop.

> Maybe on_ingress should be called Ingressalowed()

It also updates |last_used_|, if we're configured to do that. I could do that at every callsite.

> Can you assert that these can't be called once there are mappings?

It'll cost some extra code in TestNat, but ok.

Comment 28

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/5549/#review9041

> InsertSocket();
> EraseSocket();
> 
> I want to apologize for not being clear about the conventions: Methods are camelcased except for accessors which are non-camel-cased. Sorry about that.

Well, we're also using lowercase for all the stuff we inherit from NrSocket (sendto, etc). I'm inclined to just use the same convention everywhere here. It's a lot easier that way.

Comment 29

[Eric Rescorla (:ekr)]

https://reviewboard.mozilla.org/r/5549/#review9029

::: media/mtransport/test/test_nr_socket_unittest.cpp:1
(Diff revision 10)
> +extern "C" {

Doesn't this need a header?

::: media/mtransport/test/test_nr_socket_unittest.cpp:64
(Diff revision 10)
> +    nsRefPtr<TestNrSocket> sock(new TestNrSocket(nat ? nat : new TestNat));
> +    nr_transport_addr address;

This seems like it creates a single-use TestNat for public addresses. Perhaps at least a comment is needed.

::: media/mtransport/test/test_nr_socket_unittest.cpp:74
(Diff revision 10)
> +  void CreateAddrs_s(size_t count, const char *ip_str, TestNat *nat) {
> +    while (count--) {
> +      auto sock = CreateTestNrSocket_s(ip_str, nat);
> +      ASSERT_TRUE(sock) << "Failed to create socket";
> +      if (!nat) {
> +        public_addrs_.push_back(sock);
> +      } else {
> +        private_addrs_.push_back(sock);
> +      }
> +    }
> +  }
> +
> +  nsRefPtr<TestNat> CreatePrivateAddrs(size_t size) {
> +    nsRefPtr<TestNat> result;
> +    sts_->Dispatch(
> +        WrapRunnableRet(this,
> +                        &TestNrSocketTest::CreatePrivateAddrs_s,
> +                        size,
> +                        &result),
> +        NS_DISPATCH_SYNC);
> +    return result;
> +  }
> +
> +  nsRefPtr<TestNat> CreatePrivateAddrs_s(size_t count) {
> +    nsRefPtr<TestNat> nat(new TestNat);
> +    CreateAddrs_s(count, "127.0.0.1", nat.get());
> +    nat->enabled_ = true;
> +    nats_.push_back(nat);
> +    return nat;
> +  }
> +

This whole mix of public and private seems confusing. Suggest two different function stacks.

::: media/mtransport/test/test_nr_socket_unittest.cpp:89
(Diff revision 10)
> +        WrapRunnableRet(this,
> +                        &TestNrSocketTest::CreatePrivateAddrs_s,
> +                        size,
> +                        &result),

You will have to rebase this once froydnjs's patch to move the return values in WrapRunnable lands.

::: media/mtransport/test/test_nr_socket_unittest.cpp:110
(Diff revision 10)
> +

Remove extra vertical whitespace.

::: media/mtransport/test/test_nr_socket_unittest.cpp:185
(Diff revision 10)
> +    const char buf[] = "foobajooba";

Nit: foobajoobafooba

::: media/mtransport/test/test_nr_socket_unittest.cpp:192
(Diff revision 10)
> +    char buf[bufSize] = {0};

Why are you initializing this buffer? You do not appear to read from it.

::: media/mtransport/test/test_nr_socket_unittest.cpp:196
(Diff revision 10)
> +    if (!r && len == 0) {

Parens please.

::: media/mtransport/test/test_nr_socket_unittest.cpp:231
(Diff revision 10)
> +  bool WaitForReadable(TestNrSocket *sock) {
> +    MOZ_ASSERT(sock);
> +    sts_->Dispatch(WrapRunnable(this,
> +                                &TestNrSocketTest::WaitForReadable_s,
> +                                sock),
> +                   NS_DISPATCH_SYNC);
> +
> +    bool res;
> +    WAIT_(wait_done_for_main_, 100, res);
> +    wait_done_for_main_ = false;
> +
> +    if (!res) {
> +      sts_->Dispatch(WrapRunnable(this,
> +                                  &TestNrSocketTest::CancelReadable_s,
> +                                  sock),
> +                     NS_DISPATCH_SYNC);
> +    }
> +
> +    return res;

Can you merge these into WaitForSocketState()

::: media/mtransport/test/test_nr_socket_unittest.cpp:33
(Diff revision 10)
> +    wait_done_for_main_(false) {

Please empty initialize the vectors to match convention.

::: media/mtransport/test/test_nr_socket_unittest.cpp:108
(Diff revision 10)
> +      nr_transport_addr *via,

Pass this as a reference, so it's clear what's going on. I wonder if it would be easier if we didn't have the default value too.

::: media/mtransport/test/test_nr_socket_unittest.cpp:374
(Diff revision 10)
> +// OS 10.6 doesn't seem to allow us to open ports on 127.0.0.2, and while linux
> +// does allow this, it has other behavior (see below) that prevents this test
> +// from working.
> +TEST_F(TestNrSocketTest, DISABLED_AddressRestrictedCone) {

Are we ever going to reenable this test? If not, let's remove.

::: media/mtransport/test/test_nr_socket_unittest.cpp:472
(Diff revision 10)
> +  ASSERT_FALSE(nr_transport_addr_cmp(&sender_external_address,
> +                                     &sender_external_address2,
> +                                     NR_TRANSPORT_ADDR_CMP_MODE_ALL))

True, false seems hard here. Maybe instead an overload for operator ==

::: media/mtransport/test/test_nr_socket_unittest.cpp:528
(Diff revision 10)
> +  ASSERT_TRUE(CheckConnectivity(private_addrs_[0],
> +                                public_addrs_[0],
> +                                &sender_external_address));

This doesn't seem like BlockUdp if you let it out.

::: media/mtransport/test/test_nr_socket_unittest.cpp:599
(Diff revision 10)
> +  PR_Sleep(2);

Check that rturn traffic works now.

::: media/mtransport/test/test_nr_socket_unittest.cpp:483
(Diff revision 10)
> +

We need a PORT_RESTRICTED test.

::: media/mtransport/test/ice_unittest.cpp:1770
(Diff revision 10)
> +TEST_F(IceConnectTest, TestGatherAddressRestrictedConeAutoPrioritize) {

I think you could lose the autoprioritize ones or the other way around.

::: media/mtransport/test/ice_unittest.cpp:1867
(Diff revision 10)
> +                           g_turn_server, kDefaultStunServerPort,
> +                           g_turn_user, password_vec, kNrIceTransportTcp));
> +  turn_servers.push_back(*NrIceTurnServer::Create(
> +                           g_turn_server, kDefaultStunServerPort,
> +                           g_turn_user, password_vec, kNrIceTransportUdp));

Indent.

::: media/mtransport/test/ice_unittest.cpp:1866
(Diff revision 10)
> +  turn_servers.push_back(*NrIceTurnServer::Create(
> +                           g_turn_server, kDefaultStunServerPort,
> +                           g_turn_user, password_vec, kNrIceTransportTcp));
> +  turn_servers.push_back(*NrIceTurnServer::Create(
> +                           g_turn_server, kDefaultStunServerPort,
> +                           g_turn_user, password_vec, kNrIceTransportUdp));

Indent.

::: media/mtransport/test/ice_unittest.cpp:1877
(Diff revision 10)
> +TEST_F(IceConnectTest, TestConnectNatBlocksUDP) {
> +  if (g_turn_server.empty())
> +    return;
> +
> +  AddStream("first", 1);
> +  UseNat();
> +  BlockUdp();
> +  std::vector<NrIceTurnServer> turn_servers;
> +  std::vector<unsigned char> password_vec(g_turn_password.begin(),
> +                                          g_turn_password.end());
> +  turn_servers.push_back(*NrIceTurnServer::Create(
> +                           g_turn_server, kDefaultStunServerPort,
> +                           g_turn_user, password_vec, kNrIceTransportTcp));
> +  turn_servers.push_back(*NrIceTurnServer::Create(
> +                           g_turn_server, kDefaultStunServerPort,
> +                           g_turn_user, password_vec, kNrIceTransportUdp));
> +  SetTurnServers(turn_servers);
> +  p1_->SetExpectedTypes(NrIceCandidate::Type::ICE_RELAYED,
> +                        NrIceCandidate::Type::ICE_RELAYED,
> +                        kNrIceTransportTcp);
> +  p2_->SetExpectedTypes(NrIceCandidate::Type::ICE_RELAYED,
> +                        NrIceCandidate::Type::ICE_RELAYED,
> +                        kNrIceTransportTcp);
> +  ASSERT_TRUE(Gather(kDefaultTimeout * 3));

Indent.

::: media/mtransport/test/ice_unittest.cpp:1904
(Diff revision 10)
> +// If there is a real symmetric NAT between us and the TURN server, we will
> +// need a relay on both sides. To make this test work reliably, we will
> +// probably need a test TURN server.
> +TEST_F(IceConnectTest, DISABLED_TestConnectSymmetricAndPortRestrictedCone) {
> +  if (g_turn_server.empty())
> +    return;

I am not reviewing the disabled ones, please push to another patch.

Comment 30

[Byron Campen [:bwc]]

Comment on attachment 8611583 [details]
MozReview Request: Bug 1035468: A NAT simulator based on NrSocket, and integrate into ice_unittest.

Bug 1035468: A NAT simulator based on NrSocket, and integrate into ice_unittest.

Comment 31

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/5549/#review9065

> Are we ever going to reenable this test? If not, let's remove.

Once we stop supporting 10.6, we should be good to go on this one. Also, would be nice to have in the tree so I can manually enable it when running on my laptop.

Comment 32

[Byron Campen [:bwc]]

Comment on attachment 8611583 [details]
MozReview Request: Bug 1035468: A NAT simulator based on NrSocket, and integrate into ice_unittest.

Incorporated feedback, and rebased. Interdiff shows some spurious changes in common.build and nr_socket_prsock.cpp because of the rebase.

Comment 33

[Byron Campen [:bwc]]

Attachment: MozReview Request: Bug 1035468 - Part 2: Integrate the nat simulator into ice_unittest

Comment 34

[Eric Rescorla (:ekr)]

Comment on attachment 8611583 [details]
MozReview Request: Bug 1035468: A NAT simulator based on NrSocket, and integrate into ice_unittest.

https://reviewboard.mozilla.org/r/5551/#review9245

::: media/mtransport/test_nr_socket.h:181
(Diff revisions 10 - 11)
> +

Extra whitespace.

::: media/mtransport/test_nr_socket.cpp:159
(Diff revisions 10 - 11)
> -bool TestNat::is_my_internal_address(nr_transport_addr *addr) const {
> +bool TestNat::is_my_internal_tuple(const nr_transport_addr &addr) const {

Maybe "is_an_internal_tuple" would be better?

::: media/mtransport/test_nr_socket.cpp:149
(Diff revisions 10 - 11)
> +bool TestNat::is_my_external_tuple(const nr_transport_addr &addr) const {

Suggest is_an_external_tuple()

::: media/mtransport/test_nr_socket.cpp:348
(Diff revisions 10 - 11)
> -    r_log(LOG_GENERIC, LOG_INFO, "TestNrSocket %s denying ingress from %s: "
> +  if (!(*port_mapping_used)) {

I assume this changed because we are now blocking UDP in both directions and so there will be no mapping?

::: media/mtransport/test_nr_socket.cpp:664
(Diff revisions 10 - 11)
> -    nsRefPtr<NrSocket> external_socket) {
> +    nsRefPtr<NrSocket> external_socket) const {

Any reason this can't be a const nsRefPtr<>&

::: media/mtransport/test/test_nr_socket_unittest.cpp:520
(Diff revisions 10 - 11)
> +  // Verify that other public IP can use the pinhole

Nit: use the original pinhole

Comment 35

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/5551/#review9461

> Suggest is_an_external_tuple()

Yeah, that reads a little better.

> I assume this changed because we are now blocking UDP in both directions and so there will be no mapping?

Yep.

> Any reason this can't be a const nsRefPtr<>&

I think that will be fine.

Comment 36

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/5549/#review9463

> Nit: foobajoobafooba

This is a common misconception. "foobajooba" doesn't separate into "fooba" and "jooba", but rather "foo" and "bajooba".

Comment 37

[Byron Campen [:bwc]]

Comment on attachment 8611583 [details]
MozReview Request: Bug 1035468: A NAT simulator based on NrSocket, and integrate into ice_unittest.

Bug 1035468: A NAT simulator based on NrSocket, and integrate into ice_unittest.

Comment 38

[Byron Campen [:bwc]]

Comment on attachment 8611583 [details]
MozReview Request: Bug 1035468: A NAT simulator based on NrSocket, and integrate into ice_unittest.

Carry forward r=ekr

Comment 39

[Pulsebot]

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

Comment 40

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

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