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

6 files changed, 108 insertions, 37 deletions

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() {