Move PseudoTCP and channel auth out of LibjingleTransportFactory.

Move PseudoTCP and channel auth out of LibjingleTransportFactory.

Previously TransportFactory interface was responsible for creation
and initialization of several protocol layers, including PseudoTCP and
authentication (TLS). Simplified it so now it only creates raw datagram
transport channel. PseudoTcpChannelFactory is now responsible for
setting up PseudoTcpAdapter and AuthenticatingChannelFactory takes care
of channel authentication. Also added DatagramChannelFactory for
Datagram channels.

This change will make it possible to replace PseudoTcpChannelFactory
with an object that creates SCTP-based channels.

Also fixed a bug in SslHmacChannelAuthenticator. It wasn't working
properly when deleted from the callback. (base::Callback objects
shouldn't be deleted while being called because when deleted they
also destroy reference parameters values they are holding).

BUG=402993
Committed: https://crrev.com/9cb142f0cb33af89ff478e69e803fdf32f09006e
Cr-Commit-Position: refs/heads/master@{#294653}

[Sergey Ulanov, 2014-09-09 01:49:17]

sergeyu@chromium.org changed reviewers:
+ wez@chromium.org

[Sergey Ulanov, 2014-09-09 01:54:58]

Patchset #2 (id:20001) has been deleted

[Wez, 2014-09-10 01:51:53]

nit: You has a typo in your CL description :)

[Wez, 2014-09-10 02:29:27]

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/authentica...
File remoting/protocol/authenticated_channel_factory.cc (right):

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/authentica...
remoting/protocol/authenticated_channel_factory.cc:24: //
CancelChannelCreation() is expected to be called before destruction.
Is that a part of the ChannelFactory interface contract? If so then consider
removing this comment.

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/authentica...
remoting/protocol/authenticated_channel_factory.cc:54:
callback.Run(socket.Pass());
It reads oddly to socket.Pass() only if socket is NULL...
scoped_ptr<net::StreamSocket>() would be more obvious, I think. ;)

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/authentica...
remoting/protocol/authenticated_channel_factory.cc:61:
channel_authenticator.release();
nit: Can you just .release() the CreateChannelAuthenticator() result directly,
without the |channel_authenticator| variable?

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/authentica...
remoting/protocol/authenticated_channel_factory.cc:78:
authenticator.reset(it->second);
Why not just delete it->second, like you do in the cancel case?

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/authentica...
remoting/protocol/authenticated_channel_factory.cc:81: DCHECK(error == net::OK
|| socket);
Do you mean (socket || error != net::OK), i.e. that either we got a socket, or
an error must have occurred?

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/authentica...
remoting/protocol/authenticated_channel_factory.cc:87: 
nit: Lose this blank line.

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/authentica...
File remoting/protocol/authenticated_channel_factory.h (right):

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/authentica...
remoting/protocol/authenticated_channel_factory.h:24: class
AuthenticatedChannelFactory : public ChannelFactory {
SecureChannelFactory?

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/authentica...
remoting/protocol/authenticated_channel_factory.h:37: typedef
std::map<std::string, ChannelAuthenticator*> ChannelsMap;
AuthenticatorMap? or ChannelAuthenticatorMap?

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/authentica...
remoting/protocol/authenticated_channel_factory.h:43: void OnAuthenticated(const
std::string& name,
nit: OnSecureChannelCreated?

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/channel_au...
File remoting/protocol/channel_authenticator.h (right):

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/channel_au...
remoting/protocol/channel_authenticator.h:27: // calling authenticator.
Suggest moving this comment to the SecureAndAuthenticate API

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/channel_au...
remoting/protocol/channel_authenticator.h:35: // before this method returns.
e.g. .. method returns, and may delete the calling authenticator.

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/channel_fa...
File remoting/protocol/channel_factory.h (right):

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/channel_fa...
remoting/protocol/channel_factory.h:20: class ChannelFactory : public
base::NonThreadSafe {
StreamChannelFactory?

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/channel_fa...
remoting/protocol/channel_factory.h:34: // channels for which the |callback|
hasn't been called yet.
nit: All channels must be destroyed, and Cancel...() called for any pending
channels, before the factory is destroyed.

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/datagram_c...
File remoting/protocol/datagram_channel_factory.h (right):

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/datagram_c...
remoting/protocol/datagram_channel_factory.h:24: // failed for any reason.
Callback may be called synchronously, before the
nit: There is no "connection" for a datagram channel, though.

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/datagram_c...
remoting/protocol/datagram_channel_factory.h:27: // channels for which the
|callback| hasn't been called yet.
See comments on ChannelFactory

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/jingle_ses...
File remoting/protocol/jingle_session.cc (right):

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/jingle_ses...
remoting/protocol/jingle_session.cc:614: pseudotcp_channel_factory_.get(),
authenticator_.get()));
nit: Although it's an extra line, I'd suggest wrapping before new and before
authenticator, for readability.

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/jingle_ses...
File remoting/protocol/jingle_session.h (right):

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/jingle_ses...
remoting/protocol/jingle_session.h:140: void OnAuthenticated();
This name becomes confusing given that it is the trigger for authentication the
channels, i.e. it comes *before* they are authenticated. If you rename the
AuthenticatedChannelFactory to SecureChannelFactory then I think that works out
OK, though.

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/jingle_ses...
File remoting/protocol/jingle_session_unittest.cc (right):

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/jingle_ses...
remoting/protocol/jingle_session_unittest.cc:522: if (!client_callback_called) {
Why would the callback not be called, after this change?

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/pseudotcp_...
File remoting/protocol/pseudotcp_channel_factory.cc (right):

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/pseudotcp_...
remoting/protocol/pseudotcp_channel_factory.cc:39: // ChannelFactory interface.
nit: Remove this comment

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/pseudotcp_...
remoting/protocol/pseudotcp_channel_factory.cc:63:
jingle_glue::PseudoTcpAdapter* socket =
nit: adaptor?

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/pseudotcp_...
remoting/protocol/pseudotcp_channel_factory.cc:94: // Return NULL if Connect()
failed.
nit: Does that really need a comment?

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/pseudotcp_...
File remoting/protocol/pseudotcp_channel_factory.h (right):

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/pseudotcp_...
remoting/protocol/pseudotcp_channel_factory.h:23: DatagramChannelFactory*
datagram_channel_factory);
nit: Comment on lifetime of this?

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/pseudotcp_...
remoting/protocol/pseudotcp_channel_factory.h:41: typedef std::map<std::string,
net::StreamSocket*> PendingSocketsMap;
nit: Move this to the top of private section.

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/ssl_hmac_c...
File remoting/protocol/ssl_hmac_channel_authenticator.cc (right):

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/ssl_hmac_c...
remoting/protocol/ssl_hmac_channel_authenticator.cc:288:
NotifyResult(static_cast<net::Error>(error), scoped_ptr<net::StreamSocket>());
This has undefined and potentially scary behaviour if |error| is not a valid
enum value; consider using int in NotifyResult as well?

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/ssl_hmac_c...
File remoting/protocol/ssl_hmac_channel_authenticator.h (right):

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/ssl_hmac_c...
remoting/protocol/ssl_hmac_channel_authenticator.h:81: void
NotifyResult(net::Error error, scoped_ptr<net::StreamSocket> socket);
Add a comment to clarify what the difference between NotifyError and
NotifyResult(net::Error error,...) is?

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/ssl_hmac_c...
File remoting/protocol/ssl_hmac_channel_authenticator_unittest.cc (right):

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/ssl_hmac_c...
remoting/protocol/ssl_hmac_channel_authenticator_unittest.cc:114:
host_auth_.reset();
Why's this change required?

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/transport.h
File remoting/protocol/transport.h (right):

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/transport....
remoting/protocol/transport.h:99: virtual void Connect(const ConnectedCallback&
callback) = 0;
nit: Add a comment to clarify where the difference between Initialize and
Connect is.

[Sergey Ulanov, 2014-09-10 21:50:59]

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/authentica...
File remoting/protocol/authenticated_channel_factory.cc (right):

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/authentica...
remoting/protocol/authenticated_channel_factory.cc:24: //
CancelChannelCreation() is expected to be called before destruction.
On 2014/09/10 02:29:25, Wez wrote:
> Is that a part of the ChannelFactory interface contract? If so then consider
> removing this comment.

I think it's still useful to make it clear what this DCHECK is for.

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/authentica...
remoting/protocol/authenticated_channel_factory.cc:54:
callback.Run(socket.Pass());
On 2014/09/10 02:29:25, Wez wrote:
> It reads oddly to socket.Pass() only if socket is NULL...
> scoped_ptr<net::StreamSocket>() would be more obvious, I think. ;)

yes, but it's more to type :(. Done.

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/authentica...
remoting/protocol/authenticated_channel_factory.cc:61:
channel_authenticator.release();
On 2014/09/10 02:29:25, Wez wrote:
> nit: Can you just .release() the CreateChannelAuthenticator() result directly,
> without the |channel_authenticator| variable?

Done.

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/authentica...
remoting/protocol/authenticated_channel_factory.cc:78:
authenticator.reset(it->second);
On 2014/09/10 02:29:25, Wez wrote:
> Why not just delete it->second, like you do in the cancel case?

Done.

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/authentica...
remoting/protocol/authenticated_channel_factory.cc:81: DCHECK(error == net::OK
|| socket);
On 2014/09/10 02:29:25, Wez wrote:
> Do you mean (socket || error != net::OK), i.e. that either we got a socket, or
> an error must have occurred?

Yes. Replaced it with a stricter DCHECK and fixed a bug in
FakeChannelAuthenticator.

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/authentica...
remoting/protocol/authenticated_channel_factory.cc:87: 
On 2014/09/10 02:29:26, Wez wrote:
> nit: Lose this blank line.

Done.

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/authentica...
File remoting/protocol/authenticated_channel_factory.h (right):

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/authentica...
remoting/protocol/authenticated_channel_factory.h:24: class
AuthenticatedChannelFactory : public ChannelFactory {
On 2014/09/10 02:29:26, Wez wrote:
> SecureChannelFactory?

Yes, thank you!

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/authentica...
remoting/protocol/authenticated_channel_factory.h:37: typedef
std::map<std::string, ChannelAuthenticator*> ChannelsMap;
On 2014/09/10 02:29:26, Wez wrote:
> AuthenticatorMap? or ChannelAuthenticatorMap?

Done.

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/authentica...
remoting/protocol/authenticated_channel_factory.h:43: void OnAuthenticated(const
std::string& name,
On 2014/09/10 02:29:26, Wez wrote:
> nit: OnSecureChannelCreated?

Done.

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/channel_au...
File remoting/protocol/channel_authenticator.h (right):

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/channel_au...
remoting/protocol/channel_authenticator.h:27: // calling authenticator.
On 2014/09/10 02:29:26, Wez wrote:
> Suggest moving this comment to the SecureAndAuthenticate API

Done.

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/channel_au...
remoting/protocol/channel_authenticator.h:35: // before this method returns.
On 2014/09/10 02:29:26, Wez wrote:
> e.g. .. method returns, and may delete the calling authenticator.

Done.

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/channel_fa...
File remoting/protocol/channel_factory.h (right):

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/channel_fa...
remoting/protocol/channel_factory.h:20: class ChannelFactory : public
base::NonThreadSafe {
On 2014/09/10 02:29:26, Wez wrote:
> StreamChannelFactory?

Done.

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/channel_fa...
remoting/protocol/channel_factory.h:34: // channels for which the |callback|
hasn't been called yet.
On 2014/09/10 02:29:26, Wez wrote:
> nit: All channels must be destroyed, and Cancel...() called for any pending
> channels, before the factory is destroyed.

Done.

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/datagram_c...
File remoting/protocol/datagram_channel_factory.h (right):

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/datagram_c...
remoting/protocol/datagram_channel_factory.h:24: // failed for any reason.
Callback may be called synchronously, before the
On 2014/09/10 02:29:26, Wez wrote:
> nit: There is no "connection" for a datagram channel, though.

There might be in some cases. E.g. for secure DTLS channel we would want to
finish DTLS handshake before returning it, which may fail.

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/datagram_c...
remoting/protocol/datagram_channel_factory.h:27: // channels for which the
|callback| hasn't been called yet.
On 2014/09/10 02:29:26, Wez wrote:
> See comments on ChannelFactory

Done.

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/jingle_ses...
File remoting/protocol/jingle_session.cc (right):

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/jingle_ses...
remoting/protocol/jingle_session.cc:614: pseudotcp_channel_factory_.get(),
authenticator_.get()));
On 2014/09/10 02:29:26, Wez wrote:
> nit: Although it's an extra line, I'd suggest wrapping before new and before
> authenticator, for readability.

Done.

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/jingle_ses...
File remoting/protocol/jingle_session.h (right):

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/jingle_ses...
remoting/protocol/jingle_session.h:140: void OnAuthenticated();
On 2014/09/10 02:29:26, Wez wrote:
> This name becomes confusing given that it is the trigger for authentication
the
> channels, i.e. it comes *before* they are authenticated. If you rename the
> AuthenticatedChannelFactory to SecureChannelFactory then I think that works
out
> OK, though.

Done.

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/jingle_ses...
File remoting/protocol/jingle_session_unittest.cc (right):

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/jingle_ses...
remoting/protocol/jingle_session_unittest.cc:522: if (!client_callback_called) {
On 2014/09/10 02:29:26, Wez wrote:
> Why would the callback not be called, after this change?

It's not called and DCHECKs were failing because CancelChannelCreation() wasn't
called. JingleSession wasn't DCHECKing that CancelChannelCreation() is called
before the destructor (fixed it now), but SecureChannelFactory does.

There is nothing that prescribes the order in which the client and host
callbacks are called in this case, so the test hast to handle both cases when
the client callback is called and when it's not.

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/pseudotcp_...
File remoting/protocol/pseudotcp_channel_factory.cc (right):

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/pseudotcp_...
remoting/protocol/pseudotcp_channel_factory.cc:39: // ChannelFactory interface.
On 2014/09/10 02:29:27, Wez wrote:
> nit: Remove this comment

Done.

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/pseudotcp_...
remoting/protocol/pseudotcp_channel_factory.cc:63:
jingle_glue::PseudoTcpAdapter* socket =
On 2014/09/10 02:29:26, Wez wrote:
> nit: adaptor?

adaptEr for consistency :)

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/pseudotcp_...
remoting/protocol/pseudotcp_channel_factory.cc:94: // Return NULL if Connect()
failed.
On 2014/09/10 02:29:26, Wez wrote:
> nit: Does that really need a comment?

Done.

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/pseudotcp_...
File remoting/protocol/pseudotcp_channel_factory.h (right):

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/pseudotcp_...
remoting/protocol/pseudotcp_channel_factory.h:23: DatagramChannelFactory*
datagram_channel_factory);
On 2014/09/10 02:29:27, Wez wrote:
> nit: Comment on lifetime of this?

Done.

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/pseudotcp_...
remoting/protocol/pseudotcp_channel_factory.h:41: typedef std::map<std::string,
net::StreamSocket*> PendingSocketsMap;
On 2014/09/10 02:29:27, Wez wrote:
> nit: Move this to the top of private section.

Done.

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/ssl_hmac_c...
File remoting/protocol/ssl_hmac_channel_authenticator.cc (right):

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/ssl_hmac_c...
remoting/protocol/ssl_hmac_channel_authenticator.cc:288:
NotifyResult(static_cast<net::Error>(error), scoped_ptr<net::StreamSocket>());
On 2014/09/10 02:29:27, Wez wrote:
> This has undefined and potentially scary behaviour if |error| is not a valid
> enum value; consider using int in NotifyResult as well?

We will still need to convert int to net::Error because most net APIs return
int. It's better to use int everywhere for |error| - updated
ChannelAuthenticator interface now.

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/ssl_hmac_c...
File remoting/protocol/ssl_hmac_channel_authenticator.h (right):

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/ssl_hmac_c...
remoting/protocol/ssl_hmac_channel_authenticator.h:81: void
NotifyResult(net::Error error, scoped_ptr<net::StreamSocket> socket);
On 2014/09/10 02:29:27, Wez wrote:
> Add a comment to clarify what the difference between NotifyError and
> NotifyResult(net::Error error,...) is?

Renamed NotifyResult to CallDoneCallback().

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/ssl_hmac_c...
File remoting/protocol/ssl_hmac_channel_authenticator_unittest.cc (right):

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/ssl_hmac_c...
remoting/protocol/ssl_hmac_channel_authenticator_unittest.cc:114:
host_auth_.reset();
On 2014/09/10 02:29:27, Wez wrote:
> Why's this change required?

Previously SslHmacChannelAuthenticator wasn't calling the callback properly
which would lead to crash when it's deleted when the callback is called. This
change increases coverage for this case, but wouldn't catch the bug we had
before. Updated this test now so it crashes without the fix in
ssl_hmac_channel_authenticator.cc .

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/transport.h
File remoting/protocol/transport.h (right):

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/transport....
remoting/protocol/transport.h:99: virtual void Connect(const ConnectedCallback&
callback) = 0;
On 2014/09/10 02:29:27, Wez wrote:
> nit: Add a comment to clarify where the difference between Initialize and
> Connect is.

Actually we no longer need them separate. Removed Initialize().

[Wez, 2014-09-10 22:19:42]

LGTM - this is a great piece of cleanup - thanks for taking care of it!

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/authentica...
File remoting/protocol/authenticated_channel_factory.cc (right):

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/authentica...
remoting/protocol/authenticated_channel_factory.cc:24: //
CancelChannelCreation() is expected to be called before destruction.
On 2014/09/10 21:50:57, Sergey Ulanov wrote:
> On 2014/09/10 02:29:25, Wez wrote:
> > Is that a part of the ChannelFactory interface contract? If so then consider
> > removing this comment.
> 
> I think it's still useful to make it clear what this DCHECK is for.

You have an equivalent check in the PseudoTcpChannelFactory, though, and don't
have a comment there. ;)

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/datagram_c...
File remoting/protocol/datagram_channel_factory.h (right):

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/datagram_c...
remoting/protocol/datagram_channel_factory.h:24: // failed for any reason.
Callback may be called synchronously, before the
On 2014/09/10 21:50:58, Sergey Ulanov wrote:
> On 2014/09/10 02:29:26, Wez wrote:
> > nit: There is no "connection" for a datagram channel, though.
> 
> There might be in some cases. E.g. for secure DTLS channel we would want to
> finish DTLS handshake before returning it, which may fail.

True; maybe refer to it as "channel setup" rather than "connection"? Up to you,
though.

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/jingle_ses...
File remoting/protocol/jingle_session_unittest.cc (right):

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/jingle_ses...
remoting/protocol/jingle_session_unittest.cc:522: if (!client_callback_called) {
On 2014/09/10 21:50:59, Sergey Ulanov wrote:
> On 2014/09/10 02:29:26, Wez wrote:
> > Why would the callback not be called, after this change?
> 
> It's not called and DCHECKs were failing because CancelChannelCreation()
wasn't
> called. JingleSession wasn't DCHECKing that CancelChannelCreation() is called
> before the destructor (fixed it now), but SecureChannelFactory does.
> 
> There is nothing that prescribes the order in which the client and host
> callbacks are called in this case, so the test hast to handle both cases when
> the client callback is called and when it's not.

Should we make the test ordering deterministic, and break it into two tests, one
for each of the orderings, then?

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/pseudotcp_...
File remoting/protocol/pseudotcp_channel_factory.cc (right):

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/pseudotcp_...
remoting/protocol/pseudotcp_channel_factory.cc:63:
jingle_glue::PseudoTcpAdapter* socket =
On 2014/09/10 21:50:59, Sergey Ulanov wrote:
> On 2014/09/10 02:29:26, Wez wrote:
> > nit: adaptor?
> 
> adaptEr for consistency :)

:D

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/ssl_hmac_c...
File remoting/protocol/ssl_hmac_channel_authenticator_unittest.cc (right):

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/ssl_hmac_c...
remoting/protocol/ssl_hmac_channel_authenticator_unittest.cc:114:
host_auth_.reset();
On 2014/09/10 21:50:59, Sergey Ulanov wrote:
> On 2014/09/10 02:29:27, Wez wrote:
> > Why's this change required?
> 
> Previously SslHmacChannelAuthenticator wasn't calling the callback properly
> which would lead to crash when it's deleted when the callback is called. This
> change increases coverage for this case, but wouldn't catch the bug we had
> before. Updated this test now so it crashes without the fix in
> ssl_hmac_channel_authenticator.cc .

Acknowledged.

https://codereview.chromium.org/551173004/diff/40001/remoting/protocol/jingle...
File remoting/protocol/jingle_session.cc (right):

https://codereview.chromium.org/551173004/diff/40001/remoting/protocol/jingle...
remoting/protocol/jingle_session.cc:272: DCHECK(!it->second->is_connected());
Is there any possibility of a race condition here, between cancel requests and
us being notified that the underlying channel is connected?

https://codereview.chromium.org/551173004/diff/40001/remoting/protocol/libjin...
File remoting/protocol/libjingle_transport_factory.cc (right):

https://codereview.chromium.org/551173004/diff/40001/remoting/protocol/libjin...
remoting/protocol/libjingle_transport_factory.cc:144: DCHECK(!name.empty());
DCHECK(!callback.is_null())?

[Sergey Ulanov, 2014-09-10 22:49:31]

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/authentica...
File remoting/protocol/authenticated_channel_factory.cc (right):

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/authentica...
remoting/protocol/authenticated_channel_factory.cc:24: //
CancelChannelCreation() is expected to be called before destruction.
On 2014/09/10 22:19:41, Wez wrote:
> On 2014/09/10 21:50:57, Sergey Ulanov wrote:
> > On 2014/09/10 02:29:25, Wez wrote:
> > > Is that a part of the ChannelFactory interface contract? If so then
consider
> > > removing this comment.
> > 
> > I think it's still useful to make it clear what this DCHECK is for.
> 
> You have an equivalent check in the PseudoTcpChannelFactory, though, and don't
> have a comment there. ;)

Done.

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/datagram_c...
File remoting/protocol/datagram_channel_factory.h (right):

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/datagram_c...
remoting/protocol/datagram_channel_factory.h:24: // failed for any reason.
Callback may be called synchronously, before the
On 2014/09/10 22:19:41, Wez wrote:
> On 2014/09/10 21:50:58, Sergey Ulanov wrote:
> > On 2014/09/10 02:29:26, Wez wrote:
> > > nit: There is no "connection" for a datagram channel, though.
> > 
> > There might be in some cases. E.g. for secure DTLS channel we would want to
> > finish DTLS handshake before returning it, which may fail.
> 
> True; maybe refer to it as "channel setup" rather than "connection"? Up to
you,
> though.

Done.

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/jingle_ses...
File remoting/protocol/jingle_session_unittest.cc (right):

https://codereview.chromium.org/551173004/diff/1/remoting/protocol/jingle_ses...
remoting/protocol/jingle_session_unittest.cc:522: if (!client_callback_called) {
On 2014/09/10 22:19:41, Wez wrote:
> On 2014/09/10 21:50:59, Sergey Ulanov wrote:
> > On 2014/09/10 02:29:26, Wez wrote:
> > > Why would the callback not be called, after this change?
> > 
> > It's not called and DCHECKs were failing because CancelChannelCreation()
> wasn't
> > called. JingleSession wasn't DCHECKing that CancelChannelCreation() is
called
> > before the destructor (fixed it now), but SecureChannelFactory does.
> > 
> > There is nothing that prescribes the order in which the client and host
> > callbacks are called in this case, so the test hast to handle both cases
when
> > the client callback is called and when it's not.
> 
> Should we make the test ordering deterministic, and break it into two tests,
one
> for each of the orderings, then?

Done. I don't think we need two tests here - this test verifies what happens on
the side of the connection that rejects channel auth. What happens on the other
end is irrelevant.

https://codereview.chromium.org/551173004/diff/40001/remoting/protocol/jingle...
File remoting/protocol/jingle_session.cc (right):

https://codereview.chromium.org/551173004/diff/40001/remoting/protocol/jingle...
remoting/protocol/jingle_session.cc:272: DCHECK(!it->second->is_connected());
On 2014/09/10 22:19:42, Wez wrote:
> Is there any possibility of a race condition here, between cancel requests and
> us being notified that the underlying channel is connected?

I don't think so. Both events happen on the same thread.

https://codereview.chromium.org/551173004/diff/40001/remoting/protocol/libjin...
File remoting/protocol/libjingle_transport_factory.cc (right):

https://codereview.chromium.org/551173004/diff/40001/remoting/protocol/libjin...
remoting/protocol/libjingle_transport_factory.cc:144: DCHECK(!name.empty());
On 2014/09/10 22:19:42, Wez wrote:
> DCHECK(!callback.is_null())?

Done.

[Sergey Ulanov, 2014-09-10 22:49:39]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2014-09-10 22:49:43]

The CQ bit was unchecked by sergeyu@chromium.org

[Sergey Ulanov, 2014-09-10 22:49:57]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2014-09-10 22:51:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/551173004/60001

[commit-bot: I haz the power, 2014-09-10 23:57:04]

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

[commit-bot: I haz the power, 2014-09-10 23:57:07]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/54343)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/59274)
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Sergey Ulanov, 2014-09-11 18:23:11]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2014-09-11 18:24:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/551173004/80001

[commit-bot: I haz the power, 2014-09-11 19:06:22]

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

[commit-bot: I haz the power, 2014-09-11 19:06:23]

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

[Sergey Ulanov, 2014-09-11 19:55:40]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2014-09-11 20:07:41]

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

[commit-bot: I haz the power, 2014-09-11 21:56:43]

Committed patchset #5 (id:100001) as e9adf5f95329906487e4777aa89daf2b5624e150

[commit-bot: I haz the power, 2014-09-11 22:11:58]

Patchset 5 (id:??) landed as
https://crrev.com/28d886c967e016a5d5812be43cd5916f577c2e10
Cr-Commit-Position: refs/heads/master@{#294474}

[nasko, 2014-09-11 22:36:24]

A revert of this CL (patchset #5 id:100001) has been created in
https://codereview.chromium.org/570463002/ by nasko@chromium.org.

The reason for reverting is: Failed to compile on android builder.

http://build.chromium.org/p/chromium.webkit/builders/Android%20Builder/builds...

In file included from ../../remoting/protocol/pseudotcp_channel_factory.h:11:0,
                 from ../../remoting/protocol/pseudotcp_channel_factory.cc:5:
../../remoting/protocol/stream_channel_factory.h:34:41:error: 'string' in
namespace 'std' does not name a type
   virtual void CreateChannel(const std::string& name,
                                         ^
../../remoting/protocol/stream_channel_factory.h:40:49:error: 'string' in
namespace 'std' does not name a type
   virtual void CancelChannelCreation(const std::string& name) = 0;
....

[Sergey Ulanov, 2014-09-12 17:55:14]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2014-09-12 18:19:24]

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

[commit-bot: I haz the power, 2014-09-12 20:49:07]

Committed patchset #6 (id:120001) as f2553532c19671791ed8b213d8e8cc0ebe8efd51

[commit-bot: I haz the power, 2014-09-12 20:52:27]

Patchset 6 (id:??) landed as
https://crrev.com/9cb142f0cb33af89ff478e69e803fdf32f09006e
Cr-Commit-Position: refs/heads/master@{#294653}