Add detection of Shoutcast responses and accept them as valid HTTP/0.9

net/http/http_stream_parser_unittest.cc

Index: net/http/http_stream_parser_unittest.cc
diff --git a/net/http/http_stream_parser_unittest.cc b/net/http/http_stream_parser_unittest.cc
index 9fa3bc0dc8276003110414337fac31b87869b717..b653068c03ef4aeacb8869d10e496930f86db150 100644
--- a/net/http/http_stream_parser_unittest.cc
+++ b/net/http/http_stream_parser_unittest.cc
@@ -1213,6 +1213,12 @@ class SimpleGetRunner {
     reads_.push_back(MockRead(SYNCHRONOUS, sequence_number_++, data.data()));
   }
 
+  // Simple overload - the above method requires using std::strings that outlive
+  // the function call.  This version worse with inlined C-style strings.

[eroman, 2017/01/24 00:37:29]

worse --> works

[mmenke, 2017/01/24 17:27:02]

Done.

+  void AddRead(const char* data) {

[eroman, 2017/01/24 00:37:29]

Or could have a single version which takes a base::StringPiece.

[mmenke, 2017/01/24 17:27:02]

Having a StringPiece that's created for a function call and destroyed
immediately after, but that is required to guarantee that the memory isn't freed
until the end of the test seems ugly.  The AddRead call that takes a std::string
has the same issue, but the StringPiece approach seems to even further obscure
things, so going to keep the overload.

+    reads_.push_back(MockRead(SYNCHRONOUS, sequence_number_++, data));
+  }
+
   void SetupParserAndSendRequest() {
     reads_.push_back(MockRead(SYNCHRONOUS, 0, sequence_number_++));  // EOF
 
@@ -1281,50 +1287,55 @@ TEST(HttpStreamParser, Http09PortTests) {
     const char* url;
     bool http_09_on_non_default_ports_enabled;
 
-    // Expected result when trying to read headers.
-    Error expected_header_error;
+    // Expected result when trying to read headers and response is an HTTP/0.9
+    // non-shoutcast response.

[eroman, 2017/01/24 00:37:29]

nit: consistency in writing shoutcast (unittest) vs Shoutcast (non-test code)

[mmenke, 2017/01/24 17:27:02]

Done.

+    Error expected_09_header_error;
+
+    // Expected result when trying to read headers for a shoutcast response.
+    Error expected_shoutcast_header_error;
   };
 
   const TestCase kTestCases[] = {
       // Default ports should work for HTTP/0.9, regardless of whether the port
       // is explicitly specified or not.
-      {"http://foo.com/", false, OK},
-      {"http://foo.com:80/", false, OK},
-      {"https://foo.com/", false, OK},
-      {"https://foo.com:443/", false, OK},
+      {"http://foo.com/", false, OK, OK},
+      {"http://foo.com:80/", false, OK, OK},
+      {"https://foo.com/", false, OK, OK},
+      {"https://foo.com:443/", false, OK, OK},
 
       // Non-standard ports should not support HTTP/0.9, by default.
-      {"http://foo.com:8080/", false, ERR_INVALID_HTTP_RESPONSE},
-      {"https://foo.com:8080/", false, ERR_INVALID_HTTP_RESPONSE},
-      {"http://foo.com:443/", false, ERR_INVALID_HTTP_RESPONSE},
-      {"https://foo.com:80/", false, ERR_INVALID_HTTP_RESPONSE},
+      {"http://foo.com:8080/", false, ERR_INVALID_HTTP_RESPONSE, OK},
+      {"https://foo.com:8080/", false, ERR_INVALID_HTTP_RESPONSE,
+       ERR_INVALID_HTTP_RESPONSE},
+      {"http://foo.com:443/", false, ERR_INVALID_HTTP_RESPONSE, OK},
+      {"https://foo.com:80/", false, ERR_INVALID_HTTP_RESPONSE,
+       ERR_INVALID_HTTP_RESPONSE},
 
       // Allowing non-default ports should not break the default ones.
-      {"http://foo.com/", true, OK},
-      {"http://foo.com:80/", true, OK},
-      {"https://foo.com/", true, OK},
-      {"https://foo.com:443/", true, OK},
+      {"http://foo.com/", true, OK, OK},
+      {"http://foo.com:80/", true, OK, OK},
+      {"https://foo.com/", true, OK, OK},
+      {"https://foo.com:443/", true, OK, OK},
 
       // Check that non-default ports works.
-      {"http://foo.com:8080/", true, OK},
-      {"https://foo.com:8080/", true, OK},
-      {"http://foo.com:443/", true, OK},
-      {"https://foo.com:80/", true, OK},
+      {"http://foo.com:8080/", true, OK, OK},
+      {"https://foo.com:8080/", true, OK, OK},
+      {"http://foo.com:443/", true, OK, OK},
+      {"https://foo.com:80/", true, OK, OK},
   };
 
-  std::string response = "hello\r\nworld\r\n";
-  int response_size = response.size();
+  const std::string kResponse = "hello\r\nworld\r\n";
 
   for (const auto& test_case : kTestCases) {
     SimpleGetRunner get_runner;
     get_runner.set_url(GURL(test_case.url));
     get_runner.set_http_09_on_non_default_ports_enabled(
         test_case.http_09_on_non_default_ports_enabled);
-    get_runner.AddRead(response);
+    get_runner.AddRead(kResponse);
     get_runner.SetupParserAndSendRequest();
 
-    get_runner.ReadHeadersExpectingError(test_case.expected_header_error);
-    if (test_case.expected_header_error != OK)
+    get_runner.ReadHeadersExpectingError(test_case.expected_09_header_error);
+    if (test_case.expected_09_header_error != OK)
       continue;
 
     ASSERT_TRUE(get_runner.response_info()->headers);
@@ -1332,12 +1343,59 @@ TEST(HttpStreamParser, Http09PortTests) {
               get_runner.response_info()->headers->GetStatusLine());
 
     EXPECT_EQ(0, get_runner.parser()->received_bytes());
-    int read_lengths[] = {response_size, 0};
-    get_runner.ReadBody(response_size, read_lengths);
-    EXPECT_EQ(response_size, get_runner.parser()->received_bytes());
+    int read_lengths[] = {kResponse.size(), 0};
+    get_runner.ReadBody(kResponse.size(), read_lengths);
+    EXPECT_EQ(kResponse.size(),
+              static_cast<size_t>(get_runner.parser()->received_bytes()));
     EXPECT_EQ(HttpResponseInfo::CONNECTION_INFO_HTTP0_9,
               get_runner.response_info()->connection_info);
   }
+
+  const std::string kShoutcastResponse = "ICY 200 blah\r\n\r\n";

[eroman, 2017/01/24 00:37:29]

Can you also include a test where icy is the prefix for a longer word?

Say, "iCyCastle"

Speaking of which, is the decision to test for a prefix (rather than whitespace
separated word) for implementation simplicity, or for compatibility?

[mmenke, 2017/01/24 17:27:02]

Test added (Though I went with iCyCreamSundae, a very important distinction).

I did it for compatibility.  FireFox treats it like "HTTP".  Considered checking
for a "/" or whitespace, but then I looked at our HTTP treatment, and all we
require is that a response start with "HTTP", regardless of what follows it. 
Admittedly, FireFox may be pickier there than we are in the HTTP case, not sure.

+  for (const auto& test_case : kTestCases) {
+    SimpleGetRunner get_runner;
+    get_runner.set_url(GURL(test_case.url));
+    get_runner.set_http_09_on_non_default_ports_enabled(
+        test_case.http_09_on_non_default_ports_enabled);
+    get_runner.AddRead(kShoutcastResponse);
+    get_runner.SetupParserAndSendRequest();
+
+    get_runner.ReadHeadersExpectingError(
+        test_case.expected_shoutcast_header_error);
+    if (test_case.expected_shoutcast_header_error != OK)
+      continue;
+
+    ASSERT_TRUE(get_runner.response_info()->headers);
+    EXPECT_EQ("HTTP/0.9 200 OK",
+              get_runner.response_info()->headers->GetStatusLine());
+
+    EXPECT_EQ(0, get_runner.parser()->received_bytes());
+    int read_lengths[] = {kShoutcastResponse.size(), 0};
+    get_runner.ReadBody(kShoutcastResponse.size(), read_lengths);
+    EXPECT_EQ(kShoutcastResponse.size(),
+              static_cast<size_t>(get_runner.parser()->received_bytes()));
+    EXPECT_EQ(HttpResponseInfo::CONNECTION_INFO_HTTP0_9,
+              get_runner.response_info()->connection_info);
+  }
+}
+
+// Make sure that Shoutcast is recognized when receiving one byte at a time.
+TEST(HttpStreamParser, ShoutcastSingleByteReads) {
+  SimpleGetRunner get_runner;
+  get_runner.set_url(GURL("http://foo.com:8080/"));
+  get_runner.set_http_09_on_non_default_ports_enabled(false);
+  get_runner.AddRead("i");
+  get_runner.AddRead("c");
+  get_runner.AddRead("Y");
+  // Needed because HttpStreamParser::Read returns ERR_CONNECTION_CLOSED on
+  // small response headers, which HttpNetworkTransaction replaces with net::OK.
+  // TODO(mmenke): Can we just change that behavior?
+  get_runner.AddRead(" Extra stuff");
+  get_runner.SetupParserAndSendRequest();
+
+  get_runner.ReadHeadersExpectingError(OK);
+  EXPECT_EQ("HTTP/0.9 200 OK",
+            get_runner.response_info()->headers->GetStatusLine());
 }
 
 // Make sure that HTTP/0.9 isn't allowed in the truncated header case on a weird