Support delegate deleting itself from OnError

Support delegate deleting itself from OnError

Modify SocketStream::Finish so that it works correctly if the delegate
calls DetachDelegate() and then deletes itself from within the OnError()
callback.

Test the delegate deleting itself in OnError().

SocketStream::Delegate is permitted to call
SocketStream::DetachDelegate() and then delete itself from within the
OnError method. Ensure this works correctly.

BUG=
TEST=net_unittests layout tests

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=202414

[tyoshino (SeeGerritForStatus), 2013-05-27 05:55:36]

https://codereview.chromium.org/15989003/diff/1/net/socket_stream/socket_stre...
File net/socket_stream/socket_stream.cc (right):

https://codereview.chromium.org/15989003/diff/1/net/socket_stream/socket_stre...
net/socket_stream/socket_stream.cc:296: return;
what is this change for?

https://codereview.chromium.org/15989003/diff/1/net/socket_stream/socket_stre...
File net/socket_stream/socket_stream_unittest.cc (right):

https://codereview.chromium.org/15989003/diff/1/net/socket_stream/socket_stre...
net/socket_stream/socket_stream_unittest.cc:188: const CompletionCallback&
callback)
two arg constructor. it's ok but you don't to specify explicit.

https://codereview.chromium.org/15989003/diff/1/net/socket_stream/socket_stre...
net/socket_stream/socket_stream_unittest.cc:197: callback_.Run(error);
let's copy callback_ and run at the end of this method.

https://codereview.chromium.org/15989003/diff/1/net/socket_stream/socket_stre...
net/socket_stream/socket_stream_unittest.cc:199: CHECK_EQ(self_->get(), this);
this should go to the constructor?

basically we don't want to die only for one expectation failure. please try
using ASSERT_EQ, ADD_FAILURE, and return where you can. if not, it's ok to use
CHECKs.

https://codereview.chromium.org/15989003/diff/1/net/socket_stream/socket_stre...
net/socket_stream/socket_stream_unittest.cc:207:
CHECK_EQ(socket_stream_->delegate(), this);
ditto

https://codereview.chromium.org/15989003/diff/1/net/socket_stream/socket_stre...
net/socket_stream/socket_stream_unittest.cc:212: NOTREACHED();
ditto. if you can't use ADD_FAILURE, use CHECK instead of NOTREACHED which is
DCHECK.

https://codereview.chromium.org/15989003/diff/1/net/socket_stream/socket_stre...
net/socket_stream/socket_stream_unittest.cc:215: NOTREACHED();
ditto

https://codereview.chromium.org/15989003/diff/1/net/socket_stream/socket_stre...
net/socket_stream/socket_stream_unittest.cc:219: NOTREACHED();
ditto

https://codereview.chromium.org/15989003/diff/1/net/socket_stream/socket_stre...
net/socket_stream/socket_stream_unittest.cc:221: virtual void
OnClose(SocketStream* socket) OVERRIDE { NOTREACHED(); }
ditto

[Adam Rice, 2013-05-27 07:16:33]

https://codereview.chromium.org/15989003/diff/1/net/socket_stream/socket_stre...
File net/socket_stream/socket_stream.cc (right):

https://codereview.chromium.org/15989003/diff/1/net/socket_stream/socket_stre...
net/socket_stream/socket_stream.cc:296: return;
On 2013/05/27 05:55:36, tyoshino wrote:
> what is this change for?

Previously, Finish() would set delegate_ to NULL, which would prevent the code
in this function from being executed.

However, if Finish() sets delegate_ to NULL, then it is unable to tell whether
or not the pointer is still valid.

So I am trying to achieve the same effect by checking the value of next_state_,
which should not be STATE_NONE until Finish() is called.

https://codereview.chromium.org/15989003/diff/1/net/socket_stream/socket_stre...
File net/socket_stream/socket_stream_unittest.cc (right):

https://codereview.chromium.org/15989003/diff/1/net/socket_stream/socket_stre...
net/socket_stream/socket_stream_unittest.cc:188: const CompletionCallback&
callback)
On 2013/05/27 05:55:36, tyoshino wrote:
> two arg constructor. it's ok but you don't to specify explicit.

I changed it to a one arg constructor.

https://codereview.chromium.org/15989003/diff/1/net/socket_stream/socket_stre...
net/socket_stream/socket_stream_unittest.cc:197: callback_.Run(error);
On 2013/05/27 05:55:36, tyoshino wrote:
> let's copy callback_ and run at the end of this method.

Done.

https://codereview.chromium.org/15989003/diff/1/net/socket_stream/socket_stre...
net/socket_stream/socket_stream_unittest.cc:199: CHECK_EQ(self_->get(), this);
On 2013/05/27 05:55:36, tyoshino wrote:
> this should go to the constructor?
> 
> basically we don't want to die only for one expectation failure. please try
> using ASSERT_EQ, ADD_FAILURE, and return where you can. if not, it's ok to use
> CHECKs.

I used a CHECK here because I was verifying that the test was correct, not the
code. But I found a way to make it simpler so this check was not needed. For the
others I used EXPECT_EQ and ADD_FAILURE.

https://codereview.chromium.org/15989003/diff/1/net/socket_stream/socket_stre...
net/socket_stream/socket_stream_unittest.cc:207:
CHECK_EQ(socket_stream_->delegate(), this);
On 2013/05/27 05:55:36, tyoshino wrote:
> ditto

Done.

https://codereview.chromium.org/15989003/diff/1/net/socket_stream/socket_stre...
net/socket_stream/socket_stream_unittest.cc:212: NOTREACHED();
On 2013/05/27 05:55:36, tyoshino wrote:
> ditto. if you can't use ADD_FAILURE, use CHECK instead of NOTREACHED which is
> DCHECK.

Done.

https://codereview.chromium.org/15989003/diff/1/net/socket_stream/socket_stre...
net/socket_stream/socket_stream_unittest.cc:215: NOTREACHED();
On 2013/05/27 05:55:36, tyoshino wrote:
> ditto

Done.

https://codereview.chromium.org/15989003/diff/1/net/socket_stream/socket_stre...
net/socket_stream/socket_stream_unittest.cc:219: NOTREACHED();
On 2013/05/27 05:55:36, tyoshino wrote:
> ditto

Done.

https://codereview.chromium.org/15989003/diff/1/net/socket_stream/socket_stre...
net/socket_stream/socket_stream_unittest.cc:221: virtual void
OnClose(SocketStream* socket) OVERRIDE { NOTREACHED(); }
On 2013/05/27 05:55:36, tyoshino wrote:
> ditto

Done.

[tyoshino (SeeGerritForStatus), 2013-05-27 08:09:13]

LGTM

https://codereview.chromium.org/15989003/diff/1/net/socket_stream/socket_stre...
File net/socket_stream/socket_stream.cc (right):

https://codereview.chromium.org/15989003/diff/1/net/socket_stream/socket_stre...
net/socket_stream/socket_stream.cc:296: return;
I see. Thanks

https://codereview.chromium.org/15989003/diff/1/net/socket_stream/socket_stre...
File net/socket_stream/socket_stream_unittest.cc (right):

https://codereview.chromium.org/15989003/diff/1/net/socket_stream/socket_stre...
net/socket_stream/socket_stream_unittest.cc:199: CHECK_EQ(self_->get(), this);
Great!

[commit-bot: I haz the power, 2013-05-27 08:09:38]

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

[commit-bot: I haz the power, 2013-05-27 09:18:00]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-05-27 09:50:20]

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

[commit-bot: I haz the power, 2013-05-27 10:22:05]

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

[commit-bot: I haz the power, 2013-05-27 13:24:35]

Change committed as 202414