Test for WebSocketJob being deleted on the stack

net/websockets/websocket_job_test.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/websockets/websocket_job.h"
 
 #include <string>
 #include <vector>
 
 #include "base/bind.h"
@@ skipping 298 matching lines @@
 
  private:
   OrderedSocketData* data_;
   scoped_ptr<SpdySessionDependencies> session_deps_;
   scoped_refptr<HttpNetworkSession> http_session_;
   base::WeakPtr<SpdySession> session_;
   HostPortPair host_port_pair_;
   SpdySessionKey spdy_session_key_;
 };
 
+class DeletingSocketStreamDelegate : public SocketStream::Delegate {
+ public:
+  DeletingSocketStreamDelegate()
+      : delete_next_(false) {}
+
+  // Since this class needs to be able to delete |job_|, it must be the only
+  // reference holder (except for temporary references). Provide access to the
+  // pointer for tests to use.
+  WebSocketJob* job() { return job_.get(); }
+
+  void set_job(WebSocketJob* job) { job_ = job; }
+
+  // After calling this, the next call to a method on this delegate will delete
+  // the WebSocketJob object.
+  void set_delete_next(bool delete_next) { delete_next_ = delete_next; }
+
+  void DeleteJobMaybe() {
+    if (delete_next_) {
+      job_->DetachContext();
+      job_->DetachDelegate();
+      job_ = NULL;
+    }
+  }
+
+  // SocketStream::Delegate implementation
+
+  // OnStartOpenConnection() is not implemented by SocketStreamDispatcherHost
+
+  virtual void OnConnected(SocketStream* socket,
+                           int max_pending_send_allowed) OVERRIDE {
+    DeleteJobMaybe();
+  }
+
+  virtual void OnSentData(SocketStream* socket, int amount_sent) OVERRIDE {
+    DeleteJobMaybe();
+  }
+
+  virtual void OnReceivedData(SocketStream* socket,
+                              const char* data,
+                              int len) OVERRIDE {
+    DeleteJobMaybe();
+  }
+
+  virtual void OnClose(SocketStream* socket) OVERRIDE { DeleteJobMaybe(); }
+
+  virtual void OnAuthRequired(SocketStream* socket,
+                              AuthChallengeInfo* auth_info) OVERRIDE {
+    DeleteJobMaybe();
+  }
+
+  virtual void OnSSLCertificateError(SocketStream* socket,
+                                     const SSLInfo& ssl_info,
+                                     bool fatal) OVERRIDE {
+    DeleteJobMaybe();
+  }
+
+  virtual void OnError(const SocketStream* socket, int error) OVERRIDE {
+    DeleteJobMaybe();
+  }
+
+  // CanGetCookies() and CanSetCookies() do not appear to be able to delete the
+  // WebSocketJob object.
+
+ private:
+  scoped_refptr<WebSocketJob> job_;
+  bool delete_next_;
+};
+
 }  // namespace
 
 class WebSocketJobTest : public PlatformTest,
                          public ::testing::WithParamInterface<NextProto> {
  public:
   WebSocketJobTest() : spdy_util_(GetParam()) {}
 
   virtual void SetUp() OVERRIDE {
     stream_type_ = STREAM_INVALID;
     cookie_store_ = new MockCookieStore;
@@ skipping 144 matching lines @@
   static const char* const kHandshakeResponseForSpdy[];
   static const size_t kHandshakeRequestWithoutCookieLength;
   static const size_t kHandshakeRequestWithCookieLength;
   static const size_t kHandshakeRequestWithFilteredCookieLength;
   static const size_t kHandshakeResponseWithoutCookieLength;
   static const size_t kHandshakeResponseWithCookieLength;
   static const size_t kDataHelloLength;
   static const size_t kDataWorldLength;
 };
 
+// Tests using this fixture verify that the WebSocketJob can handle being
+// deleted while calling back to the delegate correctly. These tests need to be
+// run under AddressSanitizer or other systems for detecting use-after-free
+// errors in order to find problems.
+class WebSocketJobDeleteTest : public ::testing::Test {
+ protected:
+  WebSocketJobDeleteTest()
+      : delegate_(new DeletingSocketStreamDelegate),
+        cookie_store_(new MockCookieStore),
+        context_(new MockURLRequestContext(cookie_store_.get())) {
+    WebSocketJob* websocket = new WebSocketJob(delegate_.get());
+    delegate_->set_job(websocket);
+
+    socket_ = new MockSocketStream(
+        GURL("ws://127.0.0.1/"), websocket, context_.get(), NULL);
+
+    websocket->InitSocketStream(socket_.get());
+  }
+
+  void SetDeleteNext() { return delegate_->set_delete_next(true); }
+  WebSocketJob* job() { return delegate_->job(); }
+
+  scoped_ptr<DeletingSocketStreamDelegate> delegate_;
+  scoped_refptr<MockCookieStore> cookie_store_;
+  scoped_ptr<MockURLRequestContext> context_;
+  scoped_refptr<SocketStream> socket_;
+};
+
 const char WebSocketJobTest::kHandshakeRequestWithoutCookie[] =
     "GET /demo HTTP/1.1\r\n"
     "Host: example.com\r\n"
     "Upgrade: WebSocket\r\n"
     "Connection: Upgrade\r\n"
     "Sec-WebSocket-Key: dGhlIHNhbXBsZSBub25jZQ==\r\n"
     "Origin: http://example.com\r\n"
     "Sec-WebSocket-Protocol: sample\r\n"
     "Sec-WebSocket-Version: 13\r\n"
     "\r\n";
@@ skipping 622 matching lines @@
 TEST_P(WebSocketJobTest, ThrottlingSpdy) {
   WebSocketJob::set_websocket_over_spdy_enabled(false);
   TestConnectBySpdy(SPDY_OFF, THROTTLING_ON);
 }
 
 TEST_P(WebSocketJobTest, ThrottlingSpdySpdyEnabled) {
   WebSocketJob::set_websocket_over_spdy_enabled(true);
   TestConnectBySpdy(SPDY_ON, THROTTLING_ON);
 }
 
+TEST_F(WebSocketJobDeleteTest, OnClose) {
+  SetDeleteNext();
+  // This one is tricky because OnClose() sets WebSocketJob::_socket to NULL,
+  // so detaching it from within DeletingSocketStreamDelegate::DeleteJobMaybe()
+  // doesn't work. Detach it first to prevent a DCHECK(!delegate_) failure in
+  // the SocketStream destructor.
+  socket_->DetachDelegate();

[tyoshino (SeeGerritForStatus), 2014/04/04 04:39:07]

is this done by some code before OnClose() is called in real world?

[Adam Rice, 2014/04/04 06:00:36]

It appears OnClose() is called by real code only in SocketStream::Finish(). That
function does delegate_ = NULL after the call, which is sufficient to avoid the
DCHECK() in the destructor.

It's also called by WebSocketJob::OnCloseSpdyStream(). I have no idea what is
going on there, but since it isn't actually used I'm not going to worry about
it.

[tyoshino (SeeGerritForStatus), 2014/04/04 06:04:45]

Ah, right! Thanks for explanation.

+  job()->OnClose(socket_.get());
+  EXPECT_FALSE(job());
+}
+
+TEST_F(WebSocketJobDeleteTest, OnAuthRequired) {
+  SetDeleteNext();
+  job()->OnAuthRequired(socket_.get(), NULL);
+  EXPECT_FALSE(job());
+}
+
+TEST_F(WebSocketJobDeleteTest, OnSSLCertificateError) {
+  SSLInfo ssl_info;
+  SetDeleteNext();
+  job()->OnSSLCertificateError(socket_.get(), ssl_info, true);
+  EXPECT_FALSE(job());
+}
+
+TEST_F(WebSocketJobDeleteTest, OnError) {
+  SetDeleteNext();
+  job()->OnError(socket_.get(), ERR_CONNECTION_RESET);
+  EXPECT_FALSE(job());
+}
+
+TEST_F(WebSocketJobDeleteTest, OnSentSpdyHeaders) {
+  job()->Connect();
+  SetDeleteNext();
+  job()->OnSentSpdyHeaders();
+  EXPECT_FALSE(job());
+}
+
+TEST_F(WebSocketJobDeleteTest, OnSentHandshakeRequest) {
+  static const char kMinimalRequest[] =
+      "GET /demo HTTP/1.1\r\n"
+      "Host: example.com\r\n"
+      "Upgrade: WebSocket\r\n"
+      "Connection: Upgrade\r\n"
+      "Sec-WebSocket-Key: dGhlIHNhbXBsZSBub25jZQ==\r\n"
+      "Origin: http://example.com\r\n"
+      "Sec-WebSocket-Version: 13\r\n"
+      "\r\n";
+  const size_t kMinimalRequestSize = arraysize(kMinimalRequest) - 1;
+  job()->Connect();
+  job()->SendData(kMinimalRequest, kMinimalRequestSize);
+  SetDeleteNext();
+  job()->OnSentData(socket_.get(), kMinimalRequestSize);
+  EXPECT_FALSE(job());
+}
+
+TEST_F(WebSocketJobDeleteTest, NotifyHeadersComplete) {
+  static const char kMinimalResponse[] =
+      "HTTP/1.1 101 Switching Protocols\r\n"
+      "Upgrade: websocket\r\n"
+      "Connection: Upgrade\r\n"
+      "Sec-WebSocket-Accept: s3pPLMBiTxaQ9kYGzzhZRbK+xOo=\r\n"
+      "\r\n";
+  job()->Connect();
+  SetDeleteNext();
+  job()->OnReceivedData(
+      socket_.get(), kMinimalResponse, arraysize(kMinimalResponse) - 1);
+  EXPECT_FALSE(job());
+}
+
 // TODO(toyoshim): Add tests to verify throttling, SPDY stream limitation.
 // TODO(toyoshim,yutak): Add tests to verify closing handshake.
 }  // namespace net