950660 - Support TCP nr_socket in content process.

Status: RESOLVED FIXED

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

Comment 1

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

This needs bind() for TCPSocketChild, but TCPSocketChild doesn't provide such function. To add bind() in TCPSocketChild, we will need to add bind function in SocketTransport, which TCPSocket uses.

Comment 2

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: WIP: Support Bind in content process through TCPSocket

Comment 3

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: WIP: Part 2: bridge TCPSocketChild and nr_socket

Comment 4

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: WIP: Support Bind in TCPSocketChild.

Rebasing.

Comment 5

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: WIP: Bridge ns_socket and TCPSocketChild

Comment 6

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: WIP: Support Bind in TCPSocketChild.

Comment 7

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: WIP: Bridge ns_socket and TCPSocketChild

Comment 8

[Eric Rescorla (:ekr)]

Maire,

This is fairly high priority to get done, I think. Do we have an estimate/target?

Comment 9

[Gian-Carlo Pascutto [:gcp]]

Hi Patrick, are you able to continue work on this code? Do you need someome to give feedback/review it?

Comment 10

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

I am actually working on other bugs and may not be able to finish this quickly. These patches are still WIPs, they are not ready to review.

Comment 11

[Maire Reavy [:mreavy]]

After talking with blassey and gcp, I'd like to get this into Fx36.

Hi Patrick -- I know you said in Comment 10 that you "may not be able to finish this quickly".  Do you think you'd be able to finish this in time for Fx36?  (Fx36 uplifts on November 24th.)

Comment 12

[Maire Reavy [:mreavy]]

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #11)
> After talking with blassey and gcp, I'd like to get this into Fx36.
> 
> Hi Patrick -- I know you said in Comment 10 that you "may not be able to
> finish this quickly".  Do you think you'd be able to finish this in time for
> Fx36?  (Fx36 uplifts on November 24th.)

Forgot to add the needinfo to Patrick. ^^^

Comment 13

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

I think I can finish this by Nov. 24.

Comment 14

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: WIP: Bridge ns_socket and TCPSocketChild

Updating WIP. Requesting feedback just to make sure I am on the right track. EKR, is the logic of packet filter in TCP the same as what in UDP, so I can implement it based on UDP packet filter (just do some change to fit API of TCPSocket)?

Comment 15

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 2: Bridge ns_socket and TCPSocketChild

Comment 16

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 1: TCPSocket IPC classes change

Comment 17

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 1: TCPSocket IPC classes change

Changes in TCP's IPC classes to support bind(). Also make TCPSocketChild be able to pass native array to listener without having to translate to js first.

Comment 18

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 2: Bridge ns_socket and TCPSocketChild

It now is able to reach network from content side, although it still needs some modification to be able to read/write correctly.

Comment 19

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: 0001-Bug-950660-Part-1-Support-bind-in-content-process-th.patch

Comment 20

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 2: Bridge ns_socket and TCPSocketChild

It can allocate TURN now. Running mochitests.

Comment 21

[Nils Ohlmeier [:drno]]

(In reply to Patrick Wang (Chih-Kai Wang) (:kk1fff) from comment #20)
> It can allocate TURN now. Running mochitests.

I forgot to mention that earlier: for your own devlopment/testing purposes you can provide a TURN server via the turnConfig.js file to the mochitests (assuming you have setup a TURN server your self somewhere). Then the mochitests should at least connect to the TURN server and request allocations.

Comment 22

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Nils Ohlmeier [:drno] from comment #21)
> I forgot to mention that earlier: for your own devlopment/testing purposes
> you can provide a TURN server via the turnConfig.js file to the mochitests
> (assuming you have setup a TURN server your self somewhere). Then the
> mochitests should at least connect to the TURN server and request
> allocations.

Yes, that is what I am going to do.

Comment 23

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Sad, my try push on TBPL is not allowed to connect to my own turn server. Nils, do you know where to host a TURN so TBPL is able to connect to? Or where should I push to get it run the tests with turn configured?

Comment 24

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 1: Change in TCPSocket IPC

Comment 25

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 2: Bridge ns_socket and TCPSocketChild

Comment 26

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

I've tested the patch locally with mochitest as well as apprtc. It basically works. However, in apprtc, the speed of consuming data is way slower than speed of data feeding from network, and the receiving buffer grows fast and the video stuck after a few seconds.

Comment 27

[Nils Ohlmeier [:drno]]

(In reply to Patrick Wang (Chih-Kai Wang) (:kk1fff) from comment #23)
> Sad, my try push on TBPL is not allowed to connect to my own turn server.
> Nils, do you know where to host a TURN so TBPL is able to connect to? Or
> where should I push to get it run the tests with turn configured?

As I explained in the separate email: no test on TBPL will ever be allowed to connect to a TURN server.
And there is no way for you (yet) to push any try build which would test TURN for you.

If you are satisfied with the local testing I can try to build a local build with your patch and manually deploy it in our QA lab and give a try there.

Comment 28

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Part 2: Bridge ns_socket and TCPSocketChild

Fixed the problem that cause video stop after connecting with TURN TCP. It can now work with apprtc.

Comment 29

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

EKR, do you have time to review this or could you suggest a reviewer? Thanks!

Comment 30

[Eric Rescorla (:ekr)]

I won'd have time this week at least.

Comment 31

[Honza Bambas (:mayhemer)]

Comment on attachment 8506209 [details] [diff] [review]
Part 1: Change in TCPSocket IPC

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

- a test is needed to at least exercise the new code
- please, when you ask for a review make sure you've added good comments to all idl modification
- I cannot for sure say this all is correct, the TCPSocket IPC code is a mess (not your fault :))

I'll gladly check an update

::: dom/network/PTCPSocket.ipdl
@@ +41,5 @@
>    Open(nsString host, uint16_t port, bool useSSL, nsString binaryType);
>  
> +  OpenBind(nsCString host, uint16_t port,
> +           nsCString localAddr, uint16_t localPort,
> +           bool useSSL, nsCString binaryType);

comments

::: dom/network/TCPSocket.js
@@ +488,5 @@
> +
> +  /**
> +   * Open input/output stream for the socket transport. 
> +   */
> +  openWithExistSocketTransport: function(host, port,

put this method under open() method definition

::: dom/network/TCPSocketParent.cpp
@@ +224,5 @@
> +  PRNetAddr prAddr;
> +  PR_InitializeNetAddr(PR_IpAddrAny, aLocalPort, &prAddr);
> +  PRStatus status = PR_StringToNetAddr(aLocalAddr.BeginReading(), &prAddr);
> +  if (status != PR_SUCCESS) {
> +    return true;

and how is the called going to get to know we failed here?

@@ +229,5 @@
> +  }
> +
> +  mozilla::net::NetAddr addr;
> +  PRNetAddrToNetAddr(&prAddr, &addr);
> +  socketTransport->Bind(&addr);

please check the result

@@ +249,5 @@
> +
> +  mIntermediary->OpenWithExistSocketTransport(this, aRemoteHost, aRemotePort,
> +                                              socketTransport,
> +                                              aUseSSL, aBinaryType, appId,
> +                                              getter_AddRefs(mSocket));

and if this fails?

::: dom/network/TCPSocketParentIntermediary.js
@@ +68,5 @@
> +      return null;
> +    }
> +
> +    let socketInternal = socket.QueryInterface(Ci.nsITCPSocketInternal);
> +    socketInternal.setAppId(appId);

you should set this before opening the |socket|.

::: dom/network/interfaces/nsIDOMTCPSocket.idl
@@ +220,2 @@
>  interface nsITCPSocketInternal : nsISupports {
> +  const unsigned long BUFFER_SIZE = 65536;

comment

@@ +288,5 @@
>                             in unsigned long byteLength, in unsigned long trackingNumber);
> +
> +  // Connect existing socket transport to the TCPSocket object.
> +  // @param socketTransport
> +  //        The socket transport object that will back the TCPSocket.

@result as well please

@@ +297,5 @@
> +                                               in DOMString binaryType);
> +};
> +
> +// Let IPC classes can pass native objects without having to
> +// transform to JSObject first.

doxygen style, check phrasing

@@ +300,5 @@
> +// Let IPC classes can pass native objects without having to
> +// transform to JSObject first.
> +[uuid(bb2289bc-7dfa-485a-aa88-09ae1ddd0248)]
> +interface nsITCPSocketInternalNative : nsITCPSocketInternal {
> +  void callListenerNativeArray([array, size_is(len)] in octet buf, in unsigned long len);

needs detailed comments, also on all arguments

::: dom/network/interfaces/nsITCPSocketChild.idl
@@ +35,5 @@
> +  [noscript]
> +  void sendWindowlessOpenBind(in nsITCPSocketInternal socket,
> +                              in AUTF8String remoteHost, in unsigned short remotePort,
> +                              in AUTF8String localHost, in unsigned short localPort,
> +                              in boolean ssl);

comments needed

put sendWindowlessOpenBind under sendOpen()

::: dom/network/interfaces/nsITCPSocketParent.idl
@@ +12,5 @@
>  
>  // Interface required to allow the TCP socket object (TCPSocket.js) in the
>  // parent process to talk to the parent IPC actor, TCPSocketParent, which
>  // is written in C++.
> +[scriptable, uuid(c434224a-dbb7-4869-8b2b-e49cee990e85)]

why?

@@ +87,5 @@
> +
> +  // Connect existing socket transport to the TCPSocket object.
> +  // @param socketTransport
> +  //        The socket transport object that will back the TCPSocket.
> +  nsIDOMTCPSocket openWithExistSocketTransport(in nsITCPSocketParent parent,

openWithSocketTransport will do.  Correct is "Existing" while we are long enough already.

Comment 32

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Honza Bambas (:mayhemer) from comment #31)
> Comment on attachment 8506209 [details] [diff] [review]
> Part 1: Change in TCPSocket IPC
> 
> Review of attachment 8506209 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> - please, when you ask for a review make sure you've added good comments to
> all idl modification

I'll do this next time, sorry about that.

> - I cannot for sure say this all is correct, the TCPSocket IPC code is a
> mess (not your fault :))

Yes, the code there is kind of messy, maybe we should rewrite all of them in C++ someday...

I'll update the patch. Thank you for the feedback.

Comment 33

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch Part: 1 - TCPSocket change.

Testing is working in process. Posting this updated version of part 1 for feedback.

Comment 34

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch Part: 2: Bridge ns_socket and TCPSocketChild

Comment 35

[Byron Campen [:bwc]]

Comment on attachment 8545032 [details] [diff] [review]
Patch Part: 2: Bridge ns_socket and TCPSocketChild

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

A few important issues, and several nits.

::: media/mtransport/nr_socket_prsock.cpp
@@ +711,2 @@
>                                               const nsACString &filename,
>                                               uint32_t line_number) {

Make sure to fixup the whitespace here and in other places.

@@ +1072,5 @@
> +// TCPSocket.
> +class NrTcpSocketIpc::TcpSocketReadyRunner: public nsRunnable
> +{
> +public:
> +  TcpSocketReadyRunner(NrTcpSocketIpc *sck)

This should be marked explicit.

@@ +1114,5 @@
> +  } else {
> +    r_log(LOG_GENERIC, LOG_INFO,
> +          "Not handling state changing to %s from TCPSocketChild",
> +          NS_ConvertUTF16toUTF8(readyState).get());
> +    return NS_OK;

Should this really be ok? It is pretty difficult to figure out what might end up here.

@@ +1138,5 @@
> +
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP NrTcpSocketIpc::CallListenerNativeArray(uint8_t *buf, uint32_t len) {

Wrap to 80 columns, here and in other places.

@@ +1165,5 @@
> +                mozilla::WrapRunnable(nsRefPtr<NrTcpSocketIpc>(this),
> +                                      &NrTcpSocketIpc::update_state_s,
> +                                      NR_CLOSED),
> +                false);
> +  

Trailing ws, here and in some other places, highlighted in red on splinter.

@@ +1178,5 @@
> +}
> +
> +NS_IMETHODIMP NrTcpSocketIpc::CallListenerData(const nsAString &type,
> +                                               const nsAString &data) {
> +  return NS_OK;

Why is this NS_OK instead of NS_ERROR_NOT_IMPLEMENTED?

@@ +1224,5 @@
> +  return NS_ERROR_NOT_IMPLEMENTED;
> +}
> +
> +//
> +// NrBaseSocket methods.

NrSocketBase

@@ +1244,5 @@
> +  }
> +
> +  if ((r=nr_transport_addr_get_addrstring_and_port(addr, &host, &port))) {
> +    ABORT(r);
> +  }

We aren't actually doing anything with |host| or |port| here, but I suppose it would make some sense to perform this as a sanity check instead of waiting for |connect|. If we want to keep this here, add a comment that this is a sanity-check, and add a MOZ_ASSERT to the code in connect that grabs the host and port from |my_addr_|.

@@ +1291,5 @@
> +  //remove all enqueued messages
> +  {
> +    std::queue<RefPtr<nr_tcp_message> > empty;
> +    std::swap(msg_queue_, empty);
> +  }

This is kinda ugly; why not use a std::list or std::deque so we have a |clear| method? (std::queue uses std::deque under the hood anyway) Alternately, you can assign an empty std::queue.

@@ +1329,5 @@
> +
> +int NrTcpSocketIpc::write(const void *msg, size_t len, size_t *written) {
> +  int _status = 0;
> +  if (state_ != NR_CONNECTED)
> +    ABORT(R_FAILED);

Wrap in '{}'

@@ +1333,5 @@
> +    ABORT(R_FAILED);
> +
> +  if (buffered_byte_ >= nsITCPSocketInternal::BUFFER_SIZE) {
> +    ABORT(R_WOULDBLOCK);
> +  }

What if buffered_byte_ + len > BUFFER_SIZE? Won't this fail when we call |SendSendArray|?

@@ +1354,5 @@
> +
> +int NrTcpSocketIpc::read(void* buf, size_t maxlen, size_t *len) { 
> +  int _status = 0;
> +  if (state_ != NR_CONNECTED)
> +    ABORT(R_FAILED);

Braces.

@@ +1390,5 @@
> +    RUN_ON_THREAD(sts_thread_,
> +                  mozilla::WrapRunnable(nsRefPtr<NrTcpSocketIpc>(this),
> +                                        &NrTcpSocketIpc::update_state_s,
> +                                        NR_CLOSED),
> +                  false);

NS_DISPATCH_NORMAL

@@ +1392,5 @@
> +                                        &NrTcpSocketIpc::update_state_s,
> +                                        NR_CLOSED),
> +                  false);
> +    MOZ_ASSERT(false, "Failed to create TCPSocketChild.");
> +  }

return;

@@ +1400,5 @@
> +                                        local_addr, local_port,
> +                                        /* use ssl */ false);
> +}
> +
> +void NrTcpSocketIpc::write_m(InfallibleTArray<uint8_t>* arr,

The ownership semantics here would be more clear if this took the nsAutoPtr passed by the dispatch, and used |SendData| (ie; passed a reference).

@@ +1424,5 @@
> +  if (tracking_number != tracking_number_) {
> +    return;
> +  }
> +
> +  buffered_byte_ = buffered_amount;

So, if we buffer multiple messages, we only update the current buffer size (and fire the writeable callback) once the last buffered message is being sent? This seems wrong.

@@ +1428,5 @@
> +  buffered_byte_ = buffered_amount;
> +  maybe_post_socket_ready();
> +}
> +
> +void NrTcpSocketIpc::recv_message_s(nr_tcp_message *msg) {

Ownership semantics would be more clear if this took an nsRefPtr<>&

@@ +1438,5 @@
> +void NrTcpSocketIpc::update_state_s(NrSocketIpcState next_state) {
> +  ASSERT_ON_THREAD(sts_thread_);
> +  if (state_ == NR_CONNECTING) {
> +    if (next_state == NR_CONNECTED) {
> +      state_ = NR_CONNECTED;

Shouldn't we update this for things like NR_CLOSING/NR_CLOSED?

@@ +1518,5 @@
> +        break;
> +      case IPPROTO_TCP:
> +        sock = new NrTcpSocketIpc(main_thread.get());
> +        break;
> +    }

Probably couldn't hurt to have a default case here with a MOZ_ASSERT/ABORT.

::: media/mtransport/nr_socket_prsock.h
@@ +193,1 @@
>                      public nsIUDPSocketInternal {

Fixup indentation.

@@ +241,5 @@
>    ReentrantMonitor monitor_;
>  };
>  
> +struct nr_tcp_message {
> +  nr_tcp_message(nsAutoPtr<DataBuffer> &data)

explicit

@@ +248,5 @@
> +  }
> +
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(nr_tcp_message);
> +
> +  uint8_t *reading_pointer() {

Let's make this const uint8_t* reading_pointer() const

@@ +252,5 @@
> +  uint8_t *reading_pointer() {
> +    return data->data() + read_byte;
> +  }
> +
> +  size_t unread_byte() {

Should probably be called |unread_bytes|, and const.

@@ +256,5 @@
> +  size_t unread_byte() {
> +    return data->len() - read_byte;
> +  }
> +
> +  size_t read_byte;

read_bytes. Also, you're almost all the way to encapsulating this and |data|, why not go the rest of the way?

@@ +280,5 @@
> +  NS_DECL_THREADSAFE_ISUPPORTS
> +  NS_DECL_NSITCPSOCKETINTERNAL
> +  NS_DECL_NSITCPSOCKETINTERNALNATIVE
> +
> +  NrTcpSocketIpc(const nsCOMPtr<nsIEventTarget> &main_thread);

explicit

@@ +293,5 @@
> +  virtual int getaddr(nr_transport_addr *addrp);
> +  virtual void close();
> +  virtual int connect(nr_transport_addr *addr);
> +  virtual int write(const void *msg, size_t len, size_t *written);
> +  virtual int read(void* buf, size_t maxlen, size_t *len);

Let's put MOZ_OVERRIDE on these.

@@ +318,5 @@
> +
> +  // variables that can only be accessed on STS.
> +  NrSocketIpcState state_;
> +  std::queue<RefPtr<nr_tcp_message> > msg_queue_;
> +  uint32_t buffered_byte_;

buffered_bytes_

Comment 36

[Honza Bambas (:mayhemer)]

Patrick, sorry for such a long delay to take a look at the patch.  maybe it's also because of one thing - I'm not sure what it is about.  Can you please say few words what you are doing?  What is nr_socket?

Comment 37

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Sure. nr_socket is the interface that WebRTC uses for accessing network resource. In single process model, it uses a adapter nr_socket_prsock to interact with PRSocket and attach the FD to socket transport service. Because the e10s design in WebRTC puts all network stack in child, it needs to expose socket to child (with a packet filter to make sure malicious data won't be sent across processes). The UDP part has already done in bug 869869, and I am implementing to TCP part in this bug.

Since the adapter, nr_socket_prsock, needs read/write functions, I use TCPSocket's IPC protocol to transmit the data across processes, and since it needs to bind to a specific local address and bind() is not exposed to javascript, I add another function in TCPSocketInternal interface to initialise TCPSocket with an nsSocketTransport created in C++ code.

The packet filter for TCP socket haven't been done yet, I'll post the patch here when I finished it.

Comment 38

[Honza Bambas (:mayhemer)]

Comment on attachment 8545023 [details] [diff] [review]
Patch Part: 1 - TCPSocket change.

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

I'm always lost in this code...  Please add comments, when anything is not finished in the patch, please mark it or explain in a bz comment.  It's hard to orient in this...

::: dom/network/TCPSocket.js
@@ +623,5 @@
> +   * Open input/output stream for the socket transport.
> +   */
> +  openWithSocketTransport: function(host, port,
> +                                    socketTransport,
> +                                    useSSL, binaryType) {

useSSL unused...

::: dom/network/TCPSocketParent.cpp
@@ +214,5 @@
> +  nsCOMPtr<nsISocketTransportService> sts =
> +    do_GetService("@mozilla.org/network/socket-transport-service;1", &rv);
> +  if (NS_FAILED(rv)) {
> +    FireInteralError(this, __LINE__);
> +  }

any return?

@@ +217,5 @@
> +    FireInteralError(this, __LINE__);
> +  }
> +
> +  nsCOMPtr<nsISocketTransport> socketTransport;
> +  rv = sts->CreateTransport(nullptr, 0,

how is the aUseSSL arg used?  why aren't you creating the socket with ssl here?

@@ +222,5 @@
> +                            aRemoteHost, aRemotePort,
> +                            nullptr, getter_AddRefs(socketTransport));
> +  if (NS_FAILED(rv)) {
> +    FireInteralError(this, __LINE__);
> +  }

here as well..

@@ +239,5 @@
> +  PRNetAddrToNetAddr(&prAddr, &addr);
> +  rv = socketTransport->Bind(&addr);
> +  if (NS_FAILED(rv)) {
> +    FireInteralError(this, __LINE__);
> +  }

again

::: dom/network/TCPSocketParentIntermediary.js
@@ +58,5 @@
> +                                    socketTransport,
> +                                    useSSL, binaryType, appId) {
> +    let baseSocket = Cc["@mozilla.org/tcp-socket;1"].createInstance(Ci.nsITCPSocketInternal);
> +
> +    let socket = baseSocket.openWithExistSocketTransport(host, port,

shouldn't this be "openWithSocketTransport" ?

::: dom/network/interfaces/nsIDOMTCPSocket.idl
@@ +309,1 @@
>  };

better comments please.  should be doxygen actually, but it's too late for this file :(((

::: dom/network/interfaces/nsITCPSocketChild.idl
@@ +35,5 @@
> +  [noscript]
> +  void sendWindowlessOpenBind(in nsITCPSocketInternal socket,
> +                              in AUTF8String remoteHost, in unsigned short remotePort,
> +                              in AUTF8String localHost, in unsigned short localPort,
> +                              in boolean ssl);

needs iid change.

and of course comments.  who is the caller?  how is this used?  where it ends?

::: dom/network/interfaces/nsITCPSocketParent.idl
@@ +12,5 @@
>  
>  // Interface required to allow the TCP socket object (TCPSocket.js) in the
>  // parent process to talk to the parent IPC actor, TCPSocketParent, which
>  // is written in C++.
> +[scriptable, uuid(c434224a-dbb7-4869-8b2b-e49cee990e85)]

why this uuid change?

@@ +87,5 @@
>    void onRecvSendArrayBuffer(in jsval data, in uint32_t trackingNumber);
> +
> +  // Connect existing socket transport to the TCPSocket object.
> +  // @param socketTransport
> +  //        The socket transport object that will back the TCPSocket.

who is the caller?  how is this used?  more rich comments please.

@@ +92,5 @@
> +  nsIDOMTCPSocket openWithSocketTransport(in nsITCPSocketParent parent,
> +                                          in AUTF8String host,
> +                                          in unsigned short port,
> +                                          in nsISocketTransport socketTransport,
> +                                          in boolean useSSL,

host and port are readable from socketTransport.  useSSL can IMO be set as useSSL = !!socketTransport.securityInfo.  Please verify that, if so, remove the args.

Comment 39

[Maire Reavy [:mreavy]]

Hi Patrick -- Can you provide us a update on where the patches in this bug stand?  

e10s will be getting enabled by default in Firefox Desktop very soon, and we are also looking to land Bug 891551 soon. Thank you!

Comment 40

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #39)
> Hi Patrick -- Can you provide us a update on where the patches in this bug
> stand?  
> 
> e10s will be getting enabled by default in Firefox Desktop very soon, and we
> are also looking to land Bug 891551 soon. Thank you!

Sorry for delaying the bug landing. I'm recently working on other B2G blockers and am not very sure when I can be back with this. If we have to land this soon, we'll probably need someone else to continue the work.

Comment 41

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

This is very closely related to the work I've been doing on UDPSocket & PBackground for mtransport; taking this bug to finish polishing the patches and get it landed.

Comment 43

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

Attachment: Part 1 interdiffs

Comment 44

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

Attachment: Patch 2 interdiffs

Comment 45

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

Attachment: Add stubs for listen() and accept() in NrTcpSocketIpc, overrides

Comment 46

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

Attachment: interdiffs from bwc's review

Comment 47

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

Attachment: Improve use of TCPSocket to track in-flight writes and suppress extra runnables

Adds lots of logging to nr_socket_buffered_stun.c

Comment 48

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

Attachment: Part 1: Support bind in content process

Merges original Part 1 with interdiffs in https://bugzilla.mozilla.org/attachment.cgi?id=8628145

Comment 49

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

Attachment: [PATCH 2/2] : Part 2: Bridge TCPSocketChild to nr_socket

Merges original Part 2 with interdiffs in https://bugzilla.mozilla.org/attachment.cgi?id=8628146

Comment 50

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

Comment on attachment 8628147 [details] [diff] [review]
Add stubs for listen() and accept() in NrTcpSocketIpc, overrides

Will be merged with Part 2 for landing

Comment 51

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

Comment on attachment 8628148 [details] [diff] [review]
interdiffs from bwc's review

This is just the changes requested due to Byron's old review

Will be merged with part 2 for landing

Comment 52

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

Comment on attachment 8628149 [details] [diff] [review]
Improve use of TCPSocket to track in-flight writes and suppress extra runnables

With this it works, though it seems to over-buffer still, perhaps due to hitting MainThread on both content and master sides...

A follow-on for better performance in e10s will be to rewrite TCPSocket.js in C++, and then move at least this usage to PBackground.  (Bug to be filed)

Comment 53

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

Note: you may well want to review from the interdiffs; this is one place where splinter does a much worse job unless you explicitly upload interdiffs (which I have here).

Comment 54

[Josh Matthews [:jdm]]

(In reply to Randell Jesup [:jesup] from comment #52)
> Comment on attachment 8628149 [details] [diff] [review]
> Improve use of TCPSocket to track in-flight writes and suppress extra
> runnables
> 
> With this it works, though it seems to over-buffer still, perhaps due to
> hitting MainThread on both content and master sides...
> 
> A follow-on for better performance in e10s will be to rewrite TCPSocket.js
> in C++, and then move at least this usage to PBackground.  (Bug to be filed)

Bug 885982.

Comment 55

[Byron Campen [:bwc]]

Comment on attachment 8628147 [details] [diff] [review]
Add stubs for listen() and accept() in NrTcpSocketIpc, overrides

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

r+ with minor fixes.

::: media/mtransport/nr_socket_prsock.cpp
@@ +1751,5 @@
>   abort:
>    return _status;
>  }
>  
> +int NrTcpSocketIpc::listen(int backlog) {

We need this (and |accept|) for ICE TCP. We can do this in a separate bug, just file and comment here. (We disable ICE TCP if on e10s already, we would just have to ensure we continue to do so until these functions are ready to go)

::: media/mtransport/nr_socket_prsock.h
@@ +328,5 @@
>  
>    explicit NrTcpSocketIpc(nsIThread* aThread);
>  
>    // Implementations of the NrSocketBase APIs
> +  virtual int create(nr_transport_addr *addr) override;

The latest code convention says that we should not use both virtual and override (although I think this rule is a bit silly).

Comment 56

[Byron Campen [:bwc]]

Comment on attachment 8628152 [details] [diff] [review]
[PATCH 2/2] : Part 2: Bridge TCPSocketChild to nr_socket

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

It looks like this hasn't changed since the last review, and all the updates are in the interdiff?

Comment 57

[Byron Campen [:bwc]]

Comment on attachment 8628152 [details] [diff] [review]
[PATCH 2/2] : Part 2: Bridge TCPSocketChild to nr_socket

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

It looks like this has some (but not all) of my review feedback in it. Some more (but still not all) of what remains is in the interdiff. I've pointed out the stuff that hasn't been addressed yet.

::: media/mtransport/nr_socket_prsock.cpp
@@ +1552,5 @@
> +}
> +
> +NS_IMETHODIMP NrTcpSocketIpc::CallListenerData(const nsAString &type,
> +                                               const nsAString &data) {
> +  return NS_OK;

Still not sure why this doesn't return NS_ERROR_NOT_IMPLEMENTED

@@ +1724,5 @@
> + abort:
> +  return _status;
> +}
> +
> +int NrTcpSocketIpc::read(void* buf, size_t maxlen, size_t *len) { 

Trailing ws not fixed in interdiff.

@@ +1812,5 @@
> +  if (tracking_number != tracking_number_) {
> +    return;
> +  }
> +
> +  buffered_byte_ = buffered_amount;

I did not see the following question addressed in the interdiff or the ticket:

So, if we buffer multiple messages, we only update the current buffer size (and fire the writeable callback) once the last buffered message is being sent? This seems wrong.

@@ +1826,5 @@
> +void NrTcpSocketIpc::update_state_s(NrSocketIpcState next_state) {
> +  ASSERT_ON_THREAD(sts_thread_);
> +  if (state_ == NR_CONNECTING) {
> +    if (next_state == NR_CONNECTED) {
> +      state_ = NR_CONNECTED;

I did not see the following question addressed in the interdiff or the ticket:

Shouldn't we update this for things like NR_CLOSING/NR_CLOSED?

::: media/mtransport/nr_socket_prsock.h
@@ +302,5 @@
> +  }
> +
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(nr_tcp_message);
> +
> +  uint8_t *reading_pointer() {

Let's make this const uint8_t* reading_pointer() const

@@ +306,5 @@
> +  uint8_t *reading_pointer() {
> +    return data->data() + read_byte;
> +  }
> +
> +  size_t unread_byte() {

Should probably be called |unread_bytes|, and const.

@@ +310,5 @@
> +  size_t unread_byte() {
> +    return data->len() - read_byte;
> +  }
> +
> +  size_t read_byte;

read_bytes

@@ +311,5 @@
> +    return data->len() - read_byte;
> +  }
> +
> +  size_t read_byte;
> +  nsAutoPtr<DataBuffer> data;

Does anything use |data| directly? Can we encapsulate?

@@ +367,5 @@
> +
> +  // variables that can only be accessed on STS.
> +  NrSocketIpcState state_;
> +  std::queue<RefPtr<nr_tcp_message>> msg_queue_;
> +  uint32_t buffered_byte_;

buffered_bytes_

Comment 58

[Byron Campen [:bwc]]

Comment on attachment 8628148 [details] [diff] [review]
interdiffs from bwc's review

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

r+ with fixes

::: media/mtransport/nr_socket_prsock.cpp
@@ +1545,1 @@
>    

Trailing ws.

@@ +1708,5 @@
>    if (state_ != NR_CONNECTED) {
>      ABORT(R_FAILED);
>    }
>  
> +  if (buffered_byte_ + len >= nsITCPSocketInternal::BUFFER_SIZE) {

If it equals the buffer size, we're ok right?

Comment 59

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

> > A follow-on for better performance in e10s will be to rewrite TCPSocket.js
> > in C++, and then move at least this usage to PBackground.  (Bug to be filed)
> 
> Bug 885982.

Thanks!  That could make life simpler

Comment 60

[Byron Campen [:bwc]]

Comment on attachment 8628149 [details] [diff] [review]
Improve use of TCPSocket to track in-flight writes and suppress extra runnables

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

I'll probably need to see this again for another pass. It seems that you tripped over bug 1035428, and I'll get that landed today (if the trees reopen).

::: media/mtransport/nr_socket_prsock.cpp
@@ +1661,5 @@
>    if (state_ == NR_CLOSED || state_ == NR_CLOSING) {
>      return;
>    }
>  
> +  state_ = mirror_state_ = NR_CLOSING;

Hang on, we're setting mirror_state_ from STS too? This seems dangerous to me. Is this an optimization that is really worth the complexity?

@@ +1832,5 @@
>  void NrTcpSocketIpc::message_sent_s(uint32_t buffered_amount,
>                                      uint32_t tracking_number) {
>    ASSERT_ON_THREAD(sts_thread_);
>    if (tracking_number != tracking_number_) {
> +    for (; writes_in_flight_acked_ < tracking_number;

We don't need |writes_in_flight_acked_|. We can get the count of unacked writes from tracking_number_ - tracking_number, and pop off of writes_in_flight until the size matches. Then we also don't need the MOZ_ASSERT below, or |sz|. We also don't need the if/else that wraps all this. I'm pretty sure the following does the job (modulo logging):

ASSERT_ON_THREAD(sts_thread_);

size_t num_unacked_writes = tracking_number_ - tracking_number;
while (writes_in_flight.size() > num_unacked_writes) {
   writes_in_flight.pop_front();
}

for (size_t unacked_write_len : writes_in_flight) {
   buffered_amount += unacked_write_len;
}

buffered_bytes_ = buffered_amount;
maybe_post_socket_ready();

@@ +1846,2 @@
>      r_log(LOG_GENERIC, LOG_ERR,
> +          "UpdateBufferedAmount: (tracking numbers don't match: %u:%u) %u (%u), waiting: %s",

printf pedantry: uint32_t doesn't necessarily match the width of unsigned, here and in other places.

@@ +1865,3 @@
>    }
> +  r_log(LOG_GENERIC, LOG_ERR,
> +        "UpdateBufferedAmount: (tracking %u): %u, waiting: %s",

<insert printf pedantry here>

@@ +1896,5 @@
>      if (poll_flags() & PR_POLL_WRITE) {
> +      // This effectively polls via the event loop until the
> +      // NR_ASYNC_WAIT_WRITE is no longer armed.  It stops if the TCP
> +      // buffer has more than a small amount of data, and resumes when the
> +      // TCP buffer nears empty (triggered by UpdateBufferedAmount())

The second sentence in this comment is very confusing. The code simply fires the writeable callback as soon as there is any room in the TCP buffer, but the comment makes it sound like there is some fuzzy logic here.

@@ +1901,2 @@
>        if (buffered_byte_ < nsITCPSocketInternal::BUFFER_SIZE) {
> +        r_log(LOG_GENERIC, LOG_INFO, "Firing write callback (%u)",

<insert printf pedantry here>

@@ +1906,5 @@
>        }
>      }
>      if (poll_flags() & PR_POLL_READ) {
>        if (msg_queue_.size()) {
> +        r_log(LOG_GENERIC, LOG_INFO, "Firing read callback (%u)",

<insert printf pedantry here>

::: media/mtransport/third_party/nICEr/src/stun/nr_socket_buffered_stun.c
@@ +374,5 @@
>    assert(!sock->connected);
>  
>    sock->connected = 1;
> +  if (sock->pending) {
> +    r_log(LOG_GENERIC, LOG_INFO, "Invoking writable_cb on connected (%u)", (uint32_t) sock->pending);

<insert printf pedantry here>

@@ +419,5 @@
>  
>    /* Buffers are close to full, report error. Do this now so we never
>       get partial writes */
>    if ((sock->pending + len) > sock->max_pending) {
> +    r_log(LOG_GENERIC, LOG_INFO, "Write buffer for %s full (%u + %u > %u) - re-arming @%p",

<insert printf pedantry here>

@@ +424,5 @@
> +          sock->remote_addr.as_string, (uint32_t)sock->pending, (uint32_t)len, (uint32_t)sock->max_pending,
> +          &(sock->pending));
> +
> +    {
> +      /* Re-arm just in case */

Maybe the upper layer wasn't keeping this going due to bug 1035428?

@@ +446,3 @@
>          ABORT(r);
> +      }
> +      r_log(LOG_GENERIC, LOG_INFO, "Write of %d blocked for %s", len,

printf pedantry: We shouldn't be using %d with a size_t

@@ +476,5 @@
>        ABORT(r);
>  
>      NR_ASYNC_WAIT(fd, NR_ASYNC_WAIT_WRITE, nr_socket_buffered_stun_writable_cb, sock);
>    }
> +  r_log(LOG_GENERIC, LOG_INFO, "Write buffer not empty for %s  %u - %s armed (@%p)",

<insert printf pedantry here>

@@ +512,5 @@
>      sock->pending -= written;
>  
>      if (n1->r_offset < n1->length) {
>        /* We wrote something, but not everything */
> +      r_log(LOG_GENERIC, LOG_INFO, "Write in callback didn't write all (remaining %d of %d) for %s",

%d isn't what you want for UINT4.

@@ +527,5 @@
>    assert(!sock->pending);
>    _status=0;
>  abort:
> +  r_log(LOG_GENERIC, LOG_INFO, "Writable_cb %s (%u (%p) pending)",
> +        sock->remote_addr.as_string, (uint32_t)sock->pending, &(sock->pending));

Why are we logging a pointer here? Also, %u doesn't necessarily match uint32_t.

@@ +530,5 @@
> +  r_log(LOG_GENERIC, LOG_INFO, "Writable_cb %s (%u (%p) pending)",
> +        sock->remote_addr.as_string, (uint32_t)sock->pending, &(sock->pending));
> +  if (sock->pending) {
> +    NR_ASYNC_WAIT(s, NR_ASYNC_WAIT_WRITE, nr_socket_buffered_stun_writable_cb, sock);
> +  }

Ok, this seems to be trying to do the same thing as the patch on bug 1035428. Let's remove this, and I will get that patch checked in as soon as the trees reopen.

Comment 61

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

> [PATCH 2/2] : Part 2: Bridge TCPSocketChild to nr_socket

> It looks like this has some (but not all) of my review feedback in it. Some
> more (but still not all) of what remains is in the interdiff. I've pointed
> out the stuff that hasn't been addressed yet.

Thanks.  This *should* just be the original patch + the interdiffs to unbitrot (attachment 8628146 [details] [diff] [review]), before I started on the response to your review of patrick's patch.

> ::: media/mtransport/nr_socket_prsock.cpp
> @@ +1552,5 @@
> > +}
> > +
> > +NS_IMETHODIMP NrTcpSocketIpc::CallListenerData(const nsAString &type,
> > +                                               const nsAString &data) {
> > +  return NS_OK;
> 
> Still not sure why this doesn't return NS_ERROR_NOT_IMPLEMENTED

Done

> 
> @@ +1724,5 @@
> > + abort:
> > +  return _status;
> > +}
> > +
> > +int NrTcpSocketIpc::read(void* buf, size_t maxlen, size_t *len) { 
> 
> Trailing ws not fixed in interdiff.

Done (globally to the file)

> 
> @@ +1812,5 @@
> > +  if (tracking_number != tracking_number_) {
> > +    return;
> > +  }
> > +
> > +  buffered_byte_ = buffered_amount;
> 
> I did not see the following question addressed in the interdiff or the
> ticket:
> 
> So, if we buffer multiple messages, we only update the current buffer size
> (and fire the writeable callback) once the last buffered message is being
> sent? This seems wrong.

This is addressed (and more so, to include in-flight data) in the code in the Improvements patch (attachment 8628149 [details] [diff] [review])

> 
> @@ +1826,5 @@
> > +void NrTcpSocketIpc::update_state_s(NrSocketIpcState next_state) {
> > +  ASSERT_ON_THREAD(sts_thread_);
> > +  if (state_ == NR_CONNECTING) {
> > +    if (next_state == NR_CONNECTED) {
> > +      state_ = NR_CONNECTED;
> 
> I did not see the following question addressed in the interdiff or the
> ticket:
> 
> Shouldn't we update this for things like NR_CLOSING/NR_CLOSED?

maybe_post_socket_ready() does nothing except in the CONNECTED case, so I don't think this needs updating.  (unless you think it should do something on a transition to CLOSING/CLOSED.)

> ::: media/mtransport/nr_socket_prsock.h
> @@ +302,5 @@
> > +  }
> > +
> > +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(nr_tcp_message);
> > +
> > +  uint8_t *reading_pointer() {
> 
> Let's make this const uint8_t* reading_pointer() const

Done. (ditto for the other bits in this section)
 
> @@ +311,5 @@
> > +    return data->len() - read_byte;
> > +  }
> > +
> > +  size_t read_byte;
> > +  nsAutoPtr<DataBuffer> data;
> 
> Does anything use |data| directly? Can we encapsulate?

Not for Tcp, so made it private.

> 
> @@ +367,5 @@
> > +
> > +  // variables that can only be accessed on STS.
> > +  NrSocketIpcState state_;
> > +  std::queue<RefPtr<nr_tcp_message>> msg_queue_;
> > +  uint32_t buffered_byte_;
> 
> buffered_bytes_

done

Comment 62

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

> I'll probably need to see this again for another pass. It seems that you
> tripped over bug 1035428, and I'll get that landed today (if the trees
> reopen).

Aha!  yeah, I hadn't realized that was a general bug and not specific to this patchset.

> 
> ::: media/mtransport/nr_socket_prsock.cpp
> @@ +1661,5 @@
> >    if (state_ == NR_CLOSED || state_ == NR_CLOSING) {
> >      return;
> >    }
> >  
> > +  state_ = mirror_state_ = NR_CLOSING;
> 
> Hang on, we're setting mirror_state_ from STS too? This seems dangerous to
> me. Is this an optimization that is really worth the complexity?

Maybe not.  let me see.... I was trying to avoid constantly locking, in particular to avoid locking during normal operation (I don't care when there are actual state changes, it's the on-every-packet locking I want to avoid).  Likely there's another way to deal with it.

> 
> @@ +1832,5 @@
> >  void NrTcpSocketIpc::message_sent_s(uint32_t buffered_amount,
> >                                      uint32_t tracking_number) {
> >    ASSERT_ON_THREAD(sts_thread_);
> >    if (tracking_number != tracking_number_) {
> > +    for (; writes_in_flight_acked_ < tracking_number;
> 
> We don't need |writes_in_flight_acked_|. We can get the count of unacked
> writes from tracking_number_ - tracking_number, and pop off of
> writes_in_flight until the size matches. Then we also don't need the
> MOZ_ASSERT below, or |sz|. We also don't need the if/else that wraps all
> this. I'm pretty sure the following does the job (modulo logging):

Yeah, much simpler, thanks.  Dangers of iterative development.
 
> @@ +1846,2 @@
> >      r_log(LOG_GENERIC, LOG_ERR,
> > +          "UpdateBufferedAmount: (tracking numbers don't match: %u:%u) %u (%u), waiting: %s",
> 
> printf pedantry: uint32_t doesn't necessarily match the width of unsigned,
> here and in other places.

Barring annoyance of doing "foo=" PRIu32 " blah blah" everywhere in the tree, since we demand (in all sorts of places) that sizeof(int) >= sizeof(int32_t) (and same for unsigned/uint32_t), we can assume %u can handle uint32_t.  No one is going to build mozilla for 16-bit ints.... ;-)  And we have all of 3 uses of PRIu32 in the tree, and one is to test if it works.

PRIu64/etc is useful for printing uint64_t's in a safe-from-warnings way.  We have 107 of those.

> @@ +1896,5 @@
> >      if (poll_flags() & PR_POLL_WRITE) {
> > +      // This effectively polls via the event loop until the
> > +      // NR_ASYNC_WAIT_WRITE is no longer armed.  It stops if the TCP
> > +      // buffer has more than a small amount of data, and resumes when the
> > +      // TCP buffer nears empty (triggered by UpdateBufferedAmount())
> 
> The second sentence in this comment is very confusing. The code simply fires
> the writeable callback as soon as there is any room in the TCP buffer, but
> the comment makes it sound like there is some fuzzy logic here. 

I'll rewrite for clarity.  I needed to write stuff down to ensure I understood what was happening given no docs on what it was doing or why.

> @@ +424,5 @@
> > +          sock->remote_addr.as_string, (uint32_t)sock->pending, (uint32_t)len, (uint32_t)sock->max_pending,
> > +          &(sock->pending));
> > +
> > +    {
> > +      /* Re-arm just in case */
> 
> Maybe the upper layer wasn't keeping this going due to bug 1035428?

That may well be the case.  It was freezing on me; this may be overkill.
 
> @@ +446,3 @@
> >          ABORT(r);
> > +      }
> > +      r_log(LOG_GENERIC, LOG_INFO, "Write of %d blocked for %s", len,
> 
> printf pedantry: We shouldn't be using %d with a size_t

True!  That generally is PRIu64 (and cast to uint64_t).  There's no standard formatting for size_t...  To be ultra-pedantic, one can cast to uintmax_t and use PRIuMAX.  There are 0 uses of this in the tree, though, and lots of size_t's printed.

> @@ +512,5 @@
> >      sock->pending -= written;
> >  
> >      if (n1->r_offset < n1->length) {
> >        /* We wrote something, but not everything */
> > +      r_log(LOG_GENERIC, LOG_INFO, "Write in callback didn't write all (remaining %d of %d) for %s",
> 
> %d isn't what you want for UINT4.

Right.

> 
> @@ +527,5 @@
> >    assert(!sock->pending);
> >    _status=0;
> >  abort:
> > +  r_log(LOG_GENERIC, LOG_INFO, "Writable_cb %s (%u (%p) pending)",
> > +        sock->remote_addr.as_string, (uint32_t)sock->pending, &(sock->pending));
> 
> Why are we logging a pointer here? Also, %u doesn't necessarily match
> uint32_t.

pointer: So I can separate out the different TCP-TURN streams in the logs.

> 
> @@ +530,5 @@
> > +  r_log(LOG_GENERIC, LOG_INFO, "Writable_cb %s (%u (%p) pending)",
> > +        sock->remote_addr.as_string, (uint32_t)sock->pending, &(sock->pending));
> > +  if (sock->pending) {
> > +    NR_ASYNC_WAIT(s, NR_ASYNC_WAIT_WRITE, nr_socket_buffered_stun_writable_cb, sock);
> > +  }
> 
> Ok, this seems to be trying to do the same thing as the patch on bug
> 1035428. Let's remove this, and I will get that patch checked in as soon as
> the trees reopen.

Cool!

Comment 63

[Byron Campen [:bwc]]

(In reply to Randell Jesup [:jesup] from comment #62)
> > @@ +1846,2 @@
> > >      r_log(LOG_GENERIC, LOG_ERR,
> > > +          "UpdateBufferedAmount: (tracking numbers don't match: %u:%u) %u (%u), waiting: %s",
> > 
> > printf pedantry: uint32_t doesn't necessarily match the width of unsigned,
> > here and in other places.
> 
> Barring annoyance of doing "foo=" PRIu32 " blah blah" everywhere in the
> tree, since we demand (in all sorts of places) that sizeof(int) >=
> sizeof(int32_t) (and same for unsigned/uint32_t), we can assume %u can
> handle uint32_t.  No one is going to build mozilla for 16-bit ints.... ;-) 
> And we have all of 3 uses of PRIu32 in the tree, and one is to test if it
> works.
> 
> PRIu64/etc is useful for printing uint64_t's in a safe-from-warnings way. 
> We have 107 of those.
> 

     Actually, I'm far less concerned with overflow (one thing that can happen if sizeof(unsigned) < sizeof(uint32_t)) than I am with offset problems when reading varargs off the stack. Depending on how stuff is packed, you might be ok if the sizes of the types don't match, but you might not, and you can get all kinds of nasty buffer overrun bugs if you mess up. I always explicitly cast to exactly what the format string expects.

Comment 64

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

(In reply to Byron Campen [:bwc] from comment #63)

> > And we have all of 3 uses of PRIu32 in the tree, and one is to test if it
> > works.
> > 
> > PRIu64/etc is useful for printing uint64_t's in a safe-from-warnings way. 
> > We have 107 of those.
> > 
> 
>      Actually, I'm far less concerned with overflow (one thing that can
> happen if sizeof(unsigned) < sizeof(uint32_t)) than I am with offset
> problems when reading varargs off the stack. Depending on how stuff is
> packed, you might be ok if the sizes of the types don't match, but you might
> not, and you can get all kinds of nasty buffer overrun bugs if you mess up.
> I always explicitly cast to exactly what the format string expects.

C (and C++) demand that all arguments with size < int or unsigned get promoted to that on the stack (roughly, the spec verbiage is longer ;-)).  So you can be sure uint32_t will be at minimum sizeof(unsigned), and only larger if sizeof(unsigned) < sizeof(uint32_t), which effectively is only for 16-bit-int compilers.

I personally find that unneeded (int)/(unsigned) casts makes things harder to read, and will hide an accidental case of 32-bit value changing to 64 (perhaps because of expression promotion), and the cast will silently truncate it without a warning, while leaving it will cause our compilers and static analysis to flag it as incorrect.  (Our normal builds do check these for most cases.)

If you want I'll do it, but with modern printf analysis I think it increases the chances of an error slipping through (albeit safely).

Comment 65

[Byron Campen [:bwc]]

(In reply to Randell Jesup [:jesup] from comment #64)
> (In reply to Byron Campen [:bwc] from comment #63)
> 
> > > And we have all of 3 uses of PRIu32 in the tree, and one is to test if it
> > > works.
> > > 
> > > PRIu64/etc is useful for printing uint64_t's in a safe-from-warnings way. 
> > > We have 107 of those.
> > > 
> > 
> >      Actually, I'm far less concerned with overflow (one thing that can
> > happen if sizeof(unsigned) < sizeof(uint32_t)) than I am with offset
> > problems when reading varargs off the stack. Depending on how stuff is
> > packed, you might be ok if the sizes of the types don't match, but you might
> > not, and you can get all kinds of nasty buffer overrun bugs if you mess up.
> > I always explicitly cast to exactly what the format string expects.
> 
> C (and C++) demand that all arguments with size < int or unsigned get
> promoted to that on the stack (roughly, the spec verbiage is longer ;-)). 
> So you can be sure uint32_t will be at minimum sizeof(unsigned), and only
> larger if sizeof(unsigned) < sizeof(uint32_t), which effectively is only for
> 16-bit-int compilers.
> 
> I personally find that unneeded (int)/(unsigned) casts makes things harder
> to read, and will hide an accidental case of 32-bit value changing to 64
> (perhaps because of expression promotion), and the cast will silently
> truncate it without a warning, while leaving it will cause our compilers and
> static analysis to flag it as incorrect.  (Our normal builds do check these
> for most cases.)
> 
> If you want I'll do it, but with modern printf analysis I think it increases
> the chances of an error slipping through (albeit safely).

   Ok, for this case we're fine.

Comment 66

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

Attachment: Tweak buffered_stun unit test to allow re-arming of callbacks

this is due to the (perhaps now overly-paranoid) rearming on full-buffer done in nr_socket_buffered_stun_write()

Comment 67

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

Comment on attachment 8628918 [details] [diff] [review]
Tweak buffered_stun unit test to allow re-arming of callbacks

Actually I think we may not need the re-arming, checking

Comment 68

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

Attachment: interdiffs from second review of patch 2

Comment 69

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

Attachment: Improve use of TCPSocket to track in-flight writes and suppress extra runnables

with interdiffs folded in

Comment 70

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

Comment on attachment 8628947 [details] [diff] [review]
interdiffs from second review of patch 2

Actually just review these interdiffs due to patch ordering

Comment 71

[Byron Campen [:bwc]]

Comment on attachment 8628947 [details] [diff] [review]
interdiffs from second review of patch 2

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

I'm still not thrilled about mirror_state; how many dispatches is this going to save us on average (it seems like this should rarely happen, but maybe I'm mistaken?), on a class that needs to do a dispatch for every packet sent or received, and that needs a minimum of 5 or so for basic setup/teardown?

(In reply to Randell Jesup [:jesup] from comment #62)
> > @@ +527,5 @@
> > >    assert(!sock->pending);
> > >    _status=0;
> > >  abort:
> > > +  r_log(LOG_GENERIC, LOG_INFO, "Writable_cb %s (%u (%p) pending)",
> > > +        sock->remote_addr.as_string, (uint32_t)sock->pending, &(sock->pending));
> > 
> > Why are we logging a pointer here? Also, %u doesn't necessarily match
> > uint32_t.
> 
> pointer: So I can separate out the different TCP-TURN streams in the logs.

Ok, then I would suggest something like "Writable_cb %s (sockp=%p), %u pending", and log |sock| instead of &(sock->pending).

::: media/mtransport/nr_socket_prsock.cpp
@@ +1861,5 @@
>  }
>  
>  void NrTcpSocketIpc::update_state_s(NrSocketIpcState next_state) {
>    ASSERT_ON_THREAD(sts_thread_);
> +  // only allow valid transitions

We should probably assert or crash on invalid transitions.

Comment 72

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

(In reply to Byron Campen [:bwc] from comment #71)
> Comment on attachment 8628947 [details] [diff] [review]
> interdiffs from second review of patch 2
> 
> Review of attachment 8628947 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm still not thrilled about mirror_state; how many dispatches is this going
> to save us on average (it seems like this should rarely happen, but maybe
> I'm mistaken?), on a class that needs to do a dispatch for every packet sent
> or received, and that needs a minimum of 5 or so for basic setup/teardown?

It's one dispatch per packet (i.e. many hundreds per second) - every incoming packet throws a runnable to update the state, even if it hasn't changed, unless I can track the state in the io_thread.  I have a version that locks all over the place (not tested yet) to avoid mirror_state.  Larger re-engineering might let us ignore state issues, or figure out some safe invariants.  If we could ignore the state in STSThread, we could just maintain it in the io_thread alone.

> (In reply to Randell Jesup [:jesup] from comment #62)
> > > @@ +527,5 @@
> > > >    assert(!sock->pending);
> > > >    _status=0;
> > > >  abort:
> > > > +  r_log(LOG_GENERIC, LOG_INFO, "Writable_cb %s (%u (%p) pending)",
> > > > +        sock->remote_addr.as_string, (uint32_t)sock->pending, &(sock->pending));
> > > 
> > > Why are we logging a pointer here? Also, %u doesn't necessarily match
> > > uint32_t.
> > 
> > pointer: So I can separate out the different TCP-TURN streams in the logs.
> 
> Ok, then I would suggest something like "Writable_cb %s (sockp=%p), %u
> pending", and log |sock| instead of &(sock->pending).

Ok

> 
> ::: media/mtransport/nr_socket_prsock.cpp
> @@ +1861,5 @@
> >  }
> >  
> >  void NrTcpSocketIpc::update_state_s(NrSocketIpcState next_state) {
> >    ASSERT_ON_THREAD(sts_thread_);
> > +  // only allow valid transitions
> 
> We should probably assert or crash on invalid transitions.

Ok

Comment 73

[Byron Campen [:bwc]]

(In reply to Randell Jesup [:jesup] from comment #72)
> (In reply to Byron Campen [:bwc] from comment #71)
> > Comment on attachment 8628947 [details] [diff] [review]
> > interdiffs from second review of patch 2
> > 
> > Review of attachment 8628947 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > I'm still not thrilled about mirror_state; how many dispatches is this going
> > to save us on average (it seems like this should rarely happen, but maybe
> > I'm mistaken?), on a class that needs to do a dispatch for every packet sent
> > or received, and that needs a minimum of 5 or so for basic setup/teardown?
> 
> It's one dispatch per packet (i.e. many hundreds per second) - every
> incoming packet throws a runnable to update the state, even if it hasn't
> changed, unless I can track the state in the io_thread.

Oh wow. That is not what I was expecting. mirror_state seems worth it now.

Comment 74

[Honza Bambas (:mayhemer)]

Comment on attachment 8628150 [details] [diff] [review]
Part 1: Support bind in content process

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

This has almost didn't change since I was doing feedback on it the last time (comment 38).

Please update according my requirements and then request review again.

::: dom/network/interfaces/nsITCPSocketParent.idl
@@ +89,5 @@
>    void onRecvSendArrayBuffer(in jsval data, in uint32_t trackingNumber);
> +
> +  // Connect existing socket transport to the TCPSocket object.
> +  // @param socketTransport
> +  //        The socket transport object that will back the TCPSocket.

I asked for better comment.  This is unaddressed.  Each arg needs an explanation.  Explain how the object state changes after this.  What is referenced by the object etc...  So much to say.

Comment 75

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

> - a test is needed to at least exercise the new code

We need to test this in mozTrap/qeverify, as we have no support in mochitest automation for TURN (let alone TCP-TURN).  I'll see if there's some way to hack up a test, but for now we need to just include it (with the other TURN tests) in mozTrap (repeated manual testing), and also test in the more-real-network setup used in our 'steeplechase' webrtc lab automated tests once it's up to testing TURN.

(Unless commented on below, the other review issues mentioned should be dealt with - many by Patrick a while ago):

> ::: dom/network/TCPSocketParentIntermediary.js
> @@ +68,5 @@
> > +      return null;
> > +    }
> > +
> > +    let socketInternal = socket.QueryInterface(Ci.nsITCPSocketInternal);
> > +    socketInternal.setAppId(appId);
> 
> you should set this before opening the |socket|.

Thanks, missed that one.


> @@ +297,5 @@
> > +                                               in DOMString binaryType);
> > +};
> > +
> > +// Let IPC classes can pass native objects without having to
> > +// transform to JSObject first.
> 
> doxygen style, check phrasing

Ok.

> ::: dom/network/interfaces/nsITCPSocketParent.idl
> @@ +89,5 @@
> >    void onRecvSendArrayBuffer(in jsval data, in uint32_t trackingNumber);
> > +
> > +  // Connect existing socket transport to the TCPSocket object.
> > +  // @param socketTransport
> > +  //        The socket transport object that will back the TCPSocket.
> 
> I asked for better comment.  This is unaddressed.  Each arg needs an
> explanation.  Explain how the object state changes after this.  What is
> referenced by the object etc...  So much to say.

Ok.  It's documented almost identically to the existing open() method above (with has a 1-liner explanation and no description of parameters), with the addition of documenting the new socketTransport parameter.  I'm adding some additional comments and doxygen arg docs, though most of that code predated my involvement.

Comment 76

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

Attachment: interdiffs from Honza's review

Comment 77

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

Attachment: Part 1: Support bind in content process

rolled-up patch 1 for review; see interdiffs in separate patch

Comment 78

[Nils Ohlmeier [:drno]]

(In reply to Randell Jesup [:jesup] from comment #75)
> > - a test is needed to at least exercise the new code
> 
> We need to test this in mozTrap/qeverify, as we have no support in mochitest
> automation for TURN (let alone TCP-TURN).  I'll see if there's some way to
> hack up a test, but for now we need to just include it (with the other TURN
> tests) in mozTrap (repeated manual testing), and also test in the
> more-real-network setup used in our 'steeplechase' webrtc lab automated
> tests once it's up to testing TURN.

FYI steeplechase in the webrtc lab tests TURN and even TURN TCP already. But e10s is off in the profiles in use.

Comment 79

[Honza Bambas (:mayhemer)]

Comment on attachment 8630626 [details] [diff] [review]
interdiffs from Honza's review

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

This idiff is pretty strange...  which two patches this diffs?

Comment 80

[Honza Bambas (:mayhemer)]

Comment on attachment 8630627 [details] [diff] [review]
Part 1: Support bind in content process

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

(In reply to Honza Bambas (:mayhemer) from comment #38)
> 
> ::: dom/network/TCPSocketParent.cpp
> @@ +214,5 @@
> > +  nsCOMPtr<nsISocketTransportService> sts =
> > +    do_GetService("@mozilla.org/network/socket-transport-service;1", &rv);
> > +  if (NS_FAILED(rv)) {
> > +    FireInteralError(this, __LINE__);
> > +  }
> 
> any return?

Left unaddressed, on more places.  If it's on purpose please add a comment.

> ::: dom/network/interfaces/nsITCPSocketChild.idl
> @@ +35,5 @@
> > +  [noscript]
> > +  void sendWindowlessOpenBind(in nsITCPSocketInternal socket,
> > +                              in AUTF8String remoteHost, in unsigned short remotePort,
> > +                              in AUTF8String localHost, in unsigned short localPort,
> > +                              in boolean ssl);
> 
> needs iid change.
> 
> and of course comments.  who is the caller?  how is this used?  where it
> ends?

Completely unaddressed.  Applies to all changes to this file.




OK, this is pretty complicated.  I need to reorient myself in this whole TCP js socket IPC mess.  It will take some time to this review.  Btw, I find the comment still somewhat inefficient to understand the changes.  Also - more importantly - there never been a good and reasonably detailed explanation of this patch in a bugzilla comment!  That would speed the review up as well...

Comment 81

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

> This idiff is pretty strange...  which two patches this diffs?

The previous Part 1 (https://bugzilla.mozilla.org/attachment.cgi?id=8628150), and the one I just put up (attachment 8630627 [details] [diff] [review]).

> Review of attachment 8630627 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to Honza Bambas (:mayhemer) from comment #38)
> > 
> > ::: dom/network/TCPSocketParent.cpp
> > @@ +214,5 @@
> > > +  nsCOMPtr<nsISocketTransportService> sts =
> > > +    do_GetService("@mozilla.org/network/socket-transport-service;1", &rv);
> > > +  if (NS_FAILED(rv)) {
> > > +    FireInteralError(this, __LINE__);
> > > +  }
> > 
> > any return?
> 
> Left unaddressed, on more places.  If it's on purpose please add a comment.

Aha.  I mistakenly assumed they'd all been addressed by Patrick.  Adding "return true;" after each FireInternalError() there that doesn't have one.

> > ::: dom/network/interfaces/nsITCPSocketChild.idl
> > @@ +35,5 @@
> > > +  [noscript]
> > > +  void sendWindowlessOpenBind(in nsITCPSocketInternal socket,
> > > +                              in AUTF8String remoteHost, in unsigned short remotePort,
> > > +                              in AUTF8String localHost, in unsigned short localPort,
> > > +                              in boolean ssl);
> > 
> > needs iid change.
> > 
> > and of course comments.  who is the caller?  how is this used?  where it
> > ends?
> 
> Completely unaddressed.  Applies to all changes to this file.

Ditto - I saw he'd added comments there.  I'll change the uuid, and update comments as best I can.  It's called from media/mtransport/nr_socket_prsock.cpp (see patch 2) on MainThread (io_thread_ is MainThread for NrTcpSocketIpc until we can move TCPSocket to use PBackground).  Unlike nsIUDPSocketChild.idl, this specifies the local and remote host/ports, for use in RecvOpenBind() in the parent.

I'm at a disadvantage here in explaining every detail in this, since I came in and took over this almost-complete patchset to unbitrot, fix a few problems and land it, after he updated the patch due to your original review.  The comments on this method currently mirror (roughly, to a similar level of detail) the comments on open() right above it (which date back to the original checkin on bug 770778 by jdm).

> OK, this is pretty complicated.  I need to reorient myself in this whole TCP
> js socket IPC mess.  It will take some time to this review.  Btw, I find the
> comment still somewhat inefficient to understand the changes.  Also - more
> importantly - there never been a good and reasonably detailed explanation of
> this patch in a bugzilla comment!  That would speed the review up as well...

Basically, patch 1 is to support the equivalent to what was landed in UDPSocket long ago (for b2g and now e10s support) to allow Bind() from content (over e10s/IPC), using a specific transport, and allow using raw arrays instead of jsvals (sendSendArray()). This lets mtransport work.

This patch supports mtransport (for WebRTC), which creates encrypted links over UDP, TURN-UDP, or TURN-TCP, using ICE to find internal/external port values and prop open NAT ports (to try to avoid TURN (relay of traffic via a server == $$)).  ICE-TCP is well under development in mtransport as well and should be preffed-on soon

Comment 82

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

Attachment: interdiffs for latest Patch 1 against previous Patch 1

Comment 83

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

Attachment: Part 1: Support bind in content process

Comment 84

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

the interdiffs are from  attachment 8630627 [details] [diff] [review] to attachment 8631193 [details] [diff] [review] (the new patch up for review).

Please find my on IRC when you have a chance so we can discuss this in realtime; I hang out in #media but also am in #necko; just ping.  I'm on EDT so I have a fair bit of overlap with Europe.  Thanks!

Comment 85

[Honza Bambas (:mayhemer)]

(In reply to Randell Jesup [:jesup] from comment #83)
> Created attachment 8631193 [details] [diff] [review]
> Part 1: Support bind in content process

And again, absolutely no word of any explanation...

Comment 86

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

(In reply to Honza Bambas (:mayhemer) from comment #85)
> (In reply to Randell Jesup [:jesup] from comment #83)
> > Created attachment 8631193 [details] [diff] [review]
> > Part 1: Support bind in content process
> 
> And again, absolutely no word of any explanation...

See comment 81, please also find my on irc if possible.

Comment 87

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

> See comment 81, please also find my on irc if possible.

To speed things up (and hopefully avoid more review cycles), what would help me is to indicate what's missing - comments in the code, in the idl files, parameter documentation, or description here in the bug about what this is for and why (beyond what's covered in comment 81).  I'd love to resolve your concerns, but I'm a bit confused about exactly what they are at this point.

I do apologize for not vetting Patrick's old update after initial review to ensure it covered every point made.

Thanks!

Comment 88

[Honza Bambas (:mayhemer)]

(In reply to Randell Jesup [:jesup] from comment #83)
> Created attachment 8631193 [details] [diff] [review]
> Part 1: Support bind in content process

I'll start with the review tomorrow.

Comment 89

[Honza Bambas (:mayhemer)]

Comment on attachment 8631193 [details] [diff] [review]
Part 1: Support bind in content process

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

None of the patches in this bug is using this new functionality.  I don't see any "openBind()" exposed on nsIDOMTCPSocket (intended?)

::: dom/network/TCPSocket.js
@@ +632,5 @@
> +   * Open input/output stream for the socket transport.
> +   */
> +  openWithSocketTransport: function(host, port,
> +                                    socketTransport,
> +                                    useSSL, binaryType) {

useSSL unused (add a comment why)

::: dom/network/TCPSocketParent.cpp
@@ +236,5 @@
> +
> +  nsCOMPtr<nsISocketTransport> socketTransport;
> +  rv = sts->CreateTransport(nullptr, 0,
> +                            aRemoteHost, aRemotePort,
> +                            nullptr, getter_AddRefs(socketTransport));

here aUseSSL should be utilized

::: dom/network/TCPSocketParentIntermediary.js
@@ +37,5 @@
>    },
>  
>    open: function(aParentSide, aHost, aPort, aUseSSL, aBinaryType,
>                   aAppId, aInBrowser) {
> +    let socketInternal = socket.QueryInterface(Ci.nsITCPSocketInternal);

what is the "socket" here?  this is definitely wrong.  a bad rebase?

@@ +71,2 @@
>      let socketInternal = socket.QueryInterface(Ci.nsITCPSocketInternal);
> +    socketInternal.setAppId(appId);

no inBrowser interest?

::: dom/network/interfaces/nsIDOMTCPSocket.idl
@@ +298,5 @@
> +  // @param socketTransport
> +  //        The socket transport object that will back the TCPSocket.
> +  // @return The new TCPSocket instance.
> +  nsIDOMTCPSocket openWithSocketTransport(in DOMString host,
> +                                          in unsigned short port,

host and port are properties of nsISocketTransport, this seems to be redundant to carry them here too

@@ +300,5 @@
> +  // @return The new TCPSocket instance.
> +  nsIDOMTCPSocket openWithSocketTransport(in DOMString host,
> +                                          in unsigned short port,
> +                                          in nsISocketTransport socketTransport,
> +                                          in bool useSSL,

as said earlier, can be found out via nsISocketTransport.securityInfo != null

@@ +306,5 @@
> +};
> +
> +/**
> + * nsITCPSocketInternalNative lets IPC classes pass native objects without
> + * having to transform them to JSObject.

implementor?

::: dom/network/interfaces/nsITCPSocketParent.idl
@@ +88,5 @@
>    // Called when received a child request to send an array buffer.
>    void onRecvSendArrayBuffer(in jsval data, in uint32_t trackingNumber);
> +
> +  // Connect existing socket transport, created from socket-transport-service,
> +  // to the TCPSocket object.  Used in RecvOpenBind().

to _a_ TCPSocket object (since it's returned)

@@ +97,5 @@
> +  // @param socketTransport
> +  //        The socket transport object that will back the TCPSocket.
> +  // @useSSL unused currently
> +  // @binaryType "string" or "arraybuffer"
> +  // @appId appId to associate with this socket via TCPSocketParentIntermediary

"appId to associate with this socket via TCPSocketParentIntermediary"

some rephrase would be good

Comment 90

[Honza Bambas (:mayhemer)]

Are you aware of bug 885982?  You should rebase your patches on top of patches from that bug.  It gets get rid of all the complexity and JS/C++ jumps.

Comment 91

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

Thanks!

> None of the patches in this bug is using this new functionality.  I don't
> see any "openBind()" exposed on nsIDOMTCPSocket (intended?)

In dom/network/TCPSocketChild.cpp, see the mods that add SendWindowlessOpenBind() which does SendOpenBind().
In RecvOpenBind(), it does rv = socketTransport->Bind(&addr);

Perhaps there's some unneeded code in the JS... I'll look more closely at that (that's Patrick's code).
 
> ::: dom/network/TCPSocket.js
> @@ +632,5 @@
> > +   * Open input/output stream for the socket transport.
> > +   */
> > +  openWithSocketTransport: function(host, port,
> > +                                    socketTransport,
> > +                                    useSSL, binaryType) {
> 
> useSSL unused (add a comment why)

ok (I'll need to dig through this to figure out the exact reason).  Note that all WebRTC traffic through mtransport is encrypted with DTLS-SRTP (mandatory, using PFS keying), so little additional information leakage can occur if the TCP connection to the TURN server is unencrypted.  See RFC 5766, section 2.1 (Transports) and 17.1.6 (Eavesdropping Traffic), and current mtransport implementation doesn't support STUNS/TURNS (i.e. TURN over TLS) - see bug 1056934 (which I believe may be the reason for the above).  If so, I'll comment it and that it needs to be updated for that bug.

> ::: dom/network/TCPSocketParent.cpp
> @@ +236,5 @@
> > +
> > +  nsCOMPtr<nsISocketTransport> socketTransport;
> > +  rv = sts->CreateTransport(nullptr, 0,
> > +                            aRemoteHost, aRemotePort,
> > +                            nullptr, getter_AddRefs(socketTransport));
> 
> here aUseSSL should be utilized

see previous comment...  I presume you're referring to the first parameter for CreateTransport?
 
> ::: dom/network/TCPSocketParentIntermediary.js
> @@ +37,5 @@
> >    },
> >  
> >    open: function(aParentSide, aHost, aPort, aUseSSL, aBinaryType,
> >                   aAppId, aInBrowser) {
> > +    let socketInternal = socket.QueryInterface(Ci.nsITCPSocketInternal);
> 
> what is the "socket" here?  this is definitely wrong.  a bad rebase?

Looks that way, thanks.
 
> @@ +71,2 @@
> >      let socketInternal = socket.QueryInterface(Ci.nsITCPSocketInternal);
> > +    socketInternal.setAppId(appId);
> 
> no inBrowser interest?

We don't have that as an option today... and for mtransport/WebRTC, I don't think we need it if I correctly infer what it's for.

> 
> ::: dom/network/interfaces/nsIDOMTCPSocket.idl
> @@ +298,5 @@
> > +  // @param socketTransport
> > +  //        The socket transport object that will back the TCPSocket.
> > +  // @return The new TCPSocket instance.
> > +  nsIDOMTCPSocket openWithSocketTransport(in DOMString host,
> > +                                          in unsigned short port,
> 
> host and port are properties of nsISocketTransport, this seems to be
> redundant to carry them here too

Sure.  Removed them, and set that._host/port from socketTransport.host/port in TCPSocket.js

> @@ +300,5 @@
> > +  // @return The new TCPSocket instance.
> > +  nsIDOMTCPSocket openWithSocketTransport(in DOMString host,
> > +                                          in unsigned short port,
> > +                                          in nsISocketTransport socketTransport,
> > +                                          in bool useSSL,
> 
> as said earlier, can be found out via nsISocketTransport.securityInfo != null

Per nsISocketTransport.idl, this is only available *after* it's connected, so using it here would always return null I presume.  Perhaps I'm wrong (perhaps it's already connected by this point); if so great.

> 
> @@ +306,5 @@
> > +};
> > +
> > +/**
> > + * nsITCPSocketInternalNative lets IPC classes pass native objects without
> > + * having to transform them to JSObject.
> 
> implementor?

From Patch 2: nr_socket_prsock.cpp:
NS_IMETHODIMP NrTcpSocketIpc::CallListenerNativeArray(uint8_t *buf,

> 
> ::: dom/network/interfaces/nsITCPSocketParent.idl
> @@ +88,5 @@
> >    // Called when received a child request to send an array buffer.
> >    void onRecvSendArrayBuffer(in jsval data, in uint32_t trackingNumber);
> > +
> > +  // Connect existing socket transport, created from socket-transport-service,
> > +  // to the TCPSocket object.  Used in RecvOpenBind().
> 
> to _a_ TCPSocket object (since it's returned)

ok

> 
> @@ +97,5 @@
> > +  // @param socketTransport
> > +  //        The socket transport object that will back the TCPSocket.
> > +  // @useSSL unused currently
> > +  // @binaryType "string" or "arraybuffer"
> > +  // @appId appId to associate with this socket via TCPSocketParentIntermediary
> 
> "appId to associate with this socket via TCPSocketParentIntermediary"
> 
> some rephrase would be good

Perhaps:
  // @appId appId of the app to associate with this socket, or NO_APP_ID

Comment 92

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

(In reply to Honza Bambas (:mayhemer) from comment #90)
> Are you aware of bug 885982?  You should rebase your patches on top of
> patches from that bug.  It gets get rid of all the complexity and JS/C++
> jumps.

yes, jdm brought that to my attention a few days ago; at the time it was unclear who would land first and have to rebase.... I had expected to land first as that wasn't up for reviews at the time, IIRC.

It did stop me from filing one of two followups - rewrite TCPSocket.js in C++.  I still need to file a followup for converting TCPSocket.js to PBackground (at least optionally, ala UDPSocket)

Comment 93

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

Attachment: Part 1: Support bind in content process

Putting up patches that apply on top of bug 885982 (TCPSocket rewrite in C++) for feedback (not yet fully wokring)

Comment 94

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

Attachment: Part 2 - Change TCPSocket interface to TCPSocketChild (WIP)

Comment 95

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

Attachment: Part 3 - make TCPSocket/TCPSocketChild interface an IDL interface (WIP)

Comment 96

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

Attachment: Part 4 (was 2): Bridge TCPSocketChild to nr_socket

Improve use of TCPSocket to track in-flight writes and suppress extra runnables
Adds lots of logging to nr_socket_buffered_stun.c

Comment 97

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

Attachment: part 5 - Rework mtransport code to use new TCPSocketChild interface (WIP)

Comment 98

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

Attachment: NOT FOR CHECKIN - disable all UDP to force TURN-TCP

Comment 99

[Josh Matthews [:jdm]]

Comment on attachment 8655488 [details] [diff] [review]
Part 2 - Change TCPSocket interface to TCPSocketChild (WIP)

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

::: dom/network/TCPSocketChild.cpp
@@ +165,5 @@
>      const SendableData& data = aData.get_SendableData();
>  
>      if (data.type() == SendableData::TArrayOfuint8_t) {
> +      mSocket->FireDataEvent(aType, data.get_ArrayOfuint8_t());
> +      return true;

This return isn't necessary.

@@ +216,5 @@
>    }
>    return NS_OK;
>  }
>  
> +nsresult

Does this still need to be nsresult?

::: dom/network/TCPSocketParent.cpp
@@ +230,5 @@
>      appId = tab->OwnAppId();
>    }
>  
> +  mSocket = new TCPSocket(nullptr, NS_ConvertUTF8toUTF16(aRemoteHost), aRemotePort, aUseSSL, aUseArrayBuffers);
> +  mSocket->SetAppIdAndBrowser(appId, true);

Are you sure you want true as the second argument here?

Comment 100

[Josh Matthews [:jdm]]

Comment on attachment 8655489 [details] [diff] [review]
Part 3 - make TCPSocket/TCPSocketChild interface an IDL interface (WIP)

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

This looks about what I expected, but hard to say without the actual interface.

::: dom/network/TCPSocketChild.h
@@ +10,5 @@
>  #include "mozilla/net/PTCPSocketChild.h"
>  #include "nsCycleCollectionParticipant.h"
>  #include "nsCOMPtr.h"
>  #include "js/TypeDecls.h"
> +#include "nsIDOMTCPSocket.h"

This can be a forward declaration, I believe.

::: dom/network/interfaces/moz.build
@@ +4,5 @@
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  XPIDL_SOURCES += [
> +    'nsIDOMTCPSocket.idl',

This file is missing. Also, please remove the DOM from the name, as that has implications for how XPConnect can choose to expose it.

Comment 101

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

Attachment: Part 3 - make TCPSocket/TCPSocketChild interface an IDL interface (WIP)

Comment 102

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

Attachment: part 5 - Rework mtransport code to use new TCPSocketChild interface (WIP)

changed idl file name

Comment 103

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

Attachment: Part 2 - Change TCPSocket interface to TCPSocketChild (WIP)

Updated patches; TCP TURN now works again (the new TCPSocket::InitWithTransport() lured me into a honeypot)

Comment 104

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

Attachment: Part 3 - make TCPSocket/TCPSocketChild interface an IDL interface (WIP)

Comment 105

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

Attachment: Part 3 - make TCPSocket/TCPSocketChild interface an IDL interface (WIP)

minor fix to forward def

Comment 106

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

Attachment: part 5 - Rework mtransport code to use new TCPSocketChild interface

r? jdm for the adaptations to the modified TCPSocketChild/Callback interface

Comment 107

[Josh Matthews [:jdm]]

Comment on attachment 8656405 [details] [diff] [review]
Part 2 - Change TCPSocket interface to TCPSocketChild (WIP)

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

::: dom/network/TCPSocket.cpp
@@ +262,5 @@
> +
> +  mTransport->SetEventSink(this, mainThread);
> +
> +  nsresult rv = CreateStream();
> +  NS_ENSURE_SUCCESS(rv, rv);

Let's call this from Init and avoid the duplication between the two methods.

@@ +485,5 @@
> +TCPSocket::FireDataEvent(const nsAString& aType,
> +                         const InfallibleTArray<uint8_t>& buffer)
> +{
> +  AutoJSAPI api;
> +  api.Init(GetOwner());

This is working off an old revision of the TCPSocket patches; in newer ones we check the return value.

@@ +500,5 @@
> +TCPSocket::FireDataEvent(const nsAString& aType,
> +                         const nsAString& aString)
> +{
> +  AutoJSAPI api;
> +  api.Init(GetOwner());

Same here.

::: dom/network/TCPSocketChild.h
@@ +58,5 @@
>                      uint32_t aByteOffset,
>                      uint32_t aByteLength,
>                      uint32_t aTrackingNumber,
>                      JSContext* aCx);
> +  void SendSendArray(nsTArray<uint8_t>* arr,

Any reason to use * instead of &?

::: dom/network/TCPSocketParent.cpp
@@ +222,5 @@
>    }
>  
>    // Obtain App ID
>    uint32_t appId = nsIScriptSecurityManager::NO_APP_ID;
> +  bool     inBrowser = false;

nit: not a fan of this spacing.

@@ +233,3 @@
>    }
>  
> +  mSocket = new TCPSocket(nullptr, NS_ConvertUTF8toUTF16(aRemoteHost), aRemotePort, aUseSSL, aUseArrayBuffers);

Can we use an nsString in the IPDL instead converting here?

@@ +236,3 @@
>    mSocket->SetAppIdAndBrowser(appId, inBrowser);
>    mSocket->SetSocketBridgeParent(this);
> +  NS_ENSURE_SUCCESS(mSocket->InitWithUnconnectedTransport(socketTransport), true);

Can we do this in a separate `nsresult rv = ...;` line?

Comment 108

[Josh Matthews [:jdm]]

Comment on attachment 8656413 [details] [diff] [review]
Part 3 - make TCPSocket/TCPSocketChild interface an IDL interface (WIP)

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

::: dom/network/TCPSocket.h
@@ -167,5 @@
> -    CLOSING,
> -    CLOSED
> -  };
> -
> -  // Initialize this socket from a low-level connection that hasn't connected yet

Was this comment removal intentional?

@@ +174,5 @@
>    // Store and reset any saved network stats for this socket.
>    void SaveNetworkStats(bool aEnforce);
>  #endif
>  
> +  nsITCPSocketCallback::ReadyState mReadyState;

I think this may be based on an out of date patch? The most recent ones use a DOM enum.

::: dom/network/interfaces/nsITCPSocketCallback.idl
@@ +17,5 @@
> +[ref] native nsUint8TArrayRef(InfallibleTArray<uint8_t>);
> +
> +
> +/*
> + * This interface is implemented in TCPSocket.js as an internal interfaces

TCPSocket.cpp

@@ +27,5 @@
> +interface nsITCPSocketCallback : nsISupports {
> +  // Limitation of TCPSocket's buffer size.
> +  const unsigned long BUFFER_SIZE = 65536;
> +%{C++
> +  enum ReadyState {

Add a comment that this duplicates the enum members in TCPSocket.webidl.

@@ +42,5 @@
> +  // Dispatch a "data" event at this object with a string
> +  void fireDataStringEvent(in DOMString type, in DOMString data);
> +
> +  // Dispatch a "data" event at this object with an Array
> +  void fireDataArrayEvent(in DOMString type,[const] in nsUint8TArrayRef data);

nit: space after ,

@@ +52,5 @@
> +  void fireEvent(in DOMString type);
> +
> +  // Update the DOM object's readyState.
> +  // @param readyState
> +  //        new ready state to be set to TCPSocket.

"set to TCPSocket"?

@@ +56,5 @@
> +  //        new ready state to be set to TCPSocket.
> +  void updateReadyState(in unsigned long readystate);
> +
> +  // Update the DOM object's bufferedAmount value with a tracking number to
> +  // ensure the update request is sent after child's send() invocation.

The second line here isn't very clear. "to ensure outdated values are ignored", perhaps?

Comment 109

[Josh Matthews [:jdm]]

Comment on attachment 8656405 [details] [diff] [review]
Part 2 - Change TCPSocket interface to TCPSocketChild (WIP)

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

::: dom/network/TCPSocket.h
@@ +63,5 @@
>  private:
>    virtual ~LegacyMozTCPSocket();
>  };
>  
> +class TCPSocket : public DOMEventTargetHelper

This change doesn't appear to be necessary, given the later patches?

Comment 110

[Josh Matthews [:jdm]]

Comment on attachment 8656414 [details] [diff] [review]
part 5 - Rework mtransport code to use new TCPSocketChild interface

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

Seems sensible. Nice work!

Comment 111

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

Attachment: part 6 - unbitrot to apply on top of current bug 885982 patches

Straightforward unbitrotting
Also pushed patch queue to ssh://hg.mozilla.org/users/rjesup_wgate.com/tcpturn_e10s with a patch to do the nit-resolutions from the reviews (nits will be merged into their patches before landing)

Comment 112

[Josh Matthews [:jdm]]

Comment on attachment 8657519 [details] [diff] [review]
part 6 - unbitrot to apply on top of current bug 885982 patches

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

::: dom/network/TCPSocketChild.h
@@ +10,5 @@
>  #include "mozilla/net/PTCPSocketChild.h"
>  #include "nsCycleCollectionParticipant.h"
>  #include "nsCOMPtr.h"
>  #include "js/TypeDecls.h"
> +#include "mozilla/dom/TypedArray.h"

What's this for?

::: dom/network/interfaces/nsITCPSocketCallback.idl
@@ +38,5 @@
>    // Dispatch a "data" event at this object with an Array
>    void fireDataArrayEvent(in DOMString type,[const] in nsUint8TArrayRef data);
>  
>    // Dispatch a "data" event at this object with an ArrayBuffer argument
> +  void fireDataEvent(in JSContextPtr cx, in DOMString type, in jsval data);

This can use [implicit_jscontext] instead, I think.

Comment 113

[Josh Matthews [:jdm]]

I note that http://hg.mozilla.org/users/rjesup_wgate.com/tcpturn_e10s/file/2e941600f812/nit_resolution#l42 changes the behaviour in the e10s case, as we won't be marked as connecting while the parent process initiates the connection any longer.

Comment 114

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

(In reply to Josh Matthews [:jdm] from comment #112)

> ::: dom/network/TCPSocketChild.h

> >  #include "js/TypeDecls.h"
> > +#include "mozilla/dom/TypedArray.h"
> 
> What's this for?

ArrayBuffer wasn't defined by anything included by TCPSocketChild.h, so pulling it in from nr_socket_prsock.h caused undefined type errors.

> ::: dom/network/interfaces/nsITCPSocketCallback.idl
> @@ +38,5 @@
> >    // Dispatch a "data" event at this object with an Array
> >    void fireDataArrayEvent(in DOMString type,[const] in nsUint8TArrayRef data);
> >  
> >    // Dispatch a "data" event at this object with an ArrayBuffer argument
> > +  void fireDataEvent(in JSContextPtr cx, in DOMString type, in jsval data);
> 
> This can use [implicit_jscontext] instead, I think.

Tried that; it puts the JSContext on the end of the arg list, instead of the front.  This setup with JSContextPtr is used elsewhere for a ptr at the front of the list.  Alternatively, we could move the JSCOntext to the end for everything.

Comment 115

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

Attachment: nit resolutions

Added the Connecting state setting to ::Init() for the e10s case

Comment 116

[Josh Matthews [:jdm]]

(In reply to Randell Jesup [:jesup] from comment #114)
> (In reply to Josh Matthews [:jdm] from comment #112)
> 
> > ::: dom/network/TCPSocketChild.h
> 
> > >  #include "js/TypeDecls.h"
> > > +#include "mozilla/dom/TypedArray.h"
> > 
> > What's this for?
> 
> ArrayBuffer wasn't defined by anything included by TCPSocketChild.h, so
> pulling it in from nr_socket_prsock.h caused undefined type errors.

Let's use a forward declaration instead, if possible.

> > ::: dom/network/interfaces/nsITCPSocketCallback.idl
> > @@ +38,5 @@
> > >    // Dispatch a "data" event at this object with an Array
> > >    void fireDataArrayEvent(in DOMString type,[const] in nsUint8TArrayRef data);
> > >  
> > >    // Dispatch a "data" event at this object with an ArrayBuffer argument
> > > +  void fireDataEvent(in JSContextPtr cx, in DOMString type, in jsval data);
> > 
> > This can use [implicit_jscontext] instead, I think.
> 
> Tried that; it puts the JSContext on the end of the arg list, instead of the
> front.  This setup with JSContextPtr is used elsewhere for a ptr at the
> front of the list.  Alternatively, we could move the JSCOntext to the end
> for everything.

Ok, the current change is fine with me.

Comment 117

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3f9843c44939
https://hg.mozilla.org/integration/mozilla-inbound/rev/af45bcfab412
https://hg.mozilla.org/integration/mozilla-inbound/rev/dfaa6b689bc0
https://hg.mozilla.org/integration/mozilla-inbound/rev/8eda15c59d26

Comment 118

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3f9843c44939
https://hg.mozilla.org/mozilla-central/rev/af45bcfab412
https://hg.mozilla.org/mozilla-central/rev/dfaa6b689bc0
https://hg.mozilla.org/mozilla-central/rev/8eda15c59d26