A new authenticated connection evicts an old one.

ChromotingHost can have multiple connections, but only one
authenticated connection. When a connection is
authenticated, the host disconnects all other connections.

The result is that if a client has disconnected without the
host noticing, another client can connect immediately,
without having to wait for the older connection to time out.

The new ClientSession class encapsulates a
ConnectionToClient and per-client state. It has taken the
HostStub implementation away from DesktopEnvironment.


BUG=70013
TEST=extra unit test; also see repro steps in BUG

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

[Sergey Ulanov, 2011-03-18 18:12:21]

I disagree with the way you change HostStub interface.

I think that if we want to decouple lifetime of connection from lifetime of
DesktopEnvironment, then DesktopEnvironment should not be implementing HostStub.
DesktopEnvironment should not connections about connections in any way - it just
provides a way to access screen and inject events, that's all.

The HostStub interface is inherently stateful, so it has to be handled by an
object that is one per connection, and that is aware of the connection lifetime.
Maybe create new object (e.g. ClientSession) that will implement this interface?

http://codereview.chromium.org/6711033/diff/1/remoting/protocol/host_control_...
File remoting/protocol/host_control_sender.cc (right):

http://codereview.chromium.org/6711033/diff/1/remoting/protocol/host_control_...
remoting/protocol/host_control_sender.cc:34: void
HostControlSender::BeginSessionRequest(ConnectionToClient* /*connection*/,
Why is it commented?

http://codereview.chromium.org/6711033/diff/1/remoting/protocol/host_message_...
File remoting/protocol/host_message_dispatcher.h (right):

http://codereview.chromium.org/6711033/diff/1/remoting/protocol/host_message_...
remoting/protocol/host_message_dispatcher.h:66: ConnectionToClient* connection_;
With this change, HostMessageDispatcher hold reference to ConnectionToClient,
and ConnectionToClient holds reference to HostMessageDispatcher. That is not
good, and I would like to avoid this pattern if possible,

http://codereview.chromium.org/6711033/diff/1/remoting/protocol/host_stub.h
File remoting/protocol/host_stub.h (right):

http://codereview.chromium.org/6711033/diff/1/remoting/protocol/host_stub.h#n...
remoting/protocol/host_stub.h:31: ConnectionToClient* connection,
Can we avoid adding this parameter? HostStub is supposed to be used on both side
of the connection, yet, ConnectionToClient doesn't make sense on the client
side.

[Sergey Ulanov, 2011-03-18 18:20:50]

On Fri, Mar 18, 2011 at 11:12 AM, <sergeyu@chromium.org> wrote:

> I disagree with the way you change HostStub interface.
>
> I think that if we want to decouple lifetime of connection from lifetime of
> DesktopEnvironment, then DesktopEnvironment should not be implementing
> HostStub.
> DesktopEnvironment should not connections about connections in any way - it
> just
>
I meant "should not care about connections", sorry.


> provides a way to access screen and inject events, that's all.
>
> The HostStub interface is inherently stateful, so it has to be handled by
> an
> object that is one per connection, and that is aware of the connection
> lifetime.
> Maybe create new object (e.g. ClientSession) that will implement this
> interface?
>
>
>
>
http://codereview.chromium.org/6711033/diff/1/remoting/protocol/host_control_...
> File remoting/protocol/host_control_sender.cc (right):
>
>
>
http://codereview.chromium.org/6711033/diff/1/remoting/protocol/host_control_...
> remoting/protocol/host_control_sender.cc:34: void
> HostControlSender::BeginSessionRequest(ConnectionToClient*
> /*connection*/,
> Why is it commented?
>
>
>
http://codereview.chromium.org/6711033/diff/1/remoting/protocol/host_message_...
> File remoting/protocol/host_message_dispatcher.h (right):
>
>
>
http://codereview.chromium.org/6711033/diff/1/remoting/protocol/host_message_...
> remoting/protocol/host_message_dispatcher.h:66: ConnectionToClient*
> connection_;
> With this change, HostMessageDispatcher hold reference to
> ConnectionToClient, and ConnectionToClient holds reference to
> HostMessageDispatcher. That is not good, and I would like to avoid this
> pattern if possible,
>
> http://codereview.chromium.org/6711033/diff/1/remoting/protocol/host_stub.h
> File remoting/protocol/host_stub.h (right):
>
>
>
http://codereview.chromium.org/6711033/diff/1/remoting/protocol/host_stub.h#n...
> remoting/protocol/host_stub.h:31: ConnectionToClient* connection,
> Can we avoid adding this parameter? HostStub is supposed to be used on
> both side of the connection, yet, ConnectionToClient doesn't make sense
> on the client side.
>
>
> http://codereview.chromium.org/6711033/
>

[simonmorris, 2011-03-22 13:09:09]

I've implemented a ClientSession class, as Sergey suggested.

http://codereview.chromium.org/6711033/diff/1/remoting/protocol/host_control_...
File remoting/protocol/host_control_sender.cc (right):

http://codereview.chromium.org/6711033/diff/1/remoting/protocol/host_control_...
remoting/protocol/host_control_sender.cc:34: void
HostControlSender::BeginSessionRequest(ConnectionToClient* /*connection*/,
On 2011/03/18 18:12:21, sergeyu wrote:
> Why is it commented?

Done.

http://codereview.chromium.org/6711033/diff/1/remoting/protocol/host_message_...
File remoting/protocol/host_message_dispatcher.h (right):

http://codereview.chromium.org/6711033/diff/1/remoting/protocol/host_message_...
remoting/protocol/host_message_dispatcher.h:66: ConnectionToClient* connection_;
On 2011/03/18 18:12:21, sergeyu wrote:
> With this change, HostMessageDispatcher hold reference to ConnectionToClient,
> and ConnectionToClient holds reference to HostMessageDispatcher. That is not
> good, and I would like to avoid this pattern if possible,

Done.

http://codereview.chromium.org/6711033/diff/1/remoting/protocol/host_stub.h
File remoting/protocol/host_stub.h (right):

http://codereview.chromium.org/6711033/diff/1/remoting/protocol/host_stub.h#n...
remoting/protocol/host_stub.h:31: ConnectionToClient* connection,
On 2011/03/18 18:12:21, sergeyu wrote:
> Can we avoid adding this parameter? HostStub is supposed to be used on both
side
> of the connection, yet, ConnectionToClient doesn't make sense on the client
> side.

Done.

[awong, 2011-03-22 15:11:56]

Drive-by (no need to wait for me to re-review).

Mostly some style issues.  One potential bad-behavior caused by DCHECK flagged.

I'll let Sergey do the final review + LGTM.

Thanks for making this change!  It will be very nice to not have to deal with
the timeout.

http://codereview.chromium.org/6711033/diff/6002/remoting/host/chromoting_hos...
File remoting/host/chromoting_host.cc (right):

http://codereview.chromium.org/6711033/diff/6002/remoting/host/chromoting_hos...
remoting/host/chromoting_host.cc:145: }
Do we care to call clients_.clear() here to drop all the scoped_refptrs?  Or do
we not want to potentially delete the ClientSessions yet?

http://codereview.chromium.org/6711033/diff/6002/remoting/host/chromoting_hos...
remoting/host/chromoting_host.cc:183: // the recorder.
Not completely sure what this means:

  "the only reason for running the recorder"

I think we're saying that a screen recorder only exists for an authenticated
session, correct?

Is there a race condition here if a new connection is made, authenticates, and
then suddenly disconnects?  I guess all that would happen is that both clients
would get disconnected...not worth worrying about then?

http://codereview.chromium.org/6711033/diff/6002/remoting/host/chromoting_hos...
remoting/host/chromoting_host.cc:319: NULL,
Is this parameter ever non-NULL now?

http://codereview.chromium.org/6711033/diff/6002/remoting/host/chromoting_hos...
remoting/host/chromoting_host.cc:386: std::vector<scoped_refptr<ClientSession> >
clients_copy(clients_);
Add a note for why we need to copy |clients_|.

http://codereview.chromium.org/6711033/diff/6002/remoting/host/chromoting_hos...
remoting/host/chromoting_host.cc:395: DCHECK(recorder_.get() == NULL);
This should be a hard requirement right?

I would CHECK fail here rather than DCHECK.  Otherwise, in release mode, we'd
skip this, and happily reuse the old recorder below.

http://codereview.chromium.org/6711033/diff/6002/remoting/host/chromoting_hos...
remoting/host/chromoting_host.cc:401: DCHECK(desktop_environment_->capturer());
It's not obvious why we're checking for the existence of capturer() here.  I
would have expected that similar to context_, destktop_environment_ was assumed
to be initialized once we started executing code in this class.

http://codereview.chromium.org/6711033/diff/6002/remoting/host/chromoting_host.h
File remoting/host/chromoting_host.h (right):

http://codereview.chromium.org/6711033/diff/6002/remoting/host/chromoting_hos...
remoting/host/chromoting_host.h:125: void add_client(ClientSession* client) {
Chromium style says, add_client() shoudl only be used if this is a trivial
setter.  Please rename AddClient(), and take the implementation out-of-line.

http://codereview.chromium.org/6711033/diff/6002/remoting/host/chromoting_hos...
File remoting/host/chromoting_host_unittest.cc (right):

http://codereview.chromium.org/6711033/diff/6002/remoting/host/chromoting_hos...
remoting/host/chromoting_host_unittest.cc:162: void
SimulateSecondClientConnection() {
This is nit-picky, but I dislike the lack of symmetry between
SimulateClientConnection(0) and SimulateSecondClientConnection.

I'd either do SimulateFirstConnection and SimulateSecondConnection, or bind the
argument in via a callback.  Either method would be fine with me.

For the last option, you want to look at "CreateFunctor()".  The call should be
something like "CreateFunctor(this,
&ChromotingHostTest::SimulateClientConnection, 1)" where pass the result into
Invoke I think.  Do a google.com/codesearch over the chromium sources for
CreateFunctor to see how it's done.  Note that I'm in the middle of replacing
this API with base::Callback, but CreateFunctor should work for now.

http://codereview.chromium.org/6711033/diff/6002/remoting/host/chromoting_hos...
remoting/host/chromoting_host_unittest.cc:290: InSequence s;
I'm always a little dubious when I see InSequences.  I know the pattern was here
already, but as a sanity check anyways, do we really need to guarantee the
sequence of operations to assert the invariants we are testing for in this case?

http://codereview.chromium.org/6711033/diff/6002/remoting/host/client_session.h
File remoting/host/client_session.h (right):

http://codereview.chromium.org/6711033/diff/6002/remoting/host/client_session...
remoting/host/client_session.h:42: // Disconnect this client.
client -> Clientsession

http://codereview.chromium.org/6711033/diff/6002/remoting/host/client_session...
remoting/host/client_session.h:45: protocol::ConnectionToClient* connection();
make a const method?

http://codereview.chromium.org/6711033/diff/6002/remoting/protocol/connection...
File remoting/protocol/connection_to_client.h (right):

http://codereview.chromium.org/6711033/diff/6002/remoting/protocol/connection...
remoting/protocol/connection_to_client.h:84: virtual ~ConnectionToClient();
Does this need to be virtual?

[Sergey Ulanov, 2011-03-22 23:19:14]

Looks mostly good. One more nit in addition to alberts.
I disagree with addition of support for multiple concurrent clients in
ChromotingHost (what's the point of implementing it if it doesn't really work?),
but it's ok to keep it if you believe we need it.
The next step would be to remove authentication state from stubs.

http://codereview.chromium.org/6711033/diff/6002/remoting/host/chromoting_hos...
File remoting/host/chromoting_host.cc (left):

http://codereview.chromium.org/6711033/diff/6002/remoting/host/chromoting_hos...
remoting/host/chromoting_host.cc:199: // TODO(hclam): Stop only if the last
connection disconnected.
I don't think we can remove this TODO yet. Recorder is still stopped if there
are still active connections

http://codereview.chromium.org/6711033/diff/6002/remoting/protocol/connection...
File remoting/protocol/connection_to_client.h (right):

http://codereview.chromium.org/6711033/diff/6002/remoting/protocol/connection...
remoting/protocol/connection_to_client.h:84: virtual ~ConnectionToClient();
On 2011/03/22 15:11:56, awong wrote:
> Does this need to be virtual?
I think the rule is to make destructor virtual if there is at least one virtual
method.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Inheri...

[simonmorris, 2011-03-23 09:17:52]

On 2011/03/22 23:19:14, sergeyu wrote:
> Looks mostly good. One more nit in addition to alberts.
> I disagree with addition of support for multiple concurrent clients in
> ChromotingHost (what's the point of implementing it if it doesn't really
work?),
> but it's ok to keep it if you believe we need it.

The only reason for allowing multiple unauthenticated clients in this
CL is to let a new connection evict an old one, so that a user can
reconnect immediately.

Only one authenticated client is allowed, which means, effectively,
only one client.

> The next step would be to remove authentication state from stubs.
>

Yes - I'll start work on that today.

[simonmorris, 2011-03-23 10:35:20]

http://codereview.chromium.org/6711033/diff/6002/remoting/host/chromoting_hos...
File remoting/host/chromoting_host.cc (left):

http://codereview.chromium.org/6711033/diff/6002/remoting/host/chromoting_hos...
remoting/host/chromoting_host.cc:199: // TODO(hclam): Stop only if the last
connection disconnected.
On 2011/03/22 23:19:14, sergeyu wrote:
> I don't think we can remove this TODO yet. Recorder is still stopped if there
> are still active connections

Done.

http://codereview.chromium.org/6711033/diff/6002/remoting/host/chromoting_hos...
File remoting/host/chromoting_host.cc (right):

http://codereview.chromium.org/6711033/diff/6002/remoting/host/chromoting_hos...
remoting/host/chromoting_host.cc:145: }
Added clients_.clear() .

http://codereview.chromium.org/6711033/diff/6002/remoting/host/chromoting_hos...
remoting/host/chromoting_host.cc:183: // the recorder.
Clarified comment.

I don't think that scenario causes a problem.

http://codereview.chromium.org/6711033/diff/6002/remoting/host/chromoting_hos...
remoting/host/chromoting_host.cc:319: NULL,
Only in unit tests, and it's better to be more concise here
at the expense of verbosity there. Removed that parameter.

http://codereview.chromium.org/6711033/diff/6002/remoting/host/chromoting_hos...
remoting/host/chromoting_host.cc:386: std::vector<scoped_refptr<ClientSession> >
clients_copy(clients_);
On 2011/03/22 15:11:56, awong wrote:
> Add a note for why we need to copy |clients_|.

Done.

http://codereview.chromium.org/6711033/diff/6002/remoting/host/chromoting_hos...
remoting/host/chromoting_host.cc:395: DCHECK(recorder_.get() == NULL);
On 2011/03/22 15:11:56, awong wrote:
> This should be a hard requirement right?
> 
> I would CHECK fail here rather than DCHECK.  Otherwise, in release mode, we'd
> skip this, and happily reuse the old recorder below.

Done.

http://codereview.chromium.org/6711033/diff/6002/remoting/host/chromoting_hos...
remoting/host/chromoting_host.cc:401: DCHECK(desktop_environment_->capturer());
A comment elsewhere suggests that this test is a relic of old
code. Deleted.

http://codereview.chromium.org/6711033/diff/6002/remoting/host/chromoting_host.h
File remoting/host/chromoting_host.h (right):

http://codereview.chromium.org/6711033/diff/6002/remoting/host/chromoting_hos...
remoting/host/chromoting_host.h:125: void add_client(ClientSession* client) {
On 2011/03/22 15:11:56, awong wrote:
> Chromium style says, add_client() shoudl only be used if this is a trivial
> setter.  Please rename AddClient(), and take the implementation out-of-line.

Done.

http://codereview.chromium.org/6711033/diff/6002/remoting/host/chromoting_hos...
File remoting/host/chromoting_host_unittest.cc (right):

http://codereview.chromium.org/6711033/diff/6002/remoting/host/chromoting_hos...
remoting/host/chromoting_host_unittest.cc:162: void
SimulateSecondClientConnection() {
Done (with CreateFunctor).

http://codereview.chromium.org/6711033/diff/6002/remoting/host/chromoting_hos...
remoting/host/chromoting_host_unittest.cc:290: InSequence s;
I think it's worth knowing that no video is sent to
connection 1 after any video is sent to connection 2: it's a
(weak) test that connection 1 has been disconnected.

http://codereview.chromium.org/6711033/diff/6002/remoting/host/client_session.h
File remoting/host/client_session.h (right):

http://codereview.chromium.org/6711033/diff/6002/remoting/host/client_session...
remoting/host/client_session.h:42: // Disconnect this client.
On 2011/03/22 15:11:56, awong wrote:
> client -> Clientsession

Done.

http://codereview.chromium.org/6711033/diff/6002/remoting/host/client_session...
remoting/host/client_session.h:45: protocol::ConnectionToClient* connection();
On 2011/03/22 15:11:56, awong wrote:
> make a const method?

Done.

http://codereview.chromium.org/6711033/diff/6002/remoting/protocol/connection...
File remoting/protocol/connection_to_client.h (right):

http://codereview.chromium.org/6711033/diff/6002/remoting/protocol/connection...
remoting/protocol/connection_to_client.h:84: virtual ~ConnectionToClient();
Yes: this is a base class for MockConnectionToClient.