Revert of Per-renderer WebSocket throttling (patchset #13 id:300001 of...

Revert of Per-renderer WebSocket throttling (patchset #13 id:300001 of https://codereview.chromium.org/972963002/)

Reason for revert:
Suspected to have caused WebSocket breakage.
Failure seen on apprtc.appspot.com and in browser_tests.  Run the manual MANUAL_WorksWithAppRTC test for repro.

See https://code.google.com/p/chromium/issues/detail?id=467471&thanks=467471&ts=1426498439 for details.

Original issue's description:
> Per-renderer WebSocket throttling
>
> Design Doc:
> https://docs.google.com/document/d/1aw2oN5PKfk-1gLnBrlv1OwLA8K3-ykM2ckwX2lubTg4/edit?usp=sharing
>
> BUG=459377
>
> Committed: https://crrev.com/58d344316cf2b2cc110619c0fea4979ef143b3a9
> Cr-Commit-Position: refs/heads/master@{#320074}

TBR=ricea@chromium.org,hiroshige@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=459377

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

Cr-Commit-Position: refs/heads/master@{#320705}

content/browser/renderer_host/websocket_dispatcher_host.cc

@@ -9,8 +9,6 @@
 
 #include "base/callback.h"
 #include "base/logging.h"
-#include "base/numerics/safe_conversions.h"
-#include "base/rand_util.h"
 #include "base/stl_util.h"
 #include "content/browser/child_process_security_policy_impl.h"
 #include "content/browser/renderer_host/websocket_host.h"
@@ -25,10 +23,6 @@ namespace {
 // fully-qualified every time.
 typedef WebSocketDispatcherHost::WebSocketHostState WebSocketHostState;
 
-// Max number of pending connections per WebSocketDispatcherHost
-// used for per-renderer WebSocket throttling.
-const int kMaxPendingWebSocketConnections = 255;
-
 }  // namespace
 
 WebSocketDispatcherHost::WebSocketDispatcherHost(
@@ -39,12 +33,7 @@ WebSocketDispatcherHost::WebSocketDispatcherHost(
       get_context_callback_(get_context_callback),
       websocket_host_factory_(
           base::Bind(&WebSocketDispatcherHost::CreateWebSocketHost,
-                     base::Unretained(this))),
-      num_pending_connections_(0),
-      num_current_succeeded_connections_(0),
-      num_previous_succeeded_connections_(0),
-      num_current_failed_connections_(0),
-      num_previous_failed_connections_(0) {}
+                     base::Unretained(this))) {}
 
 WebSocketDispatcherHost::WebSocketDispatcherHost(
     int process_id,
@@ -53,18 +42,10 @@ WebSocketDispatcherHost::WebSocketDispatcherHost(
     : BrowserMessageFilter(WebSocketMsgStart),
       process_id_(process_id),
       get_context_callback_(get_context_callback),
-      websocket_host_factory_(websocket_host_factory),
-      num_pending_connections_(0),
-      num_current_succeeded_connections_(0),
-      num_previous_succeeded_connections_(0),
-      num_current_failed_connections_(0),
-      num_previous_failed_connections_(0) {}
+      websocket_host_factory_(websocket_host_factory) {}
 
-WebSocketHost* WebSocketDispatcherHost::CreateWebSocketHost(
-    int routing_id,
-    base::TimeDelta delay) {
-  return new WebSocketHost(
-      routing_id, this, get_context_callback_.Run(), delay);
+WebSocketHost* WebSocketDispatcherHost::CreateWebSocketHost(int routing_id) {
+  return new WebSocketHost(routing_id, this, get_context_callback_.Run());
 }
 
 bool WebSocketDispatcherHost::OnMessageReceived(const IPC::Message& message) {
@@ -92,24 +73,8 @@ bool WebSocketDispatcherHost::OnMessageReceived(const IPC::Message& message) {
       // little extreme. So for now just ignore the bogus request.
       return true;  // We handled the message (by ignoring it).
     }
-    if (num_pending_connections_ >= kMaxPendingWebSocketConnections) {
-      if(!Send(new WebSocketMsg_NotifyFailure(routing_id,
-          "Error in connection establishment: net::ERR_INSUFFICIENT_RESOURCES"
-          ))) {
-        DVLOG(1) << "Sending of message type "
-                 << "WebSocketMsg_NotifyFailure failed.";
-      }
-      return true;
-    }
-    host = websocket_host_factory_.Run(routing_id, CalculateDelay());
+    host = websocket_host_factory_.Run(routing_id);
     hosts_.insert(WebSocketHostTable::value_type(routing_id, host));
-    ++num_pending_connections_;
-    if (!throttling_period_timer_.IsRunning())
-      throttling_period_timer_.Start(
-          FROM_HERE,
-          base::TimeDelta::FromMinutes(2),
-          this,
-          &WebSocketDispatcherHost::ThrottlingPeriodTimerCallback);
   }
   if (!host) {
     DVLOG(1) << "Received invalid routing ID " << routing_id
@@ -147,14 +112,6 @@ WebSocketHostState WebSocketDispatcherHost::SendAddChannelResponse(
     int routing_id,
     const std::string& selected_protocol,
     const std::string& extensions) {
-  // Update throttling counters (success).
-  WebSocketHost* host = GetHost(routing_id);
-  DCHECK(host);
-  host->OnHandshakeSucceeded();
-  --num_pending_connections_;
-  DCHECK_GE(num_pending_connections_, 0);
-  ++num_current_succeeded_connections_;
-
   return SendOrDrop(new WebSocketMsg_AddChannelResponse(
       routing_id, selected_protocol, extensions));
 }
@@ -235,55 +192,8 @@ WebSocketDispatcherHost::~WebSocketDispatcherHost() {
 void WebSocketDispatcherHost::DeleteWebSocketHost(int routing_id) {
   WebSocketHostTable::iterator it = hosts_.find(routing_id);
   DCHECK(it != hosts_.end());
-  DCHECK(it->second);
-  if (!it->second->handshake_succeeded()) {
-    // Update throttling counters (failure).
-    --num_pending_connections_;
-    DCHECK_GE(num_pending_connections_, 0);
-    ++num_current_failed_connections_;
-  }
-
   delete it->second;
   hosts_.erase(it);
-
-  DCHECK_LE(base::checked_cast<size_t>(num_pending_connections_),
-            hosts_.size());
-}
-
-int64_t WebSocketDispatcherHost::num_failed_connections() const {
-  return num_previous_failed_connections_ +
-         num_current_failed_connections_;
-}
-
-int64_t WebSocketDispatcherHost::num_succeeded_connections() const {
-  return num_previous_succeeded_connections_ +
-         num_current_succeeded_connections_;
-}
-
-// Calculate delay as described in
-// the per-renderer WebSocket throttling design doc:
-// https://docs.google.com/document/d/1aw2oN5PKfk-1gLnBrlv1OwLA8K3-ykM2ckwX2lubTg4/edit?usp=sharing
-base::TimeDelta WebSocketDispatcherHost::CalculateDelay() const {
-  int64_t f = num_failed_connections();
-  int64_t s = num_succeeded_connections();
-  int p = num_pending_connections();
-  return base::TimeDelta::FromMilliseconds(
-      base::RandInt(1000, 5000) *
-      (1 << std::min(p + f / (s + 1), INT64_C(16))) / 65536);
-}
-
-void WebSocketDispatcherHost::ThrottlingPeriodTimerCallback() {
-  num_previous_failed_connections_ = num_current_failed_connections_;
-  num_current_failed_connections_ = 0;
-
-  num_previous_succeeded_connections_ = num_current_succeeded_connections_;
-  num_current_succeeded_connections_ = 0;
-
-  if (num_pending_connections_ == 0 &&
-      num_previous_failed_connections_ == 0 &&
-      num_previous_succeeded_connections_ == 0) {
-    throttling_period_timer_.Stop();
-  }
 }
 
 }  // namespace content

content/browser/renderer_host/websocket_dispatcher_host.h

@@ -5,15 +5,13 @@
 #ifndef CONTENT_BROWSER_RENDERER_HOST_WEBSOCKET_DISPATCHER_HOST_H_
 #define CONTENT_BROWSER_RENDERER_HOST_WEBSOCKET_DISPATCHER_HOST_H_
 
-#include <stdint.h>
 #include <string>
 #include <vector>
 
+#include "base/basictypes.h"
 #include "base/callback.h"
 #include "base/compiler_specific.h"
 #include "base/containers/hash_tables.h"
-#include "base/time/time.h"
-#include "base/timer/timer.h"
 #include "content/common/content_export.h"
 #include "content/common/websocket.h"
 #include "content/public/browser/browser_message_filter.h"
@@ -34,10 +32,9 @@ class CONTENT_EXPORT WebSocketDispatcherHost : public BrowserMessageFilter {
  public:
   typedef base::Callback<net::URLRequestContext*()> GetRequestContextCallback;
 
-  // Given a routing_id and delay, WebSocketHostFactory returns a new
-  // instance of WebSocketHost or its subclass.
-  typedef base::Callback<WebSocketHost*(int, base::TimeDelta)>
-      WebSocketHostFactory;
+  // Given a routing_id, WebSocketHostFactory returns a new instance of
+  // WebSocketHost or its subclass.
+  typedef base::Callback<WebSocketHost*(int)> WebSocketHostFactory;  // NOLINT
 
   // Return value for methods that may delete the WebSocketHost. This enum is
   // binary-compatible with net::WebSocketEventInterface::ChannelState, to make
@@ -52,6 +49,12 @@ class CONTENT_EXPORT WebSocketDispatcherHost : public BrowserMessageFilter {
       int process_id,
       const GetRequestContextCallback& get_context_callback);
 
+  // For testing. Specify a factory method that creates mock version of
+  // WebSocketHost.
+  WebSocketDispatcherHost(int process_id,
+                          const GetRequestContextCallback& get_context_callback,
+                          const WebSocketHostFactory& websocket_host_factory);
+
   // BrowserMessageFilter:
   bool OnMessageReceived(const IPC::Message& message) override;
 
@@ -108,26 +111,12 @@ class CONTENT_EXPORT WebSocketDispatcherHost : public BrowserMessageFilter {
 
   int render_process_id() const { return process_id_; }
 
- protected:
-  // For testing. Specify a factory method that creates mock version of
-  // WebSocketHost.
-  WebSocketDispatcherHost(int process_id,
-                          const GetRequestContextCallback& get_context_callback,
-                          const WebSocketHostFactory& websocket_host_factory);
-
-  int num_pending_connections() const { return num_pending_connections_; }
-
-  // The number of handshakes that failed/succeeded in the current and
-  // previous time period, respectively.
-  int64_t num_failed_connections() const;
-  int64_t num_succeeded_connections() const;
-
-  ~WebSocketDispatcherHost() override;
-
  private:
   typedef base::hash_map<int, WebSocketHost*> WebSocketHostTable;
 
-  WebSocketHost* CreateWebSocketHost(int routing_id, base::TimeDelta delay);
+  ~WebSocketDispatcherHost() override;
+
+  WebSocketHost* CreateWebSocketHost(int routing_id);
 
   // Looks up a WebSocketHost object by |routing_id|. Returns the object if one
   // is found, or NULL otherwise.
@@ -143,14 +132,6 @@ class CONTENT_EXPORT WebSocketDispatcherHost : public BrowserMessageFilter {
   // removes it from the |hosts_| table.
   void DeleteWebSocketHost(int routing_id);
 
-  // Calculates the delay for per-renderer WebSocket throttling.
-  base::TimeDelta CalculateDelay() const;
-
-  // Rotates the counts of successful and failed connections for current
-  // and previous time periods. Called every two minutes while the counts
-  // are non-zero.
-  void ThrottlingPeriodTimerCallback();
-
   // Table of WebSocketHost objects, owned by this object, indexed by
   // routing_id.
   WebSocketHostTable hosts_;
@@ -164,22 +145,6 @@ class CONTENT_EXPORT WebSocketDispatcherHost : public BrowserMessageFilter {
 
   WebSocketHostFactory websocket_host_factory_;
 
-  // Timer and counters for per-renderer WebSocket throttling.
-  base::RepeatingTimer<WebSocketDispatcherHost> throttling_period_timer_;
-
-  // The current number of pending connections.
-  int num_pending_connections_;
-
-  // The number of handshakes that failed in the current and previous time
-  // period.
-  int64_t num_current_succeeded_connections_;
-  int64_t num_previous_succeeded_connections_;
-
-  // The number of handshakes that succeeded in the current and previous time
-  // period.
-  int64_t num_current_failed_connections_;
-  int64_t num_previous_failed_connections_;
-
   DISALLOW_COPY_AND_ASSIGN(WebSocketDispatcherHost);
 };
 

content/browser/renderer_host/websocket_host.cc

@@ -312,15 +312,10 @@ void WebSocketEventHandler::SSLErrorHandlerDelegate::ContinueSSLRequest() {
 
 WebSocketHost::WebSocketHost(int routing_id,
                              WebSocketDispatcherHost* dispatcher,
-                             net::URLRequestContext* url_request_context,
-                             base::TimeDelta delay)
+                             net::URLRequestContext* url_request_context)
     : dispatcher_(dispatcher),
       url_request_context_(url_request_context),
-      routing_id_(routing_id),
-      delay_(delay),
-      pending_flow_control_quota_(0),
-      handshake_succeeded_(false),
-      weak_ptr_factory_(this) {
+      routing_id_(routing_id) {
   DVLOG(1) << "WebSocketHost: created routing_id=" << routing_id;
 }
 
@@ -354,44 +349,11 @@ void WebSocketHost::OnAddChannelRequest(
            << origin.string() << "\"";
 
   DCHECK(!channel_);
-  if (delay_ > base::TimeDelta()) {
-    base::MessageLoop::current()->PostDelayedTask(
-        FROM_HERE,
-        base::Bind(&WebSocketHost::AddChannel,
-                   weak_ptr_factory_.GetWeakPtr(),
-                   socket_url,
-                   requested_protocols,
-                   origin,
-                   render_frame_id),
-        delay_);
-  } else {
-    AddChannel(socket_url, requested_protocols, origin, render_frame_id);
-  }
-}
-
-void WebSocketHost::AddChannel(
-    const GURL& socket_url,
-    const std::vector<std::string>& requested_protocols,
-    const url::Origin& origin,
-    int render_frame_id) {
-  DVLOG(3) << "WebSocketHost::AddChannel"
-           << " routing_id=" << routing_id_ << " socket_url=\"" << socket_url
-           << "\" requested_protocols=\""
-           << JoinString(requested_protocols, ", ") << "\" origin=\""
-           << origin.string() << "\"";
-
-  DCHECK(!channel_);
-
   scoped_ptr<net::WebSocketEventInterface> event_interface(
       new WebSocketEventHandler(dispatcher_, routing_id_, render_frame_id));
   channel_.reset(
       new net::WebSocketChannel(event_interface.Pass(), url_request_context_));
   channel_->SendAddChannelRequest(socket_url, requested_protocols, origin);
-
-  if (pending_flow_control_quota_ > 0) {
-    channel_->SendFlowControl(pending_flow_control_quota_);
-    pending_flow_control_quota_ = 0;
-  }
 }
 
 void WebSocketHost::OnSendFrame(bool fin,
@@ -409,14 +371,7 @@ void WebSocketHost::OnFlowControl(int64 quota) {
   DVLOG(3) << "WebSocketHost::OnFlowControl"
            << " routing_id=" << routing_id_ << " quota=" << quota;
 
-  if (!channel_) {
-    // WebSocketChannel is not yet created due to the delay introduced by
-    // per-renderer WebSocket throttling.
-    // SendFlowControl() is called after WebSocketChannel is created.
-    pending_flow_control_quota_ += quota;
-    return;
-  }
-
+  DCHECK(channel_);
   channel_->SendFlowControl(quota);
 }
 
@@ -427,18 +382,7 @@ void WebSocketHost::OnDropChannel(bool was_clean,
            << " routing_id=" << routing_id_ << " was_clean=" << was_clean
            << " code=" << code << " reason=\"" << reason << "\"";
 
-  if (!channel_) {
-    // WebSocketChannel is not yet created due to the delay introduced by
-    // per-renderer WebSocket throttling.
-    WebSocketDispatcherHost::WebSocketHostState result =
-        dispatcher_->DoDropChannel(routing_id_,
-                                   false,
-                                   net::kWebSocketErrorAbnormalClosure,
-                                   "");
-    DCHECK_EQ(WebSocketDispatcherHost::WEBSOCKET_HOST_DELETED, result);
-    return;
-  }
-
+  DCHECK(channel_);
   // TODO(yhirano): Handle |was_clean| appropriately.
   channel_->StartClosingHandshake(code, reason);
 }

content/browser/renderer_host/websocket_host.h

@@ -9,8 +9,6 @@
 #include <vector>
 
 #include "base/memory/scoped_ptr.h"
-#include "base/memory/weak_ptr.h"
-#include "base/time/time.h"
 #include "content/common/content_export.h"
 #include "content/common/websocket.h"
 
@@ -39,8 +37,7 @@ class CONTENT_EXPORT WebSocketHost {
  public:
   WebSocketHost(int routing_id,
                 WebSocketDispatcherHost* dispatcher,
-                net::URLRequestContext* url_request_context,
-                base::TimeDelta delay);
+                net::URLRequestContext* url_request_context);
   virtual ~WebSocketHost();
 
   // The renderer process is going away.
@@ -53,9 +50,6 @@ class CONTENT_EXPORT WebSocketHost {
 
   int routing_id() const { return routing_id_; }
 
-  bool handshake_succeeded() const { return handshake_succeeded_; }
-  void OnHandshakeSucceeded() { handshake_succeeded_ = true; }
-
  private:
   // Handlers for each message type, dispatched by OnMessageReceived(), as
   // defined in content/common/websocket_messages.h
@@ -65,11 +59,6 @@ class CONTENT_EXPORT WebSocketHost {
                            const url::Origin& origin,
                            int render_frame_id);
 
-  void AddChannel(const GURL& socket_url,
-                  const std::vector<std::string>& requested_protocols,
-                  const url::Origin& origin,
-                  int render_frame_id);
-
   void OnSendFrame(bool fin,
                    WebSocketMessageType type,
                    const std::vector<char>& data);
@@ -90,20 +79,6 @@ class CONTENT_EXPORT WebSocketHost {
   // The ID used to route messages.
   const int routing_id_;
 
-  // Delay used for per-renderer WebSocket throttling.
-  base::TimeDelta delay_;
-
-  // SendFlowControl() is delayed when OnFlowControl() is called before
-  // AddChannel() is called.
-  // Zero indicates there is no pending SendFlowControl().
-  int64_t pending_flow_control_quota_;
-
-  // handshake_succeeded_ is set and used by WebSocketDispatcherHost
-  // to manage counters for per-renderer WebSocket throttling.
-  bool handshake_succeeded_;
-
-  base::WeakPtrFactory<WebSocketHost> weak_ptr_factory_;
-
   DISALLOW_COPY_AND_ASSIGN(WebSocketHost);
 };