Add CancelChannelCreation() in protocol::Session interface.

Add CancelChannelCreation() in protocol::Session interface.

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


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110401

[Wez, 2011-11-15 22:29:13]

http://codereview.chromium.org/8573013/diff/1/remoting/protocol/connection_to...
File remoting/protocol/connection_to_client.cc (right):

http://codereview.chromium.org/8573013/diff/1/remoting/protocol/connection_to...
remoting/protocol/connection_to_client.cc:167: dispatcher_.reset();
Are these changes related to this CL?

http://codereview.chromium.org/8573013/diff/1/remoting/protocol/fake_session.h
File remoting/protocol/fake_session.h (right):

http://codereview.chromium.org/8573013/diff/1/remoting/protocol/fake_session....
remoting/protocol/fake_session.h:170: virtual void Close() OVERRIDE;
nit: These aren't related to this CL?

http://codereview.chromium.org/8573013/diff/1/remoting/protocol/pepper_channel.h
File remoting/protocol/pepper_channel.h (right):

http://codereview.chromium.org/8573013/diff/1/remoting/protocol/pepper_channe...
remoting/protocol/pepper_channel.h:44: virtual bool connected() const = 0;
nit: connected -> is_connected?

http://codereview.chromium.org/8573013/diff/1/remoting/protocol/pepper_stream...
File remoting/protocol/pepper_stream_channel.cc (right):

http://codereview.chromium.org/8573013/diff/1/remoting/protocol/pepper_stream...
remoting/protocol/pepper_stream_channel.cc:84: session_->OnDeleteChannel(this);
This looks like a bug-fix, necessary but unrelated to the CL?

http://codereview.chromium.org/8573013/diff/1/remoting/protocol/protobuf_vide...
File remoting/protocol/protobuf_video_reader.h (right):

http://codereview.chromium.org/8573013/diff/1/remoting/protocol/protobuf_vide...
remoting/protocol/protobuf_video_reader.h:42: bool creation_finished_;
Where is this used?

http://codereview.chromium.org/8573013/diff/1/remoting/protocol/protobuf_vide...
File remoting/protocol/protobuf_video_writer.cc (right):

http://codereview.chromium.org/8573013/diff/1/remoting/protocol/protobuf_vide...
remoting/protocol/protobuf_video_writer.cc:56:
session_->CancelChannelCreation(kVideoChannelName);
Clear |session_| here, or move this code into the dtor, now that we never call
Close()?

http://codereview.chromium.org/8573013/diff/1/remoting/protocol/rtp_video_wri...
File remoting/protocol/rtp_video_writer.cc (right):

http://codereview.chromium.org/8573013/diff/1/remoting/protocol/rtp_video_wri...
remoting/protocol/rtp_video_writer.cc:80: }
Shouldn't this also clear |session_|?  Can we just get rid of Close() and rely
on the dtor?

http://codereview.chromium.org/8573013/diff/1/remoting/protocol/session.h
File remoting/protocol/session.h (right):

http://codereview.chromium.org/8573013/diff/1/remoting/protocol/session.h#new...
remoting/protocol/session.h:94: // CreateStreamChannel() and
CreateDatagramChannel() calls. Can be
Wording is a little confusing.  I'd suggest:

"Cancels a pending CreateStreamChannel() or CreateDatagramChannel() operation
for the named channel.  If the channel creation already completed then
cancelling it has no effect."

Except that, looking at the Pepper implementation of CancelChannelCreation(), I
think it actually tears down the channel, doesn't it?

[Sergey Ulanov, 2011-11-16 00:01:33]

http://codereview.chromium.org/8573013/diff/1/remoting/protocol/connection_to...
File remoting/protocol/connection_to_client.cc (right):

http://codereview.chromium.org/8573013/diff/1/remoting/protocol/connection_to...
remoting/protocol/connection_to_client.cc:167: dispatcher_.reset();
On 2011/11/15 22:29:13, Wez wrote:
> Are these changes related to this CL?
Yes. Now these objects call session_ at destruction so they must be always
destroyed before session_.

http://codereview.chromium.org/8573013/diff/1/remoting/protocol/fake_session.h
File remoting/protocol/fake_session.h (right):

http://codereview.chromium.org/8573013/diff/1/remoting/protocol/fake_session....
remoting/protocol/fake_session.h:170: virtual void Close() OVERRIDE;
On 2011/11/15 22:29:13, Wez wrote:
> nit: These aren't related to this CL?
CancelChannelCreation() is a new method in Session interface, and this header
would be inconsistent if I had OVERRIDE only for one method, so I had to add
OVERRIDE for all other methods.

http://codereview.chromium.org/8573013/diff/1/remoting/protocol/pepper_channel.h
File remoting/protocol/pepper_channel.h (right):

http://codereview.chromium.org/8573013/diff/1/remoting/protocol/pepper_channe...
remoting/protocol/pepper_channel.h:44: virtual bool connected() const = 0;
On 2011/11/15 22:29:13, Wez wrote:
> nit: connected -> is_connected?

Done.

http://codereview.chromium.org/8573013/diff/1/remoting/protocol/pepper_stream...
File remoting/protocol/pepper_stream_channel.cc (right):

http://codereview.chromium.org/8573013/diff/1/remoting/protocol/pepper_stream...
remoting/protocol/pepper_stream_channel.cc:84: session_->OnDeleteChannel(this);
On 2011/11/15 22:29:13, Wez wrote:
> This looks like a bug-fix, necessary but unrelated to the CL?

Actually it wasn't a bug, and it is required for this CL. Previously
PepperStreamChannel was deleting itself and calling this method in
OnChannelDeleted(). With this change this object may be also deleted in
PepperStreamChannel, so I had to remove this call here from OnChannelDeleted().

http://codereview.chromium.org/8573013/diff/1/remoting/protocol/protobuf_vide...
File remoting/protocol/protobuf_video_reader.h (right):

http://codereview.chromium.org/8573013/diff/1/remoting/protocol/protobuf_vide...
remoting/protocol/protobuf_video_reader.h:42: bool creation_finished_;
On 2011/11/15 22:29:13, Wez wrote:
> Where is this used?

Removed

http://codereview.chromium.org/8573013/diff/1/remoting/protocol/protobuf_vide...
File remoting/protocol/protobuf_video_writer.cc (right):

http://codereview.chromium.org/8573013/diff/1/remoting/protocol/protobuf_vide...
remoting/protocol/protobuf_video_writer.cc:56:
session_->CancelChannelCreation(kVideoChannelName);
On 2011/11/15 22:29:13, Wez wrote:
> Clear |session_| here, 
Done.

> or move this code into the dtor, now that we never call Close()?

Other channel readers/writes have Close(), so I'd like to keep it for now, at
least for consistency. I may remove Close() method from all channel dispatchers
later, when I'm sure we don't need it.

http://codereview.chromium.org/8573013/diff/1/remoting/protocol/rtp_video_wri...
File remoting/protocol/rtp_video_writer.cc (right):

http://codereview.chromium.org/8573013/diff/1/remoting/protocol/rtp_video_wri...
remoting/protocol/rtp_video_writer.cc:80: }
On 2011/11/15 22:29:13, Wez wrote:
> Shouldn't this also clear |session_|?

Done.

> Can we just get rid of Close() and rely on the dtor?

Maybe, but I'd like to do it in a separate CL.

http://codereview.chromium.org/8573013/diff/1/remoting/protocol/session.h
File remoting/protocol/session.h (right):

http://codereview.chromium.org/8573013/diff/1/remoting/protocol/session.h#new...
remoting/protocol/session.h:94: // CreateStreamChannel() and
CreateDatagramChannel() calls. Can be
On 2011/11/15 22:29:13, Wez wrote:
> Wording is a little confusing.  I'd suggest:
> 
> "Cancels a pending CreateStreamChannel() or CreateDatagramChannel() operation
> for the named channel.  If the channel creation already completed then
> cancelling it has no effect."

Done. Thanks for suggestion.

> Except that, looking at the Pepper implementation of CancelChannelCreation(),
I
> think it actually tears down the channel, doesn't it?

It tears it down only when it is not connected, i.e. is_connected()==false.

[Wez, 2011-11-16 22:02:18]

lgtm

[commit-bot: I haz the power, 2011-11-16 22:17:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/8573013/6001

[commit-bot: I haz the power, 2011-11-17 00:48:14]

Change committed as 110401