Support delegate deleting itself from OnError

net/socket_stream/socket_stream_unittest.cc

Index: net/socket_stream/socket_stream_unittest.cc
diff --git a/net/socket_stream/socket_stream_unittest.cc b/net/socket_stream/socket_stream_unittest.cc
index b0734fc5fb5b0bd7d98e04fca93560e40458993e..9014066e784aa86b5fa78eb0b52bad47f4113adf 100644
--- a/net/socket_stream/socket_stream_unittest.cc
+++ b/net/socket_stream/socket_stream_unittest.cc
@@ -181,6 +181,53 @@ class SocketStreamEventRecorder : public SocketStream::Delegate {
   DISALLOW_COPY_AND_ASSIGN(SocketStreamEventRecorder);
 };
 
+// This is used for the test OnErrorDetachDelegate.
+class SelfDeletingDelegate : public SocketStream::Delegate {
+ public:
+  explicit SelfDeletingDelegate(scoped_ptr<SelfDeletingDelegate>* self,
+                                const CompletionCallback& callback)

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

two arg constructor. it's ok but you don't to specify explicit.

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

I changed it to a one arg constructor.

+      : self_(self), socket_stream_(), callback_(callback) {}
+
+  virtual ~SelfDeletingDelegate() {}
+
+  // Call DetachDelegate() and then delete |this|. The callback is called first,
+  // to provide a way for the test to complete (the test won't complete
+  // synchronously, it will return to the message loop first).
+  virtual void OnError(const SocketStream* socket, int error) OVERRIDE {
+    callback_.Run(error);

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

let's copy callback_ and run at the end of this method.

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

Done.

+    socket_stream_->DetachDelegate();
+    CHECK_EQ(self_->get(), this);

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

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.

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

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.

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

Great!

+    self_->reset();
+  }
+
+  // This can't be passed in the constructor because this object needs to be
+  // created before SocketStream.
+  void set_socket_stream(const scoped_refptr<SocketStream>& socket_stream) {
+    socket_stream_ = socket_stream;
+    CHECK_EQ(socket_stream_->delegate(), this);

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

ditto

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

Done.

+  }
+
+  virtual void OnConnected(SocketStream* socket, int max_pending_send_allowed)
+      OVERRIDE {
+    NOTREACHED();

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

ditto. if you can't use ADD_FAILURE, use CHECK instead of NOTREACHED which is
DCHECK.

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

Done.

+  }
+  virtual void OnSentData(SocketStream* socket, int amount_sent) OVERRIDE {
+    NOTREACHED();

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

ditto

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

Done.

+  }
+  virtual void OnReceivedData(SocketStream* socket, const char* data, int len)
+      OVERRIDE {
+    NOTREACHED();

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

ditto

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

Done.

+  }
+  virtual void OnClose(SocketStream* socket) OVERRIDE { NOTREACHED(); }

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

ditto

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

Done.

+
+ private:
+  scoped_ptr<SelfDeletingDelegate>* const self_;
+  scoped_refptr<SocketStream> socket_stream_;
+  const CompletionCallback callback_;
+
+  DISALLOW_COPY_AND_ASSIGN(SelfDeletingDelegate);
+};
+
 class TestURLRequestContextWithProxy : public TestURLRequestContext {
  public:
   explicit TestURLRequestContextWithProxy(const std::string& proxy)
@@ -846,4 +893,30 @@ TEST_F(SocketStreamTest, BeforeConnectFailed) {
   EXPECT_EQ(SocketStreamEvent::EVENT_CLOSE, events[1].event_type);
 }
 
+// Check that a connect failure, followed by the delegate calling DetachDelegate
+// and deleting itself in the OnError callback, is handled correctly.
+TEST_F(SocketStreamTest, OnErrorDetachDelegate) {
+  MockClientSocketFactory mock_socket_factory;
+  TestCompletionCallback test_callback;
+
+  scoped_ptr<SelfDeletingDelegate> delegate;
+  delegate.reset(new SelfDeletingDelegate(&delegate, test_callback.callback()));
+  MockConnect mock_connect(ASYNC, ERR_CONNECTION_REFUSED);
+  StaticSocketDataProvider data;
+  data.set_connect_data(mock_connect);
+  mock_socket_factory.AddSocketDataProvider(&data);
+
+  TestURLRequestContext context;
+  scoped_refptr<SocketStream> socket_stream(
+      new SocketStream(GURL("ws://localhost:9998/echo"), delegate.get()));
+  socket_stream->set_context(&context);
+  socket_stream->SetClientSocketFactory(&mock_socket_factory);
+  delegate->set_socket_stream(socket_stream);
+
+  socket_stream->Connect();
+
+  test_callback.WaitForResult();
+  EXPECT_TRUE(!delegate);
+}
+
 }  // namespace net