Handle disconnection events properly for unauthenicated clients.

remoting/host/chromoting_host.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "remoting/host/chromoting_host.h"
 
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/logging.h"
 #include "base/message_loop_proxy.h"
@@ skipping 232 matching lines @@
 
 void ChromotingHost::OnSessionClosed(ClientSession* client) {
   DCHECK(context_->network_message_loop()->BelongsToCurrentThread());
 
   scoped_ptr<ClientSession> client_destroyer(client);
 
   ClientList::iterator it = std::find(clients_.begin(), clients_.end(), 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()));

[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();
+    }
+    desktop_environment_->OnSessionFinished();
   }
-
-  FOR_EACH_OBSERVER(HostStatusObserver, status_observers_,
-                    OnClientDisconnected(client->client_jid()));
-
-  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();
 }
 
 void ChromotingHost::OnSessionSequenceNumber(ClientSession* session,
                                              int64 sequence_number) {
   DCHECK(context_->network_message_loop()->BelongsToCurrentThread());
   if (recorder_.get())
     recorder_->UpdateSequenceNumber(sequence_number);
 }
 
 void ChromotingHost::OnSessionRouteChange(
@@ skipping 152 matching lines @@
                     OnShutdown());
 
   for (std::vector<base::Closure>::iterator it = shutdown_tasks_.begin();
        it != shutdown_tasks_.end(); ++it) {
     it->Run();
   }
   shutdown_tasks_.clear();
 }
 
 }  // namespace remoting