Delete Session and SessionManager object synchronously.

JingleSession and JingleSessionManager were previously deleted
asychronously, but it is not necessary anymore arter we've switched to
PepperSession and PepperSessionManager. Instead changed 
LibjingleTransportFactory to delete libjingle objects asynchronously.
Also fixed some bugs related that would cause crash when channel auth 
fails.

BUG=115374

Review URL: http://codereview.chromium.org/9433027

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@123436 0039d316-1c4b-4281-b951-d872f2087c98

9 files changed, 136 insertions, 70 deletions

diff --git a/jingle/glue/pseudotcp_adapter.cc b/jingle/glue/pseudotcp_adapter.cc
index cb6ed91..b5da932 100644
--- a/jingle/glue/pseudotcp_adapter.cc
+++ b/jingle/glue/pseudotcp_adapter.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// 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.
 
@@ -53,6 +53,8 @@ class PseudoTcpAdapter::Core : public cricket::IPseudoTcpNotify,
   void SetReceiveBufferSize(int32 size);
   void SetSendBufferSize(int32 size);
 
+  void DeleteSocket();
+
  private:
   // These are invoked by the underlying Socket, and may trigger callbacks.
   // They hold a reference to |this| while running, to protect from deletion.
@@ -282,6 +284,10 @@ void PseudoTcpAdapter::Core::SetSendBufferSize(int32 size) {
   pseudo_tcp_.SetOption(cricket::PseudoTcp::OPT_SNDBUF, size);
 }
 
+void PseudoTcpAdapter::Core::DeleteSocket() {
+  socket_.reset();
+}
+
 cricket::IPseudoTcpNotify::WriteResult PseudoTcpAdapter::Core::TcpWritePacket(
     PseudoTcp* tcp,
     const char* buffer,
@@ -299,9 +305,14 @@ cricket::IPseudoTcpNotify::WriteResult PseudoTcpAdapter::Core::TcpWritePacket(
 
   // Our underlying socket is datagram-oriented, which means it should either
   // send exactly as many bytes as we requested, or fail.
-  int result = socket_->Write(write_buffer, len,
-                              base::Bind(&PseudoTcpAdapter::Core::OnWritten,
-                                         base::Unretained(this)));
+  int result;
+  if (socket_.get()) {
+    result = socket_->Write(write_buffer, len,
+                            base::Bind(&PseudoTcpAdapter::Core::OnWritten,
+                                       base::Unretained(this)));
+  } else {
+    result = net::ERR_CONNECTION_CLOSED;
+  }
   if (result == net::ERR_IO_PENDING) {
     socket_write_pending_ = true;
     return IPseudoTcpNotify::WR_SUCCESS;
@@ -318,14 +329,13 @@ void PseudoTcpAdapter::Core::DoReadFromSocket() {
   if (!socket_read_buffer_)
     socket_read_buffer_ = new net::IOBuffer(kReadBufferSize);
 
-  while (true) {
-    int result = socket_->Read(socket_read_buffer_, kReadBufferSize,
-                               base::Bind(&PseudoTcpAdapter::Core::OnRead,
-                                          base::Unretained(this)));
-    if (result == net::ERR_IO_PENDING)
-      break;
-
-    HandleReadResults(result);
+  int result = 1;
+  while (socket_.get() && result > 0) {
+    result = socket_->Read(socket_read_buffer_, kReadBufferSize,
+                           base::Bind(&PseudoTcpAdapter::Core::OnRead,
+                                      base::Unretained(this)));
+    if (result != net::ERR_IO_PENDING)
+      HandleReadResults(result);
   }
 }
 
@@ -385,6 +395,9 @@ PseudoTcpAdapter::PseudoTcpAdapter(net::Socket* socket)
 
 PseudoTcpAdapter::~PseudoTcpAdapter() {
   Disconnect();
+
+  // Make sure that the underlying socket is destroyed before PseudoTcp.
+  core_->DeleteSocket();
 }
 
 int PseudoTcpAdapter::Read(net::IOBuffer* buffer, int buffer_size,
diff --git a/remoting/host/chromoting_host.cc b/remoting/host/chromoting_host.cc
index 920ad1e..0b9ee8f 100644
--- a/remoting/host/chromoting_host.cc
+++ b/remoting/host/chromoting_host.cc
@@ -39,7 +39,6 @@ ChromotingHost::ChromotingHost(
     : context_(context),
       desktop_environment_(environment),
       network_settings_(network_settings),
-      have_shared_secret_(false),
       signal_strategy_(signal_strategy),
       stopping_recorders_(0),
       state_(kInitial),
@@ -101,18 +100,10 @@ void ChromotingHost::Shutdown(const base::Closure& shutdown_task) {
     clients_.front()->Disconnect();
   }
 
-  // Stop session manager.
-  if (session_manager_.get()) {
-    session_manager_->Close();
-    // It may not be safe to delete |session_manager_| here becase
-    // this method may be invoked in response to a libjingle event and
-    // libjingle's sigslot doesn't handle it properly, so postpone the
-    // deletion.
-    context_->network_message_loop()->DeleteSoon(
-        FROM_HERE, session_manager_.release());
-    have_shared_secret_ = false;
-  }
+  // Destroy session manager.
+  session_manager_.reset();
 
+  // Stop screen recorder
   if (recorder_.get()) {
     StopScreenRecorder();
   } else if (!stopping_recorders_) {
diff --git a/remoting/host/chromoting_host.h b/remoting/host/chromoting_host.h
index dc06acc..dd1e1cb 100644
--- a/remoting/host/chromoting_host.h
+++ b/remoting/host/chromoting_host.h
@@ -177,15 +177,6 @@ class ChromotingHost : public base::RefCountedThreadSafe<ChromotingHost>,
   DesktopEnvironment* desktop_environment_;
   protocol::NetworkSettings network_settings_;
 
-  // TODO(lambroslambrou): The following is a temporary fix for Me2Me
-  // (crbug.com/105995), pending the AuthenticatorFactory work.
-  // Cache the shared secret, in case SetSharedSecret() is called before the
-  // session manager has been created.
-  // The |have_shared_secret_| flag is to distinguish SetSharedSecret() not
-  // being called at all, from being called with an empty string.
-  std::string shared_secret_;
-  bool have_shared_secret_;
-
   // Connection objects.
   SignalStrategy* signal_strategy_;
   scoped_ptr<protocol::SessionManager> session_manager_;
diff --git a/remoting/protocol/connection_to_client.cc b/remoting/protocol/connection_to_client.cc
index 69b1624..b88cb06 100644
--- a/remoting/protocol/connection_to_client.cc
+++ b/remoting/protocol/connection_to_client.cc
@@ -31,10 +31,6 @@ ConnectionToClient::ConnectionToClient(protocol::Session* session)
 }
 
 ConnectionToClient::~ConnectionToClient() {
-  if (session_.get()) {
-    base::MessageLoopProxy::current()->DeleteSoon(
-        FROM_HERE, session_.release());
-  }
 }
 
 void ConnectionToClient::SetEventHandler(EventHandler* event_handler) {
@@ -53,12 +49,7 @@ void ConnectionToClient::Disconnect() {
   CloseChannels();
 
   DCHECK(session_.get());
-  Session* session = session_.release();
-
-  // It may not be safe to delete |session_| here becase this method
-  // may be invoked in resonse to a libjingle event and libjingle's
-  // sigslot doesn't handle it properly, so postpone the deletion.
-  base::MessageLoopProxy::current()->DeleteSoon(FROM_HERE, session);
+  scoped_ptr<Session> session = session_.Pass();
 
   // This should trigger OnConnectionClosed() event and this object
   // may be destroyed as the result.
diff --git a/remoting/protocol/fake_authenticator.cc b/remoting/protocol/fake_authenticator.cc
index 1790464..52c78c0 100644
--- a/remoting/protocol/fake_authenticator.cc
+++ b/remoting/protocol/fake_authenticator.cc
@@ -6,6 +6,7 @@
 
 #include "base/message_loop.h"
 #include "base/string_number_conversions.h"
+#include "net/base/io_buffer.h"
 #include "net/socket/stream_socket.h"
 #include "remoting/base/constants.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -15,8 +16,10 @@ namespace remoting {
 namespace protocol {
 
 FakeChannelAuthenticator::FakeChannelAuthenticator(bool accept, bool async)
-    : accept_(accept),
+    : result_(accept ? net::OK : net::ERR_FAILED),
       async_(async),
+      did_read_bytes_(false),
+      did_write_bytes_(false),
       ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)) {
 }
 
@@ -26,29 +29,49 @@ FakeChannelAuthenticator::~FakeChannelAuthenticator() {
 void FakeChannelAuthenticator::SecureAndAuthenticate(
     scoped_ptr<net::StreamSocket> socket,
     const DoneCallback& done_callback) {
-  net::Error error;
-
-  if (accept_) {
-    error = net::OK;
-  } else {
-    error = net::ERR_FAILED;
-    socket.reset();
-  }
+  socket_ = socket.Pass();
 
   if (async_) {
-    MessageLoop::current()->PostTask(FROM_HERE, base::Bind(
-        &FakeChannelAuthenticator::CallCallback, weak_factory_.GetWeakPtr(),
-        done_callback, error, base::Passed(&socket)));
+    done_callback_ = done_callback;
+
+    scoped_refptr<net::IOBuffer> write_buf = new net::IOBuffer(1);
+    write_buf->data()[0] = 0;
+    int result = socket_->Write(
+        write_buf, 1, base::Bind(&FakeChannelAuthenticator::OnAuthBytesWritten,
+                                 weak_factory_.GetWeakPtr()));
+    if (result != net::ERR_IO_PENDING) {
+      // This will not call the callback because |did_read_bytes_| is
+      // still set to false.
+      OnAuthBytesWritten(result);
+    }
+
+    scoped_refptr<net::IOBuffer> read_buf = new net::IOBuffer(1);
+    result = socket_->Read(
+        read_buf, 1, base::Bind(&FakeChannelAuthenticator::OnAuthBytesRead,
+                                weak_factory_.GetWeakPtr()));
+    if (result != net::ERR_IO_PENDING)
+      OnAuthBytesRead(result);
   } else {
-    done_callback.Run(error, socket.Pass());
+    if (result_ != net::OK)
+      socket_.reset();
+    done_callback.Run(result_, socket_.Pass());
   }
 }
 
-void FakeChannelAuthenticator::CallCallback(
-    const DoneCallback& done_callback,
-    net::Error error,
-    scoped_ptr<net::StreamSocket> socket) {
-  done_callback.Run(error, socket.Pass());
+void FakeChannelAuthenticator::OnAuthBytesWritten(int result) {
+  EXPECT_EQ(1, result);
+  EXPECT_FALSE(did_write_bytes_);
+  did_write_bytes_ = true;
+  if (did_read_bytes_)
+    done_callback_.Run(result_, socket_.Pass());
+}
+
+void FakeChannelAuthenticator::OnAuthBytesRead(int result) {
+  EXPECT_EQ(1, result);
+  EXPECT_FALSE(did_read_bytes_);
+  did_read_bytes_ = true;
+  if (did_write_bytes_)
+    done_callback_.Run(result_, socket_.Pass());
 }
 
 FakeAuthenticator::FakeAuthenticator(
diff --git a/remoting/protocol/fake_authenticator.h b/remoting/protocol/fake_authenticator.h
index 1363f29..2e60758 100644
--- a/remoting/protocol/fake_authenticator.h
+++ b/remoting/protocol/fake_authenticator.h
@@ -5,6 +5,7 @@
 #ifndef REMOTING_PROTOCOL_FAKE_AUTHENTICATOR_H_
 #define REMOTING_PROTOCOL_FAKE_AUTHENTICATOR_H_
 
+#include "base/callback.h"
 #include "base/memory/weak_ptr.h"
 #include "remoting/protocol/authenticator.h"
 #include "remoting/protocol/channel_authenticator.h"
@@ -24,13 +25,21 @@ class FakeChannelAuthenticator : public ChannelAuthenticator {
 
  private:
   void CallCallback(
-      const DoneCallback& done_callback,
       net::Error error,
       scoped_ptr<net::StreamSocket> socket);
 
-  bool accept_;
+  void OnAuthBytesWritten(int result);
+  void OnAuthBytesRead(int result);
+
+  net::Error result_;
   bool async_;
 
+  scoped_ptr<net::StreamSocket> socket_;
+  DoneCallback done_callback_;
+
+  bool did_read_bytes_;
+  bool did_write_bytes_;
+
   base::WeakPtrFactory<FakeChannelAuthenticator> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(FakeChannelAuthenticator);
diff --git a/remoting/protocol/libjingle_transport_factory.cc b/remoting/protocol/libjingle_transport_factory.cc
index c2665da..3dbedf7 100644
--- a/remoting/protocol/libjingle_transport_factory.cc
+++ b/remoting/protocol/libjingle_transport_factory.cc
@@ -4,6 +4,7 @@
 
 #include "remoting/protocol/libjingle_transport_factory.h"
 
+#include "base/message_loop_proxy.h"
 #include "jingle/glue/channel_socket_adapter.h"
 #include "jingle/glue/pseudotcp_adapter.h"
 #include "jingle/glue/utils.h"
@@ -97,6 +98,15 @@ LibjingleStreamTransport::~LibjingleStreamTransport() {
   event_handler_->OnTransportDeleted(this);
   // Channel should be already destroyed if we were connected.
   DCHECK(!is_connected() || socket_.get() == NULL);
+
+  if (channel_.get()) {
+    base::MessageLoopProxy::current()->DeleteSoon(
+        FROM_HERE, channel_.release());
+  }
+  if (port_allocator_.get()) {
+    base::MessageLoopProxy::current()->DeleteSoon(
+        FROM_HERE, port_allocator_.release());
+  }
 }
 
 void LibjingleStreamTransport::Initialize(
@@ -160,6 +170,7 @@ void LibjingleStreamTransport::Connect(
   // Create net::Socket adapter for the P2PTransportChannel.
   scoped_ptr<jingle_glue::TransportChannelSocketAdapter> channel_adapter(
       new jingle_glue::TransportChannelSocketAdapter(channel_.get()));
+
   channel_adapter->SetOnDestroyedCallback(base::Bind(
       &LibjingleStreamTransport::OnChannelDestroyed, base::Unretained(this)));
 
@@ -273,13 +284,27 @@ void LibjingleStreamTransport::OnChannelDestroyed() {
 void LibjingleStreamTransport::NotifyConnected(
     scoped_ptr<net::StreamSocket> socket) {
   DCHECK(!is_connected());
-  callback_.Run(socket.Pass());
+  StreamTransport::ConnectedCallback callback = callback_;
   callback_.Reset();
+  callback.Run(socket.Pass());
 }
 
 void LibjingleStreamTransport::NotifyConnectFailed() {
+  DCHECK(!is_connected());
+
   socket_.reset();
-  channel_.reset();
+
+  // This method may be called in response to a libjingle signal, so
+  // libjingle objects must be deleted asynchronously.
+  if (channel_.get()) {
+    base::MessageLoopProxy::current()->DeleteSoon(
+        FROM_HERE, channel_.release());
+  }
+  if (port_allocator_.get()) {
+    base::MessageLoopProxy::current()->DeleteSoon(
+        FROM_HERE, port_allocator_.release());
+  }
+
   authenticator_.reset();
 
   NotifyConnected(scoped_ptr<net::StreamSocket>(NULL));
@@ -294,6 +319,12 @@ LibjingleTransportFactory::LibjingleTransportFactory()
 }
 
 LibjingleTransportFactory::~LibjingleTransportFactory() {
+  // This method may be called in response to a libjingle signal, so
+  // libjingle objects must be deleted asynchronously.
+  base::MessageLoopProxy::current()->DeleteSoon(
+      FROM_HERE, socket_factory_.release());
+  base::MessageLoopProxy::current()->DeleteSoon(
+      FROM_HERE, network_manager_.release());
 }
 
 scoped_ptr<StreamTransport> LibjingleTransportFactory::CreateStreamTransport() {
diff --git a/remoting/protocol/pepper_session_unittest.cc b/remoting/protocol/pepper_session_unittest.cc
index f889984..edf6f75 100644
--- a/remoting/protocol/pepper_session_unittest.cc
+++ b/remoting/protocol/pepper_session_unittest.cc
@@ -226,7 +226,7 @@ class PepperSessionTest : public testing::Test {
     message_loop_.RunAllPending();
   }
 
-  void CreateChannel() {
+  void CreateChannel(bool expect_fail) {
     client_session_->CreateStreamChannel(kChannelName, base::Bind(
         &PepperSessionTest::OnClientChannelCreated, base::Unretained(this)));
     host_session_->CreateStreamChannel(kChannelName, base::Bind(
@@ -238,6 +238,14 @@ class PepperSessionTest : public testing::Test {
     EXPECT_CALL(host_channel_callback_, OnDone(_))
         .WillOnce(QuitThreadOnCounter(&counter));
     message_loop_.Run();
+
+    if (expect_fail) {
+      // At least one socket should fail to connect.
+      EXPECT_TRUE((!client_socket_.get()) || (!host_socket_.get()));
+    } else {
+      EXPECT_TRUE(client_socket_.get());
+      EXPECT_TRUE(host_socket_.get());
+    }
   }
 
   JingleThreadMessageLoop message_loop_;
@@ -323,13 +331,13 @@ TEST_F(PepperSessionTest, ConnectWithBadMultistepAuth) {
   InitiateConnection(3, FakeAuthenticator::ACCEPT, true);
 }
 
-// Verify that data can sent over stream channel.
+// Verify that data can be sent over stream channel.
 TEST_F(PepperSessionTest, TestStreamChannel) {
   CreateSessionManagers(1, FakeAuthenticator::ACCEPT);
   ASSERT_NO_FATAL_FAILURE(
       InitiateConnection(1, FakeAuthenticator::ACCEPT, false));
 
-  ASSERT_NO_FATAL_FAILURE(CreateChannel());
+  ASSERT_NO_FATAL_FAILURE(CreateChannel(false));
 
   StreamConnectionTester tester(host_socket_.get(), client_socket_.get(),
                                 kMessageSize, kMessages);
@@ -344,7 +352,7 @@ TEST_F(PepperSessionTest, TestMultistepAuthStreamChannel) {
   ASSERT_NO_FATAL_FAILURE(
       InitiateConnection(3, FakeAuthenticator::ACCEPT, false));
 
-  ASSERT_NO_FATAL_FAILURE(CreateChannel());
+  ASSERT_NO_FATAL_FAILURE(CreateChannel(false));
 
   StreamConnectionTester tester(host_socket_.get(), client_socket_.get(),
                                 kMessageSize, kMessages);
@@ -353,5 +361,14 @@ TEST_F(PepperSessionTest, TestMultistepAuthStreamChannel) {
   tester.CheckResults();
 }
 
+// Verify that we shutdown properly when channel authentication fails.
+TEST_F(PepperSessionTest, TestFailedChannelAuth) {
+  CreateSessionManagers(1, FakeAuthenticator::ACCEPT);
+  ASSERT_NO_FATAL_FAILURE(
+      InitiateConnection(1, FakeAuthenticator::REJECT_CHANNEL, false));
+
+  ASSERT_NO_FATAL_FAILURE(CreateChannel(true));
+}
+
 }  // namespace protocol
 }  // namespace remoting
diff --git a/remoting/protocol/pepper_transport_factory.cc b/remoting/protocol/pepper_transport_factory.cc
index 87ec5d4..5f63712 100644
--- a/remoting/protocol/pepper_transport_factory.cc
+++ b/remoting/protocol/pepper_transport_factory.cc
@@ -254,8 +254,8 @@ void PepperStreamTransport::OnAuthenticationDone(
 void PepperStreamTransport::NotifyConnected(
     scoped_ptr<net::StreamSocket> socket) {
   DCHECK(!connected_);
-  callback_.Run(socket.Pass());
   connected_ = true;
+  callback_.Run(socket.Pass());
 }
 
 void PepperStreamTransport::NotifyConnectFailed() {