Add WebSocket cookie tests.

net/websockets/websocket_stream_cookie_test.cc

Index: net/websockets/websocket_stream_cookie_test.cc
diff --git a/net/websockets/websocket_stream_cookie_test.cc b/net/websockets/websocket_stream_cookie_test.cc
new file mode 100644
index 0000000000000000000000000000000000000000..d83866bbc9f8a280c14c1b8732a4f7297a85ff0a
--- /dev/null
+++ b/net/websockets/websocket_stream_cookie_test.cc
@@ -0,0 +1,496 @@
+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "net/websockets/websocket_stream.h"
+
+#include <string>
+#include "base/run_loop.h"
+#include "base/strings/string_util.h"
+#include "base/strings/stringprintf.h"
+#include "net/websockets/websocket_stream_test_util.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "url/gurl.h"
+
+namespace net {
+namespace {
+
+using ::testing::TestWithParam;
+using ::testing::ValuesIn;
+
+const char* const kNoCookieHeader = "";
+
+class TestBase : public WebSocketStreamCreateTestBase {
+ public:
+  void CreateAndConnect(const GURL& url,
+                        const std::string& origin,
+                        const std::string& cookie_header,
+                        const std::string& response_body) {
+    CHECK(cookie_header.empty() || EndsWith(cookie_header, "\r\n", true));

[Ryan Sleevi, 2015/02/02 20:14:41]

Comment explaining why?

[yhirano, 2015/02/09 10:50:14]

Done.

+
+    url_request_context_host_.SetExpectations(
+        WebSocketStandardRequestWithCookies(url.path(), url.host(), origin,
+                                            cookie_header, std::string()),
+        response_body);
+    CreateAndConnectStream(url.spec(), NoSubProtocols(), origin, nullptr);
+  }
+
+  static std::string AddCRLFIfNotEmpty(const std::string& s) {

[Ryan Sleevi, 2015/02/02 20:14:41]

No need to make this static.

[yhirano, 2015/02/09 10:50:14]

Done.

+    return s.empty() ? s : s + "\r\n";
+  }
+};
+
+class ClientUseCookieParameter {

[yhirano, 2015/02/02 05:39:57]

I made this (and the other) class POD.

[Ryan Sleevi, 2015/02/02 20:14:41]

Make this a struct then, since you're following struct rules -
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Structs_vs._C...

[yhirano, 2015/02/09 10:50:14]

Done.

+ public:
+  // The URL for the WebSocket connection.

[yhirano, 2015/02/02 05:29:56]

Comment: done

+  const char* const url;

[yhirano, 2015/02/02 05:29:56]

I don't see any harm from constness.

+  // The URL for the previously set cookies.
+  const char* const cookie_url;
+  // The previously set cookies contents.
+  const char* const cookie_line;
+  // The Cookie: HTTP header expected to appear in the WS request. An empty
+  // string means there is no Cookie: header.
+  const char* const cookie_header;
+};
+
+class WebSocketStreamClientUseCookieTest
+    : public TestBase,
+      public TestWithParam<ClientUseCookieParameter> {
+ public:
+  ~WebSocketStreamClientUseCookieTest() override {
+    // Permit any endpoint locks to be released.
+    stream_request_.reset();
+    stream_.reset();
+    RunUntilIdle();
+  }
+
+  void SetCookie(CookieStore* store,
+                 const GURL& url,
+                 const std::string& cookie_line) {
+    class RunLoop {

[yhirano, 2015/02/02 05:29:56]

I will fix this helper if necessary after the GetCookies discussion settles.

[yhirano, 2015/02/09 10:50:14]

Done.

+     public:
+      void Run() { run_loop_.Run(); }
+      void Quit(bool unused) { run_loop_.Quit(); }
+
+     private:
+      base::RunLoop run_loop_;
+    };
+
+    RunLoop run_loop;
+    store->SetCookieWithOptionsAsync(
+        url, cookie_line, CookieOptions(),
+        base::Bind(&RunLoop::Quit, base::Unretained(&run_loop)));
+    run_loop.Run();
+  }
+};
+
+class ServerSetCookieParameter {

[Ryan Sleevi, 2015/02/02 20:14:41]

ditto struct

[yhirano, 2015/02/09 10:50:14]

Done.

+ public:
+  // The URL for the WebSocket connection.
+  const char* const url;
+  // The URL used to query cookies after the response received.
+  const char* const cookie_url;
+  // The cookies expected to appear for |cookie_url| inquiry.
+  const char* const cookie_line;
+  // The Set-Cookie: HTTP header attached to the response.
+  const char* const cookie_header;
+};
+
+class WebSocketStreamServerSetCookieTest
+    : public TestBase,
+      public TestWithParam<ServerSetCookieParameter> {
+ public:
+  ~WebSocketStreamServerSetCookieTest() override {
+    // Permit any endpoint locks to be released.
+    stream_request_.reset();
+    stream_.reset();
+    RunUntilIdle();
+  }
+
+  std::string GetCookies(CookieStore* store, const GURL& url) {
+    struct GetCookiesHelper {

[yhirano, 2015/02/02 05:29:56]

Ryan's comment
(https://codereview.chromium.org/869073002/diff/140001/net/websockets/websocke...):

static void StashCookiesHelper(const base::Closure& task, const
base::WeakPtr<std::string>& weak_cookies, const std::string& incoming_cookies) {
  if (weak_cookies.get())
    *weak_cookies = incoming_cookies;
  PostTask(task);
}

std::string cookies;
base::WeakPtrFactory<std::string> weak_cookies(&cookies);
base::Bind(&StashCookiesHelper, run_loop.QuitClosure(),
weak_cookies.GetWeakPtr())


Benefits: If the message loop gets quit because of some other reason, things
don't blow up if the task is still pending. And you avoid the class helper and
base::Unretained(), which are non-idiomatic Chromium threading.

[yhirano, 2015/02/02 05:39:57]

I'm not sure what you presume: If you are worrying that the message loop gets
quit without GetCookiesHelper::Quit call, you should check it explicitly to
avoid accidental pass. That means you should add another WeakPtr<bool> parameter
to your function but it is messy. It's more reasonable to add a bool member to
GetCookiesHelper. I can remove base::Unretained by using scoped_ref_ptr if you
prefer.

[Ryan Sleevi, 2015/02/02 20:14:41]

The concern here is that the message loop is quit by some other task prior to
the StashCookiesHelper being run, and then that task being run.

In a practical sense, it's not a real issue. However, it highlights that your
coding pattern is playing very fast and very loose with memory safety, and just
because it's test code isn't a good reason for that.

Tests, like production code, should assume hostility / weird things, and be
coded safely. base::Unretained() is always a read flag for me.
I don't follow your logic. The WeakPtr *is* the bool. If the message loop is
quit
scoped_refptr<> is equally an anti-pattern used to hide over people not
understanding / documenting their threading model. That's why I tried to make it
more explicit.

[yhirano, 2015/02/03 01:26:32]

When the message loop is quit without calling StashCookiesHelper, you need to
fail the test because this function does not work as expected. In order to
detect it you need a boolean.

[Ryan Sleevi, 2015/02/03 02:16:27]

I still find this code not up to the degree that we expect of //net, and in
particular, too "novel" in the way it deals with threading.

base::Unretained() remains an anti-pattern that I will flag in reviews, because
it causes way too much reader difficulty in reasoning about the correctness of
the code. The added nesting of the run_loop_ is similarly difficult to
comprehend.

[yhirano, 2015/02/03 03:41:10]

Sorry, I still don't understand.

I understand your point that it might be possible that message loop is quit
without calling the callback.
I said that your solution was insufficient. If you care about the case, you need
to notify that to the caller.

I don't understand your point about threading: are you talking about the case
when the callback runs on another thread?

[yhirano, 2015/02/09 10:50:14]

Although I am not 100% sure what you suggested, but I did my best to presume.

+      void Run() { run_loop_.Run(); }
+      void Quit(const std::string& cookies) {
+        cookies_ = cookies;
+        run_loop_.Quit();
+      }
+
+      const std::string& cookies() const { return cookies_; }
+
+     private:
+      base::RunLoop run_loop_;
+      std::string cookies_;
+    };
+    GetCookiesHelper helper;
+
+    store->GetCookiesWithOptionsAsync(
+        url, CookieOptions(),
+        base::Bind(&GetCookiesHelper::Quit, base::Unretained(&helper)));
+    helper.Run();
+    return helper.cookies();
+  }
+};
+
+TEST_P(WebSocketStreamClientUseCookieTest, ClientUseCookie) {
+  // For wss tests.
+  ssl_data_.push_back(new SSLSocketDataProvider(ASYNC, OK));
+
+  CookieStore* store =
+      url_request_context_host_.GetURLRequestContext()->cookie_store();
+
+  const GURL url(GetParam().url);
+  const GURL cookie_url(GetParam().cookie_url);
+  const std::string origin("http://www.example.com");
+  const std::string cookie_line(GetParam().cookie_line);
+  const std::string cookie_header(AddCRLFIfNotEmpty(GetParam().cookie_header));
+
+  SetCookie(store, cookie_url, cookie_line);
+
+  CreateAndConnect(url, origin, cookie_header, WebSocketStandardResponse(""));
+
+  RunUntilIdle();
+  EXPECT_FALSE(has_failed());
+}
+
+TEST_P(WebSocketStreamServerSetCookieTest, ServerSetCookie) {
+  // For wss tests.

[yhirano, 2015/02/02 05:29:56]

Ryan's comment:
This comment doesn't make sense. It sounds like you're uncertain whether it's
necessary, but it is (judging by your test below)

[yhirano, 2015/02/02 05:39:57]

I'm not certain because it depends parameters. Fixed the comment.

[yhirano, 2015/02/02 06:11:58]

Sorry, I meant 'it depends on parameters.'

+  ssl_data_.push_back(new SSLSocketDataProvider(ASYNC, OK));
+
+  const GURL url(GetParam().url);
+  const GURL cookie_url(GetParam().cookie_url);
+  const std::string origin("http://www.example.com");
+  const std::string cookie_line(GetParam().cookie_line);
+  const std::string cookie_header(AddCRLFIfNotEmpty(GetParam().cookie_header));

[yhirano, 2015/02/02 05:29:56]

I don't have a strong opinion whether we enforce "\r\n" or not.

+
+  const std::string response = base::StringPrintf(
+      "HTTP/1.1 101 Switching Protocols\r\n"
+      "Upgrade: websocket\r\n"
+      "Connection: Upgrade\r\n"
+      "%s"
+      "Sec-WebSocket-Accept: s3pPLMBiTxaQ9kYGzzhZRbK+xOo=\r\n"
+      "\r\n",
+      cookie_header.c_str());
+
+  CookieStore* store =
+      url_request_context_host_.GetURLRequestContext()->cookie_store();
+
+  CreateAndConnect(url, origin, "", response);
+
+  RunUntilIdle();
+  EXPECT_FALSE(has_failed());
+
+  EXPECT_EQ(cookie_line, GetCookies(store, cookie_url));
+}
+
+// Test parameters definitions follow...
+
+const ClientUseCookieParameter kClientUseCookieParameters[] = {
+    // Non-secure cookies for ws
+    {"ws://www.example.com",
+     "http://www.example.com",
+     "test-cookie",
+     "Cookie: test-cookie"},
+
+    {"ws://www.example.com",
+     "https://www.example.com",
+     "test-cookie",
+     "Cookie: test-cookie"},
+
+    {"ws://www.example.com",
+     "ws://www.example.com",
+     "test-cookie",
+     "Cookie: test-cookie"},
+
+    {"ws://www.example.com",
+     "wss://www.example.com",
+     "test-cookie",
+     "Cookie: test-cookie"},
+
+    // Non-secure cookies for wss
+    {"wss://www.example.com",
+     "http://www.example.com",
+     "test-cookie",
+     "Cookie: test-cookie"},
+
+    {"wss://www.example.com",
+     "https://www.example.com",
+     "test-cookie",
+     "Cookie: test-cookie"},
+
+    {"wss://www.example.com",
+     "ws://www.example.com",
+     "test-cookie",
+     "Cookie: test-cookie"},
+
+    {"wss://www.example.com",
+     "wss://www.example.com",
+     "test-cookie",
+     "Cookie: test-cookie"},
+
+    // Secure-cookies for ws
+    {"ws://www.example.com",
+     "https://www.example.com",
+     "test-cookie; secure",
+     kNoCookieHeader},
+
+    {"ws://www.example.com",
+     "wss://www.example.com",
+     "test-cookie; secure",
+     kNoCookieHeader},
+
+    // Secure-cookies for wss
+    {"wss://www.example.com",
+     "https://www.example.com",
+     "test-cookie; secure",
+     "Cookie: test-cookie"},
+
+    {"wss://www.example.com",
+     "wss://www.example.com",
+     "test-cookie; secure",
+     "Cookie: test-cookie"},
+
+    // Non-secure cookies for ws (sharing domain)
+    {"ws://www.example.com",
+     "http://www2.example.com",
+     "test-cookie; Domain=example.com",
+     "Cookie: test-cookie"},
+
+    {"ws://www.example.com",
+     "https://www2.example.com",
+     "test-cookie; Domain=example.com",
+     "Cookie: test-cookie"},
+
+    {"ws://www.example.com",
+     "ws://www2.example.com",
+     "test-cookie; Domain=example.com",
+     "Cookie: test-cookie"},
+
+    {"ws://www.example.com",
+     "wss://www2.example.com",
+     "test-cookie; Domain=example.com",
+     "Cookie: test-cookie"},
+
+    // Non-secure cookies for wss (sharing domain)
+    {"wss://www.example.com",
+     "http://www2.example.com",
+     "test-cookie; Domain=example.com",
+     "Cookie: test-cookie"},
+
+    {"wss://www.example.com",
+     "https://www2.example.com",
+     "test-cookie; Domain=example.com",
+     "Cookie: test-cookie"},
+
+    {"wss://www.example.com",
+     "ws://www2.example.com",
+     "test-cookie; Domain=example.com",
+     "Cookie: test-cookie"},
+
+    {"wss://www.example.com",
+     "wss://www2.example.com",
+     "test-cookie; Domain=example.com",
+     "Cookie: test-cookie"},
+
+    // Secure-cookies for ws (sharing domain)
+    {"ws://www.example.com",
+     "https://www2.example.com",
+     "test-cookie; Domain=example.com; secure",
+     kNoCookieHeader},
+
+    {"ws://www.example.com",
+     "wss://www2.example.com",
+     "test-cookie; Domain=example.com; secure",
+     kNoCookieHeader},
+
+    // Secure-cookies for wss (sharing domain)
+    {"wss://www.example.com",
+     "https://www2.example.com",
+     "test-cookie; Domain=example.com; secure",
+     "Cookie: test-cookie"},
+
+    {"wss://www.example.com",
+     "wss://www2.example.com",
+     "test-cookie; Domain=example.com; secure",
+     "Cookie: test-cookie"},
+
+    // Non-matching cookies for ws
+    {"ws://www.example.com",
+     "http://www2.example.com",
+     "test-cookie",
+     kNoCookieHeader},
+
+    {"ws://www.example.com",
+     "https://www2.example.com",
+     "test-cookie",
+     kNoCookieHeader},
+
+    {"ws://www.example.com",
+     "ws://www2.example.com",
+     "test-cookie",
+     kNoCookieHeader},
+
+    {"ws://www.example.com",
+     "wss://www2.example.com",
+     "test-cookie",
+     kNoCookieHeader},
+
+    // Non-matching cookies for wss
+    {"wss://www.example.com",
+     "http://www2.example.com",
+     "test-cookie",
+     kNoCookieHeader},
+
+    {"wss://www.example.com",
+     "https://www2.example.com",
+     "test-cookie",
+     kNoCookieHeader},
+
+    {"wss://www.example.com",
+     "ws://www2.example.com",
+     "test-cookie",
+     kNoCookieHeader},
+
+    {"wss://www.example.com",
+     "wss://www2.example.com",
+     "test-cookie",
+     kNoCookieHeader},
+};
+
+const ServerSetCookieParameter kServerSetCookieParameters[] = {
+    // Cookies coming from ws
+    {"ws://www.example.com",
+     "http://www.example.com",
+     "test-cookie",
+     "Set-Cookie: test-cookie"},
+
+    {"ws://www.example.com",
+     "https://www.example.com",
+     "test-cookie",
+     "Set-Cookie: test-cookie"},
+
+    {"ws://www.example.com",
+     "ws://www.example.com",
+     "test-cookie",
+     "Set-Cookie: test-cookie"},
+
+    {"ws://www.example.com",
+     "wss://www.example.com",
+     "test-cookie",
+     "Set-Cookie: test-cookie"},
+
+    // Cookies coming from wss
+    {"wss://www.example.com",
+     "http://www.example.com",
+     "test-cookie",
+     "Set-Cookie: test-cookie"},
+
+    {"wss://www.example.com",
+     "https://www.example.com",
+     "test-cookie",
+     "Set-Cookie: test-cookie"},
+
+    {"wss://www.example.com",
+     "ws://www.example.com",
+     "test-cookie",
+     "Set-Cookie: test-cookie"},
+
+    {"wss://www.example.com",
+     "wss://www.example.com",
+     "test-cookie",
+     "Set-Cookie: test-cookie"},
+
+    // cookies coming from ws (sharing domain)
+    {"ws://www.example.com",
+     "http://www2.example.com",
+     "test-cookie",
+     "Set-Cookie: test-cookie; Domain=example.com"},
+
+    {"ws://www.example.com",
+     "https://www2.example.com",
+     "test-cookie",
+     "Set-Cookie: test-cookie; Domain=example.com"},
+
+    {"ws://www.example.com",
+     "ws://www2.example.com",
+     "test-cookie",
+     "Set-Cookie: test-cookie; Domain=example.com"},
+
+    {"ws://www.example.com",
+     "wss://www2.example.com",
+     "test-cookie",
+     "Set-Cookie: test-cookie; Domain=example.com"},
+
+    // cookies coming from wss (sharing domain)
+    {"wss://www.example.com",
+     "http://www2.example.com",
+     "test-cookie",
+     "Set-Cookie: test-cookie; Domain=example.com"},
+
+    {"wss://www.example.com",
+     "https://www2.example.com",
+     "test-cookie",
+     "Set-Cookie: test-cookie; Domain=example.com"},
+
+    {"wss://www.example.com",
+     "ws://www2.example.com",
+     "test-cookie",
+     "Set-Cookie: test-cookie; Domain=example.com"},
+
+    {"wss://www.example.com",
+     "wss://www2.example.com",
+     "test-cookie",
+     "Set-Cookie: test-cookie; Domain=example.com"},
+
+    // Non-matching cookies coming from ws
+    {"ws://www.example.com",
+     "http://www2.example.com",
+     "",
+     "Set-Cookie: test-cookie"},
+
+    {"ws://www.example.com",
+     "https://www2.example.com",
+     "",
+     "Set-Cookie: test-cookie"},
+
+    {"ws://www.example.com",
+     "ws://www2.example.com",
+     "",
+     "Set-Cookie: test-cookie"},
+
+    {"ws://www.example.com",
+     "wss://www2.example.com",
+     "",
+     "Set-Cookie: test-cookie"},
+
+    // Non-matching cookies coming from wss
+    {"wss://www.example.com",
+     "http://www2.example.com",
+     "",
+     "Set-Cookie: test-cookie"},
+
+    {"wss://www.example.com",
+     "https://www2.example.com",
+     "",
+     "Set-Cookie: test-cookie"},
+
+    {"wss://www.example.com",
+     "ws://www2.example.com",
+     "",
+     "Set-Cookie: test-cookie"},
+
+    {"wss://www.example.com",
+     "wss://www2.example.com",
+     "",
+     "Set-Cookie: test-cookie"},
+};
+
+INSTANTIATE_TEST_CASE_P(WebSocketStreamClientUseCookieTest,
+                        WebSocketStreamClientUseCookieTest,
+                        ValuesIn(kClientUseCookieParameters));

[Ryan Sleevi, 2015/02/02 20:14:41]

Move this to line 358 - keep your code & instantiations close.

[yhirano, 2015/02/09 10:50:14]

Done.

+
+INSTANTIATE_TEST_CASE_P(WebSocketStreamServerSetCookieTest,
+                        WebSocketStreamServerSetCookieTest,
+                        ValuesIn(kServerSetCookieParameters));
+
+}  // namespace
+}  // namespace net