From f2d6a00399cc2171b4180ca29be5e0dd3154454a Mon Sep 17 00:00:00 2001 From: "sergeyu@chromium.org" Date: Thu, 17 Nov 2011 00:48:12 +0000 Subject: 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 | 7 +++--- remoting/protocol/fake_session.cc | 3 +++ remoting/protocol/fake_session.h | 37 ++++++++++++++++-------------- remoting/protocol/jingle_session.cc | 10 +++++++- remoting/protocol/jingle_session.h | 5 +++- remoting/protocol/pepper_channel.h | 5 +++- remoting/protocol/pepper_session.cc | 8 +++++++ remoting/protocol/pepper_session.h | 1 + remoting/protocol/pepper_stream_channel.cc | 13 +++++++---- remoting/protocol/pepper_stream_channel.h | 3 ++- remoting/protocol/protobuf_video_reader.cc | 11 ++++++--- remoting/protocol/protobuf_video_reader.h | 2 ++ remoting/protocol/protobuf_video_writer.cc | 14 ++++++++--- remoting/protocol/protobuf_video_writer.h | 2 ++ remoting/protocol/protocol_mock_objects.h | 1 + remoting/protocol/rtp_video_reader.cc | 12 +++++++--- remoting/protocol/rtp_video_reader.h | 2 ++ remoting/protocol/rtp_video_writer.cc | 11 +++++++-- remoting/protocol/rtp_video_writer.h | 6 +++-- remoting/protocol/session.h | 5 ++++ 20 files changed, 116 insertions(+), 42 deletions(-) (limited to 'remoting/protocol') diff --git a/remoting/protocol/connection_to_client.cc b/remoting/protocol/connection_to_client.cc index 7579463..d31165f 100644 --- a/remoting/protocol/connection_to_client.cc +++ b/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 diff --git a/remoting/protocol/fake_session.cc b/remoting/protocol/fake_session.cc index a794e41..478dbac 100644 --- a/remoting/protocol/fake_session.cc +++ b/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_; } diff --git a/remoting/protocol/fake_session.h b/remoting/protocol/fake_session.h index de56176..c7be7e4 100644 --- a/remoting/protocol/fake_session.h +++ b/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_; diff --git a/remoting/protocol/jingle_session.cc b/remoting/protocol/jingle_session.cc index cbc404e..e0b9450 100644 --- a/remoting/protocol/jingle_session.cc +++ b/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() { diff --git a/remoting/protocol/jingle_session.h b/remoting/protocol/jingle_session.h index 787cc62..cc5195a 100644 --- a/remoting/protocol/jingle_session.h +++ b/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 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 candidate_config_; // Channels that are currently being connected. - std::map channel_connectors_; + ChannelConnectorsMap channel_connectors_; scoped_ptr control_channel_socket_; scoped_ptr event_channel_socket_; diff --git a/remoting/protocol/pepper_channel.h b/remoting/protocol/pepper_channel.h index 6ae46bb..cb77b17 100644 --- a/remoting/protocol/pepper_channel.h +++ b/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); diff --git a/remoting/protocol/pepper_session.cc b/remoting/protocol/pepper_session.cc index 373bfc3..ce56fd5 100644 --- a/remoting/protocol/pepper_session.cc +++ b/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(); diff --git a/remoting/protocol/pepper_session.h b/remoting/protocol/pepper_session.h index d44112a..eaaaa6e 100644 --- a/remoting/protocol/pepper_session.h +++ b/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; diff --git a/remoting/protocol/pepper_stream_channel.cc b/remoting/protocol/pepper_stream_channel.cc index 5f8afab..84abe25 100644 --- a/remoting/protocol/pepper_stream_channel.cc +++ b/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; } } diff --git a/remoting/protocol/pepper_stream_channel.h b/remoting/protocol/pepper_stream_channel.h index 4041a68..dbe206e 100644 --- a/remoting/protocol/pepper_stream_channel.h +++ b/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; diff --git a/remoting/protocol/protobuf_video_reader.cc b/remoting/protocol/protobuf_video_reader.cc index cf5d1a9..0bcc9fe 100644 --- a/remoting/protocol/protobuf_video_reader.cc +++ b/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))); } diff --git a/remoting/protocol/protobuf_video_reader.h b/remoting/protocol/protobuf_video_reader.h index 29dd5df..ed1452a 100644 --- a/remoting/protocol/protobuf_video_reader.h +++ b/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_; diff --git a/remoting/protocol/protobuf_video_writer.cc b/remoting/protocol/protobuf_video_writer.cc index 88941c4..26b0f95 100644 --- a/remoting/protocol/protobuf_video_writer.cc +++ b/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, diff --git a/remoting/protocol/protobuf_video_writer.h b/remoting/protocol/protobuf_video_writer.h index ab353db..9c33bd3 100644 --- a/remoting/protocol/protobuf_video_writer.h +++ b/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. diff --git a/remoting/protocol/protocol_mock_objects.h b/remoting/protocol/protocol_mock_objects.h index 3a06c1d..61d7602 100644 --- a/remoting/protocol/protocol_mock_objects.h +++ b/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*()); diff --git a/remoting/protocol/rtp_video_reader.cc b/remoting/protocol/rtp_video_reader.cc index 91f608a..e5f9f8b 100644 --- a/remoting/protocol/rtp_video_reader.cc +++ b/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)); diff --git a/remoting/protocol/rtp_video_reader.h b/remoting/protocol/rtp_video_reader.h index 160bbad..30693f8 100644 --- a/remoting/protocol/rtp_video_reader.h +++ b/remoting/protocol/rtp_video_reader.h @@ -68,6 +68,8 @@ class RtpVideoReader : public VideoReader { // |kReceiverReportsIntervalMs|. void SendReceiverReportIf(); + Session* session_; + bool initialized_; InitializedCallback initialized_callback_; diff --git a/remoting/protocol/rtp_video_writer.cc b/remoting/protocol/rtp_video_writer.cc index c188d66..7d83d06 100644 --- a/remoting/protocol/rtp_video_writer.cc +++ b/remoting/protocol/rtp_video_writer.cc @@ -1,4 +1,4 @@ -// 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, diff --git a/remoting/protocol/rtp_video_writer.h b/remoting/protocol/rtp_video_writer.h index 93d2a64..9d49006 100644 --- a/remoting/protocol/rtp_video_writer.h +++ b/remoting/protocol/rtp_video_writer.h @@ -1,4 +1,4 @@ -// 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_; diff --git a/remoting/protocol/session.h b/remoting/protocol/session.h index e0a91a0..32e2de9 100644 --- a/remoting/protocol/session.h +++ b/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; -- cgit v1.1