Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2093923004:
[Cast Channel] Add real SSL tests to CastSocketTest (Closed)
|
Created:
4 years, 6 months ago by btolsch Modified:
3 years, 5 months ago CC:
cbentzel+watch_chromium.org, chromium-apps-reviews_chromium.org, chromium-reviews, davidben, extensions-reviews_chromium.org, Ryan Sleevi Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Cast Channel] Add real SSL tests to CastSocketTest
CastSocket is tested using a mocked transport which does not use SSL.
This can and did lead to integration problems when SSL code is changed
(https://crbug.com/622098). This change adds two tests to
CastSocketTest which use an SSL socket for their transport instead of
the mocked TCP socket.
BUG=622554
Review-Url: https://codereview.chromium.org/2093923004
Cr-Commit-Position: refs/heads/master@{#484819}
Committed: https://chromium.googlesource.com/chromium/src/+/25ff9b7cb09ab7db6dd453c5d5c43779a58c8159
Patch Set 1 #
Total comments: 16
Patch Set 2 : Responding to rsleevi's comments #
Total comments: 6
Patch Set 3 : Use real TCP sockets instead of fakes #
Total comments: 2
Patch Set 4 : Separate ssl and mock cast socket tests #
Total comments: 20
Patch Set 5 : Respond to comments and nits #
Total comments: 4
Patch Set 6 : Use continuous loop to accept TCP connection #
Total comments: 12
Patch Set 7 : Rebase and respond to comments #
Total comments: 6
Patch Set 8 : Respond to comments #Patch Set 9 : Rebase #
Messages
Total messages: 61 (24 generated)
btolsch@chromium.org changed reviewers: + imcheng@chromium.org, rsleevi@chromium.org, wez@chromium.org
PTAL, thanks!
rsleevi@chromium.org changed reviewers: + imcheng@google.com - imcheng@chromium.org
My initial reaction is that socket_test_util is growing too large (we spent time the other quarter to try to reduce its size). I totally get the argument for reusability, but experience has been that this sort of reuse gets in the way more than it helps, especially when it's only a second callsite. I'm curious to get David's thoughts - just to sanity check whether I'm being unreasonable about my concern - in thinking it wouldn't be the end of the world to have this logic just duplicated in the Cast code, scoped to just what was necessary.
https://codereview.chromium.org/2093923004/diff/1/extensions/browser/api/cast... File extensions/browser/api/cast_channel/cast_socket_unittest.cc (right): https://codereview.chromium.org/2093923004/diff/1/extensions/browser/api/cast... extensions/browser/api/cast_channel/cast_socket_unittest.cc:322: fake_socket_(nullptr) {} Need more documentation throughout this file. It's a bit complex to try to figure out all the bits (fake_socket vs fake_client_socket vs server_socket) https://codereview.chromium.org/2093923004/diff/1/extensions/browser/api/cast... extensions/browser/api/cast_channel/cast_socket_unittest.cc:494: "unittest.selfsigned.der"); I have no idea why we've got this cert. I've added it to my list of cleanups for //net. The reason why it's not ideal to use this is that it's generated by hand, which can be a real pain. Does using ok_cert.pem work here? Why or why not? I would recommend that for the new code https://codereview.chromium.org/2093923004/diff/1/extensions/browser/api/cast... extensions/browser/api/cast_channel/cast_socket_unittest.cc:974: base::Unretained(&handler_))); BUG?: You should be checking/storing the result code https://codereview.chromium.org/2093923004/diff/1/extensions/browser/api/cast... extensions/browser/api/cast_channel/cast_socket_unittest.cc:975: server_ret = handshake_callback.GetResult(server_ret); DESIGN: server_ret = handshake_callback.GetResult(socket_->Connect(...)); https://codereview.chromium.org/2093923004/diff/1/extensions/browser/api/cast... extensions/browser/api/cast_channel/cast_socket_unittest.cc:994: EXPECT_GT(read_result, 0); DESIGN: Replace 992-994 with read_result = read_callback.GetResult(server_socket_->Read(...)); https://codereview.chromium.org/2093923004/diff/1/extensions/browser/api/cast... extensions/browser/api/cast_channel/cast_socket_unittest.cc:997: std::string(challenge_buffer->data(), challenge_buffer_length)); BUG: Note |read_result| may be <= challenge_buffer_length That is, there's NO guarantee that server_socket_->Read() will read all of challenge_buffer_length in a single call https://codereview.chromium.org/2093923004/diff/1/extensions/browser/api/cast... extensions/browser/api/cast_channel/cast_socket_unittest.cc:1008: reply_buffer.get(), reply_buffer->size(), write_callback.callback()); Same remarks about write & write sizes https://codereview.chromium.org/2093923004/diff/1/extensions/browser/api/cast... extensions/browser/api/cast_channel/cast_socket_unittest.cc:1012: write_result = write_callback.GetResult(write_result); Same remarks about coalescing the GetResult()
btolsch@chromium.org changed reviewers: + imcheng@chromium.org - imcheng@google.com
I also felt unsure about pulling FakeSocket into socket_test_util since it was only the second use. I will definitely defer to your judgment on that. PTAL, thanks! https://codereview.chromium.org/2093923004/diff/1/extensions/browser/api/cast... File extensions/browser/api/cast_channel/cast_socket_unittest.cc (right): https://codereview.chromium.org/2093923004/diff/1/extensions/browser/api/cast... extensions/browser/api/cast_channel/cast_socket_unittest.cc:322: fake_socket_(nullptr) {} On 2016/06/24 21:44:51, Ryan Sleevi wrote: > Need more documentation throughout this file. It's a bit complex to try to > figure out all the bits (fake_socket vs fake_client_socket vs server_socket) Done. https://codereview.chromium.org/2093923004/diff/1/extensions/browser/api/cast... extensions/browser/api/cast_channel/cast_socket_unittest.cc:494: "unittest.selfsigned.der"); On 2016/06/24 21:44:51, Ryan Sleevi wrote: > I have no idea why we've got this cert. I've added it to my list of cleanups for > //net. > > The reason why it's not ideal to use this is that it's generated by hand, which > can be a real pain. Does using ok_cert.pem work here? Why or why not? I would > recommend that for the new code Done. This, among other things, was taken from SSLServerSocketTest. https://codereview.chromium.org/2093923004/diff/1/extensions/browser/api/cast... extensions/browser/api/cast_channel/cast_socket_unittest.cc:974: base::Unretained(&handler_))); On 2016/06/24 21:44:51, Ryan Sleevi wrote: > BUG?: You should be checking/storing the result code This is CastSocket::Connect and it returns void. https://codereview.chromium.org/2093923004/diff/1/extensions/browser/api/cast... extensions/browser/api/cast_channel/cast_socket_unittest.cc:975: server_ret = handshake_callback.GetResult(server_ret); On 2016/06/24 21:44:51, Ryan Sleevi wrote: > DESIGN: server_ret = handshake_callback.GetResult(socket_->Connect(...)); Done. https://codereview.chromium.org/2093923004/diff/1/extensions/browser/api/cast... extensions/browser/api/cast_channel/cast_socket_unittest.cc:994: EXPECT_GT(read_result, 0); On 2016/06/24 21:44:51, Ryan Sleevi wrote: > DESIGN: Replace 992-994 with > > read_result = read_callback.GetResult(server_socket_->Read(...)); Done. https://codereview.chromium.org/2093923004/diff/1/extensions/browser/api/cast... extensions/browser/api/cast_channel/cast_socket_unittest.cc:997: std::string(challenge_buffer->data(), challenge_buffer_length)); On 2016/06/24 21:44:51, Ryan Sleevi wrote: > BUG: Note |read_result| may be <= challenge_buffer_length > > That is, there's NO guarantee that server_socket_->Read() will read all of > challenge_buffer_length in a single call Done. https://codereview.chromium.org/2093923004/diff/1/extensions/browser/api/cast... extensions/browser/api/cast_channel/cast_socket_unittest.cc:1008: reply_buffer.get(), reply_buffer->size(), write_callback.callback()); On 2016/06/24 21:44:51, Ryan Sleevi wrote: > Same remarks about write & write sizes Done. https://codereview.chromium.org/2093923004/diff/1/extensions/browser/api/cast... extensions/browser/api/cast_channel/cast_socket_unittest.cc:1012: write_result = write_callback.GetResult(write_result); On 2016/06/24 21:44:51, Ryan Sleevi wrote: > Same remarks about coalescing the GetResult() Done.
Description was changed from ========== [Cast Channel] Add real SSL tests to CastSocketTest CastSocket is tested using a mocked transport which does not use SSL. This can and did lead to integration problems when SSL code is changed (https://crbug.com/622098). This change adds two tests to CastSocketTest which use an SSL socket for their transport instead of the mocked TCP socket. BUG=622554 ========== to ========== [Cast Channel] Add real SSL tests to CastSocketTest CastSocket is tested using a mocked transport which does not use SSL. This can and did lead to integration problems when SSL code is changed (https://crbug.com/622098). This change adds two tests to CastSocketTest which use an SSL socket for their transport instead of the mocked TCP socket. BUG=622554 ==========
btolsch@chromium.org changed reviewers: + mfoltz@chromium.org - wez@chromium.org
Friendly ping. Any consensus on socket_test_util or any other issues?
davidben@chromium.org changed reviewers: + davidben@chromium.org, mmenke@chromium.org
On 2016/06/24 21:30:44, Ryan Sleevi wrote: > My initial reaction is that socket_test_util is growing too large (we spent time > the other quarter to try to reduce its size). I totally get the argument for > reusability, but experience has been that this sort of reuse gets in the way > more than it helps, especially when it's only a second callsite. > > I'm curious to get David's thoughts - just to sanity check whether I'm being > unreasonable about my concern - in thinking it wouldn't be the end of the world > to have this logic just duplicated in the Cast code, scoped to just what was > necessary. Sorry, missed this. I think I more-or-less don't care strongly either way. But I'm also not familiar with the hodge-podge of test sockets we have. (I don't quite understand why FakeSocket is to live in //net but FakeTCPClientSocket is to live outside??) +mmenke is probably more familiar with this test stuff and has more definitive views than me. I'm pretty firmly off in BoringSSL C land these days.
A couple things: * If we want a new test socket class, I agree that it would be better of in its own file. * The class seems to need a lot more documentation. * Is this the best way to do this? Is there some reason for not using real SSL server/client sockets in the cast unittest?
Regarding using real sockets, this solution seemed preferable to adding support for the CastMessage protobuf to the python test server. I'm willing to go either route though.
On 2016/06/30 22:05:58, btolsch wrote: > Regarding using real sockets, this solution seemed preferable to adding support > for the CastMessage protobuf to the python test server. I'm willing to go > either route though. We have server and client C++ sockets. Shouldn't have to use the python test server, no? I agree that adding stuff to the python test server isn't a great approach.
On 2016/06/30 22:11:02, mmenke wrote: > We have server and client C++ sockets. Shouldn't have to use the python test > server, no? I agree that adding stuff to the python test server isn't a great > approach. Is your Cast API HTTP-and-protobufs based? Or Sockets-and-protobufs? HTTP-and-protobufs you can use EmbeddedTestServer Sockets-and-protobufs can use https://cs.chromium.org/chromium/src/net/socket/ssl_server_socket.h?rcl=0&l=5 (it's acceptable for tests, but it's not for production use)
On 2016/06/30 22:19:24, Ryan Sleevi wrote: > On 2016/06/30 22:11:02, mmenke wrote: > > We have server and client C++ sockets. Shouldn't have to use the python test > > server, no? I agree that adding stuff to the python test server isn't a great > > approach. > > Is your Cast API HTTP-and-protobufs based? Or Sockets-and-protobufs? It's Sockets-and-protobufs. > HTTP-and-protobufs you can use EmbeddedTestServer > Sockets-and-protobufs can use > https://cs.chromium.org/chromium/src/net/socket/ssl_server_socket.h?rcl=0&l=5 > (it's acceptable for tests, but it's not for production use) I think there's some confusion because these tests do use SSLServerSocket. What is being faked, at least for what I'm adding, is the underlying transport (what would be a tcp socket in real use).
wez@chromium.org changed reviewers: + wez@chromium.org
Do we really need to use the fake sockets, or could we just use normal TCP sockets - create a TCPServerSocket, grab the IP address info, pass that to a TCPClientSocket, and pass those to use instead of the MockTCPSocket? That would assume that TCP loopback works on the system running the test, but that seems not entirely unreasonable. Or is there something specific about the fake that is interesting to test? https://codereview.chromium.org/2093923004/diff/20001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_socket_unittest.cc (right): https://codereview.chromium.org/2093923004/diff/20001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_unittest.cc:511: std::unique_ptr<net::StreamSocket> server_socket( nit: fake_server_socket? https://codereview.chromium.org/2093923004/diff/20001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_unittest.cc:596: content::TestBrowserThreadBundle::IO_MAINLOOP}; nit: Is there a good reason to use {} init rather than (), given that the other members (e.g. logger_) are initialized in the constructor? https://codereview.chromium.org/2093923004/diff/20001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_unittest.cc:1071: socket_->SetFakeSocket(fake_client_socket_.get()); Is it necessary for the test to retain ownership of the fake_client_socket_, or could you pass ownership into the |socket_| instead?
On 2016/07/01 01:27:56, Wez wrote: > Do we really need to use the fake sockets, or could we just use normal TCP > sockets - create a TCPServerSocket, grab the IP address info, pass that to a > TCPClientSocket, and pass those to use instead of the MockTCPSocket? That sounds like exactly what I should've done. No more changes in net/ now :) . e/b/a/cast_channel reviewers PTAL. > That would assume that TCP loopback works on the system running the test, but > that seems not entirely unreasonable. > > Or is there something specific about the fake that is interesting to test?
https://codereview.chromium.org/2093923004/diff/20001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_socket_unittest.cc (right): https://codereview.chromium.org/2093923004/diff/20001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_unittest.cc:511: std::unique_ptr<net::StreamSocket> server_socket( On 2016/07/01 01:27:56, Wez wrote: > nit: fake_server_socket? This was effectively changed to |accepted_socket|. https://codereview.chromium.org/2093923004/diff/20001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_unittest.cc:596: content::TestBrowserThreadBundle::IO_MAINLOOP}; On 2016/07/01 01:27:56, Wez wrote: > nit: Is there a good reason to use {} init rather than (), given that the other > members (e.g. logger_) are initialized in the constructor? No, just a habit that slipped through. Moved to constructor. https://codereview.chromium.org/2093923004/diff/20001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_unittest.cc:1071: socket_->SetFakeSocket(fake_client_socket_.get()); On 2016/07/01 01:27:56, Wez wrote: > Is it necessary for the test to retain ownership of the fake_client_socket_, or > could you pass ownership into the |socket_| instead? Done (passed ownership to |socket_|).
mmenke@chromium.org changed reviewers: - mmenke@chromium.org
A couple of design observations before getting into a line-by-line review. You've modified TestCastSocket to have pretty different behavior based on setting tcp_client_socket_ after construction. I would consider creating a TestCastSocketBase with subclassses MockTestCastSocket and SslTestCastSocket so that it's clear what kind of socket you're getting when you new one up. Similarly you might create a subclass of the test base class, like SslCastSocketTest, that sets up the SslTestCastSocket, and factor out the per-test-case setup code. IMO this makes it easier to set up new test cases, versus having to copy-paste code from other tests to get the sockets set up correctly.
https://codereview.chromium.org/2093923004/diff/40001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_socket_unittest.cc (right): https://codereview.chromium.org/2093923004/diff/40001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_unittest.cc:234: use_tcp_socket_(false) {} This looks like it could be removed and replaced with tcp_client_socket_ which has conversions to true/false. Or is CreateSslSocket() called before SetTcpSocket() (which doesn't quite make sense to me)?
I split the ssl and "mock transport" test cases which hopefully improves readability. PTAL, thanks! https://codereview.chromium.org/2093923004/diff/40001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_socket_unittest.cc (right): https://codereview.chromium.org/2093923004/diff/40001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_unittest.cc:234: use_tcp_socket_(false) {} On 2016/07/02 01:44:55, mfoltz_ooo_until_july_10 wrote: > This looks like it could be removed and replaced with tcp_client_socket_ which > has conversions to true/false. Or is CreateSslSocket() called before > SetTcpSocket() (which doesn't quite make sense to me)? The boolean has been removed by splitting the test classes but FWIW the boolean was necessary here (probably adding to the reasons to split the test cases). tcp_client_socket_ would be set to nullptr in the move in CreateTcpSocket(), which is called before CreateSslSocket(). So when CreateSslSocket() runs, tcp_client_socket_ would always be nullptr. This call ordering happens inside CastSocketImpl and therefore might not be clear.
One round of comments (I didn't take a close look at SslCastSocketTest and related tests yet.) https://codereview.chromium.org/2093923004/diff/60001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_socket_unittest.cc (right): https://codereview.chromium.org/2093923004/diff/60001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_unittest.cc:218: void SetExtractCertResult(bool value) { extract_cert_result_ = value; } s/value/extract_result/ https://codereview.chromium.org/2093923004/diff/60001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_unittest.cc:220: void SetVerifyChallengeResult(bool value) { s/value/verify_challenge/ https://codereview.chromium.org/2093923004/diff/60001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_unittest.cc:226: bool TestVerifyChannelPolicyNone() { nit: This isn't actually "testing" in the sense there aren't ASSERT or EXPECT statments. Instead, maybe just HasChannelPolicyNone(). https://codereview.chromium.org/2093923004/diff/60001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_unittest.cc:231: bool TestVerifyChannelPolicyAudioOnly() { HasChannelPolicyAudioOnly() https://codereview.chromium.org/2093923004/diff/60001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_unittest.cc:237: void DisallowVerifyChallengeResult() { verify_challenge_disallow_ = true; } Why not SetDisallowVerifyChallengeResult like the other flags here? https://codereview.chromium.org/2093923004/diff/60001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_unittest.cc:242: ? net::ImportCertFromFile(net::GetTestCertsDirectory(), Can net::ImportCertFromFile return an error? Should the result be tested with an ASSERT to fail fast? https://codereview.chromium.org/2093923004/diff/60001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_unittest.cc:248: EXPECT_FALSE(verify_challenge_disallow_); But this flag is set by the the test fixture, not the socket being tested. Do you mean ASSERT_FALSE()? https://codereview.chromium.org/2093923004/diff/60001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_unittest.cc:272: CreateIPEndPointForTest(), CHANNEL_AUTH_TYPE_SSL, kDistantTimeoutMillis, I wonder if anyone actually uses CHANNEL_AUTH_TYPE_SSL. If not, it would be a good cleanup to remove it. https://codereview.chromium.org/2093923004/diff/60001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_unittest.cc:449: DISALLOW_COPY_AND_ASSIGN(CastSocketTestBase); I don't think test objects are normally copied/assigned by gtest, but I suppose this can't hurt.
https://codereview.chromium.org/2093923004/diff/60001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_socket_unittest.cc (right): https://codereview.chromium.org/2093923004/diff/60001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_unittest.cc:272: CreateIPEndPointForTest(), CHANNEL_AUTH_TYPE_SSL, kDistantTimeoutMillis, On 2016/07/28 at 00:10:01, mark a. foltz wrote: > I wonder if anyone actually uses CHANNEL_AUTH_TYPE_SSL. If not, it would be a good cleanup to remove it. Note, I was thinking this would be a separate patch as there is a lot of code in other files that would be deleted.
https://codereview.chromium.org/2093923004/diff/60001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_socket_unittest.cc (right): https://codereview.chromium.org/2093923004/diff/60001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_unittest.cc:218: void SetExtractCertResult(bool value) { extract_cert_result_ = value; } On 2016/07/28 00:10:01, mfoltz_ooo_7-29_8-14 wrote: > s/value/extract_result/ Done. https://codereview.chromium.org/2093923004/diff/60001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_unittest.cc:220: void SetVerifyChallengeResult(bool value) { On 2016/07/28 00:10:01, mfoltz_ooo_7-29_8-14 wrote: > s/value/verify_challenge/ Done. https://codereview.chromium.org/2093923004/diff/60001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_unittest.cc:226: bool TestVerifyChannelPolicyNone() { On 2016/07/28 00:10:01, mfoltz_ooo_7-29_8-14 wrote: > nit: This isn't actually "testing" in the sense there aren't ASSERT or EXPECT > statments. Instead, maybe just HasChannelPolicyNone(). Done. https://codereview.chromium.org/2093923004/diff/60001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_unittest.cc:231: bool TestVerifyChannelPolicyAudioOnly() { On 2016/07/28 00:10:01, mfoltz_ooo_7-29_8-14 wrote: > HasChannelPolicyAudioOnly() Done. https://codereview.chromium.org/2093923004/diff/60001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_unittest.cc:237: void DisallowVerifyChallengeResult() { verify_challenge_disallow_ = true; } On 2016/07/28 00:10:01, mfoltz_ooo_7-29_8-14 wrote: > Why not SetDisallowVerifyChallengeResult like the other flags here? Copied from original class, and this is actually dead because there is a SetVerifyChallengeResult(). Removed this. https://codereview.chromium.org/2093923004/diff/60001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_unittest.cc:242: ? net::ImportCertFromFile(net::GetTestCertsDirectory(), On 2016/07/28 00:10:01, mfoltz_ooo_7-29_8-14 wrote: > Can net::ImportCertFromFile return an error? Should the result be tested with > an ASSERT to fail fast? Done but with EXPECT, can't use ASSERT in functions that return a value. https://codereview.chromium.org/2093923004/diff/60001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_unittest.cc:248: EXPECT_FALSE(verify_challenge_disallow_); On 2016/07/28 00:10:01, mfoltz_ooo_7-29_8-14 wrote: > But this flag is set by the the test fixture, not the socket being tested. > Do you mean ASSERT_FALSE()? ASSERT_* can't be used in functions that return a value. https://codereview.chromium.org/2093923004/diff/60001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_unittest.cc:272: CreateIPEndPointForTest(), CHANNEL_AUTH_TYPE_SSL, kDistantTimeoutMillis, On 2016/07/28 00:10:01, mfoltz_ooo_7-29_8-14 wrote: > I wonder if anyone actually uses CHANNEL_AUTH_TYPE_SSL. If not, it would be a > good cleanup to remove it. It looks like there are no legitimate uses so a follow-up to this could remove it. https://codereview.chromium.org/2093923004/diff/60001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_unittest.cc:272: CreateIPEndPointForTest(), CHANNEL_AUTH_TYPE_SSL, kDistantTimeoutMillis, On 2016/07/28 00:11:49, mfoltz_ooo_7-29_8-14 wrote: > On 2016/07/28 at 00:10:01, mark a. foltz wrote: > > I wonder if anyone actually uses CHANNEL_AUTH_TYPE_SSL. If not, it would be a > good cleanup to remove it. > > Note, I was thinking this would be a separate patch as there is a lot of code in > other files that would be deleted. Acknowledged. https://codereview.chromium.org/2093923004/diff/60001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_unittest.cc:449: DISALLOW_COPY_AND_ASSIGN(CastSocketTestBase); On 2016/07/28 00:10:01, mfoltz_ooo_7-29_8-14 wrote: > I don't think test objects are normally copied/assigned by gtest, but I suppose > this can't hurt. Carried over from old code. Removed.
lgtm Thanks for improving our test coverage. Please file a cleanup crbug for removing CHANNEL_AUTH_TYPE_SSL.
The CQ bit was checked by btolsch@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2093923004/diff/80001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_socket_unittest.cc (right): https://codereview.chromium.org/2093923004/diff/80001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_unittest.cc:516: base::Unretained(this)))); DESIGN: You shouldn't assume that these sockets will require an asynchronous operation. That is, stash the RV, RunPendingTests, then assert the values. This is true for pretty much any of the //net functions that could behave asynchronously (e.g. if they accept a callback). The //net paradigm is they can be sync-or-async. https://codereview.chromium.org/2093923004/diff/80001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_unittest.cc:517: RunPendingTasks(); BUG: You only call RunUntilIdle, but that doesn't allow sufficient time if the kernel is doing asynchronous processing. If you look at the //net TestCompletionCallback paradigm, it's necessary to continue to spin the event loop until your event succeeds. This is particularly important since posted tasks can take precedence over IO interrupts.
I'll try to do a more thorough review tomorrow; if you don't hear from me, ping me in the event it slipped off my radar :)
On 2016/08/17 00:57:22, Ryan Sleevi (slow) wrote: > I'll try to do a more thorough review tomorrow; if you don't hear from me, ping > me in the event it slipped off my radar :) Thanks! Also if you can, PTAL at L518 and the TCP socket binding. It is failing on windows and mac release bots and on windows locally. I'm not sure about the root cause or how to fix it but if you have any clues about it I would appreciate it.
On 2016/08/17 01:04:28, btolsch wrote: > On 2016/08/17 00:57:22, Ryan Sleevi (slow) wrote: > > I'll try to do a more thorough review tomorrow; if you don't hear from me, > ping > > me in the event it slipped off my radar :) > > Thanks! Also if you can, PTAL at L518 and the TCP socket binding. It is > failing on windows and mac release bots and on windows locally. I'm not sure > about the root cause or how to fix it but if you have any clues about it I would > appreciate it. The 518 failure would result from BUG I mentioned.
rsleevi PTAL, thanks! https://codereview.chromium.org/2093923004/diff/80001/extensions/browser/api/... File extensions/browser/api/cast_channel/cast_socket_unittest.cc (right): https://codereview.chromium.org/2093923004/diff/80001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_unittest.cc:516: base::Unretained(this)))); On 2016/08/17 00:56:32, Ryan Sleevi (slow) wrote: > DESIGN: You shouldn't assume that these sockets will require an asynchronous > operation. That is, stash the RV, RunPendingTests, then assert the values. > > This is true for pretty much any of the //net functions that could behave > asynchronously (e.g. if they accept a callback). The //net paradigm is they can > be sync-or-async. Done. https://codereview.chromium.org/2093923004/diff/80001/extensions/browser/api/... extensions/browser/api/cast_channel/cast_socket_unittest.cc:517: RunPendingTasks(); On 2016/08/17 00:56:32, Ryan Sleevi (slow) wrote: > BUG: You only call RunUntilIdle, but that doesn't allow sufficient time if the > kernel is doing asynchronous processing. > > If you look at the //net TestCompletionCallback paradigm, it's necessary to > continue to spin the event loop until your event succeeds. This is particularly > important since posted tasks can take precedence over IO interrupts. Done.
rsleevi@chromium.org changed reviewers: + svaldez@chromium.org - rsleevi@chromium.org
Sorry for the delay in responding here. I've cc'd svaldez@ in the hope he'll be able to guide/take over this review, as he worked to get EmbeddedTestServer up. I think that the mismatch between the Cast socket API patterns and the //net API patterns here are likely going to be a continued source of pain, at least for writing, if not for maintaining, because the API patterns and guarantees are very different. I've tried to highlight below some of the problem areas, but it might be better if Steven could work and help with a sketch of a //net-blessed way of doing things. Without wanting to seem unnecessarily negative, the mismatch between these two means that there's still a fair amount of work needed to 'make it safe' (that is, the state machines safely handle sync-or-async completion of result). We have helpers in //net to make this easy, and you can see it used throughout //net tests, but I'm not sure if the current approach can be salvaged. https://codereview.chromium.org/2093923004/diff/100001/extensions/browser/api... File extensions/browser/api/cast_channel/cast_socket_unittest.cc (right): https://codereview.chromium.org/2093923004/diff/100001/extensions/browser/api... extensions/browser/api/cast_channel/cast_socket_unittest.cc:402: using TestCastSocketBase::TestCastSocketBase; I've rarely seen this pattern in Chrome, and when I've seen it in not-Chrome, it's usually a signal that the object hierarchy is weird or not right. It's unclear to me why this (and line 413), but they are big warning signs. https://codereview.chromium.org/2093923004/diff/100001/extensions/browser/api... extensions/browser/api/cast_channel/cast_socket_unittest.cc:468: socket()->Connect(std::move(delegate_), It still continues to surprise me that Cast sockets are designed async-only, given how much in //net we've tried to focus on sync-or-async (w/ state machines), to resolve UAF and other lifetime issues. Because of this weirdness (which is preexisting), it makes it really hard for me to reassure myself that the following code (on line 471) is correct. https://codereview.chromium.org/2093923004/diff/100001/extensions/browser/api... extensions/browser/api/cast_channel/cast_socket_unittest.cc:471: RunPendingTasks(); What happens if Connect() failed? Isn't line 472 intrinsically dangerous then - since you may be accessing the socket outside of its guarantees? One way to resolve this would be to have the OnConnectComplete handler_ be responsible for setting up this OnMessage/CreateAuthReply https://codereview.chromium.org/2093923004/diff/100001/extensions/browser/api... extensions/browser/api/cast_channel/cast_socket_unittest.cc:491: net::ImportCertFromFile(net::GetTestCertsDirectory(), "ok_cert.pem"); net::GetTestCertsDirectory() is more of an 'internal implementation detail' of //net, at least for purposes of "serve using this cert". That's because we can, have, and will change aspects of things like ok_cert.pem, which is 'safe' if you're using [Spawned/Embedded]TestServer (which are part of //net), but 'not safe' if you're outside (read: it's not always easy or obvious to maintain "Some Other Server's Code") Since you're in effect setting up your own EmbeddedCastTestServer-like thing here, it may be useful to create & maintain your own test certs for the purpose of Cast; that way your tests don't have the additional dependency on //net policies || needs. https://codereview.chromium.org/2093923004/diff/100001/extensions/browser/api... extensions/browser/api/cast_channel/cast_socket_unittest.cc:515: RunPendingTasks(); This will run indefinitely if either Accept() or Connect() synchronously complete, which they totally can. You need to check the result value of both (for things like net::ERR_IO_PENDING) https://codereview.chromium.org/2093923004/diff/100001/extensions/browser/api... extensions/browser/api/cast_channel/cast_socket_unittest.cc:516: } Because of how you've structured TcpAcceptCallback & TcpConnectCallback, if the callbacks are invoked with an error code, then accept/connect_succeeded_ will never be true, and the loop will run indefinitely.
On 2016/09/16 01:39:07, Ryan Sleevi (slow) wrote: > Sorry for the delay in responding here. > > I've cc'd svaldez@ in the hope he'll be able to guide/take over this review, as > he worked to get EmbeddedTestServer up. I think that the mismatch between the > Cast socket API patterns and the //net API patterns here are likely going to be > a continued source of pain, at least for writing, if not for maintaining, > because the API patterns and guarantees are very different. > > I've tried to highlight below some of the problem areas, but it might be better > if Steven could work and help with a sketch of a //net-blessed way of doing > things. Without wanting to seem unnecessarily negative, the mismatch between > these two means that there's still a fair amount of work needed to 'make it > safe' (that is, the state machines safely handle sync-or-async completion of > result). We have helpers in //net to make this easy, and you can see it used > throughout //net tests, but I'm not sure if the current approach can be > salvaged. > > https://codereview.chromium.org/2093923004/diff/100001/extensions/browser/api... > File extensions/browser/api/cast_channel/cast_socket_unittest.cc (right): > > https://codereview.chromium.org/2093923004/diff/100001/extensions/browser/api... > extensions/browser/api/cast_channel/cast_socket_unittest.cc:402: using > TestCastSocketBase::TestCastSocketBase; > I've rarely seen this pattern in Chrome, and when I've seen it in not-Chrome, > it's usually a signal that the object hierarchy is weird or not right. > > It's unclear to me why this (and line 413), but they are big warning signs. > > https://codereview.chromium.org/2093923004/diff/100001/extensions/browser/api... > extensions/browser/api/cast_channel/cast_socket_unittest.cc:468: > socket()->Connect(std::move(delegate_), > It still continues to surprise me that Cast sockets are designed async-only, > given how much in //net we've tried to focus on sync-or-async (w/ state > machines), to resolve UAF and other lifetime issues. > > Because of this weirdness (which is preexisting), it makes it really hard for me > to reassure myself that the following code (on line 471) is correct. > > https://codereview.chromium.org/2093923004/diff/100001/extensions/browser/api... > extensions/browser/api/cast_channel/cast_socket_unittest.cc:471: > RunPendingTasks(); > What happens if Connect() failed? Isn't line 472 intrinsically dangerous then - > since you may be accessing the socket outside of its guarantees? > > One way to resolve this would be to have the OnConnectComplete handler_ be > responsible for setting up this OnMessage/CreateAuthReply > > https://codereview.chromium.org/2093923004/diff/100001/extensions/browser/api... > extensions/browser/api/cast_channel/cast_socket_unittest.cc:491: > net::ImportCertFromFile(net::GetTestCertsDirectory(), "ok_cert.pem"); > net::GetTestCertsDirectory() is more of an 'internal implementation detail' of > //net, at least for purposes of "serve using this cert". > > That's because we can, have, and will change aspects of things like ok_cert.pem, > which is 'safe' if you're using [Spawned/Embedded]TestServer (which are part of > //net), but 'not safe' if you're outside (read: it's not always easy or obvious > to maintain "Some Other Server's Code") > > Since you're in effect setting up your own EmbeddedCastTestServer-like thing > here, it may be useful to create & maintain your own test certs for the purpose > of Cast; that way your tests don't have the additional dependency on //net > policies || needs. > > https://codereview.chromium.org/2093923004/diff/100001/extensions/browser/api... > extensions/browser/api/cast_channel/cast_socket_unittest.cc:515: > RunPendingTasks(); > This will run indefinitely if either Accept() or Connect() synchronously > complete, which they totally can. You need to check the result value of both > (for things like net::ERR_IO_PENDING) > > https://codereview.chromium.org/2093923004/diff/100001/extensions/browser/api... > extensions/browser/api/cast_channel/cast_socket_unittest.cc:516: } > Because of how you've structured TcpAcceptCallback & TcpConnectCallback, if the > callbacks are invoked with an error code, then accept/connect_succeeded_ will > never be true, and the loop will run indefinitely. If this can wait a couple days, I can just add a non-HTTP mode to EmbeddedTestServer that will let tests hook in and parse the request in whatever way they want and sent a blob of bytes in the response, which should let you use that directly in this code.
Brandon: Is this CL still relevant to land, or shall we close it out?
btolsch@chromium.org changed reviewers: - davidben@chromium.org, imcheng@chromium.org
I'd like to take another shot at landing these tests. @svaldez: I responded to rsleevi's comments but EmbeddedTestServer never got a non-HTTP mode so I'm continuing with the current path. PTAL and let me know if you think there are any remaining problems. @mfoltz, @wez: This patch is mostly the same as before. https://codereview.chromium.org/2093923004/diff/100001/extensions/browser/api... File extensions/browser/api/cast_channel/cast_socket_unittest.cc (right): https://codereview.chromium.org/2093923004/diff/100001/extensions/browser/api... extensions/browser/api/cast_channel/cast_socket_unittest.cc:402: using TestCastSocketBase::TestCastSocketBase; On 2016/09/16 01:39:06, Ryan Sleevi (slow through 6-27 wrote: > I've rarely seen this pattern in Chrome, and when I've seen it in not-Chrome, > it's usually a signal that the object hierarchy is weird or not right. > > It's unclear to me why this (and line 413), but they are big warning signs. This line is just so I don't have to repeat the constructor arguments of TestCastSocketBase because nothing is added. Line 413 is not actually doing anything other than to say "MockTestCastSocket overrides this method, but I'm not." I removed it since this might actually be confusing. https://codereview.chromium.org/2093923004/diff/100001/extensions/browser/api... extensions/browser/api/cast_channel/cast_socket_unittest.cc:468: socket()->Connect(std::move(delegate_), On 2016/09/16 01:39:06, Ryan Sleevi (slow through 6-27 wrote: > It still continues to surprise me that Cast sockets are designed async-only, > given how much in //net we've tried to focus on sync-or-async (w/ state > machines), to resolve UAF and other lifetime issues. > > Because of this weirdness (which is preexisting), it makes it really hard for me > to reassure myself that the following code (on line 471) is correct. Acknowledged. https://codereview.chromium.org/2093923004/diff/100001/extensions/browser/api... extensions/browser/api/cast_channel/cast_socket_unittest.cc:471: RunPendingTasks(); On 2016/09/16 01:39:06, Ryan Sleevi (slow through 6-27 wrote: > What happens if Connect() failed? Isn't line 472 intrinsically dangerous then - > since you may be accessing the socket outside of its guarantees? > > One way to resolve this would be to have the OnConnectComplete handler_ be > responsible for setting up this OnMessage/CreateAuthReply This is existing code that I just moved here. 472 isn't really accessing socket_, it's accessing the transport directly and injecting an auth reply as if the socket were connecting to a real device. https://codereview.chromium.org/2093923004/diff/100001/extensions/browser/api... extensions/browser/api/cast_channel/cast_socket_unittest.cc:491: net::ImportCertFromFile(net::GetTestCertsDirectory(), "ok_cert.pem"); On 2016/09/16 01:39:06, Ryan Sleevi (slow through 6-27 wrote: > net::GetTestCertsDirectory() is more of an 'internal implementation detail' of > //net, at least for purposes of "serve using this cert". > > That's because we can, have, and will change aspects of things like ok_cert.pem, > which is 'safe' if you're using [Spawned/Embedded]TestServer (which are part of > //net), but 'not safe' if you're outside (read: it's not always easy or obvious > to maintain "Some Other Server's Code") > > Since you're in effect setting up your own EmbeddedCastTestServer-like thing > here, it may be useful to create & maintain your own test certs for the purpose > of Cast; that way your tests don't have the additional dependency on //net > policies || needs. Added a new cert to //components/test/data/cast_channel/ and also used it in ExtractPeerCert() above. https://codereview.chromium.org/2093923004/diff/100001/extensions/browser/api... extensions/browser/api/cast_channel/cast_socket_unittest.cc:515: RunPendingTasks(); On 2016/09/16 01:39:06, Ryan Sleevi (slow through 6-27 wrote: > This will run indefinitely if either Accept() or Connect() synchronously > complete, which they totally can. You need to check the result value of both > (for things like net::ERR_IO_PENDING) Done. https://codereview.chromium.org/2093923004/diff/100001/extensions/browser/api... extensions/browser/api/cast_channel/cast_socket_unittest.cc:516: } On 2016/09/16 01:39:06, Ryan Sleevi (slow through 6-27 wrote: > Because of how you've structured TcpAcceptCallback & TcpConnectCallback, if the > callbacks are invoked with an error code, then accept/connect_succeeded_ will > never be true, and the loop will run indefinitely. I revised this to simply save the result so the main loop can check it instead.
Sounds like the ETS rewrite needs to be more in-depth to get a raw socket mode, so doing something like this for now is probably the best choice to get these tests in. https://codereview.chromium.org/2093923004/diff/120001/components/cast_channe... File components/cast_channel/cast_socket_unittest.cc (right): https://codereview.chromium.org/2093923004/diff/120001/components/cast_channe... components/cast_channel/cast_socket_unittest.cc:446: socket_ = SslTestCastSocket::CreateSecure(logger_); Is there a reason to split CreateSSLSockets and CreateSSLCastSocket? Can SSLCastSocketTest just have an initialization method that initializes all the sockets together. https://codereview.chromium.org/2093923004/diff/120001/components/cast_channe... components/cast_channel/cast_socket_unittest.cc:1012: socket_->SetTcpSocket(std::move(tcp_client_socket_)); Consider merging these three lines into a CreateSocket/Initialize method and then having a separate method that handles all the Connect/Handshake logic. Then you can have the tests just have a preamble of: EXPECT_TRUE(CreateSockets()) EXPECT_TRUE(ConnectSockets()) // Actual Test Logic https://codereview.chromium.org/2093923004/diff/120001/components/test/data/c... File components/test/data/cast_channel/self_signed.pem (right): https://codereview.chromium.org/2093923004/diff/120001/components/test/data/c... components/test/data/cast_channel/self_signed.pem:62: Subject: CN=a89edd6572c2a2ba6b93f91be851946ce0610036 - Can you generate a cert with a Subject something like: 'C=US, ST=California, L=Mountain View, O=Test CA, CN=127.0.0.1' and a longer expiration date.
The CQ bit was checked by btolsch@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by btolsch@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
Thanks for taking a look at this svaldez. Android bots seems to be failing without the patch, but PTAL at the responses. https://codereview.chromium.org/2093923004/diff/120001/components/cast_channe... File components/cast_channel/cast_socket_unittest.cc (right): https://codereview.chromium.org/2093923004/diff/120001/components/cast_channe... components/cast_channel/cast_socket_unittest.cc:446: socket_ = SslTestCastSocket::CreateSecure(logger_); On 2017/07/06 15:00:17, svaldez wrote: > Is there a reason to split CreateSSLSockets and CreateSSLCastSocket? Can > SSLCastSocketTest just have an initialization method that initializes all the > sockets together. Done. https://codereview.chromium.org/2093923004/diff/120001/components/cast_channe... components/cast_channel/cast_socket_unittest.cc:1012: socket_->SetTcpSocket(std::move(tcp_client_socket_)); On 2017/07/06 15:00:17, svaldez wrote: > Consider merging these three lines into a CreateSocket/Initialize method and > then having a separate method that handles all the Connect/Handshake logic. > > Then you can have the tests just have a preamble of: > > EXPECT_TRUE(CreateSockets()) > EXPECT_TRUE(ConnectSockets()) > > // Actual Test Logic Done. https://codereview.chromium.org/2093923004/diff/120001/components/test/data/c... File components/test/data/cast_channel/self_signed.pem (right): https://codereview.chromium.org/2093923004/diff/120001/components/test/data/c... components/test/data/cast_channel/self_signed.pem:62: Subject: CN=a89edd6572c2a2ba6b93f91be851946ce0610036 - On 2017/07/06 15:00:17, svaldez wrote: > Can you generate a cert with a Subject something like: > > 'C=US, ST=California, L=Mountain View, O=Test CA, CN=127.0.0.1' > > and a longer expiration date. Absolutely. I wasn't sure what was appropriate here.
lgtm, the Android failures seem to be infra issues.
The CQ bit was checked by btolsch@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org Link to the patchset: https://codereview.chromium.org/2093923004/#ps160001 (title: "Rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1499394746841590,
"parent_rev": "6a1ea85d513d78b51f14136c94f43c5e19d52803", "commit_rev":
"25ff9b7cb09ab7db6dd453c5d5c43779a58c8159"}
Message was sent while issue was closed.
Description was changed from ========== [Cast Channel] Add real SSL tests to CastSocketTest CastSocket is tested using a mocked transport which does not use SSL. This can and did lead to integration problems when SSL code is changed (https://crbug.com/622098). This change adds two tests to CastSocketTest which use an SSL socket for their transport instead of the mocked TCP socket. BUG=622554 ========== to ========== [Cast Channel] Add real SSL tests to CastSocketTest CastSocket is tested using a mocked transport which does not use SSL. This can and did lead to integration problems when SSL code is changed (https://crbug.com/622098). This change adds two tests to CastSocketTest which use an SSL socket for their transport instead of the mocked TCP socket. BUG=622554 Review-Url: https://codereview.chromium.org/2093923004 Cr-Commit-Position: refs/heads/master@{#484819} Committed: https://chromium.googlesource.com/chromium/src/+/25ff9b7cb09ab7db6dd453c5d5c4... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/25ff9b7cb09ab7db6dd453c5d5c4... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
