Handle non-HTTP/1.1 requests more gracefully in net::HttpServer.

net/server/http_server_unittest.cc

Index: net/server/http_server_unittest.cc
diff --git a/net/server/http_server_unittest.cc b/net/server/http_server_unittest.cc
index 03544f12b3d5f82a90c83ab5d8a0ba1cfbf2d4d7..c5ab3d7885364404af1e92cb60a04707821bebeb 100644
--- a/net/server/http_server_unittest.cc
+++ b/net/server/http_server_unittest.cc
@@ -50,6 +50,7 @@
 #include "testing/gtest/include/gtest/gtest.h"
 
 using net::test::IsOk;
+using testing::_;
 
 namespace net {
 
@@ -183,6 +184,18 @@ class TestHttpClient {
   int connect_result_;
 };
 
+class MockHttpServerDelegate : public HttpServer::Delegate {
+ public:
+  MOCK_METHOD1(OnConnect, void(int connection_id));
+  MOCK_METHOD2(OnHttpRequest,
+               void(int connection_id, const HttpServerRequestInfo& info));
+  MOCK_METHOD2(OnWebSocketRequest,
+               void(int connection_id, const HttpServerRequestInfo& info));
+  MOCK_METHOD2(OnWebSocketMessage,
+               void(int connection_id, const std::string& data));
+  MOCK_METHOD1(OnClose, void(int connection_id));
+};

[mmenke, 2016/09/12 20:35:04]

We try not to use Gmock in net - it makes tests significantly harder to follow,
because of all the baked-in magic, and a lot of committers aren't familiar with
it.

Can we just do something like HttpServerTest, and make sure the request fails
with ERR_CONNECTION_CLOSED, instead?  Think that's a better test of the API
exposed to consumers of the server.

[slan, 2016/09/14 15:50:57]

OK, I removed the use of GMock and added checks for two things:

1. Proper delegate use - I added some very simple state to assert that the
delegate methods were called in the right order. We expect to receive events
when the connection is opened, and when it is closed upon seeing the bad
protocol.

2. That the server actually hangs up - I added checks on the socket used by the
client to test that the server actually disconnects when it parses the bad
header.

(I tried to modify the test to use URLFetcher, but I couldn't find where to
override the HTTP version that's used for the request. So I went lower-level - I
think sending bytes over the socket is a little clearer anyway.)

PTAL at the additions and LMK what you think, Matt.

+
 }  // namespace
 
 class HttpServerTest : public testing::Test,
@@ -198,13 +211,16 @@ class HttpServerTest : public testing::Test,
     ASSERT_THAT(server_->GetLocalAddress(&server_address_), IsOk());
   }
 
-  void OnConnect(int connection_id) override {}
+  void OnConnect(int connection_id) override {
+    mock_delegate_.OnConnect(connection_id);
+  }
 
   void OnHttpRequest(int connection_id,
                      const HttpServerRequestInfo& info) override {
     requests_.push_back(std::make_pair(info, connection_id));
     if (requests_.size() == quit_after_request_count_)
       run_loop_quit_func_.Run();
+    mock_delegate_.OnHttpRequest(connection_id, info);
   }
 
   void OnWebSocketRequest(int connection_id,
@@ -216,7 +232,9 @@ class HttpServerTest : public testing::Test,
     NOTREACHED();
   }
 
-  void OnClose(int connection_id) override {}
+  void OnClose(int connection_id) override {
+    mock_delegate_.OnClose(connection_id);
+  }
 
   bool RunUntilRequestsReceived(size_t count) {
     quit_after_request_count_ = count;
@@ -248,6 +266,7 @@ class HttpServerTest : public testing::Test,
   IPEndPoint server_address_;
   base::Closure run_loop_quit_func_;
   std::vector<std::pair<HttpServerRequestInfo, int> > requests_;
+  testing::NiceMock<MockHttpServerDelegate> mock_delegate_;
 
  private:
   size_t quit_after_request_count_;
@@ -472,6 +491,18 @@ TEST_F(HttpServerTest, SendRaw) {
   ASSERT_EQ(expected_response, response);
 }
 
+TEST_F(HttpServerTest, SendWrongProtocolRequest) {
+  EXPECT_CALL(mock_delegate_, OnConnect(_)).Times(1);
+  EXPECT_CALL(mock_delegate_, OnClose(_)).Times(1);
+
+  // Tests that non-HTTP/1.1 requests are simply ignored.
+  TestHttpClient client;
+  ASSERT_THAT(client.ConnectAndWait(server_address_), IsOk());
+
+  client.Send("GET /test HTTP/1.0\r\n\r\n");
+  ASSERT_FALSE(RunUntilRequestsReceived(1));

[mmenke, 2016/09/12 20:35:04]

Maybe also a test with "GET /test foo\r\n\r\n" and "GET / test\r\n\r\n".  Fine
to just have an array of test cases and loop through them, since they should all
fail the same way.

[slan, 2016/09/14 15:50:57]

Done.

+}
+
 class MockStreamSocket : public StreamSocket {
  public:
   MockStreamSocket()