Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
301283004:
Add tests for session cache and false start behavior. (Closed)
|
Created:
6 years, 6 months ago by davidben Modified:
6 years, 6 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd tests for session cache and false start behavior.
False start should not disable the session cache, but if we never process the
server Finished message, the session cannot be resumed.
BUG=none
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276815
Patch Set 1 #Patch Set 2 : Fix win/mac failures. #
Total comments: 10
Patch Set 3 : sleevi comments #
Total comments: 10
Patch Set 4 : sleevi comments #
Total comments: 2
Patch Set 5 : Revise comment. #Patch Set 6 : Rebase and appease MSVC #
Messages
Total messages: 22 (0 generated)
https://codereview.chromium.org/301283004/diff/2/net/socket/ssl_client_socket... File net/socket/ssl_client_socket_unittest.cc (right): https://codereview.chromium.org/301283004/diff/2/net/socket/ssl_client_socket... net/socket/ssl_client_socket_unittest.cc:722: void CreateAndConnectUpToServerFinished( s/UpToServerFinished/UntilServerFinishedReceived? The "UpTo" bit read weird at first - I thought you were saying "ConnectUp" (as a colloquial phrase). Hence the suggestion :) https://codereview.chromium.org/301283004/diff/2/net/socket/ssl_client_socket... net/socket/ssl_client_socket_unittest.cc:725: FakeBlockingStreamSocket** out_raw_transport, More documentation is needed here. My red alert alarms are going off on having a FakeBlockingStreamSocket**, because it's not clear if ownership is transferred (and this should be a scoped_ptr<FakeBlockingStreamSocket>* out_raw_socket) or not. It's also not clear whether or not it's safe to call this before the test server has been started - presumably not, given that you're using addr_ (line 728) https://codereview.chromium.org/301283004/diff/2/net/socket/ssl_client_socket... net/socket/ssl_client_socket_unittest.cc:776: client_config, &callback, &raw_transport, &sock); DANGER: If you ever added an ASSERT_() to CreateAndConnectUpToServerFinished(), your test will still continue, because it's wrapped within a function. As such, it would be good to wrap this call (and calls to this TestFalseStart function) in ASSERT_NO_FATAL_FAILURE() before continuing. eg: ASSERT_NO_FATAL_FAILURE(CreateAndConnectUpToServerFinished(...)); https://codereview.chromium.org/301283004/diff/2/net/socket/ssl_client_socket... net/socket/ssl_client_socket_unittest.cc:781: int rv = callback.GetResult(ERR_IO_PENDING); Just use int rv = callback.WaitForResult(); no need to fake it like this. https://codereview.chromium.org/301283004/diff/2/net/socket/ssl_client_socket... net/socket/ssl_client_socket_unittest.cc:2547: EXPECT_EQ(OK, callback.GetResult(ERR_IO_PENDING)); ditto WaitForResult() However, it's unclear to me how this guarantees false start here. This is why documentation on CreateAndConnect* is important.
https://codereview.chromium.org/301283004/diff/2/net/socket/ssl_client_socket... File net/socket/ssl_client_socket_unittest.cc (right): https://codereview.chromium.org/301283004/diff/2/net/socket/ssl_client_socket... net/socket/ssl_client_socket_unittest.cc:722: void CreateAndConnectUpToServerFinished( On 2014/05/31 00:46:11, Ryan Sleevi wrote: > s/UpToServerFinished/UntilServerFinishedReceived? > > The "UpTo" bit read weird at first - I thought you were saying "ConnectUp" (as a > colloquial phrase). Hence the suggestion :) Done. https://codereview.chromium.org/301283004/diff/2/net/socket/ssl_client_socket... net/socket/ssl_client_socket_unittest.cc:725: FakeBlockingStreamSocket** out_raw_transport, On 2014/05/31 00:46:11, Ryan Sleevi wrote: > More documentation is needed here. My red alert alarms are going off on having a > FakeBlockingStreamSocket**, because it's not clear if ownership is transferred > (and this should be a scoped_ptr<FakeBlockingStreamSocket>* out_raw_socket) or > not. Documented. (The FakeBlockingStreamSocket annoyingly is owned by the SSLClientSocket, so it has to be a raw pointer.) > It's also not clear whether or not it's safe to call this before the test server > has been started - presumably not, given that you're using addr_ (line 728) Documented. https://codereview.chromium.org/301283004/diff/2/net/socket/ssl_client_socket... net/socket/ssl_client_socket_unittest.cc:776: client_config, &callback, &raw_transport, &sock); On 2014/05/31 00:46:11, Ryan Sleevi wrote: > DANGER: If you ever added an ASSERT_() to CreateAndConnectUpToServerFinished(), > your test will still continue, because it's wrapped within a function. > > As such, it would be good to wrap this call (and calls to this TestFalseStart > function) in ASSERT_NO_FATAL_FAILURE() before continuing. > > eg: ASSERT_NO_FATAL_FAILURE(CreateAndConnectUpToServerFinished(...)); Oh, so that's how you do it... done. https://codereview.chromium.org/301283004/diff/2/net/socket/ssl_client_socket... net/socket/ssl_client_socket_unittest.cc:781: int rv = callback.GetResult(ERR_IO_PENDING); On 2014/05/31 00:46:11, Ryan Sleevi wrote: > Just use > > int rv = callback.WaitForResult(); > > no need to fake it like this. Done. https://codereview.chromium.org/301283004/diff/2/net/socket/ssl_client_socket... net/socket/ssl_client_socket_unittest.cc:2547: EXPECT_EQ(OK, callback.GetResult(ERR_IO_PENDING)); On 2014/05/31 00:46:11, Ryan Sleevi wrote: > ditto WaitForResult() > > However, it's unclear to me how this guarantees false start here. This is why > documentation on CreateAndConnect* is important. Done. Also added a comment.
https://codereview.chromium.org/301283004/diff/30001/net/socket/ssl_client_so... File net/socket/ssl_client_socket_unittest.cc (right): https://codereview.chromium.org/301283004/diff/30001/net/socket/ssl_client_so... net/socket/ssl_client_socket_unittest.cc:670: scoped_ptr<SpawnedTestServer> test_server_; You don't actually need to expose these as protected (allowing mutation), do you? You could just expose getters for them. In the above cases, we mutate them in derived classes, but here, I think you're just exposing for visibility, right? If so, expose simple getters (const?) https://codereview.chromium.org/301283004/diff/30001/net/socket/ssl_client_so... net/socket/ssl_client_socket_unittest.cc:735: // Must be called after StartTestServer is called. I'm guessing to continue the handshake, the caller needs to ublock out_raw_transport? If so, worth documenting. https://codereview.chromium.org/301283004/diff/30001/net/socket/ssl_client_so... net/socket/ssl_client_socket_unittest.cc:797: int rv = callback.WaitForResult(); Should you first ASSERT_TRUE(callback.have_result())? Otherwise, don't you create the opportunity to deadlock here (as long as the blocking transport is, well, blocked?) https://codereview.chromium.org/301283004/diff/30001/net/socket/ssl_client_so... net/socket/ssl_client_socket_unittest.cc:2539: EXPECT_EQ(OK, callback.GetResult(sock2->Connect(callback.callback()))); ASSERT_TRUE(sock2.get())? https://codereview.chromium.org/301283004/diff/30001/net/socket/ssl_client_so... net/socket/ssl_client_socket_unittest.cc:2568: EXPECT_EQ(OK, callback.WaitForResult()); Is this meant to be testing false starting worked? if so, same comment as above re: have_result()
https://codereview.chromium.org/301283004/diff/30001/net/socket/ssl_client_so... File net/socket/ssl_client_socket_unittest.cc (right): https://codereview.chromium.org/301283004/diff/30001/net/socket/ssl_client_so... net/socket/ssl_client_socket_unittest.cc:670: scoped_ptr<SpawnedTestServer> test_server_; On 2014/06/02 20:52:45, Ryan Sleevi wrote: > You don't actually need to expose these as protected (allowing mutation), do > you? You could just expose getters for them. > > In the above cases, we mutate them in derived classes, but here, I think you're > just exposing for visibility, right? > > If so, expose simple getters (const?) Done. https://codereview.chromium.org/301283004/diff/30001/net/socket/ssl_client_so... net/socket/ssl_client_socket_unittest.cc:735: // Must be called after StartTestServer is called. On 2014/06/02 20:52:45, Ryan Sleevi wrote: > I'm guessing to continue the handshake, the caller needs to ublock > out_raw_transport? If so, worth documenting. Done. https://codereview.chromium.org/301283004/diff/30001/net/socket/ssl_client_so... net/socket/ssl_client_socket_unittest.cc:797: int rv = callback.WaitForResult(); On 2014/06/02 20:52:45, Ryan Sleevi wrote: > Should you first ASSERT_TRUE(callback.have_result())? Otherwise, don't you > create the opportunity to deadlock here (as long as the blocking transport is, > well, blocked?) It didn't end up being true for NSS. There's a comment although... actually I think it's totally wrong. I was very confused about nss_memio's behavior at time. I poked at it some and it's from the NSS task runner. The event loop iteration on the network task runner writes out the client second leg (and triggers WaitForWrite) is not the same event loop iteration where it learns the handshake is complete. (Core::DoTransportIO is called before Core::DoConnectCallback.) Certificate verification being potentially asynchronous would also prevent us from reliably asserting here I think. I've moved and updated the comment to not be false. https://codereview.chromium.org/301283004/diff/30001/net/socket/ssl_client_so... net/socket/ssl_client_socket_unittest.cc:2539: EXPECT_EQ(OK, callback.GetResult(sock2->Connect(callback.callback()))); On 2014/06/02 20:52:45, Ryan Sleevi wrote: > ASSERT_TRUE(sock2.get())? Done. https://codereview.chromium.org/301283004/diff/30001/net/socket/ssl_client_so... net/socket/ssl_client_socket_unittest.cc:2568: EXPECT_EQ(OK, callback.WaitForResult()); On 2014/06/02 20:52:45, Ryan Sleevi wrote: > Is this meant to be testing false starting worked? > > if so, same comment as above re: have_result() More just for waiting for the connect callback to complete. FalseStartEnabled should already test that false starting worked since it's the same test. But the case this tries to test is session resumption during the window between Connect() completing and server Finished actually being processed, so I want to wait for Connect() to complete first.
LGTM, % comment nit. https://codereview.chromium.org/301283004/diff/50001/net/socket/ssl_client_so... File net/socket/ssl_client_socket_unittest.cc (right): https://codereview.chromium.org/301283004/diff/50001/net/socket/ssl_client_so... net/socket/ssl_client_socket_unittest.cc:806: // the handshake returns. The comment about cert verification shouldn't exist here. See line 594 - we use a MockCertVerifier.
https://codereview.chromium.org/301283004/diff/50001/net/socket/ssl_client_so... File net/socket/ssl_client_socket_unittest.cc (right): https://codereview.chromium.org/301283004/diff/50001/net/socket/ssl_client_so... net/socket/ssl_client_socket_unittest.cc:806: // the handshake returns. On 2014/06/11 00:15:06, Ryan Sleevi wrote: > The comment about cert verification shouldn't exist here. See line 594 - we use > a MockCertVerifier. Done.
The CQ bit was checked by davidben@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/davidben@chromium.org/301283004/70001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
On 2014/06/11 20:40:11, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > win_chromium_rel on tryserver.chromium > (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) MSVC doesn't seem to like that it can't prove the raw pointer will be initialized. Initializing to NULL.
The CQ bit was checked by davidben@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/davidben@chromium.org/301283004/90001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) ios_rel_device on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/15623) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/18761)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) mac_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
The CQ bit was checked by davidben@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/davidben@chromium.org/301283004/90001
Message was sent while issue was closed.
Change committed as 276815
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/332523008/ by rouslan@chromium.org. The reason for reverting is: This patch may have broken http://build.chromium.org/p/chromium.mac/builders/Mac%2010.7%20Tests%20%28dbg..., but we're not confident. The failure is in libjingle_unittests, e.g. ChromeAsyncSocketTest.DoubleSSLConnect. Sorry for the inconvenience. . |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
