Delete Session and SessionManager object synchronously.

remoting/protocol/libjingle_transport_factory.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/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"
 #include "net/base/net_errors.h"
 #include "remoting/protocol/channel_authenticator.h"
 #include "remoting/protocol/transport_config.h"
 #include "third_party/libjingle/source/talk/base/basicpacketsocketfactory.h"
 #include "third_party/libjingle/source/talk/base/network.h"
 #include "third_party/libjingle/source/talk/p2p/base/p2ptransportchannel.h"
 #include "third_party/libjingle/source/talk/p2p/client/httpportallocator.h"
@@ skipping 53 matching lines @@
   std::string name_;
   TransportConfig config_;
   EventHandler* event_handler_;
   StreamTransport::ConnectedCallback callback_;
   scoped_ptr<ChannelAuthenticator> authenticator_;
 
   scoped_ptr<cricket::PortAllocator> port_allocator_;
   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_;
 
   DISALLOW_COPY_AND_ASSIGN(LibjingleStreamTransport);
 };
 
 LibjingleStreamTransport::LibjingleStreamTransport(
     talk_base::NetworkManager* network_manager,
     talk_base::PacketSocketFactory* socket_factory)
     : network_manager_(network_manager),
       socket_factory_(socket_factory),
       event_handler_(NULL),
       http_port_allocator_(NULL) {
 }
 
 LibjingleStreamTransport::~LibjingleStreamTransport() {
   DCHECK(event_handler_);
   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(
     const std::string& name,
     const TransportConfig& config,
     Transport::EventHandler* event_handler,
     scoped_ptr<ChannelAuthenticator> authenticator) {
   DCHECK(CalledOnValidThread());
 
   DCHECK(!name.empty());
@@ skipping 43 matching lines @@
   channel_->SignalCandidateReady.connect(
       this, &LibjingleStreamTransport::OnCandidateReady);
   channel_->SignalRouteChange.connect(
       this, &LibjingleStreamTransport::OnRouteChange);
 
   channel_->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(
       new jingle_glue::PseudoTcpAdapter(channel_adapter.release()));
   socket_->SetSendBufferSize(kTcpSendBufferSize);
   socket_->SetReceiveBufferSize(kTcpReceiveBufferSize);
   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)));
   if (result != net::ERR_IO_PENDING)
     OnTcpConnected(result);
 }
 
 void LibjingleStreamTransport::AddRemoteCandidate(
     const cricket::Candidate& candidate) {
   DCHECK(CalledOnValidThread());
@@ skipping 82 matching lines @@
 void LibjingleStreamTransport::OnChannelDestroyed() {
   if (is_connected()) {
     // The connection socket is being deleted, so delete the transport too.
     delete this;
   }
 }
 
 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));
 }
 
 }  // namespace
 
 LibjingleTransportFactory::LibjingleTransportFactory()
     : network_manager_(new talk_base::BasicNetworkManager()),
       socket_factory_(new talk_base::BasicPacketSocketFactory(
           talk_base::Thread::Current())) {
 }
 
 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() {
   return scoped_ptr<StreamTransport>(
       new LibjingleStreamTransport(network_manager_.get(),
                                    socket_factory_.get()));
 }
 
 scoped_ptr<DatagramTransport>
 LibjingleTransportFactory::CreateDatagramTransport() {
   NOTIMPLEMENTED();
   return scoped_ptr<DatagramTransport>(NULL);
 }
 
 
 }  // namespace protocol
 }  // namespace remoting