Delete Session and SessionManager object synchronously.

remoting/protocol/libjingle_transport_factory.cc

Index: remoting/protocol/libjingle_transport_factory.cc
diff --git a/remoting/protocol/libjingle_transport_factory.cc b/remoting/protocol/libjingle_transport_factory.cc
index c2665da3daac9d03fd2a444622dc737b6b3f03a6..ef11f41ff19eff941088c0f71cc9cc6c3d44ae7d 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"
@@ -77,6 +78,8 @@ class LibjingleStreamTransport : public StreamTransport,
   cricket::HttpPortAllocator* http_port_allocator_;
   scoped_ptr<cricket::P2PTransportChannel> channel_;
 
+  jingle_glue::TransportChannelSocketAdapter* channel_adapter_;

[Wez, 2012/02/23 19:33:42]

Is this needed?

[Sergey Ulanov, 2012/02/23 22:10:06]

No. Removed.

+
   // We own |socket_| until it is connected.
   scoped_ptr<jingle_glue::PseudoTcpAdapter> socket_;
 
@@ -97,6 +100,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,8 +172,6 @@ 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)));
 
   // Configure and connect PseudoTCP adapter.
   socket_.reset(
@@ -171,6 +181,9 @@ void LibjingleStreamTransport::Connect(
   socket_->SetNoDelay(true);
   socket_->SetAckDelay(kTcpAckDelayMilliseconds);
 
+  socket_->SetOnDestroyedCallback(base::Bind(
+      &LibjingleStreamTransport::OnChannelDestroyed, base::Unretained(this)));
+
   int result = socket_->Connect(
       base::Bind(&LibjingleStreamTransport::OnTcpConnected,
                  base::Unretained(this)));
@@ -273,13 +286,25 @@ 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();
+
+  if (channel_.get()) {

[Wez, 2012/02/22 22:51:38]

Comment to clarify that we can't delete |channel_| directly because we're in the
context of a sigslot signal.

[Sergey Ulanov, 2012/02/22 23:36:32]

Done.

+    base::MessageLoopProxy::current()->DeleteSoon(
+        FROM_HERE, channel_.release());

[Wez, 2012/02/22 22:51:38]

Isn't there a risk that we'll end up processing some other libjingle signal on
|channel_| before it gets deleted?

Would it be cleaner to disconnect the signal-handlers from |channel_| at this
point?  Or is that not possible from within the context of a sigslot call-stack,
either?

[Sergey Ulanov, 2012/02/22 23:36:32]

sigslot::has_slots<> disconnects signals automatically and signal handlers
inherit it.

[Wez, 2012/02/23 19:33:42]

Right, but it does that when it's deleted, i.e. when LibjingleStreamTransport is
deleted in this case, which isn't something this function (explicitly) does.

[Sergey Ulanov, 2012/02/23 22:10:06]

LibjingleStreamTransport doesn't have any signal handlers installed for the
|channel_|. The channel adapter handles signals from the |channel_|, but it
should be deleted above when we reset |socket_|.

+  }
+  if (port_allocator_.get()) {

[Wez, 2012/02/22 22:51:38]

We might end up here due to a |channel_| signal triggered by a |port_allocator_|
signal?

[Sergey Ulanov, 2012/02/22 23:36:32]

Yes, or channel_ may try to access port_allocator_ before it is destroyed.

+    base::MessageLoopProxy::current()->DeleteSoon(
+        FROM_HERE, port_allocator_.release());
+  }
+
   authenticator_.reset();
 
   NotifyConnected(scoped_ptr<net::StreamSocket>(NULL));
@@ -294,6 +319,10 @@ LibjingleTransportFactory::LibjingleTransportFactory()
 }
 
 LibjingleTransportFactory::~LibjingleTransportFactory() {
+  base::MessageLoopProxy::current()->DeleteSoon(

[Wez, 2012/02/22 22:51:38]

Is the worry that LibjingleTransportFactory might be deleted from within a
sigslot handler?

[Sergey Ulanov, 2012/02/22 23:36:32]

Yes. Added comments

+      FROM_HERE, socket_factory_.release());
+  base::MessageLoopProxy::current()->DeleteSoon(
+      FROM_HERE, network_manager_.release());
 }
 
 scoped_ptr<StreamTransport> LibjingleTransportFactory::CreateStreamTransport() {