Add basic TCP socket mojo implementation

Add TCP socket mojo implementation.

This is a very preliminary pass and not tested very well. Open issues:

It seems there is no way to wait on a mojo data pipe without using chromium message loops, which I was reluctant to do in my example. We'll need this before doing any nontrivial sending or receiving, including testing of the infrastructure.

We need to be able to pass the net work type (IPv4 or v6) to the connect function that bypasses bind. I removed the code here and documented that it doesn't work yet.

R=yzshen@chromium.org

Committed: https://chromium.googlesource.com/chromium/src/+/4dea7b416c95292d14e2d2a2cfd18cd57268be11

[brettw, 2014-10-15 20:19:19]

brettw@chromium.org changed reviewers:
+ yzshen@chromium.org

[yzshen1, 2014-10-15 21:07:58]

https://codereview.chromium.org/657113002/diff/60001/mojo/examples/http_serve...
File mojo/examples/http_server/http_server.cc (right):

https://codereview.chromium.org/657113002/diff/60001/mojo/examples/http_serve...
mojo/examples/http_server/http_server.cc:79: // specified above. This call won't
work because the message loop is not a
You could use Environment::GetDefaultAsyncWaiter() and then use it to wait on
handles.

(The impl for non-chromium case is here:
//src/mojo/public/cpp/environment/lib/default_async_waiter.cc)

https://codereview.chromium.org/657113002/diff/60001/mojo/services/network/tc...
File mojo/services/network/tcp_connected_socket_impl.cc (right):

https://codereview.chromium.org/657113002/diff/60001/mojo/services/network/tc...
mojo/services/network/tcp_connected_socket_impl.cc:136: // TODO(brettw) report
error.
Do we want to close the pipe in this case? We don't do SendMore() anymore.

https://codereview.chromium.org/657113002/diff/60001/mojo/services/public/int...
File mojo/services/public/interfaces/network/tcp_server_socket.mojom (right):

https://codereview.chromium.org/657113002/diff/60001/mojo/services/public/int...
mojo/services/public/interfaces/network/tcp_server_socket.mojom:17: // Once a
connection has been established i(indicated by a successful
nit: an extra 'i'

[brettw, 2014-10-15 22:30:15]

Review comments addressed. I sprinkled Close() calls around the various error
cases.

[yzshen1, 2014-10-15 22:35:07]

LGTM

https://codereview.chromium.org/657113002/diff/80001/mojo/services/network/tc...
File mojo/services/network/tcp_connected_socket_impl.cc (right):

https://codereview.chromium.org/657113002/diff/80001/mojo/services/network/tc...
mojo/services/network/tcp_connected_socket_impl.cc:140: if (result < 0) {
I wonder why pending_send_ is not reset in this case.

[brettw, 2014-10-16 16:44:17]

The CQ bit was checked by brettw@chromium.org

[commit-bot: I haz the power, 2014-10-16 16:45:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/657113002/100001

[commit-bot: I haz the power, 2014-10-16 17:57:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-16 17:58:00]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[brettw, 2014-10-16 19:51:03]

The CQ bit was checked by brettw@chromium.org

[commit-bot: I haz the power, 2014-10-16 19:51:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/657113002/120001

[commit-bot: I haz the power, 2014-10-16 21:46:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-16 21:46:21]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-10-16 23:13:55]

Patchset 7 (id:??) landed as
https://crrev.com/4dea7b416c95292d14e2d2a2cfd18cd57268be11
Cr-Commit-Position: refs/heads/master@{#300003}

[brettw, 2014-10-16 23:14:48]

Committed patchset #7 (id:120001) manually as
4dea7b416c95292d14e2d2a2cfd18cd57268be11 (presubmit successful).

[willchan no longer on Chromium, 2014-10-17 00:41:00]

willchan@chromium.org changed reviewers:
+ willchan@chromium.org

[willchan no longer on Chromium, 2014-10-17 00:41:01]

https://codereview.chromium.org/657113002/diff/100001/mojo/services/network/t...
File mojo/services/network/tcp_bound_socket_impl.cc (right):

https://codereview.chromium.org/657113002/diff/100001/mojo/services/network/t...
mojo/services/network/tcp_bound_socket_impl.cc:80:
callback.Run(MakeNetworkError(net::ERR_FAILED));
We use ERR_UNEXPECTED for places where we believe the caller is fault.

https://codereview.chromium.org/657113002/diff/100001/mojo/services/network/t...
mojo/services/network/tcp_bound_socket_impl.cc:117:
pending_connect_callback_.Run(MakeNetworkError(result));
This works fine here, but in general, for net/ programming, we prefer to store
the async callback in a temporary and NULL out the member variable for the
callback, prior to invoking the callback, in order to handle re-entrancy. This
is just a convention and doesn't matter in this particular case, but I'd
encourage doing it anyway, so people copy/paste a good pattern.

[mmenke, 2014-10-17 16:49:54]

mmenke@chromium.org changed reviewers:
+ mmenke@chromium.org

[mmenke, 2014-10-17 16:49:56]

https://codereview.chromium.org/657113002/diff/120001/mojo/examples/http_serv...
File mojo/examples/http_server/http_server.cc (right):

https://codereview.chromium.org/657113002/diff/120001/mojo/examples/http_serv...
mojo/examples/http_server/http_server.cc:6: #include <arpa/inet.h>
alphabetize.

https://codereview.chromium.org/657113002/diff/120001/mojo/examples/http_serv...
mojo/examples/http_server/http_server.cc:26: "Hello, world\n";
Suggest using CRLFs, per spec.

https://codereview.chromium.org/657113002/diff/120001/mojo/examples/http_serv...
mojo/examples/http_server/http_server.cc:32: Connection(TCPConnectedSocketPtr
conn,
nit:  Google style guide discourages abbreviations, so I suggest just writing
out connection.

https://codereview.chromium.org/657113002/diff/120001/mojo/services/network/t...
File mojo/services/network/tcp_connected_socket_impl.cc (right):

https://codereview.chromium.org/657113002/diff/120001/mojo/services/network/t...
mojo/services/network/tcp_connected_socket_impl.cc:60: } else if (read_result >=
0) {
Early return is generally preferred over using "else" in Chrome code.

https://codereview.chromium.org/657113002/diff/120001/mojo/services/network/t...
mojo/services/network/tcp_connected_socket_impl.cc:62: DidReceive(true,
read_result);
Can't we call this on the read_result < 0 case, too?  DidReceive has to handle
errors, too.

https://codereview.chromium.org/657113002/diff/120001/mojo/services/network/t...
mojo/services/network/tcp_connected_socket_impl.cc:65: // TODO(brettw) notify
caller of error.
Is there a reason not to have "pending_receive_ = NULL" here?

https://codereview.chromium.org/657113002/diff/120001/mojo/services/network/t...
mojo/services/network/tcp_connected_socket_impl.cc:77: if (result < 0) {
A 0-byte read means the socket was closed.

https://codereview.chromium.org/657113002/diff/120001/mojo/services/network/t...
mojo/services/network/tcp_connected_socket_impl.cc:89: if
(completed_synchronously) {
optional:  Just a matter of personal preference, but I think it's a bit prettier
for methods like this not to have a variable indicating what the stack looks
like when they're called.  Does require an extra method, though, and a couple
more if's.

I think the code is a bit cleaner as:

void ReadMore() {
  ...
  if (result == ERR_IO_PENDING)
    return;
  if (HandleReadResult(result) == net::OK) {
    // PostTask to call ReadMore asynchronously.
  }
}

void OnReadComplete(int result) {
  if (HandleReadResult(result)== net::OK)
    ReadMore();
}

https://codereview.chromium.org/657113002/diff/120001/mojo/services/network/t...
mojo/services/network/tcp_connected_socket_impl.cc:126: // Pending I/O, wait for
result in DidSend().
Again, early return is generally preferred.

https://codereview.chromium.org/657113002/diff/120001/mojo/services/network/t...
mojo/services/network/tcp_connected_socket_impl.cc:129: DidSend(true,
write_result);
Should handle error case here.

https://codereview.chromium.org/657113002/diff/120001/mojo/services/network/t...
File mojo/services/network/tcp_connected_socket_impl.h (right):

https://codereview.chromium.org/657113002/diff/120001/mojo/services/network/t...
mojo/services/network/tcp_connected_socket_impl.h:13: #include
"net/socket/tcp_socket.h"
Think we can just forward declare this.

https://codereview.chromium.org/657113002/diff/120001/mojo/services/network/t...
mojo/services/network/tcp_connected_socket_impl.h:30: void ReceiveMore();
Really could use some more method-level docs.

https://codereview.chromium.org/657113002/diff/120001/mojo/services/network/t...
mojo/services/network/tcp_connected_socket_impl.h:31: void
OnReceiveStreamReady(MojoResult result);
Include header for MojoResult?

https://codereview.chromium.org/657113002/diff/120001/mojo/services/network/t...
mojo/services/network/tcp_connected_socket_impl.h:54:
scoped_refptr<MojoToNetPendingBuffer> pending_send_;
Need to include ref_counted.h.

https://codereview.chromium.org/657113002/diff/120001/mojo/services/network/t...
mojo/services/network/tcp_connected_socket_impl.h:56:
base::WeakPtrFactory<TCPConnectedSocketImpl> weak_ptr_factory_;
DISALLOW_COPY_AND_ASSIGN?