DevTools: Removed refcounting from AndroidWebSocket

chrome/browser/devtools/device/android_web_socket.cc

Index: chrome/browser/devtools/device/android_web_socket.cc
diff --git a/chrome/browser/devtools/device/android_web_socket.cc b/chrome/browser/devtools/device/android_web_socket.cc
index 81d2627c7f6c0ae1dd5f9119367a82ba8b07bd6a..c2b2bd9d6f364c47a00985c9b0d3a9c33356b92d 100644
--- a/chrome/browser/devtools/device/android_web_socket.cc
+++ b/chrome/browser/devtools/device/android_web_socket.cc
@@ -2,6 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/memory/weak_ptr.h"
 #include "base/message_loop/message_loop.h"
 #include "base/rand_util.h"
 #include "chrome/browser/devtools/device/android_device_manager.h"
@@ -27,35 +28,42 @@ class WebSocketImpl : public AndroidDeviceManager::AndroidWebSocket {
                 const std::string& url,
                 Delegate* delegate);
 
-  virtual void Connect() OVERRIDE;
-  virtual void Disconnect() OVERRIDE;
   virtual void SendFrame(const std::string& message) OVERRIDE;
-  virtual void ClearDelegate() OVERRIDE;

[dgozman, 2014/08/08 14:17:28]

Why is this method removed?

[vkuzkokov, 2014/08/08 15:59:44]

It was used to signal AndroidWebSocket not to call delegate's methods anymore.
Now AWS just deleted instead.

+  virtual ~WebSocketImpl();
 
  private:
-  friend class base::RefCountedThreadSafe<AndroidWebSocket>;
-
-  virtual ~WebSocketImpl();
 
+  class Connection {

[dgozman, 2014/08/08 14:17:28]

Please, add a comment.

This is a WebSocketImpl counterpart which is constructed on UI thread and after
that leaves entirely on handler thread. It uses weak pointer to report back to
web socket on UI thread.

[vkuzkokov, 2014/08/08 15:59:44]

Done.

+   public:
+    Connection(base::WeakPtr<WebSocketImpl> weak_web_socket,
+               net::StreamSocket* socket);
+    void StartListening();
+    void SendFrame(const std::string& message);
+    void Disconnect();
+   private:
+    void OnBytesRead(scoped_refptr<net::IOBuffer> response_buffer, int result);
+    void SendPendingRequests(int result);
+    base::WeakPtr<WebSocketImpl> weak_web_socket_;
+    scoped_ptr<net::StreamSocket> socket_;
+    std::string response_buffer_;
+    std::string request_buffer_;
+  };

[dgozman, 2014/08/08 14:17:28]

DISALLOW_COPY_AND_ASSIGN

[vkuzkokov, 2014/08/08 15:59:44]

Done.

+
+  static void ConnectedWeak(base::WeakPtr<WebSocketImpl> weak_web_socket,
+                            int result, net::StreamSocket* socket);
   void Connected(int result, net::StreamSocket* socket);
-  void StartListeningOnHandlerThread();
-  void OnBytesRead(scoped_refptr<net::IOBuffer> response_buffer, int result);
-  void SendFrameOnHandlerThread(const std::string& message);
-  void SendPendingRequests(int result);
-  void DisconnectOnHandlerThread(bool closed_by_device);
 
   void OnSocketOpened();
   void OnFrameRead(const std::string& message);
-  void OnSocketClosed(bool closed_by_device);
+  void OnSocketClosed();
 
   scoped_refptr<base::MessageLoopProxy> device_message_loop_;
   scoped_refptr<Device> device_;
   std::string socket_name_;
   std::string url_;
-  scoped_ptr<net::StreamSocket> socket_;
+  scoped_ptr<Connection> connection_;

[dgozman, 2014/08/08 14:17:28]

You should use Connection* here, since you manage it's lifetime explicitly.

[vkuzkokov, 2014/08/08 15:59:44]

Done.

   Delegate* delegate_;
-  std::string response_buffer_;
-  std::string request_buffer_;
+  base::WeakPtrFactory<WebSocketImpl> weak_factory_;
 };

[dgozman, 2014/08/08 14:17:28]

DISALLOW_COPY_AND_ASSIGN

[vkuzkokov, 2014/08/08 15:59:44]

Done.

 
 WebSocketImpl::WebSocketImpl(
@@ -68,35 +76,23 @@ WebSocketImpl::WebSocketImpl(
       device_(device),
       socket_name_(socket_name),
       url_(url),
-      delegate_(delegate) {
-}
-
-void WebSocketImpl::Connect() {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+      delegate_(delegate),
+      weak_factory_(this) {
+  DCHECK(delegate_);
   device_->HttpUpgrade(
-      socket_name_, url_, base::Bind(&WebSocketImpl::Connected, this));
-}
-
-void WebSocketImpl::Disconnect() {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  device_message_loop_->PostTask(
-      FROM_HERE,
-      base::Bind(&WebSocketImpl::DisconnectOnHandlerThread, this, false));
+      socket_name_, url_,
+      base::Bind(&WebSocketImpl::ConnectedWeak, weak_factory_.GetWeakPtr()));
 }
 
 void WebSocketImpl::SendFrame(const std::string& message) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   device_message_loop_->PostTask(
       FROM_HERE,
-      base::Bind(&WebSocketImpl::SendFrameOnHandlerThread, this, message));
+      base::Bind(&Connection::SendFrame,
+                 base::Unretained(connection_.get()), message));
 }
 
-void WebSocketImpl::ClearDelegate() {
-  delegate_ = NULL;
-}
-
-void WebSocketImpl::SendFrameOnHandlerThread(const std::string& message) {
-  DCHECK_EQ(device_message_loop_, base::MessageLoopProxy::current());
+void WebSocketImpl::Connection::SendFrame(const std::string& message) {
   int mask = base::RandInt(0, 0x7FFFFFFF);
   std::string encoded_frame = WebSocket::EncodeFrameHybi17(message, mask);
   request_buffer_ += encoded_frame;
@@ -106,41 +102,56 @@ void WebSocketImpl::SendFrameOnHandlerThread(const std::string& message) {
 
 WebSocketImpl::~WebSocketImpl() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  device_message_loop_->DeleteSoon(FROM_HERE, connection_.release());
 }
 
-void WebSocketImpl::Connected(int result, net::StreamSocket* socket) {
+WebSocketImpl::Connection::Connection(
+    base::WeakPtr<WebSocketImpl> weak_web_socket,
+    net::StreamSocket* socket)
+    : weak_web_socket_(weak_web_socket),
+      socket_(socket) {
+}
+
+// static
+void WebSocketImpl::ConnectedWeak(base::WeakPtr<WebSocketImpl> weak_web_socket,
+                                  int result, net::StreamSocket* socket_raw) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  scoped_ptr<net::StreamSocket> socket(socket_raw);

[dgozman, 2014/08/08 14:17:28]

Please add a comment:

This method needs a separate static implementation, because it should destroy
socket_raw if the owning web socket is already destroyed.

[vkuzkokov, 2014/08/08 15:59:44]

Done.

+  if (WebSocketImpl* web_socket = weak_web_socket.get())

[dgozman, 2014/08/08 14:17:28]

if (weak_web_socket)
  weak_web_socket->Connected(result, socket.release());

[vkuzkokov, 2014/08/08 15:59:43]

Done.

+    web_socket->Connected(result, socket.release());
+}
+
+void WebSocketImpl::Connected(int result, net::StreamSocket* socket_raw) {
+  scoped_ptr<net::StreamSocket> socket(socket_raw);
   if (result != net::OK || socket == NULL) {
-    OnSocketClosed(true);
+    OnSocketClosed();
     return;
   }
-  socket_.reset(socket);
+  connection_.reset(
+      new Connection(weak_factory_.GetWeakPtr(), socket.release()));
   device_message_loop_->PostTask(
       FROM_HERE,
-      base::Bind(&WebSocketImpl::StartListeningOnHandlerThread, this));
+      base::Bind(&Connection::StartListening,
+                 base::Unretained(connection_.get())));
   OnSocketOpened();
 }
 
-void WebSocketImpl::StartListeningOnHandlerThread() {
-  DCHECK_EQ(device_message_loop_, base::MessageLoopProxy::current());
+void WebSocketImpl::Connection::StartListening() {
   scoped_refptr<net::IOBuffer> response_buffer =
       new net::IOBuffer(kBufferSize);
   int result = socket_->Read(
       response_buffer.get(),
       kBufferSize,
-      base::Bind(&WebSocketImpl::OnBytesRead, this, response_buffer));
+      base::Bind(&Connection::OnBytesRead,
+                 base::Unretained(this), response_buffer));
   if (result != net::ERR_IO_PENDING)
     OnBytesRead(response_buffer, result);
 }
 
-void WebSocketImpl::OnBytesRead(
+void WebSocketImpl::Connection::OnBytesRead(
     scoped_refptr<net::IOBuffer> response_buffer, int result) {
-  DCHECK_EQ(device_message_loop_, base::MessageLoopProxy::current());
-  if (!socket_)
-    return;
-
   if (result <= 0) {
-    DisconnectOnHandlerThread(true);
+    Disconnect();
     return;
   }
 
@@ -155,31 +166,29 @@ void WebSocketImpl::OnBytesRead(
   while (parse_result == WebSocket::FRAME_OK) {
     response_buffer_ = response_buffer_.substr(bytes_consumed);
     BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
-        base::Bind(&WebSocketImpl::OnFrameRead, this, output));
+        base::Bind(&WebSocketImpl::OnFrameRead, weak_web_socket_, output));
     parse_result = WebSocket::DecodeFrameHybi17(
         response_buffer_, false, &bytes_consumed, &output);
   }
 
   if (parse_result == WebSocket::FRAME_ERROR ||
       parse_result == WebSocket::FRAME_CLOSE) {
-    DisconnectOnHandlerThread(true);
+    Disconnect();
     return;
   }
 
   result = socket_->Read(
       response_buffer.get(),
       kBufferSize,
-      base::Bind(&WebSocketImpl::OnBytesRead, this, response_buffer));
+      base::Bind(&Connection::OnBytesRead,
+                 base::Unretained(this), response_buffer));
   if (result != net::ERR_IO_PENDING)
     OnBytesRead(response_buffer, result);
 }
 
-void WebSocketImpl::SendPendingRequests(int result) {
-  DCHECK_EQ(device_message_loop_, base::MessageLoopProxy::current());
-  if (!socket_)
-    return;
+void WebSocketImpl::Connection::SendPendingRequests(int result) {
   if (result < 0) {
-    DisconnectOnHandlerThread(true);
+    Disconnect();
     return;
   }
   request_buffer_ = request_buffer_.substr(result);
@@ -189,41 +198,34 @@ void WebSocketImpl::SendPendingRequests(int result) {
   scoped_refptr<net::StringIOBuffer> buffer =
       new net::StringIOBuffer(request_buffer_);
   result = socket_->Write(buffer.get(), buffer->size(),
-                          base::Bind(&WebSocketImpl::SendPendingRequests,
-                                     this));
+                          base::Bind(&Connection::SendPendingRequests,
+                                     base::Unretained(this)));
   if (result != net::ERR_IO_PENDING)
     SendPendingRequests(result);
 }
 
-void WebSocketImpl::DisconnectOnHandlerThread(bool closed_by_device) {
-  DCHECK_EQ(device_message_loop_, base::MessageLoopProxy::current());
-  if (!socket_)
-    return;
+void WebSocketImpl::Connection::Disconnect() {
   // Wipe out socket_ first since Disconnect can re-enter this method.
   scoped_ptr<net::StreamSocket> socket(socket_.release());

[dgozman, 2014/08/08 14:17:28]

This means you need all the |if (!socket_)| checks.

[vkuzkokov, 2014/08/08 15:59:44]

Added checks to public methods (StartListening and SendFrame). Private methods
are then guaranteed to be called only while socket is alive.

-  socket->Disconnect();
   BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
-      base::Bind(&WebSocketImpl::OnSocketClosed, this, closed_by_device));
+      base::Bind(&WebSocketImpl::OnSocketClosed, weak_web_socket_));
 }
 
 void WebSocketImpl::OnSocketOpened() {
-  if (delegate_)
-    delegate_->OnSocketOpened();
+  delegate_->OnSocketOpened();
 }
 
 void WebSocketImpl::OnFrameRead(const std::string& message) {
-  if (delegate_)
-    delegate_->OnFrameRead(message);
+  delegate_->OnFrameRead(message);
 }
 
-void WebSocketImpl::OnSocketClosed(bool closed_by_device) {
-  if (delegate_)
-    delegate_->OnSocketClosed(closed_by_device);
+void WebSocketImpl::OnSocketClosed() {
+  delegate_->OnSocketClosed();
 }
 
 }  // namespace
 
-scoped_refptr<AndroidDeviceManager::AndroidWebSocket>
+AndroidDeviceManager::AndroidWebSocket*
 AndroidDeviceManager::Device::CreateWebSocket(
     const std::string& socket,
     const std::string& url,