See

https://docs.google.com/a/chromium.org/document/d/1fMxcEFiVj-H5vmuxhPqsxJAl98GS6_yZ-wqB6j9Wmy0/pub
for motivation and design.

Also add tests that it doesn't crash when deleted.

Also add the WebSocketDispatcherHost to the filters for IPC channels.

BUG=305515

Review URL: https://codereview.chromium.org/26544003

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

content/browser/renderer_host/render_process_host_impl.cc

@@ -93,6 +93,7 @@
 #include "content/browser/renderer_host/render_widget_host_impl.h"
 #include "content/browser/renderer_host/socket_stream_dispatcher_host.h"
 #include "content/browser/renderer_host/text_input_client_message_filter.h"
+#include "content/browser/renderer_host/websocket_dispatcher_host.h"
 #include "content/browser/resolve_proxy_msg_helper.h"
 #include "content/browser/service_worker/service_worker_context.h"
 #include "content/browser/service_worker/service_worker_dispatcher_host.h"
@@ -674,6 +675,13 @@ void RenderProcessHostImpl::CreateMessageFilters() {
           GetID(), request_context_callback, resource_context);
   AddFilter(socket_stream_dispatcher_host);
 
+  WebSocketDispatcherHost::GetRequestContextCallback
+      websocket_request_context_callback(
+          base::Bind(&GetRequestContext, request_context,
+                     media_request_context, ResourceType::SUB_RESOURCE));
+
+  AddFilter(new WebSocketDispatcherHost(websocket_request_context_callback));
+
   message_port_message_filter_ = new MessagePortMessageFilter(
       base::Bind(&RenderWidgetHelper::GetNextRoutingID,
                  base::Unretained(widget_helper_.get())));

content/browser/renderer_host/websocket_dispatcher_host.cc

@@ -14,6 +14,15 @@
 
 namespace content {
 
+namespace {
+
+// Many methods defined in this file return a WebSocketHostState enum
+// value. Make WebSocketHostState visible at file scope so it doesn't have to be
+// fully-qualified every time.
+typedef WebSocketDispatcherHost::WebSocketHostState WebSocketHostState;
+
+}  // namespace
+
 WebSocketDispatcherHost::WebSocketDispatcherHost(
     const GetRequestContextCallback& get_context_callback)
     : get_context_callback_(get_context_callback),
@@ -73,45 +82,59 @@ WebSocketHost* WebSocketDispatcherHost::GetHost(int routing_id) const {
   return it == hosts_.end() ? NULL : it->second;
 }
 
-void WebSocketDispatcherHost::SendOrDrop(IPC::Message* message) {
+WebSocketHostState WebSocketDispatcherHost::SendOrDrop(IPC::Message* message) {
   if (!Send(message)) {
     DVLOG(1) << "Sending of message type " << message->type()
              << " failed. Dropping channel.";
     DeleteWebSocketHost(message->routing_id());
+    return WEBSOCKET_HOST_DELETED;
   }
+  return WEBSOCKET_HOST_ALIVE;
 }
 
-void WebSocketDispatcherHost::SendAddChannelResponse(
+WebSocketHostState WebSocketDispatcherHost::SendAddChannelResponse(
     int routing_id,
     bool fail,
     const std::string& selected_protocol,
     const std::string& extensions) {
-  SendOrDrop(new WebSocketMsg_AddChannelResponse(
-      routing_id, fail, selected_protocol, extensions));
-  if (fail)
+  if (SendOrDrop(new WebSocketMsg_AddChannelResponse(
+          routing_id, fail, selected_protocol, extensions)) ==
+      WEBSOCKET_HOST_DELETED)
+    return WEBSOCKET_HOST_DELETED;
+  if (fail) {
     DeleteWebSocketHost(routing_id);
+    return WEBSOCKET_HOST_DELETED;
+  }
+  return WEBSOCKET_HOST_ALIVE;
 }
 
-void WebSocketDispatcherHost::SendFrame(int routing_id,
-                                        bool fin,
-                                        WebSocketMessageType type,
-                                        const std::vector<char>& data) {
-  SendOrDrop(new WebSocketMsg_SendFrame(routing_id, fin, type, data));
+WebSocketHostState WebSocketDispatcherHost::SendFrame(
+    int routing_id,
+    bool fin,
+    WebSocketMessageType type,
+    const std::vector<char>& data) {
+  return SendOrDrop(new WebSocketMsg_SendFrame(routing_id, fin, type, data));
 }
 
-void WebSocketDispatcherHost::SendFlowControl(int routing_id, int64 quota) {
-  SendOrDrop(new WebSocketMsg_FlowControl(routing_id, quota));
+WebSocketHostState WebSocketDispatcherHost::SendFlowControl(int routing_id,
+                                                            int64 quota) {
+  return SendOrDrop(new WebSocketMsg_FlowControl(routing_id, quota));
 }
 
-void WebSocketDispatcherHost::SendClosing(int routing_id) {
+WebSocketHostState WebSocketDispatcherHost::SendClosing(int routing_id) {
   // TODO(ricea): Implement the SendClosing IPC.
+  return WEBSOCKET_HOST_ALIVE;
 }
 
-void WebSocketDispatcherHost::DoDropChannel(int routing_id,
-                                            uint16 code,
-                                            const std::string& reason) {
-  SendOrDrop(new WebSocketMsg_DropChannel(routing_id, code, reason));
+WebSocketHostState WebSocketDispatcherHost::DoDropChannel(
+    int routing_id,
+    uint16 code,
+    const std::string& reason) {
+  if (SendOrDrop(new WebSocketMsg_DropChannel(routing_id, code, reason)) ==
+      WEBSOCKET_HOST_DELETED)
+    return WEBSOCKET_HOST_DELETED;
   DeleteWebSocketHost(routing_id);
+  return WEBSOCKET_HOST_DELETED;
 }
 
 WebSocketDispatcherHost::~WebSocketDispatcherHost() {
@@ -120,10 +143,9 @@ WebSocketDispatcherHost::~WebSocketDispatcherHost() {
 
 void WebSocketDispatcherHost::DeleteWebSocketHost(int routing_id) {
   WebSocketHostTable::iterator it = hosts_.find(routing_id);
-  if (it != hosts_.end()) {
-    delete it->second;
-    hosts_.erase(it);
-  }
+  DCHECK(it != hosts_.end());
+  delete it->second;
+  hosts_.erase(it);
 }
 
 }  // namespace content

content/browser/renderer_host/websocket_dispatcher_host.h

@@ -10,6 +10,7 @@
 
 #include "base/basictypes.h"
 #include "base/callback.h"
+#include "base/compiler_specific.h"
 #include "base/containers/hash_tables.h"
 #include "content/common/content_export.h"
 #include "content/common/websocket.h"
@@ -33,6 +34,15 @@ class CONTENT_EXPORT WebSocketDispatcherHost : public BrowserMessageFilter {
   // WebSocketHost or its subclass.
   typedef base::Callback<WebSocketHost*(int)> WebSocketHostFactory;
 
+  // Return value for methods that may delete the WebSocketHost. This enum is
+  // binary-compatible with net::WebSocketEventInterface::ChannelState, to make
+  // conversion cheap. By using a separate enum including net/ header files can
+  // be avoided.
+  enum WebSocketHostState {
+    WEBSOCKET_HOST_ALIVE,
+    WEBSOCKET_HOST_DELETED
+  };
+
   explicit WebSocketDispatcherHost(
       const GetRequestContextCallback& get_context_callback);
 
@@ -48,30 +58,36 @@ class CONTENT_EXPORT WebSocketDispatcherHost : public BrowserMessageFilter {
 
   // The following methods are used by WebSocketHost::EventInterface to send
   // IPCs from the browser to the renderer or child process. Any of them may
-  // delete the WebSocketHost on failure, leading to the WebSocketChannel and
-  // EventInterface also being deleted.
+  // return WEBSOCKET_HOST_DELETED and delete the WebSocketHost on failure,
+  // leading to the WebSocketChannel and EventInterface also being deleted.
 
   // Sends a WebSocketMsg_AddChannelResponse IPC, and then deletes and
   // unregisters the WebSocketHost if |fail| is true.
-  void SendAddChannelResponse(int routing_id,
-                              bool fail,
-                              const std::string& selected_protocol,
-                              const std::string& extensions);
+  WebSocketHostState SendAddChannelResponse(
+      int routing_id,
+      bool fail,
+      const std::string& selected_protocol,
+      const std::string& extensions) WARN_UNUSED_RESULT;
 
   // Sends a WebSocketMsg_SendFrame IPC.
-  void SendFrame(int routing_id,
-                 bool fin,
-                 WebSocketMessageType type,
-                 const std::vector<char>& data);
+  WebSocketHostState SendFrame(int routing_id,
+                               bool fin,
+                               WebSocketMessageType type,
+                               const std::vector<char>& data);
 
   // Sends a WebSocketMsg_FlowControl IPC.
-  void SendFlowControl(int routing_id, int64 quota);
+  WebSocketHostState SendFlowControl(int routing_id,
+                                     int64 quota) WARN_UNUSED_RESULT;
 
   // Sends a WebSocketMsg_SendClosing IPC
-  void SendClosing(int routing_id);
+  WebSocketHostState SendClosing(int routing_id) WARN_UNUSED_RESULT;
 
-  // Sends a WebSocketMsg_DropChannel IPC and delete and unregister the channel.
-  void DoDropChannel(int routing_id, uint16 code, const std::string& reason);
+  // Sends a WebSocketMsg_DropChannel IPC and deletes and unregisters the
+  // channel.
+  WebSocketHostState DoDropChannel(
+      int routing_id,
+      uint16 code,
+      const std::string& reason) WARN_UNUSED_RESULT;
 
  private:
   typedef base::hash_map<int, WebSocketHost*> WebSocketHostTable;
@@ -85,13 +101,13 @@ class CONTENT_EXPORT WebSocketDispatcherHost : public BrowserMessageFilter {
   WebSocketHost* GetHost(int routing_id) const;
 
   // Sends the passed in IPC::Message via the BrowserMessageFilter::Send()
-  // method. If the Send() fails, logs it and deletes the corresponding
-  // WebSocketHost object (the client is probably dead).
-  void SendOrDrop(IPC::Message* message);
+  // method. If sending the IPC fails, assumes that this connection is no
+  // longer useable, calls DeleteWebSocketHost(), and returns
+  // WEBSOCKET_HOST_DELETED. The behaviour is the same for all message types.
+  WebSocketHostState SendOrDrop(IPC::Message* message) WARN_UNUSED_RESULT;
 
   // Deletes the WebSocketHost object associated with the given |routing_id| and
-  // removes it from the |hosts_| table. Does nothing if the |routing_id| is
-  // unknown, since SendAddChannelResponse() may call this method twice.
+  // removes it from the |hosts_| table.
   void DeleteWebSocketHost(int routing_id);
 
   // Table of WebSocketHost objects, owned by this object, indexed by

content/browser/renderer_host/websocket_host.cc

@@ -17,6 +17,8 @@ namespace content {
 
 namespace {
 
+typedef net::WebSocketEventInterface::ChannelState ChannelState;
+
 // Convert a content::WebSocketMessageType to a
 // net::WebSocketFrameHeader::OpCode
 net::WebSocketFrameHeader::OpCode MessageTypeToOpCode(
@@ -48,6 +50,25 @@ WebSocketMessageType OpCodeToMessageType(
   return static_cast<WebSocketMessageType>(opCode);
 }
 
+ChannelState StateCast(WebSocketDispatcherHost::WebSocketHostState host_state) {
+  const WebSocketDispatcherHost::WebSocketHostState WEBSOCKET_HOST_ALIVE =
+      WebSocketDispatcherHost::WEBSOCKET_HOST_ALIVE;
+  const WebSocketDispatcherHost::WebSocketHostState WEBSOCKET_HOST_DELETED =
+      WebSocketDispatcherHost::WEBSOCKET_HOST_DELETED;
+
+  DCHECK(host_state == WEBSOCKET_HOST_ALIVE ||
+         host_state == WEBSOCKET_HOST_DELETED);
+  // These compile asserts verify that we can get away with using static_cast<>
+  // for the conversion.
+  COMPILE_ASSERT(static_cast<ChannelState>(WEBSOCKET_HOST_ALIVE) ==
+                     net::WebSocketEventInterface::CHANNEL_ALIVE,
+                 enum_values_must_match_for_state_alive);
+  COMPILE_ASSERT(static_cast<ChannelState>(WEBSOCKET_HOST_DELETED) ==
+                     net::WebSocketEventInterface::CHANNEL_DELETED,
+                 enum_values_must_match_for_state_deleted);
+  return static_cast<ChannelState>(host_state);
+}
+
 // Implementation of net::WebSocketEventInterface. Receives events from our
 // WebSocketChannel object. Each event is translated to an IPC and sent to the
 // renderer or child process via WebSocketDispatcherHost.
@@ -61,16 +82,16 @@ class WebSocketEventHandler : public net::WebSocketEventInterface {
   // TODO(ricea): Add |extensions| parameter to pass the list of enabled
   // WebSocket extensions through to the renderer to make it visible to
   // Javascript.
-  virtual void OnAddChannelResponse(
+  virtual ChannelState OnAddChannelResponse(
       bool fail,
       const std::string& selected_subprotocol) OVERRIDE;
-  virtual void OnDataFrame(bool fin,
-                           WebSocketMessageType type,
-                           const std::vector<char>& data) OVERRIDE;
-  virtual void OnClosingHandshake() OVERRIDE;
-  virtual void OnFlowControl(int64 quota) OVERRIDE;
-  virtual void OnDropChannel(uint16 code,
-                             const std::string& reason) OVERRIDE;
+  virtual ChannelState OnDataFrame(bool fin,
+                                   WebSocketMessageType type,
+                                   const std::vector<char>& data) OVERRIDE;
+  virtual ChannelState OnClosingHandshake() OVERRIDE;
+  virtual ChannelState OnFlowControl(int64 quota) OVERRIDE;
+  virtual ChannelState OnDropChannel(uint16 code,
+                                     const std::string& reason) OVERRIDE;
 
  private:
   WebSocketDispatcherHost* const dispatcher_;
@@ -88,35 +109,32 @@ WebSocketEventHandler::~WebSocketEventHandler() {
   DVLOG(1) << "WebSocketEventHandler destroyed routing_id= " << routing_id_;
 }
 
-void WebSocketEventHandler::OnAddChannelResponse(
+ChannelState WebSocketEventHandler::OnAddChannelResponse(
     bool fail,
     const std::string& selected_protocol) {
-  dispatcher_->SendAddChannelResponse(
-      routing_id_, fail, selected_protocol, std::string());
-  // |this| may have been deleted here.
+  return StateCast(dispatcher_->SendAddChannelResponse(
+      routing_id_, fail, selected_protocol, std::string()));
 }
 
-void WebSocketEventHandler::OnDataFrame(bool fin,
-                                        net::WebSocketFrameHeader::OpCode type,
-                                        const std::vector<char>& data) {
-  dispatcher_->SendFrame(routing_id_, fin, OpCodeToMessageType(type), data);
-  // |this| may have been deleted here.
+ChannelState WebSocketEventHandler::OnDataFrame(
+    bool fin,
+    net::WebSocketFrameHeader::OpCode type,
+    const std::vector<char>& data) {
+  return StateCast(dispatcher_->SendFrame(
+      routing_id_, fin, OpCodeToMessageType(type), data));
 }
 
-void WebSocketEventHandler::OnClosingHandshake() {
-  dispatcher_->SendClosing(routing_id_);
-  // |this| may have been deleted here.
+ChannelState WebSocketEventHandler::OnClosingHandshake() {
+  return StateCast(dispatcher_->SendClosing(routing_id_));
 }
 
-void WebSocketEventHandler::OnFlowControl(int64 quota) {
-  dispatcher_->SendFlowControl(routing_id_, quota);
-  // |this| may have been deleted here.
+ChannelState WebSocketEventHandler::OnFlowControl(int64 quota) {
+  return StateCast(dispatcher_->SendFlowControl(routing_id_, quota));
 }
 
-void WebSocketEventHandler::OnDropChannel(uint16 code,
-                                          const std::string& reason) {
-  dispatcher_->DoDropChannel(routing_id_, code, reason);
-  // |this| has been deleted here.
+ChannelState WebSocketEventHandler::OnDropChannel(uint16 code,
+                                                  const std::string& reason) {
+  return StateCast(dispatcher_->DoDropChannel(routing_id_, code, reason));
 }
 
 }  // namespace

content/browser/renderer_host/websocket_host.h

@@ -27,8 +27,8 @@ namespace content {
 
 class WebSocketDispatcherHost;
 
-// Host of WebSocketChannel. The lifetime of an instance of this class is
-// completely controlled by the WebSocketDispatcherHost.
+// Host of net::WebSocketChannel. The lifetime of an instance of this class is
+// completely controlled by the WebSocketDispatcherHost object.
 class CONTENT_EXPORT WebSocketHost {
  public:
   WebSocketHost(int routing_id,