Handle disconnection events properly for unauthenicated clients.

remoting/host/chromoting_host.cc

Index: remoting/host/chromoting_host.cc
diff --git a/remoting/host/chromoting_host.cc b/remoting/host/chromoting_host.cc
index a1285544f05a6d4bdfc8004108a684e10209688b..0bcaf1f04c536536aec4c516d7964d98cf79fce3 100644
--- a/remoting/host/chromoting_host.cc
+++ b/remoting/host/chromoting_host.cc
@@ -250,19 +250,21 @@ void ChromotingHost::OnSessionClosed(ClientSession* client) {
   CHECK(it != clients_.end());
   clients_.erase(it);
 
-  if (recorder_.get()) {
-    recorder_->RemoveConnection(client->connection());
-  }
+  if (client->is_authenticated()) {
+    if (recorder_.get()) {
+      recorder_->RemoveConnection(client->connection());

[Wez, 2012/05/18 20:20:57]

nit: It should be safe to RemoveConnection() for a connection that was never
added in the first place, so do you need this to be inside the
is_authenticated() block?

[Sergey Ulanov, 2012/05/21 18:20:13]

It doesn't make sense to remove the connection if we know it was never added.

[Wez, 2012/05/21 20:33:50]

True, but you're testing whether |recorder_| is set, so the current arrangement
is fragile in the face of changes to recorder & client lifetime, e.g. if we add
support for multiple clients then this code will break.  Always calling
RemoveConnection() makes the code more robust to those sorts of change.

[Sergey Ulanov, 2012/05/21 22:27:55]

Done.

+    }
 
-  FOR_EACH_OBSERVER(HostStatusObserver, status_observers_,
-                    OnClientDisconnected(client->client_jid()));
+    FOR_EACH_OBSERVER(HostStatusObserver, status_observers_,
+                      OnClientDisconnected(client->client_jid()));

[Wez, 2012/05/18 20:20:57]

This changes the semantics of OnClientDisconnected such that you won't see it
for clients that never authenticate; is that what you intend?

[Sergey Ulanov, 2012/05/21 18:20:13]

Yes. I think for such clients the observers will see only the OnAccessDenied()
event.

 
-  if (recorder_.get()) {
-    // Currently we don't allow more than one simultaneous connection,
-    // so we need to shutdown recorder when a client disconnects.
-    StopScreenRecorder();
+    if (recorder_.get()) {
+      // Currently we don't allow more than one simultaneous connection,
+      // so we need to shutdown recorder when a client disconnects.
+      StopScreenRecorder();
+    }
+    desktop_environment_->OnSessionFinished();
   }
-  desktop_environment_->OnSessionFinished();
 }
 
 void ChromotingHost::OnSessionSequenceNumber(ClientSession* session,