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

Review URL: https://chromiumcodereview.appspot.com/15989003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@202414 0039d316-1c4b-4281-b951-d872f2087c98

net/socket_stream/socket_stream.cc

@@ -288,6 +288,10 @@ void SocketStream::DetachDelegate() {
   if (!delegate_)
     return;
   delegate_ = NULL;
+  // Prevent the rest of the function from executing if we are being called from
+  // within Finish().
+  if (next_state_ == STATE_NONE)
+    return;
   net_log_.AddEvent(NetLog::TYPE_CANCELLED);
   // We don't need to send pending data when client detach the delegate.
   pending_write_bufs_.clear();
@@ -363,14 +367,13 @@ void SocketStream::Finish(int result) {
   DVLOG(1) << "Finish result=" << ErrorToString(result);
 
   metrics_->OnClose();
-  Delegate* delegate = delegate_;
+
+  if (result != ERR_CONNECTION_CLOSED && delegate_)
+    delegate_->OnError(this, result);
+  if (result != ERR_PROTOCOL_SWITCHED && delegate_)
+    delegate_->OnClose(this);
   delegate_ = NULL;
-  if (delegate) {
-    if (result != ERR_CONNECTION_CLOSED)
-      delegate->OnError(this, result);
-    if (result != ERR_PROTOCOL_SWITCHED)
-      delegate->OnClose(this);
-  }
+
   Release();
 }
 

net/socket_stream/socket_stream_unittest.cc

@@ -181,6 +181,54 @@ class SocketStreamEventRecorder : public SocketStream::Delegate {
   DISALLOW_COPY_AND_ASSIGN(SocketStreamEventRecorder);
 };
 
+// This is used for the test OnErrorDetachDelegate.
+class SelfDeletingDelegate : public SocketStream::Delegate {
+ public:
+  // |callback| must cause the test message loop to exit when called.
+  explicit SelfDeletingDelegate(const CompletionCallback& callback)
+      : socket_stream_(), callback_(callback) {}
+
+  virtual ~SelfDeletingDelegate() {}
+
+  // Call DetachDelegate(), delete |this|, then run the callback.
+  virtual void OnError(const SocketStream* socket, int error) OVERRIDE {
+    // callback_ will be deleted when we delete |this|, so copy it to call it
+    // afterwards.
+    CompletionCallback callback = callback_;
+    socket_stream_->DetachDelegate();
+    delete this;
+    callback.Run(OK);
+  }
+
+  // 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;
+    EXPECT_EQ(socket_stream_->delegate(), this);
+  }
+
+  virtual void OnConnected(SocketStream* socket, int max_pending_send_allowed)
+      OVERRIDE {
+    ADD_FAILURE() << "OnConnected() should not be called";
+  }
+  virtual void OnSentData(SocketStream* socket, int amount_sent) OVERRIDE {
+    ADD_FAILURE() << "OnSentData() should not be called";
+  }
+  virtual void OnReceivedData(SocketStream* socket, const char* data, int len)
+      OVERRIDE {
+    ADD_FAILURE() << "OnReceivedData() should not be called";
+  }
+  virtual void OnClose(SocketStream* socket) OVERRIDE {
+    ADD_FAILURE() << "OnClose() should not be called";
+  }
+
+ private:
+  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 +894,34 @@ 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;
+
+  // SelfDeletingDelegate is self-owning; we just need a pointer to it to
+  // connect it and the SocketStream.
+  SelfDeletingDelegate* delegate =
+      new SelfDeletingDelegate(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));
+  socket_stream->set_context(&context);
+  socket_stream->SetClientSocketFactory(&mock_socket_factory);
+  delegate->set_socket_stream(socket_stream);
+  // The delegate pointer will become invalid during the test. Set it to NULL to
+  // avoid holding a dangling pointer.
+  delegate = NULL;
+
+  socket_stream->Connect();
+
+  EXPECT_EQ(OK, test_callback.WaitForResult());
+}
+
 }  // namespace net