Fix crashes in ChromotingHost

remoting/host/chromoting_host.cc

 // Copyright (c) 2010 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/stl_util-inl.h"
 #include "base/task.h"
 #include "build/build_config.h"
 #include "remoting/base/constants.h"
 #include "remoting/base/encoder.h"
 #include "remoting/base/encoder_row_based.h"
 #include "remoting/base/encoder_vp8.h"
 #include "remoting/host/capturer.h"
 #include "remoting/host/chromoting_host_context.h"
 #include "remoting/host/event_executor.h"
 #include "remoting/host/host_config.h"
 #include "remoting/host/host_stub_fake.h"
 #include "remoting/host/screen_recorder.h"
 #include "remoting/protocol/connection_to_client.h"
 #include "remoting/protocol/host_stub.h"
 #include "remoting/protocol/input_stub.h"
 #include "remoting/protocol/jingle_session_manager.h"
 #include "remoting/protocol/session_config.h"
 
 using remoting::protocol::ConnectionToClient;
+using remoting::protocol::InputStub;
 
 namespace remoting {
 
 // static
 ChromotingHost* ChromotingHost::Create(ChromotingHostContext* context,
                                        MutableHostConfig* config) {
-  return Create(context, config,
-                Capturer::Create(context->main_message_loop()));
+  Capturer* capturer = Capturer::Create(context->main_message_loop());
+  InputStub* input_stub = CreateEventExecutor(context->main_message_loop(),
+                                              capturer);
+  return Create(context, config, capturer, input_stub);
 }
 
 // static
 ChromotingHost* ChromotingHost::Create(ChromotingHostContext* context,
                                        MutableHostConfig* config,
-                                       Capturer* capturer) {
-  return new ChromotingHost(context, config, capturer);
+                                       Capturer* capturer,
+                                       InputStub* input_stub) {
+  return new ChromotingHost(context, config, capturer, input_stub);
 }
 
 ChromotingHost::ChromotingHost(ChromotingHostContext* context,
-                               MutableHostConfig* config, Capturer* capturer)
+                               MutableHostConfig* config,
+                               Capturer* capturer,
+                               InputStub* input_stub)
     : context_(context),
       config_(config),
       capturer_(capturer),
-      input_stub_(CreateEventExecutor(
-          context->main_message_loop(), capturer)),
+      input_stub_(input_stub),
       host_stub_(new HostStubFake()),
       state_(kInitial),
       protocol_config_(protocol::CandidateSessionConfig::CreateDefault()) {
 }
 
 
 ChromotingHost::~ChromotingHost() {
 }
 
 void ChromotingHost::Start(Task* shutdown_task) {
@@ skipping 53 matching lines @@
   // No-op if this object is not started yet.
   {
     AutoLock auto_lock(lock_);
     if (state_ != kStarted) {
       state_ = kStopped;
       return;
     }
     state_ = kStopped;
   }
 
-  // Tell the session to stop and then disconnect all clients.
-  if (recorder_.get()) {
-    recorder_->Stop(NULL);
-    recorder_->RemoveAllConnections();
-  }
-
   // Disconnect the client.
   if (connection_) {
     connection_->Disconnect();
   }
 
   // Stop the heartbeat sender.
   if (heartbeat_sender_) {
     heartbeat_sender_->Stop();
   }
 
   // Stop chromotocol session manager.
   if (session_manager_) {
     session_manager_->Close(
         NewRunnableMethod(this, &ChromotingHost::OnServerClosed));
   }
 
   // Disconnect from the talk network.
   if (jingle_client_) {
     jingle_client_->Close();
   }
 
-  // Lastly call the shutdown task.
-  if (shutdown_task_.get()) {
+  // Tell the recorder to stop and then disconnect all clients.
+  if (recorder_.get()) {
+    recorder_->RemoveAllConnections();
+    recorder_->Stop(shutdown_task_.release());
+  } else {
     shutdown_task_->Run();
+    shutdown_task_.reset();
   }
 }
 
 // This method is called when a client connects.
 void ChromotingHost::OnClientConnected(ConnectionToClient* connection) {
   DCHECK_EQ(context_->main_message_loop(), MessageLoop::current());
 
   // Create a new RecordSession if there was none.
   if (!recorder_.get()) {
     // Then we create a ScreenRecorder passing the message loops that
     // it should run on.
     DCHECK(capturer_.get());
 
     Encoder* encoder = CreateEncoder(connection->session()->config());
 
     recorder_ = new ScreenRecorder(context_->main_message_loop(),
                                    context_->encode_message_loop(),
                                    context_->network_message_loop(),
-                                   capturer_.release(),
+                                   capturer_.get(),
                                    encoder);
   }
 
   // Immediately add the connection and start the session.
   recorder_->AddConnection(connection);
   recorder_->Start();
   VLOG(1) << "Session manager started";
 }
 
 void ChromotingHost::OnClientDisconnected(ConnectionToClient* connection) {
   DCHECK_EQ(context_->main_message_loop(), MessageLoop::current());
 
   // Remove the connection from the session manager and stop the session.
   // TODO(hclam): Stop only if the last connection disconnected.
   if (recorder_.get()) {
     recorder_->RemoveConnection(connection);
     recorder_->Stop(NULL);

[Sergey Ulanov, 2011/01/20 19:51:26]

Why don't we pass shutdown_task_ after the client is disconnected?

[Alpha Left Google, 2011/01/20 20:04:31]

shutdown_task_ is used only if Shutdown() is called by the client or
ChromotingHost fails to connect to talk network which is catastrophic failure.
In case of user disconnect we don't want to shutdown everything.

The use of shutdown task is to wake up the main message loop to stop all
chromoting threads.

+    recorder_ = NULL;
   }
 
   // Close the connection to connection just to be safe.
   connection->Disconnect();
 
   // Also remove reference to ConnectionToClient from this object.
   connection_ = NULL;
 }
 
 ////////////////////////////////////////////////////////////////////////////
 // protocol::ConnectionToClient::EventHandler implementations
 void ChromotingHost::OnConnectionOpened(ConnectionToClient* connection) {
   DCHECK_EQ(context_->network_message_loop(), MessageLoop::current());
 
   // Completes the connection to the client.
   VLOG(1) << "Connection to client established.";
   context_->main_message_loop()->PostTask(
       FROM_HERE,
       NewRunnableMethod(this, &ChromotingHost::OnClientConnected,
-                        connection_));
+                        make_scoped_refptr(connection)));

[Sergey Ulanov, 2011/01/20 19:51:26]

We don't really need this argument in OnClientConnected, the connection is
stored in connection_. Add DCHECK_EQ(connection, connection_), and remove
connection argument from OnClientConnected()?

[Alpha Left Google, 2011/01/20 20:04:31]

|connection| is more correct because it's referring to the current connection,
|connection_| was a bad usage since it assumes both variables are the same, this
will not hold when there's more than one connection.

 }
 
 void ChromotingHost::OnConnectionClosed(ConnectionToClient* connection) {
   DCHECK_EQ(context_->network_message_loop(), MessageLoop::current());
 
   VLOG(1) << "Connection to client closed.";
   context_->main_message_loop()->PostTask(
       FROM_HERE,
       NewRunnableMethod(this, &ChromotingHost::OnClientDisconnected,
-                        connection_));
+                        make_scoped_refptr(connection)));

[Sergey Ulanov, 2011/01/20 19:51:26]

same here

 }
 
 void ChromotingHost::OnConnectionFailed(ConnectionToClient* connection) {
   DCHECK_EQ(context_->network_message_loop(), MessageLoop::current());
 
   LOG(ERROR) << "Connection failed unexpectedly.";
   context_->main_message_loop()->PostTask(
       FROM_HERE,
       NewRunnableMethod(this, &ChromotingHost::OnClientDisconnected,
-                        connection_));
+                        make_scoped_refptr(connection)));

[Sergey Ulanov, 2011/01/20 19:51:26]

and here

 }
 
 ////////////////////////////////////////////////////////////////////////////
 // JingleClient::Callback implementations
 void ChromotingHost::OnStateChange(JingleClient* jingle_client,
                                    JingleClient::State state) {
   if (state == JingleClient::CONNECTED) {
     DCHECK_EQ(jingle_client_.get(), jingle_client);
     VLOG(1) << "Host connected as " << jingle_client->GetFullJid();
 
@@ skipping 52 matching lines @@
   }
 
   session->set_config(config);
   session->set_receiver_token(
       GenerateHostAuthToken(session->initiator_token()));
 
   *response = protocol::SessionManager::ACCEPT;
 
   VLOG(1) << "Client connected: " << session->jid();
 
-  // If we accept the connected then create a client object and set the
-  // callback.
+  // If we accept the connected then create a connection object.
   connection_ = new ConnectionToClient(context_->network_message_loop(),
                                        this, host_stub_.get(),
                                        input_stub_.get());
   connection_->Init(session);
 }
 
 void ChromotingHost::set_protocol_config(
     protocol::CandidateSessionConfig* config) {
   DCHECK(config_.get());
   DCHECK_EQ(state_, kInitial);
@@ skipping 23 matching lines @@
   return NULL;
 }
 
 std::string ChromotingHost::GenerateHostAuthToken(
     const std::string& encoded_client_token) {
   // TODO(ajwong): Return the signature of this instead.
   return encoded_client_token;
 }
 
 }  // namespace remoting