Add CancelChannelCreation() in protocol::Session interface.

The new method cancels channel creation for pending channel. This 
prevents some potential crashes.


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

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

remoting/protocol/connection_to_client.cc

@@ -162,10 +162,9 @@ void ConnectionToClient::CloseOnError() {
 }
 
 void ConnectionToClient::CloseChannels() {
-  if (video_writer_.get())
-    video_writer_->Close();
-  if (client_control_sender_.get())
-    client_control_sender_->Close();
+  video_writer_.reset();
+  client_control_sender_.reset();
+  dispatcher_.reset();
 }
 
 }  // namespace protocol

remoting/protocol/fake_session.cc

@@ -242,6 +242,9 @@ void FakeSession::CreateDatagramChannel(
   callback.Run(channel);
 }
 
+void FakeSession::CancelChannelCreation(const std::string& name) {
+}
+
 FakeSocket* FakeSession::control_channel() {
   return &control_channel_;
 }

remoting/protocol/fake_session.h

@@ -138,33 +138,36 @@ class FakeSession : public Session {
   FakeUdpSocket* GetDatagramChannel(const std::string& name);
 
   // Session interface.
-  virtual void SetStateChangeCallback(const StateChangeCallback& callback);
+  virtual void SetStateChangeCallback(
+      const StateChangeCallback& callback) OVERRIDE;
 
-  virtual Session::Error error();
+  virtual Session::Error error() OVERRIDE;
 
   virtual void CreateStreamChannel(
-      const std::string& name, const StreamChannelCallback& callback);
+      const std::string& name, const StreamChannelCallback& callback) OVERRIDE;
   virtual void CreateDatagramChannel(
-      const std::string& name, const DatagramChannelCallback& callback);
+      const std::string& name,
+      const DatagramChannelCallback& callback) OVERRIDE;
+  virtual void CancelChannelCreation(const std::string& name) OVERRIDE;
 
-  virtual FakeSocket* control_channel();
-  virtual FakeSocket* event_channel();
+  virtual FakeSocket* control_channel() OVERRIDE;
+  virtual FakeSocket* event_channel() OVERRIDE;
 
-  virtual const std::string& jid();
+  virtual const std::string& jid() OVERRIDE;
 
-  virtual const CandidateSessionConfig* candidate_config();
-  virtual const SessionConfig& config();
-  virtual void set_config(const SessionConfig& config);
+  virtual const CandidateSessionConfig* candidate_config() OVERRIDE;
+  virtual const SessionConfig& config() OVERRIDE;
+  virtual void set_config(const SessionConfig& config) OVERRIDE;
 
-  virtual const std::string& initiator_token();
-  virtual void set_initiator_token(const std::string& initiator_token);
-  virtual const std::string& receiver_token();
-  virtual void set_receiver_token(const std::string& receiver_token);
+  virtual const std::string& initiator_token() OVERRIDE;
+  virtual void set_initiator_token(const std::string& initiator_token) OVERRIDE;
+  virtual const std::string& receiver_token() OVERRIDE;
+  virtual void set_receiver_token(const std::string& receiver_token) OVERRIDE;
 
-  virtual void set_shared_secret(const std::string& secret);
-  virtual const std::string& shared_secret();
+  virtual void set_shared_secret(const std::string& secret) OVERRIDE;
+  virtual const std::string& shared_secret() OVERRIDE;
 
-  virtual void Close();
+  virtual void Close() OVERRIDE;
 
  public:
   StateChangeCallback callback_;

remoting/protocol/jingle_session.cc

@@ -172,6 +172,14 @@ void JingleSession::CreateDatagramChannel(
       name, new JingleDatagramConnector(this, name, callback));
 }
 
+void JingleSession::CancelChannelCreation(const std::string& name) {
+  ChannelConnectorsMap::iterator it = channel_connectors_.find(name);
+  if (it != channel_connectors_.end()) {
+    delete it->second;
+    channel_connectors_.erase(it);
+  }
+}
+
 net::Socket* JingleSession::control_channel() {
   DCHECK(CalledOnValidThread());
   return control_channel_socket_.get();
@@ -432,7 +440,7 @@ void JingleSession::OnChannelConnectorFinished(
     const std::string& name, JingleChannelConnector* connector) {
   DCHECK(CalledOnValidThread());
   DCHECK_EQ(channel_connectors_[name], connector);
-  channel_connectors_[name] = NULL;
+  channel_connectors_.erase(name);
 }
 
 void JingleSession::CreateChannels() {

remoting/protocol/jingle_session.h

@@ -36,6 +36,7 @@ class JingleSession : public protocol::Session,
   virtual void CreateDatagramChannel(
       const std::string& name,
       const DatagramChannelCallback& callback) OVERRIDE;
+  virtual void CancelChannelCreation(const std::string& name) OVERRIDE;
   virtual net::Socket* control_channel() OVERRIDE;
   virtual net::Socket* event_channel() OVERRIDE;
   virtual const std::string& jid() OVERRIDE;
@@ -55,6 +56,8 @@ class JingleSession : public protocol::Session,
   friend class JingleSessionManager;
   friend class JingleStreamConnector;
 
+  typedef std::map<std::string, JingleChannelConnector*> ChannelConnectorsMap;
+
   // Create a JingleSession used in client mode. A server certificate is
   // required.
   static JingleSession* CreateClientSession(JingleSessionManager* manager,
@@ -175,7 +178,7 @@ class JingleSession : public protocol::Session,
   scoped_ptr<const CandidateSessionConfig> candidate_config_;
 
   // Channels that are currently being connected.
-  std::map<std::string, JingleChannelConnector*> channel_connectors_;
+  ChannelConnectorsMap channel_connectors_;
 
   scoped_ptr<net::Socket> control_channel_socket_;
   scoped_ptr<net::Socket> event_channel_socket_;

remoting/protocol/pepper_channel.h

@@ -38,7 +38,10 @@ class PepperChannel : public base::NonThreadSafe {
   virtual void AddRemoveCandidate(const cricket::Candidate& candidate) = 0;
 
   // Name of the channel.
-  virtual const std::string& name() = 0;
+  virtual const std::string& name() const = 0;
+
+  // returns true if the channel is already connected
+  virtual bool is_connected() const = 0;
 
  protected:
   DISALLOW_COPY_AND_ASSIGN(PepperChannel);

remoting/protocol/pepper_session.cc

@@ -126,6 +126,14 @@ void PepperSession::CreateDatagramChannel(
   NOTREACHED();
 }
 
+void PepperSession::CancelChannelCreation(const std::string& name) {
+  ChannelsMap::iterator it = channels_.find(name);
+  if (it != channels_.end() && !it->second->is_connected()) {
+    delete it->second;
+    DCHECK(!channels_[name]);
+  }
+}
+
 net::Socket* PepperSession::control_channel() {
   DCHECK(CalledOnValidThread());
   return control_channel_socket_.get();

remoting/protocol/pepper_session.h

@@ -48,6 +48,7 @@ class PepperSession : public Session {
   virtual void CreateDatagramChannel(
       const std::string& name,
       const DatagramChannelCallback& callback) OVERRIDE;
+  virtual void CancelChannelCreation(const std::string& name) OVERRIDE;
   virtual net::Socket* control_channel() OVERRIDE;
   virtual net::Socket* event_channel() OVERRIDE;
   virtual const std::string& jid() OVERRIDE;

remoting/protocol/pepper_stream_channel.cc

@@ -81,6 +81,7 @@ PepperStreamChannel::PepperStreamChannel(
 }
 
 PepperStreamChannel::~PepperStreamChannel() {
+  session_->OnDeleteChannel(this);
   // Verify that the |channel_| is ether destroyed or we own it.
   DCHECK_EQ(channel_, owned_channel_.get());
   // Channel should be already destroyed if we were connected.
@@ -163,17 +164,21 @@ void PepperStreamChannel::AddRemoveCandidate(
     channel_->AddRemoteCandidate(jingle_glue::SerializeP2PCandidate(candidate));
 }
 
-const std::string& PepperStreamChannel::name() {
+const std::string& PepperStreamChannel::name() const {
   DCHECK(CalledOnValidThread());
   return name_;
 }
 
+bool PepperStreamChannel::is_connected() const {
+  DCHECK(CalledOnValidThread());
+  return connected_;
+}
+
 void PepperStreamChannel::OnChannelDeleted() {
   if (connected_) {
     channel_ = NULL;
-    // The PepperTransportSocketAdapter is being deleted, so delete the
-    // channel too.
-    session_->OnDeleteChannel(this);
+    // The PepperTransportSocketAdapter is being deleted, so delete
+    // the channel too.
     delete this;
   }
 }

remoting/protocol/pepper_stream_channel.h

@@ -39,7 +39,8 @@ class PepperStreamChannel : public PepperChannel,
                        const TransportConfig& transport_config,
                        const std::string& remote_cert) OVERRIDE;
   virtual void AddRemoveCandidate(const cricket::Candidate& candidate) OVERRIDE;
-  virtual const std::string& name() OVERRIDE;
+  virtual const std::string& name() const OVERRIDE;
+  virtual bool is_connected() const OVERRIDE;
 
   // PepperTransportSocketAdapter implementation.
   virtual void OnChannelDeleted() OVERRIDE;

remoting/protocol/protobuf_video_reader.cc

@@ -15,19 +15,24 @@ namespace remoting {
 namespace protocol {
 
 ProtobufVideoReader::ProtobufVideoReader(VideoPacketFormat::Encoding encoding)
-    : encoding_(encoding),
+    : session_(NULL),
+      encoding_(encoding),
       video_stub_(NULL) {
 }
 
-ProtobufVideoReader::~ProtobufVideoReader() { }
+ProtobufVideoReader::~ProtobufVideoReader() {
+  if (session_)
+    session_->CancelChannelCreation(kVideoChannelName);
+}
 
 void ProtobufVideoReader::Init(protocol::Session* session,
                                VideoStub* video_stub,
                                const InitializedCallback& callback) {
+  session_ = session;
   initialized_callback_ = callback;
   video_stub_ = video_stub;
 
-  session->CreateStreamChannel(
+  session_->CreateStreamChannel(
       kVideoChannelName,
       base::Bind(&ProtobufVideoReader::OnChannelReady, base::Unretained(this)));
 }

remoting/protocol/protobuf_video_reader.h

@@ -33,6 +33,8 @@ class ProtobufVideoReader : public VideoReader {
   void OnChannelReady(net::StreamSocket* socket);
   void OnNewData(VideoPacket* packet, const base::Closure& done_task);
 
+  Session* session_;
+
   InitializedCallback initialized_callback_;
 
   VideoPacketFormat::Encoding encoding_;

remoting/protocol/protobuf_video_writer.cc

@@ -17,16 +17,20 @@ namespace remoting {
 namespace protocol {
 
 ProtobufVideoWriter::ProtobufVideoWriter(base::MessageLoopProxy* message_loop)
-    : buffered_writer_(new BufferedSocketWriter(message_loop)) {
+    : session_(NULL),
+      buffered_writer_(new BufferedSocketWriter(message_loop)) {
 }
 
-ProtobufVideoWriter::~ProtobufVideoWriter() { }
+ProtobufVideoWriter::~ProtobufVideoWriter() {
+  Close();
+}
 
 void ProtobufVideoWriter::Init(protocol::Session* session,
                                const InitializedCallback& callback) {
+  session_ = session;
   initialized_callback_ = callback;
 
-  session->CreateStreamChannel(
+  session_->CreateStreamChannel(
       kVideoChannelName,
       base::Bind(&ProtobufVideoWriter::OnChannelReady, base::Unretained(this)));
 }
@@ -48,6 +52,10 @@ void ProtobufVideoWriter::OnChannelReady(net::StreamSocket* socket) {
 void ProtobufVideoWriter::Close() {
   buffered_writer_->Close();
   channel_.reset();
+  if (session_) {
+    session_->CancelChannelCreation(kVideoChannelName);
+    session_ = NULL;
+  }
 }
 
 void ProtobufVideoWriter::ProcessVideoPacket(const VideoPacket* packet,

remoting/protocol/protobuf_video_writer.h

@@ -44,6 +44,8 @@ class ProtobufVideoWriter : public VideoWriter {
  private:
   void OnChannelReady(net::StreamSocket* socket);
 
+  Session* session_;
+
   InitializedCallback initialized_callback_;
 
   // TODO(sergeyu): Remove |channel_| and let |buffered_writer_| own it.

remoting/protocol/protocol_mock_objects.h

@@ -107,6 +107,7 @@ class MockSession : public Session {
       const std::string& name, const StreamChannelCallback& callback));
   MOCK_METHOD2(CreateDatagramChannel, void(
       const std::string& name, const DatagramChannelCallback& callback));
+  MOCK_METHOD1(CancelChannelCreation, void(const std::string& name));
   MOCK_METHOD0(control_channel, net::Socket*());
   MOCK_METHOD0(event_channel, net::Socket*());
   MOCK_METHOD0(video_channel, net::Socket*());

remoting/protocol/rtp_video_reader.cc

@@ -24,27 +24,33 @@ RtpVideoReader::PacketsQueueEntry::PacketsQueueEntry()
 }
 
 RtpVideoReader::RtpVideoReader(base::MessageLoopProxy* message_loop)
-    : initialized_(false),
+    : session_(NULL),
+      initialized_(false),
       rtcp_writer_(message_loop),
       last_sequence_number_(0),
       video_stub_(NULL) {
 }
 
 RtpVideoReader::~RtpVideoReader() {
+  if (session_) {
+    session_->CancelChannelCreation(kVideoRtpChannelName);
+    session_->CancelChannelCreation(kVideoRtcpChannelName);
+  }
   ResetQueue();
 }
 
 void RtpVideoReader::Init(protocol::Session* session,
                           VideoStub* video_stub,
                           const InitializedCallback& callback) {
+  session_ = session;
   initialized_callback_ = callback;
   video_stub_ = video_stub;
 
-  session->CreateDatagramChannel(
+  session_->CreateDatagramChannel(
       kVideoRtpChannelName,
       base::Bind(&RtpVideoReader::OnChannelReady,
                  base::Unretained(this), true));
-  session->CreateDatagramChannel(
+  session_->CreateDatagramChannel(
       kVideoRtcpChannelName,
       base::Bind(&RtpVideoReader::OnChannelReady,
                  base::Unretained(this), false));

remoting/protocol/rtp_video_reader.h

@@ -68,6 +68,8 @@ class RtpVideoReader : public VideoReader {
   // |kReceiverReportsIntervalMs|.
   void SendReceiverReportIf();
 
+  Session* session_;
+
   bool initialized_;
   InitializedCallback initialized_callback_;
 

remoting/protocol/rtp_video_writer.cc

-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -21,7 +21,8 @@ const int kMtu = 1200;
 }  // namespace
 
 RtpVideoWriter::RtpVideoWriter(base::MessageLoopProxy* message_loop)
-    : initialized_(false),
+    : session_(NULL),
+      initialized_(false),
       rtp_writer_(message_loop) {
 }
 
@@ -31,6 +32,7 @@ RtpVideoWriter::~RtpVideoWriter() {
 
 void RtpVideoWriter::Init(protocol::Session* session,
                           const InitializedCallback& callback) {
+  session_ = session;
   initialized_callback_ = callback;
   session->CreateDatagramChannel(
       kVideoRtpChannelName,
@@ -72,6 +74,11 @@ void RtpVideoWriter::Close() {
   rtp_writer_.Close();
   rtp_channel_.reset();
   rtcp_channel_.reset();
+  if (session_) {
+    session_->CancelChannelCreation(kVideoRtpChannelName);
+    session_->CancelChannelCreation(kVideoRtcpChannelName);
+    session_ = NULL;
+  }
 }
 
 void RtpVideoWriter::ProcessVideoPacket(const VideoPacket* packet,

remoting/protocol/rtp_video_writer.h

-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -24,7 +24,7 @@ class RtpVideoWriter : public VideoWriter {
   virtual ~RtpVideoWriter();
 
   // VideoWriter interface.
-  virtual void Init(protocol::Session* session,
+  virtual void Init(Session* session,
                     const InitializedCallback& callback) OVERRIDE;
   virtual void Close() OVERRIDE;
 
@@ -36,6 +36,8 @@ class RtpVideoWriter : public VideoWriter {
  private:
   void OnChannelReady(bool rtp, net::Socket* socket);
 
+  Session* session_;
+
   bool initialized_;
   InitializedCallback initialized_callback_;
 

remoting/protocol/session.h

@@ -90,6 +90,11 @@ class Session : public base::NonThreadSafe {
   virtual void CreateDatagramChannel(
       const std::string& name, const DatagramChannelCallback& callback) = 0;
 
+  // Cancels a pending CreateStreamChannel() or CreateDatagramChannel()
+  // operation for the named channel. If the channel creation already
+  // completed then cancelling it has no effect.
+  virtual void CancelChannelCreation(const std::string& name) = 0;
+
   // TODO(sergeyu): Remove these methods, and use CreateChannel()
   // instead.
   virtual net::Socket* control_channel() = 0;