Added implementation of buzz::AsyncSocket that uses Chrome sockets.

Added implementation of buzz::AsyncSocket that uses Chrome sockets.

BUG=45612
TEST=new unit tests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=52289

[akalin, 2010-06-29 09:49:04]

+willchan for review (since I discussed the implementation with him)

Tests are kind of messy, but I've spent already way too long on them.  I can
clean them up in a future CL.

[akalin, 2010-06-29 09:49:47]

CC'ed a bunch of people who may be interested.

On 2010/06/29 09:49:04, akalin wrote:
> +willchan for review (since I discussed the implementation with him)
> 
> Tests are kind of messy, but I've spent already way too long on them.  I can
> clean them up in a future CL.

[willchan no longer on Chromium, 2010-06-29 21:02:17]

I started looking and only really have nits so far.  I need to read the
buzz::AsyncSocket code to understand how you are mapping things.  You seem to be
posting lots of tasks, and I want to make sure that they're all necessary.  I'll
get around to this tomorrow when I get back to SF.

http://codereview.chromium.org/2863030/diff/13001/14001
File chrome/browser/sync/tools/chrome_async_socket.cc (right):

http://codereview.chromium.org/2863030/diff/13001/14001#newcode33
chrome/browser/sync/tools/chrome_async_socket.cc:33: :
connect_callback_(ALLOW_THIS_IN_INITIALIZER_LIST(this),
I think you need compiler_specific.h for this.

http://codereview.chromium.org/2863030/diff/13001/14001#newcode54
chrome/browser/sync/tools/chrome_async_socket.cc:54:
DCHECK(client_socket_factory_);
You need logging.h.

http://codereview.chromium.org/2863030/diff/13001/14001#newcode99
chrome/browser/sync/tools/chrome_async_socket.cc:99: addrinfo* ai = new
addrinfo();
I think you are allocating with new and freeing with free().  While it's
probably safe in this case, it's inadvisable.

http://codereview.chromium.org/2863030/diff/13001/14001#newcode105
chrome/browser/sync/tools/chrome_async_socket.cc:105: sockaddr_in* addr = new
sockaddr_in();
ditto

http://codereview.chromium.org/2863030/diff/13001/14002
File chrome/browser/sync/tools/chrome_async_socket.h (right):

http://codereview.chromium.org/2863030/diff/13001/14002#newcode40
chrome/browser/sync/tools/chrome_async_socket.h:40: net::NetLog* net_log,
Nit: it's somewhat common in our net/ code to put the NetLog param last.  It's
somewhat of an "output" param.

http://codereview.chromium.org/2863030/diff/13001/14002#newcode204
chrome/browser/sync/tools/chrome_async_socket.h:204:
scoped_ptr<net::ClientSocket> transport_socket_;
It's strictly an improvement that you're using a net::ClientSocket here, but
typically the ClientSocket is owned by the ClientSocketPool, not by any client. 
I'd encourage you to do that, but I'm still happy with this incremental
improvement and am fine with you not doing that in this changelist.

[akalin, 2010-06-29 22:50:44]

On 2010/06/29 21:02:17, willchan wrote:
> I started looking and only really have nits so far.  I need to read the
> buzz::AsyncSocket code to understand how you are mapping things.  You seem to
be
> posting lots of tasks, and I want to make sure that they're all necessary. 
I'll
> get around to this tomorrow when I get back to SF.

The docs for buzz::AsyncSocket are nonexistent.  I based my implementation
mostly on examining how XmppClient
(third_party/libjingle/source/talk/xmpp/xmppclient.{h,cc}) et al. use it and
what XmppSocketAdapter
(chrome/common/net/notifier/communicator/xmpp_socket_adapter.{h,cc}) does.

The task-posting is necessary because, based on a previous implementation which
did a lot of stuff inline, I'm not confident that the libjingle code can handle
signals being triggered (like OnClose, which is triggered on any net error) when
the public functions (e.g., Connect(), Read(), etc.) are called.  So I think the
safest way to implement the public functions is to delay the real work until the
stack unwinds.  That way, all the public functions are guaranteed to not raise
signals and all signals are guaranteed to happen "asynchronously".

[akalin, 2010-06-29 22:50:55]

Please take another look.

http://codereview.chromium.org/2863030/diff/13001/14001
File chrome/browser/sync/tools/chrome_async_socket.cc (right):

http://codereview.chromium.org/2863030/diff/13001/14001#newcode33
chrome/browser/sync/tools/chrome_async_socket.cc:33: :
connect_callback_(ALLOW_THIS_IN_INITIALIZER_LIST(this),
On 2010/06/29 21:02:18, willchan wrote:
> I think you need compiler_specific.h for this.

Done.

http://codereview.chromium.org/2863030/diff/13001/14001#newcode54
chrome/browser/sync/tools/chrome_async_socket.cc:54:
DCHECK(client_socket_factory_);
On 2010/06/29 21:02:18, willchan wrote:
> You need logging.h.

Done.

http://codereview.chromium.org/2863030/diff/13001/14001#newcode99
chrome/browser/sync/tools/chrome_async_socket.cc:99: addrinfo* ai = new
addrinfo();
On 2010/06/29 21:02:18, willchan wrote:
> I think you are allocating with new and freeing with free().  While it's
> probably safe in this case, it's inadvisable.

Thanks, good catch.  Done.

http://codereview.chromium.org/2863030/diff/13001/14001#newcode105
chrome/browser/sync/tools/chrome_async_socket.cc:105: sockaddr_in* addr = new
sockaddr_in();
On 2010/06/29 21:02:18, willchan wrote:
> ditto

Done.

http://codereview.chromium.org/2863030/diff/13001/14002
File chrome/browser/sync/tools/chrome_async_socket.h (right):

http://codereview.chromium.org/2863030/diff/13001/14002#newcode40
chrome/browser/sync/tools/chrome_async_socket.h:40: net::NetLog* net_log,
On 2010/06/29 21:02:18, willchan wrote:
> Nit: it's somewhat common in our net/ code to put the NetLog param last.  It's
> somewhat of an "output" param.

Done.

http://codereview.chromium.org/2863030/diff/13001/14002#newcode204
chrome/browser/sync/tools/chrome_async_socket.h:204:
scoped_ptr<net::ClientSocket> transport_socket_;
On 2010/06/29 21:02:18, willchan wrote:
> It's strictly an improvement that you're using a net::ClientSocket here, but
> typically the ClientSocket is owned by the ClientSocketPool, not by any
client. 
> I'd encourage you to do that, but I'm still happy with this incremental
> improvement and am fine with you not doing that in this changelist.

That's definitely the plan, but I didn't want to do too much in this CL.  Added
TODO.

[willchan no longer on Chromium, 2010-07-01 21:46:28]

http://codereview.chromium.org/2863030/diff/4002/19001
File chrome/browser/sync/tools/chrome_async_socket.cc (right):

http://codereview.chromium.org/2863030/diff/4002/19001#newcode46
chrome/browser/sync/tools/chrome_async_socket.cc:46: net_log_(net_log),
You should probably use BoundNetLog::Make() here.

http://codereview.chromium.org/2863030/diff/4002/19001#newcode103
chrome/browser/sync/tools/chrome_async_socket.cc:103: addrinfo* ai =
static_cast<addrinfo*>(std::malloc(sizeof *ai));
Can you be consistent on your use of sizeof?  Here you don't have () for it, but
on the next line you do.

http://codereview.chromium.org/2863030/diff/4002/19001#newcode246
chrome/browser/sync/tools/chrome_async_socket.cc:246:
DoNonNetError(ERROR_WRONGSTATE);
Is this a bug?  It seems like this should never happen, right?  Should we
LOG(DFATAL) or something to catch this?

http://codereview.chromium.org/2863030/diff/4002/19001#newcode258
chrome/browser/sync/tools/chrome_async_socket.cc:258: std::memcpy(data,
read_buf_->data() + read_start_, *len_read);
It's a shame that we need this copy.  Is there not a way to perhaps have the
IOBuffer wrap the user-provided buffer?  I think you mentioned something about
how buzz sockets always keep reading.  Is this why it's not possible?

http://codereview.chromium.org/2863030/diff/4002/19001#newcode273
chrome/browser/sync/tools/chrome_async_socket.cc:273:
DoNonNetError(ERROR_WRONGSTATE);
ERROR_WRONGSTATE sounds like a bug.  I recommend LOG(DFATAL) here.

http://codereview.chromium.org/2863030/diff/4002/19002
File chrome/browser/sync/tools/chrome_async_socket.h (right):

http://codereview.chromium.org/2863030/diff/4002/19002#newcode41
chrome/browser/sync/tools/chrome_async_socket.h:41: size_t read_buf_size,
I'm not sure I understand why these have to be statically specified in advance.

http://codereview.chromium.org/2863030/diff/4002/19002#newcode192
chrome/browser/sync/tools/chrome_async_socket.h:192: net::NetLog* const
net_log_;
You should probably use a BoundNetLog instead.

[akalin, 2010-07-02 10:29:16]

Please take another look.

FYI, I'll be on vacation until next Friday, so I may not reply to any comments
until then.

http://codereview.chromium.org/2863030/diff/4002/19001
File chrome/browser/sync/tools/chrome_async_socket.cc (right):

http://codereview.chromium.org/2863030/diff/4002/19001#newcode46
chrome/browser/sync/tools/chrome_async_socket.cc:46: net_log_(net_log),
On 2010/07/01 21:46:29, willchan wrote:
> You should probably use BoundNetLog::Make() here.

Done.

http://codereview.chromium.org/2863030/diff/4002/19001#newcode103
chrome/browser/sync/tools/chrome_async_socket.cc:103: addrinfo* ai =
static_cast<addrinfo*>(std::malloc(sizeof *ai));
On 2010/07/01 21:46:29, willchan wrote:
> Can you be consistent on your use of sizeof?  Here you don't have () for it,
but
> on the next line you do.

Done.

http://codereview.chromium.org/2863030/diff/4002/19001#newcode246
chrome/browser/sync/tools/chrome_async_socket.cc:246:
DoNonNetError(ERROR_WRONGSTATE);
On 2010/07/01 21:46:29, willchan wrote:
> Is this a bug?  It seems like this should never happen, right?  Should we
> LOG(DFATAL) or something to catch this?

Done.

http://codereview.chromium.org/2863030/diff/4002/19001#newcode258
chrome/browser/sync/tools/chrome_async_socket.cc:258: std::memcpy(data,
read_buf_->data() + read_start_, *len_read);
On 2010/07/01 21:46:29, willchan wrote:
> It's a shame that we need this copy.  Is there not a way to perhaps have the
> IOBuffer wrap the user-provided buffer?  I think you mentioned something about
> how buzz sockets always keep reading.  Is this why it's not possible?

Yeah.  Basically, xmppclient always passes the same fixed-size buffer to Read().

http://codereview.chromium.org/2863030/diff/4002/19001#newcode273
chrome/browser/sync/tools/chrome_async_socket.cc:273:
DoNonNetError(ERROR_WRONGSTATE);
On 2010/07/01 21:46:29, willchan wrote:
> ERROR_WRONGSTATE sounds like a bug.  I recommend LOG(DFATAL) here.

Done.

http://codereview.chromium.org/2863030/diff/4002/19002
File chrome/browser/sync/tools/chrome_async_socket.h (right):

http://codereview.chromium.org/2863030/diff/4002/19002#newcode41
chrome/browser/sync/tools/chrome_async_socket.h:41: size_t read_buf_size,
On 2010/07/01 21:46:29, willchan wrote:
> I'm not sure I understand why these have to be statically specified in
advance.

We need a size to pass to transport_socket_->Read() and I don't think it's worth
the effort to try and dynamically adjust it.  Since the actual socket read
happens before the client calls Read(), we have no way of knowing how much data
to expect.

As for write, we need to queue up the data passed to Write() if there's already
a pending write.  We could have the buffer be unbounded, but it seems safer to
bound it and refuse to queue data past a certain point.

http://codereview.chromium.org/2863030/diff/4002/19002#newcode192
chrome/browser/sync/tools/chrome_async_socket.h:192: net::NetLog* const
net_log_;
On 2010/07/01 21:46:29, willchan wrote:
> You should probably use a BoundNetLog instead.

Done.

[willchan no longer on Chromium, 2010-07-07 23:28:23]

http://codereview.chromium.org/2863030/diff/4002/19002
File chrome/browser/sync/tools/chrome_async_socket.h (right):

http://codereview.chromium.org/2863030/diff/4002/19002#newcode41
chrome/browser/sync/tools/chrome_async_socket.h:41: size_t read_buf_size,
On 2010/07/02 10:29:16, akalin wrote:
> On 2010/07/01 21:46:29, willchan wrote:
> > I'm not sure I understand why these have to be statically specified in
> advance.
> 
> We need a size to pass to transport_socket_->Read() and I don't think it's
worth
> the effort to try and dynamically adjust it.  Since the actual socket read
> happens before the client calls Read(), we have no way of knowing how much
data
> to expect.
> 
> As for write, we need to queue up the data passed to Write() if there's
already
> a pending write.  We could have the buffer be unbounded, but it seems safer to
> bound it and refuse to queue data past a certain point.

Wow, this API is the suck.  Ok, I defer to you on bounding the Write() buffer,
since you understand how the data gets used better than I do, but I'm worried
that we might hit it in some cases, and the client code probably doesn't know
how to recover from it.  There's probably no way to recover.

http://codereview.chromium.org/2863030/diff/14006/11002
File chrome/browser/sync/tools/chrome_async_socket.cc (right):

http://codereview.chromium.org/2863030/diff/14006/11002#newcode143
chrome/browser/sync/tools/chrome_async_socket.cc:143:
DCHECK(!scoped_runnable_method_factory_.get());
I'm not sure I understand why you need to put the factory in a scoped_ptr. 
Can't you just rely on RevokeAll() and empty()?

http://codereview.chromium.org/2863030/diff/14006/11002#newcode158
chrome/browser/sync/tools/chrome_async_socket.cc:158:
&ChromeAsyncSocket::ProcessConnectDone, status));
You need to document this task posting stuff somewhere or have a test that
enforces that nothing gets signaled immediately or something, otherwise I can
easily imagine someone regressing this in the future, since it's not the obvious
way to write the code.

http://codereview.chromium.org/2863030/diff/14006/11002#newcode177
chrome/browser/sync/tools/chrome_async_socket.cc:177: PostDoRead();
Have you tested the TLS case?  I don't think that wrapping the transport socket
in the SSLClientSocket will work if you've already called Read() on the
transport socket.  Perhaps in SignalConnected(), StartTls() gets called, which
cancels the posted read?  This is not obvious from reading the code, and it's a
very sensitive race condition.  Are we confident that StartTls() won't be called
at some other point after ClientSocket::Read() has already been called on the
transport socket?  If this is the contract, it should be documented somewhere.

http://codereview.chromium.org/2863030/diff/14006/11002#newcode347
chrome/browser/sync/tools/chrome_async_socket.cc:347: write_end_ - status);
Is this memmove() the right strategy?  It could be expensive if the buffer is
large.

I guess I think we should use something like a queue of DrainableIOBuffer.  If
you want to enforce a limit, then perhaps it should be on the number of Write()s
that you are able to enqueue.

http://codereview.chromium.org/2863030/diff/14006/11002#newcode401
chrome/browser/sync/tools/chrome_async_socket.cc:401: // Clear out any posted
DoRead() tasks.
Again, I think you should Revoke the tasks rather than resetting the factory.

http://codereview.chromium.org/2863030/diff/14006/11004
File chrome/browser/sync/tools/chrome_async_socket_unittest.cc (right):

http://codereview.chromium.org/2863030/diff/14006/11004#newcode179
chrome/browser/sync/tools/chrome_async_socket_unittest.cc:179: class
FakeClientSocket : public net::ClientSocket {
This looks like a lot of infrastructure that already exists in
net/socket/socket_test_util.h.

http://codereview.chromium.org/2863030/diff/14006/11005
File chrome/browser/sync/tools/sync_listen_notifications.cc (right):

http://codereview.chromium.org/2863030/diff/14006/11005#newcode350
chrome/browser/sync/tools/sync_listen_notifications.cc:350:
command_line.HasSwitch("use-chrome-async-socket");
We generally add constants for this into chrome/common/chrome_switches.h|cc.

[akalin, 2010-07-13 10:11:25]

On 2010/07/07 23:28:23, willchan wrote:
> Wow, this API is the suck.  Ok, I defer to you on bounding the Write() buffer,
> since you understand how the data gets used better than I do, but I'm worried
> that we might hit it in some cases, and the client code probably doesn't know
> how to recover from it.  There's probably no way to recover.

Well, for what it's worth, the current implementation (XmppSocketAdapter) has a
DCHECK that its write buffer doesn't exceed 64k.  I did this primarily for
simplicity and that so that bugs can be shaken out easily as I test it (I also
added a DCHECK when the limit gets exceeded).  A real solution would involve
adding some sort of "Ready for writing" signal to the interface; added TODO.

> http://codereview.chromium.org/2863030/diff/14006/11002#newcode158
> chrome/browser/sync/tools/chrome_async_socket.cc:158:
> &ChromeAsyncSocket::ProcessConnectDone, status));
> You need to document this task posting stuff somewhere or have a test that
> enforces that nothing gets signaled immediately or something, otherwise I can
> easily imagine someone regressing this in the future, since it's not the
obvious
> way to write the code.

Added explanatory comments.

> http://codereview.chromium.org/2863030/diff/14006/11002#newcode177
> chrome/browser/sync/tools/chrome_async_socket.cc:177: PostDoRead();
> Have you tested the TLS case?  I don't think that wrapping the transport
socket
> in the SSLClientSocket will work if you've already called Read() on the
> transport socket.  Perhaps in SignalConnected(), StartTls() gets called, which
> cancels the posted read?  This is not obvious from reading the code, and it's
a
> very sensitive race condition.  Are we confident that StartTls() won't be
called
> at some other point after ClientSocket::Read() has already been called on the
> transport socket?  If this is the contract, it should be documented somewhere.

Yeah, StartTls() explicitly checks that there's no pending read or
posted/pending write.  I've combed through the calling code and StartTls() is
only called from a read handler, where it's safe. (Since the posted read gets
cancelled.)  The conditions under which StartTls() can be called is documented
in the .h file, but I added some more comments around the call to
transport_socket_->Read() and Write().

> http://codereview.chromium.org/2863030/diff/14006/11002#newcode347
> chrome/browser/sync/tools/chrome_async_socket.cc:347: write_end_ - status);
> Is this memmove() the right strategy?  It could be expensive if the buffer is
> large.
> 
> I guess I think we should use something like a queue of DrainableIOBuffer.  If
> you want to enforce a limit, then perhaps it should be on the number of
Write()s
> that you are able to enqueue.

The current implementation also uses memmove so I figured using it here would be
no worse.  I agree that a queue of buffers would be better, but fixing the
interface as described above ("ready to write" signal, etc.) would obviate this
problem, I think.

Please take another look.

http://codereview.chromium.org/2863030/diff/14006/11002
File chrome/browser/sync/tools/chrome_async_socket.cc (right):

http://codereview.chromium.org/2863030/diff/14006/11002#newcode143
chrome/browser/sync/tools/chrome_async_socket.cc:143:
DCHECK(!scoped_runnable_method_factory_.get());
On 2010/07/07 23:28:23, willchan wrote:
> I'm not sure I understand why you need to put the factory in a scoped_ptr. 
> Can't you just rely on RevokeAll() and empty()?

Done.

http://codereview.chromium.org/2863030/diff/14006/11002#newcode401
chrome/browser/sync/tools/chrome_async_socket.cc:401: // Clear out any posted
DoRead() tasks.
On 2010/07/07 23:28:23, willchan wrote:
> Again, I think you should Revoke the tasks rather than resetting the factory.

Done.

http://codereview.chromium.org/2863030/diff/14006/11004
File chrome/browser/sync/tools/chrome_async_socket_unittest.cc (right):

http://codereview.chromium.org/2863030/diff/14006/11004#newcode179
chrome/browser/sync/tools/chrome_async_socket_unittest.cc:179: class
FakeClientSocket : public net::ClientSocket {
On 2010/07/07 23:28:23, willchan wrote:
> This looks like a lot of infrastructure that already exists in
> net/socket/socket_test_util.h.

Ouch, that could have saved me a lot of time.  Rewrote the tests to use
socket_test_util.h.

http://codereview.chromium.org/2863030/diff/14006/11005
File chrome/browser/sync/tools/sync_listen_notifications.cc (right):

http://codereview.chromium.org/2863030/diff/14006/11005#newcode350
chrome/browser/sync/tools/sync_listen_notifications.cc:350:
command_line.HasSwitch("use-chrome-async-socket");
On 2010/07/07 23:28:23, willchan wrote:
> We generally add constants for this into chrome/common/chrome_switches.h|cc.

This is a standalone tool, though; I don't think the switches here should be on
chrome_switches.h, as those are for chrome switches only, right?  I think
whoever changed this file to use switches:: did it by mistake.

[willchan no longer on Chromium, 2010-07-13 22:59:43]

LGTM, good tests!

http://codereview.chromium.org/2863030/diff/14006/11005
File chrome/browser/sync/tools/sync_listen_notifications.cc (right):

http://codereview.chromium.org/2863030/diff/14006/11005#newcode350
chrome/browser/sync/tools/sync_listen_notifications.cc:350:
command_line.HasSwitch("use-chrome-async-socket");
On 2010/07/13 10:11:25, akalin wrote:
> On 2010/07/07 23:28:23, willchan wrote:
> > We generally add constants for this into chrome/common/chrome_switches.h|cc.
> 
> This is a standalone tool, though; I don't think the 

Oh!  I didn't noticed that.  That makes sense then.

switches here should be on
> chrome_switches.h, as those are for chrome switches only, right?  I think
> whoever changed this file to use switches:: did it by mistake.

http://codereview.chromium.org/2863030/diff/50001/51003
File chrome/browser/sync/tools/chrome_async_socket_unittest.cc (right):

http://codereview.chromium.org/2863030/diff/50001/51003#newcode38
chrome/browser/sync/tools/chrome_async_socket_unittest.cc:38:
DCHECK(!has_pending_read_);
Don't forget logging.h for this.

[akalin, 2010-07-14 00:58:06]

Thanks!  Checking in as soon as trybots pass.

http://codereview.chromium.org/2863030/diff/50001/51003
File chrome/browser/sync/tools/chrome_async_socket_unittest.cc (right):

http://codereview.chromium.org/2863030/diff/50001/51003#newcode38
chrome/browser/sync/tools/chrome_async_socket_unittest.cc:38:
DCHECK(!has_pending_read_);
On 2010/07/13 22:59:44, willchan wrote:
> Don't forget logging.h for this.

Done.