Fix crash condition if caller deletes PseudoTcpAdaptor from within a callback.

Fix crash condition if caller deletes PseudoTcpAdaptor from within a callback.

BUG=82171
TEST=


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

[Wez, 2011-05-20 07:51:31]

Patch to fix crash conditions if calling code deletes the PseudoTcpAdaptor
during a callback.

[Wez, 2011-05-20 20:11:27]

On 2011/05/20 07:51:31, Wez wrote:
> Patch to fix crash conditions if calling code deletes the PseudoTcpAdaptor
> during a callback.

Ping!

[Sergey Ulanov, 2011-05-20 22:01:40]

Looks mostly good. Just some nits.

http://codereview.chromium.org/7040021/diff/1001/jingle/glue/pseudotcp_adapte...
File jingle/glue/pseudotcp_adapter.cc (right):

http://codereview.chromium.org/7040021/diff/1001/jingle/glue/pseudotcp_adapte...
jingle/glue/pseudotcp_adapter.cc:31: virtual int Read(net::IOBuffer* buffer, int
buffer_size,
Do these methods need to be virtual?

http://codereview.chromium.org/7040021/diff/1001/jingle/glue/pseudotcp_adapte...
jingle/glue/pseudotcp_adapter.cc:143: // Reference ourself in case callbacks
result.
The comments doesn't seem to be complete.

http://codereview.chromium.org/7040021/diff/1001/jingle/glue/pseudotcp_adapte...
jingle/glue/pseudotcp_adapter.cc:152: 
Don't need this empty line

http://codereview.chromium.org/7040021/diff/1001/jingle/glue/pseudotcp_adapte...
jingle/glue/pseudotcp_adapter.cc:175: // Reference ourself in case callbacks
result.
The comments doesn't seem to be complete

http://codereview.chromium.org/7040021/diff/1001/jingle/glue/pseudotcp_adapte...
jingle/glue/pseudotcp_adapter.cc:206: if (!read_buffer_)
It's better to check for read_callback here and DCHECK that read_buffer_ !=
NULL.

http://codereview.chromium.org/7040021/diff/1001/jingle/glue/pseudotcp_adapte...
jingle/glue/pseudotcp_adapter.cc:220: if (read_callback_) {
Don't think we need this if. if read_buffer_ is set to NULL this function would
return above, and read_callback_ should not be set to NULL if read_buffer_ is
not set null.

http://codereview.chromium.org/7040021/diff/1001/jingle/glue/pseudotcp_adapte...
jingle/glue/pseudotcp_adapter.cc:243: if (write_callback_) {
Same comment as for Read(): if there is data to write write_callback_ must not
be NULL.

http://codereview.chromium.org/7040021/diff/1001/jingle/glue/pseudotcp_adapter.h
File jingle/glue/pseudotcp_adapter.h (right):

http://codereview.chromium.org/7040021/diff/1001/jingle/glue/pseudotcp_adapte...
jingle/glue/pseudotcp_adapter.h:20: class PseudoTcpAdapter : public
net::StreamSocket, base::NonThreadSafe {
Please add some comments explaining lifetime of this object and Core, and why we
need to separate them.

[Wez, 2011-05-21 08:10:55]

Comments addressed.

http://codereview.chromium.org/7040021/diff/1001/jingle/glue/pseudotcp_adapte...
File jingle/glue/pseudotcp_adapter.cc (right):

http://codereview.chromium.org/7040021/diff/1001/jingle/glue/pseudotcp_adapte...
jingle/glue/pseudotcp_adapter.cc:31: virtual int Read(net::IOBuffer* buffer, int
buffer_size,
On 2011/05/20 22:01:40, sergeyu wrote:
> Do these methods need to be virtual?

Done.

http://codereview.chromium.org/7040021/diff/1001/jingle/glue/pseudotcp_adapte...
jingle/glue/pseudotcp_adapter.cc:143: // Reference ourself in case callbacks
result.
On 2011/05/20 22:01:40, sergeyu wrote:
> The comments doesn't seem to be complete.

Done.

http://codereview.chromium.org/7040021/diff/1001/jingle/glue/pseudotcp_adapte...
jingle/glue/pseudotcp_adapter.cc:152: 
On 2011/05/20 22:01:40, sergeyu wrote:
> Don't need this empty line

It's there to mark the end of the block of code implementing the IO_PENDING hack
for remoting, referred to by the TODO.

http://codereview.chromium.org/7040021/diff/1001/jingle/glue/pseudotcp_adapte...
jingle/glue/pseudotcp_adapter.cc:175: // Reference ourself in case callbacks
result.
On 2011/05/20 22:01:40, sergeyu wrote:
> The comments doesn't seem to be complete

Done.

http://codereview.chromium.org/7040021/diff/1001/jingle/glue/pseudotcp_adapte...
jingle/glue/pseudotcp_adapter.cc:206: if (!read_buffer_)
On 2011/05/20 22:01:40, sergeyu wrote:
> It's better to check for read_callback here and DCHECK that read_buffer_ !=
> NULL.

Done.

http://codereview.chromium.org/7040021/diff/1001/jingle/glue/pseudotcp_adapte...
jingle/glue/pseudotcp_adapter.cc:220: if (read_callback_) {
On 2011/05/20 22:01:40, sergeyu wrote:
> Don't think we need this if. if read_buffer_ is set to NULL this function
would
> return above, and read_callback_ should not be set to NULL if read_buffer_ is
> not set null.

Done.

http://codereview.chromium.org/7040021/diff/1001/jingle/glue/pseudotcp_adapte...
jingle/glue/pseudotcp_adapter.cc:243: if (write_callback_) {
On 2011/05/20 22:01:40, sergeyu wrote:
> Same comment as for Read(): if there is data to write write_callback_ must not
> be NULL.

Done.

http://codereview.chromium.org/7040021/diff/1001/jingle/glue/pseudotcp_adapter.h
File jingle/glue/pseudotcp_adapter.h (right):

http://codereview.chromium.org/7040021/diff/1001/jingle/glue/pseudotcp_adapte...
jingle/glue/pseudotcp_adapter.h:20: class PseudoTcpAdapter : public
net::StreamSocket, base::NonThreadSafe {
On 2011/05/20 22:01:40, sergeyu wrote:
> Please add some comments explaining lifetime of this object and Core, and why
we
> need to separate them.

Done.

[Sergey Ulanov, 2011-05-23 20:09:05]

Just two nits. LGTM otherwise.

http://codereview.chromium.org/7040021/diff/7001/jingle/glue/pseudotcp_adapte...
File jingle/glue/pseudotcp_adapter.cc (right):

http://codereview.chromium.org/7040021/diff/7001/jingle/glue/pseudotcp_adapte...
jingle/glue/pseudotcp_adapter.cc:49: net::CompletionCallback* connect_callback_;
Can we make them private, and make Core class a friend of the PseudoTcpAdapter?

http://codereview.chromium.org/7040021/diff/7001/jingle/glue/pseudotcp_adapte...
jingle/glue/pseudotcp_adapter.cc:361: core_->connect_callback_ = NULL;
Do we also need to reset read/write buffers? Maybe add ResetCallbacks() method
in Core and use it here and in Disconnect().

[Wez, 2011-05-23 21:27:02]

PTAFL ;)

http://codereview.chromium.org/7040021/diff/7001/jingle/glue/pseudotcp_adapte...
File jingle/glue/pseudotcp_adapter.cc (right):

http://codereview.chromium.org/7040021/diff/7001/jingle/glue/pseudotcp_adapte...
jingle/glue/pseudotcp_adapter.cc:49: net::CompletionCallback* connect_callback_;
On 2011/05/23 20:09:05, sergeyu wrote:
> Can we make them private, and make Core class a friend of the
PseudoTcpAdapter?

The only use for them was to support Disconnect() and IsConnected() (an earlier
draft had them used more widely by the PseudoTcpAdapter) so I've implemented
those as boilerplate to keep things clean.

http://codereview.chromium.org/7040021/diff/7001/jingle/glue/pseudotcp_adapte...
jingle/glue/pseudotcp_adapter.cc:361: core_->connect_callback_ = NULL;
On 2011/05/23 20:09:05, sergeyu wrote:
> Do we also need to reset read/write buffers? Maybe add ResetCallbacks() method
> in Core and use it here and in Disconnect().

I've moved clearing of the callbacks and PseudoTcp::Close() into a Disconnect()
call in the Core, which is called from the dtor and public Disconnect().

[Wez, 2011-05-23 23:34:09]

Added a unit test, which helps verify that this fixes the issue.

[Sergey Ulanov, 2011-05-24 00:37:34]

Thanks for adding the test! Still LGTM, just one nit.

http://codereview.chromium.org/7040021/diff/10002/jingle/glue/pseudotcp_adapt...
File jingle/glue/pseudotcp_adapter.cc (right):

http://codereview.chromium.org/7040021/diff/10002/jingle/glue/pseudotcp_adapt...
jingle/glue/pseudotcp_adapter.cc:206: // TODO(wez): The "force" parameter to
Close() should be true, since the
Can we change it to true now? If not, explain why.

[Wez, 2011-05-24 00:57:09]

Yes, changing that flag makes no real difference, and I think it's more correct
to have it "true".  Will go ahead & check in.

http://codereview.chromium.org/7040021/diff/10002/jingle/glue/pseudotcp_adapt...
File jingle/glue/pseudotcp_adapter.cc (right):

http://codereview.chromium.org/7040021/diff/10002/jingle/glue/pseudotcp_adapt...
jingle/glue/pseudotcp_adapter.cc:206: // TODO(wez): The "force" parameter to
Close() should be true, since the
On 2011/05/24 00:37:34, sergeyu wrote:
> Can we change it to true now? If not, explain why.

Done.

[commit-bot: I haz the power, 2011-05-25 04:23:31]

Change committed as 86570