Per-renderer WebSocket throttling

content/browser/renderer_host/websocket_dispatcher_host_unittest.cc

Index: content/browser/renderer_host/websocket_dispatcher_host_unittest.cc
diff --git a/content/browser/renderer_host/websocket_dispatcher_host_unittest.cc b/content/browser/renderer_host/websocket_dispatcher_host_unittest.cc
index 0d2e7057bbcef5c0d90dadebc580843114e208bd..4368f3218dba641bcf880dbab5ef0eee2e95956c 100644
--- a/content/browser/renderer_host/websocket_dispatcher_host_unittest.cc
+++ b/content/browser/renderer_host/websocket_dispatcher_host_unittest.cc
@@ -11,10 +11,12 @@
 #include "base/bind_helpers.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
+#include "base/message_loop/message_loop.h"
 #include "content/browser/renderer_host/websocket_host.h"
 #include "content/common/websocket.h"
 #include "content/common/websocket_messages.h"
 #include "ipc/ipc_message.h"
+#include "net/websockets/websocket_errors.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
 #include "url/origin.h"
@@ -33,19 +35,30 @@ class MockWebSocketHost : public WebSocketHost {
   MockWebSocketHost(int routing_id,
                     WebSocketDispatcherHost* dispatcher,
                     net::URLRequestContext* url_request_context,
+                    base::TimeDelta delay,
                     WebSocketDispatcherHostTest* owner);
 
   ~MockWebSocketHost() override {}
 
   bool OnMessageReceived(const IPC::Message& message) override {
     received_messages_.push_back(message);
-    return true;
+    switch (message.type()) {
+      case WebSocketMsg_DropChannel::ID:
+        // Needed for PerRendererThrottlingFailedHandshakes, because without
+        // calling WebSocketHost::OnMessageReceived() (and thus
+        // WebSocketHost::OnDropChannel()), the connection stays pending and
+        // we cannot test per-renderer throttling with failed connections.
+        return WebSocketHost::OnMessageReceived(message);
+      default:

[Adam Rice, 2015/03/06 11:01:06]

Blank line above default: please.

[hiroshige, 2015/03/11 06:21:25]

Done.

+        return true;
+    }
   }
 
   void GoAway() override;
 
   std::vector<IPC::Message> received_messages_;
   base::WeakPtr<WebSocketDispatcherHostTest> owner_;
+  base::TimeDelta delay_;
 };
 
 class WebSocketDispatcherHostTest : public ::testing::Test {
@@ -82,6 +95,8 @@ class WebSocketDispatcherHostTest : public ::testing::Test {
   std::vector<MockWebSocketHost*> mock_hosts_;
   std::vector<int> gone_hosts_;
 
+  base::MessageLoop message_loop_;
+
   base::WeakPtrFactory<WebSocketDispatcherHostTest> weak_ptr_factory_;
 
  private:
@@ -89,9 +104,9 @@ class WebSocketDispatcherHostTest : public ::testing::Test {
     return NULL;
   }
 
-  WebSocketHost* CreateWebSocketHost(int routing_id) {
-    MockWebSocketHost* host =
-        new MockWebSocketHost(routing_id, dispatcher_host_.get(), NULL, this);
+  WebSocketHost* CreateWebSocketHost(int routing_id, base::TimeDelta delay) {
+    MockWebSocketHost* host = new MockWebSocketHost(
+        routing_id, dispatcher_host_.get(), NULL, delay, this);
     mock_hosts_.push_back(host);
     return host;
   }
@@ -101,9 +116,11 @@ MockWebSocketHost::MockWebSocketHost(
     int routing_id,
     WebSocketDispatcherHost* dispatcher,
     net::URLRequestContext* url_request_context,
+    base::TimeDelta delay,
     WebSocketDispatcherHostTest* owner)
-    : WebSocketHost(routing_id, dispatcher, url_request_context),
-      owner_(owner->GetWeakPtr()) {}
+    : WebSocketHost(routing_id, dispatcher, url_request_context, delay),
+      owner_(owner->GetWeakPtr()),
+      delay_(delay) {}
 
 void MockWebSocketHost::GoAway() {
   if (owner_)
@@ -215,5 +232,86 @@ TEST_F(WebSocketDispatcherHostTest, Destruct) {
   EXPECT_EQ(456, gone_hosts_[1]);
 }
 
+TEST_F(WebSocketDispatcherHostTest, PerRendererThrottling) {

[Adam Rice, 2015/03/06 11:01:06]

Please split this into three separate unit tests for the 0, non-0 and [1000,
5000] case.

[hiroshige, 2015/03/11 06:21:25]

Done.

+  GURL socket_url("ws://example.com/test");
+  std::vector<std::string> requested_protocols;
+  requested_protocols.push_back("hello");

[Adam Rice, 2015/03/06 11:01:06]

This makes no difference to the outcome of the tests so please remove it.

[hiroshige, 2015/03/11 06:21:25]

Done.

+  url::Origin origin("http://example.com/test");

[Adam Rice, 2015/03/06 11:01:06]

An origin never contains a path. It should be just "http://example.com".

[hiroshige, 2015/03/11 06:21:24]

Done. Also fixed other tests in this file.

+  int render_frame_id = -3;
+
+  for (int i = 0; i < 256; ++i) {

[Adam Rice, 2015/03/06 11:01:06]

Since you are going to need this in several tests, it might be useful to factor
it out into a separate method on the text fixture, for example:

void AddMultipleChannels(GURL url, int number_of_channels_to_add);

[hiroshige, 2015/03/11 06:21:25]

Done.
Omitted |url| parameter because there is no need to set different URLs for the
added tests.

+    int routing_id = 123 + i;
+    WebSocketHostMsg_AddChannelRequest message(
+        routing_id, socket_url, requested_protocols, origin, render_frame_id);
+    ASSERT_TRUE(dispatcher_host_->OnMessageReceived(message));
+  }
+
+  // The first 255 connections are added to mock_hosts_, staying pending.
+  // The 256th connection is rejected.
+  ASSERT_EQ(255U, mock_hosts_.size());
+
+// For the 1st--4th pending WebSocketHosts, delay is 0.
+  for (int i = 0; i < 4; ++i) {
+    MockWebSocketHost* host = mock_hosts_[i];
+    EXPECT_EQ(host->delay_, base::TimeDelta());
+  }
+
+// For the 8th--16th pending WebSocketHosts, delay is > 0.
+  for (int i = 7; i < 16; ++i) {
+    MockWebSocketHost* host = mock_hosts_[i];
+    EXPECT_GT(host->delay_, base::TimeDelta());
+  }
+
+// For the 17th-- pending WebSocketHosts, delay is >=1000 and <= 5000.
+  for (int i = 16; i < 255; ++i) {

[Adam Rice, 2015/03/06 11:01:06]

You don't need to test every number. Tests should be written as if the CPU and
compiler don't have bugs.

Of course, in reality the CPU and compiler do have bugs, but they are rarely
caught by unit tests, and they're not our problem here.

[hiroshige, 2015/03/11 06:21:25]

Done.

+    MockWebSocketHost* host = mock_hosts_[i];
+    EXPECT_GE(host->delay_, base::TimeDelta::FromMilliseconds(1000));
+    EXPECT_LE(host->delay_, base::TimeDelta::FromMilliseconds(5000));
+  }
+}
+
+TEST_F(WebSocketDispatcherHostTest, PerRendererThrottlingFailedHandshakes) {
+  GURL socket_url("ws://example.com/test");
+  std::vector<std::string> requested_protocols;
+  requested_protocols.push_back("hello");
+  url::Origin origin("http://example.com/test");
+  int render_frame_id = -4;
+
+  // 256 AddChannelRequest are sent but are cancelled by DropChannel during

[Adam Rice, 2015/03/06 11:01:06]

s/during/while/

[hiroshige, 2015/03/11 06:21:25]

Done.

+  // they are pending. They are counted as failure.
+  // Delay is increased by the increasing number of failures.
+  // However, the number of pending connections stays <= 1, so no connections
+  // are rejected.
+  for (int i = 0; i < 256; ++i) {
+    int routing_id = 123 + i;
+    WebSocketHostMsg_AddChannelRequest message(
+        routing_id, socket_url, requested_protocols, origin, render_frame_id);
+    ASSERT_TRUE(dispatcher_host_->OnMessageReceived(message));
+
+    ASSERT_EQ(static_cast<size_t>(i + 1), mock_hosts_.size());
+
+    MockWebSocketHost* host = mock_hosts_[i];
+    if (i < 4) {

[Adam Rice, 2015/03/06 11:01:06]

I like to avoid having if statements in tests because it means the reader has to
follow the logic to work out what is being tested. I think it is better to have
many short tests that are explicit about what they are testing than a few
cleverer tests.

[hiroshige, 2015/03/11 06:21:25]

Split into separate tests.

+      // For the 1st--4th WebSocketHosts, delay is 0.
+      EXPECT_EQ(host->delay_, base::TimeDelta());
+    }
+    else if (7 <= i && i < 16) {
+      // For the 8th--16th WebSocketHosts, delay is > 0.
+      EXPECT_GT(host->delay_, base::TimeDelta());
+    }
+    else if (16 <= i) {
+      // For the 17th-- WebSocketHosts, delay is >=1000 and <= 5000.
+      EXPECT_GE(host->delay_, base::TimeDelta::FromMilliseconds(1000));
+      EXPECT_LE(host->delay_, base::TimeDelta::FromMilliseconds(5000));
+    }
+
+    WebSocketMsg_DropChannel message2(
+        routing_id, false, net::kWebSocketErrorAbnormalClosure, "");
+    ASSERT_TRUE(dispatcher_host_->OnMessageReceived(message2));
+    // The connection is cancelled before WebSocketChannel is created, and
+    // thus counted as failure.
+  }
+}
+
 }  // namespace
 }  // namespace content