Fix WebSocketServer extension parser.

net/server/web_socket_encoder.cc

Index: net/server/web_socket_encoder.cc
diff --git a/net/server/web_socket_encoder.cc b/net/server/web_socket_encoder.cc
index 1a5431affd8415a23e6ee6f3a553064d1447a803..5ce07e6c54bcfe6c5cdb10003bc49e41ac9151e3 100644
--- a/net/server/web_socket_encoder.cc
+++ b/net/server/web_socket_encoder.cc
@@ -8,6 +8,8 @@
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/stringprintf.h"
 #include "net/base/io_buffer.h"
+#include "net/websockets/websocket_deflate_parameters.h"
+#include "net/websockets/websocket_extension.h"
 #include "net/websockets/websocket_extension_parser.h"
 
 namespace net {
@@ -180,144 +182,112 @@ void EncodeFrameHybi17(const std::string& message,
 }  // anonymous namespace
 
 // static
-WebSocketEncoder* WebSocketEncoder::CreateServer(
+scoped_ptr<WebSocketEncoder> WebSocketEncoder::CreateServer() {
+  return make_scoped_ptr(new WebSocketEncoder(AsServer()));
+}
+
+// static
+scoped_ptr<WebSocketEncoder> WebSocketEncoder::CreateServer(
     const std::string& request_extensions,
-    std::string* response_extensions) {
-  bool deflate;
-  bool has_client_window_bits;
-  int client_window_bits;
-  int server_window_bits;
-  bool client_no_context_takeover;
-  bool server_no_context_takeover;
-  ParseExtensions(request_extensions, &deflate, &has_client_window_bits,
-                  &client_window_bits, &server_window_bits,
-                  &client_no_context_takeover, &server_no_context_takeover);
-
-  if (deflate) {
-    *response_extensions = base::StringPrintf(
-        "permessage-deflate; server_max_window_bits=%d%s", server_window_bits,
-        server_no_context_takeover ? "; server_no_context_takeover" : "");
-    if (has_client_window_bits) {
-      base::StringAppendF(response_extensions, "; client_max_window_bits=%d",
-                          client_window_bits);
-    } else {
-      DCHECK_EQ(client_window_bits, 15);
+    WebSocketDeflateParameters* deflate_parameters,
+    std::string* failure_message) {
+  WebSocketExtensionParser parser;
+  if (!parser.Parse(request_extensions)) {
+    *failure_message = "Failed to parse extensions header.";
+    return nullptr;
+  }
+
+  WebSocketDeflateParameters offered, response;
+  for (size_t i = 0; i < parser.extensions().size(); ++i) {
+    const auto& extension = parser.extensions()[i];
+    // We only support "permessage-deflate" extension.
+    WebSocketDeflateParameters params;
+    if (!params.Initialize(extension, failure_message) ||
+        !params.IsValidAsRequest(failure_message)) {
+      return nullptr;
     }
-    return new WebSocketEncoder(true /* is_server */, server_window_bits,
-                                client_window_bits, server_no_context_takeover);
-  } else {
-    *response_extensions = std::string();
-    return new WebSocketEncoder(true /* is_server */);
+    if (i == 0) {
+      // We choose the first parameters blindly.
+      offered = params;
+    }
+  }
+  response = offered;
+  if (offered.is_client_max_window_bits_specified() &&
+      !offered.has_client_max_window_bits_value()) {
+    // We need to choose one value for the response.
+    response.SetClientMaxWindowBits(8);
+  }
+  DCHECK(response.IsValidAsResponse());
+  DCHECK(offered.IsCompatibleWith(response));
+
+  auto encoder = make_scoped_ptr(new WebSocketEncoder(AsServer(), response));
+  if (encoder->has_error_) {
+    // Failed to initialize deflater or inflater.
+    *failure_message = "Internal error.";
+    return nullptr;
   }
+  *deflate_parameters = response;
+  return encoder.Pass();
 }
 
 // static
 WebSocketEncoder* WebSocketEncoder::CreateClient(
     const std::string& response_extensions) {
-  bool deflate;
-  bool has_client_window_bits;
-  int client_window_bits;
-  int server_window_bits;
-  bool client_no_context_takeover;
-  bool server_no_context_takeover;
-  ParseExtensions(response_extensions, &deflate, &has_client_window_bits,
-                  &client_window_bits, &server_window_bits,
-                  &client_no_context_takeover, &server_no_context_takeover);
-
-  if (deflate) {
-    return new WebSocketEncoder(false /* is_server */, client_window_bits,
-                                server_window_bits, client_no_context_takeover);
-  } else {
-    return new WebSocketEncoder(false /* is_server */);
-  }
-}
-
-// static
-void WebSocketEncoder::ParseExtensions(const std::string& header_value,
-                                       bool* deflate,
-                                       bool* has_client_window_bits,
-                                       int* client_window_bits,
-                                       int* server_window_bits,
-                                       bool* client_no_context_takeover,
-                                       bool* server_no_context_takeover) {
-  *deflate = false;
-  *has_client_window_bits = false;
-  *client_window_bits = 15;
-  *server_window_bits = 15;
-  *client_no_context_takeover = false;
-  *server_no_context_takeover = false;
-
-  if (header_value.empty())
-    return;
+  // TODO(yhirano): Add a way to return an error.
 
   WebSocketExtensionParser parser;
-  if (!parser.Parse(header_value))
-    return;
-  const std::vector<WebSocketExtension>& extensions = parser.extensions();
-  // TODO(tyoshino): Fail if this method is used for parsing a response and
-  // there are multiple permessage-deflate extensions or there are any unknown
-  // extensions.
-  for (const auto& extension : extensions) {
-    if (extension.name() != "permessage-deflate") {
-      continue;
-    }
-
-    const std::vector<WebSocketExtension::Parameter>& parameters =
-        extension.parameters();
-    for (const auto& param : parameters) {
-      const std::string& name = param.name();
-      // TODO(tyoshino): Fail the connection when an invalid value is given.
-      if (name == "client_max_window_bits") {
-        *has_client_window_bits = true;
-        if (param.HasValue()) {
-          int bits = 0;
-          if (base::StringToInt(param.value(), &bits) && bits >= 8 &&
-              bits <= 15) {
-            *client_window_bits = bits;
-          }
-        }
-      }
-      if (name == "server_max_window_bits" && param.HasValue()) {
-        int bits = 0;
-        if (base::StringToInt(param.value(), &bits) && bits >= 8 && bits <= 15)
-          *server_window_bits = bits;
-      }
-      if (name == "client_no_context_takeover")
-        *client_no_context_takeover = true;
-      if (name == "server_no_context_takeover")
-        *server_no_context_takeover = true;
-    }
-    *deflate = true;
+  if (!parser.Parse(response_extensions)) {
+    // Parse error.
+    return new WebSocketEncoder(AsClient());
+  }
+  if (parser.extensions().size() != 1) {
+    // Only permessage-deflate extension is supported.
+    return new WebSocketEncoder(AsClient());
+  }
+  const auto& extension = parser.extensions()[0];
+  WebSocketDeflateParameters params;
+  std::string failure_message;
+  if (!params.Initialize(extension, &failure_message) ||
+      !params.IsValidAsResponse(&failure_message)) {
+    // Failed to parse params.
+    return new WebSocketEncoder(AsClient());
+  }
 
-    break;
+  scoped_ptr<WebSocketEncoder> encoder(
+      new WebSocketEncoder(AsClient(), params));
+  if (encoder->has_error_) {
+    // Failed to initialize deflater or inflater.
+    return new WebSocketEncoder(AsClient());
   }
+  return encoder.release();
 }
 
-WebSocketEncoder::WebSocketEncoder(bool is_server) : is_server_(is_server) {
-}
+WebSocketEncoder::WebSocketEncoder(const AsServer&)
+    : is_server_(true), has_error_(false) {}
 
-WebSocketEncoder::WebSocketEncoder(bool is_server,
-                                   int deflate_bits,
-                                   int inflate_bits,
-                                   bool no_context_takeover)
-    : is_server_(is_server) {
-  deflater_.reset(new WebSocketDeflater(
-      no_context_takeover ? WebSocketDeflater::DO_NOT_TAKE_OVER_CONTEXT
-                          : WebSocketDeflater::TAKE_OVER_CONTEXT));
-  inflater_.reset(
-      new WebSocketInflater(kInflaterChunkSize, kInflaterChunkSize));
-
-  if (!deflater_->Initialize(deflate_bits) ||
-      !inflater_->Initialize(inflate_bits)) {
-    // Disable deflate support.
-    deflater_.reset();
-    inflater_.reset();
-  }
+WebSocketEncoder::WebSocketEncoder(const AsClient&)
+    : is_server_(false), has_error_(false) {}
+
+WebSocketEncoder::WebSocketEncoder(const AsServer& server,
+                                   const WebSocketDeflateParameters& params)
+    : is_server_(true),
+      deflater_(new WebSocketDeflater(params.server_context_take_over_mode())),
+      inflater_(new WebSocketInflater(kInflaterChunkSize, kInflaterChunkSize)) {
+  has_error_ = !deflater_->Initialize(params.PermissiveServerMaxWindowBits()) ||

[dgozman, 2015/09/11 17:49:36]

Why don't you just pass |is_server|, |inflater| and |deflater| in constructor
and remove |has_error| entirely. This way, factory method will be responsible
for error handling, and you don't need AsServer and AsClient.

[yhirano, 2015/09/15 06:25:57]

Done. I generally don't like |true /* is_server */| pattern and introduced Type
enum.

+               !inflater_->Initialize(params.PermissiveClientMaxWindowBits());
 }
 
-WebSocketEncoder::~WebSocketEncoder() {
+WebSocketEncoder::WebSocketEncoder(const AsClient&,
+                                   const WebSocketDeflateParameters& params)
+    : is_server_(false),
+      deflater_(new WebSocketDeflater(params.client_context_take_over_mode())),
+      inflater_(new WebSocketInflater(kInflaterChunkSize, kInflaterChunkSize)) {
+  has_error_ = !deflater_->Initialize(params.PermissiveClientMaxWindowBits()) ||
+               !inflater_->Initialize(params.PermissiveServerMaxWindowBits());
 }
 
+WebSocketEncoder::~WebSocketEncoder() {}
+
 WebSocket::ParseResult WebSocketEncoder::DecodeFrame(
     const base::StringPiece& frame,
     int* bytes_consumed,