Make ssl_client_socket_unittest use a local server

ssl_client_socket_unittest.cc: launch local server with TestServerLauncher 
rather than use bugs.webkit.org, fixes TODO(darin)
Add tests with bad server certs

ssl_client_socket_nss.cc: fix bugs revealed by new tests

tcp_pinger.cc: helper class to do synchronous connect from tests.
Has to work inside ui tests where one can't use TestCompletionCallback.

ssl_test_util: renamed class TestServerLauncher, added Start/Stop methods.
Make part of net.lib to work around link error in test_shell_tests.

url_request_unittest.h: use TestServerLauncher to manage server.

SSL client tests disabled for now on Mac.

BUG=7114
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=9823

[dank, 2008-12-23 01:21:16]

Hi Avi,
could you have a look at the mac parts of this?
A few notes:

1) I'm still not handy with xcode (so my xcode project
change is suspect)
2) I don't know / don't remember how to install the 
certs on the mac, so SSL tests may fail
3) SSLTestUtils.VerifyServer() is failing on the mac,
and I haven't tracked that down yet.

Thanks,
Dan

[darin (slow to review), 2008-12-23 05:08:28]

ok, i see that you are trying to support blocking socket IO.  why do we need to
do that?  i think the implementation for windows is incomplete.  the IsConnected
method doesn't answer the question.  seems to me that we shouldn't try to
support blocking IO because that just complicates things.

http://codereview.chromium.org/16207/diff/222/232
File net/base/tcp_client_socket_unittest.cc (right):

http://codereview.chromium.org/16207/diff/222/232#newcode52
Line 52: TEST_F(TCPClientSocketTest, BlockingConnect) {
i don't think we implemented blocking connect on windows.

[darin (slow to review), 2008-12-23 05:09:28]

if in unit tests, you wish to block until a network operation completes, our
typical approach is to run a message loop until the completion callback fires. 
won't that work for you?

[Avi (use Gerrit), 2008-12-23 17:16:04]

http://codereview.chromium.org/16207/diff/222/227
File net/base/ssl_client_socket_unittest.cc (right):

http://codereview.chromium.org/16207/diff/222/227#newcode28
Line 28: void Start() {
It looks like we're calling this function from every test. Can we subclass the
setup and call it there rather than from each test?

http://codereview.chromium.org/16207/diff/222/228
File net/base/ssl_test_util.cc (right):

http://codereview.chromium.org/16207/diff/222/228#newcode105
Line 105: LoadTemporaryCert(GetRootCertPath()));
The equivalent call on the Mac is SecTrustCopyCustomAnchorCertificates /
SecTrustSetAnchorCertificates. But to do that you need the SecTrust, and that's
only around inside the calls. You could add a call like AddRootCert() to the
cert code to do this setup but I'll understand if you pass for now.

http://codereview.chromium.org/16207/diff/222/228#newcode143
Line 143: file_util::AppendToPath(&testserver_path, L"net");
Yes, definitely switch to FilePath here.

http://codereview.chromium.org/16207/diff/222/229
File net/base/ssl_test_util.h (right):

http://codereview.chromium.org/16207/diff/222/229#newcode27
Line 27: const std::wstring& document_root,
This is new code. Please use FilePath rather than wstring.

http://codereview.chromium.org/16207/diff/222/237
File net/url_request/url_request_unittest.cc (right):

http://codereview.chromium.org/16207/diff/222/237#newcode58
Line 58: public:
The old indentation was wrong but so is this. One space before the access
qualifier, and two for the rest.

http://codereview.chromium.org/16207/diff/222/237#newcode130
Line 130: HTTPSRequestTest() : util_() {};
And while you're at it, fix the indentation here too.

[Erik does not do reviews, 2008-12-23 17:21:21]

The trace_event.h change LGTM.

I didn't look at your overall CL, but I noticed that Rietveld is showing that
there are a lot of lint errors that you should deal with.

[ibrar, 2009-01-21 18:01:57]

http://codereview.chromium.org/16207/diff/826/830
File net/url_request/url_request_unittest.h (right):

http://codereview.chromium.org/16207/diff/826/830#newcode452
Line 452: // this shortcut could cause hangs?
Dank, I want to discuss it with you. Actually we cannot use same method as in
HTTPServer because FTP implementation require URLRequests should happen on the
same thread. And test case fail on url_request_inet_job.cc

#ifndef NDEBUG
  DCHECK(MessageLoop::current() == my_message_loop_) <<
      "All URLRequests should happen on the same thread";
#endif

That is the reason I have used this mechanism. I am currently looking at the
hang. If you have an idea please tell me.

[darin (slow to review), 2009-01-21 18:11:31]

The new FTP stack should not have that "single thread" limitation.  Perhaps we
can just focus the new test system on the new stack, or we could fix the old FTP
implementation to use a separate HINTERNET handle per thread.

[dank, 2009-01-23 19:31:53]

OK, ready for review again.

net_unittests --gtest_filter=SSLClientSocketTest.Connect
will fail on Mac and Win with CERT_AUTHORITY_INVALID
unless you have imported our test root cert, see
http://dev.chromium.org/developers/testing#TOC-SSL-tests

But even then it fails on the Mac with CERT_COMMON_NAME_INVALID, which seems
wrong.
Could be my bug, I haven't looked too hard yet.

Passes at least the tests I've tried on all three platforms.  The trybot says it
passes on Windows as well,
but fails on Mac because the cert isn't imported.
(I've had trouble getting a good trybot run on Linux
today, so I'm not sure it passes there yet.)

[dank, 2009-01-27 03:21:51]

This iteration of the change adds unit tests
with invalid certs to the lowest level ssl class,
and fixes the problems they reveal on Linux.

[darin (slow to review), 2009-01-27 19:57:03]

So, I was expecting to see some unification between the HTTPSTestServer and this
new mechanism to launch the SSL test server.  What happened to that?

I thought you could do away with the sync connect code if you just used the same
approach taken for MakeGETRequest.

[agl, 2009-01-27 23:45:08]

LGTM. Given the amount of time this has been dragging on for, the code review is
getting counter productive at this point. Further issues can be addressed in a
future CL.

[darin (slow to review), 2009-01-27 23:48:53]

I don't understand...  the comments I made before were seemingly ignored.  Some
times passed... and now we should just commit ignoring those comments?  What
gives?

[dank, 2009-01-27 23:51:11]

Darin asked:
> So, I was expecting to see some unification between 
> HTTPSTestServer and this new mechanism to launch the
> SSL test server.  What happened to that?

It's there.  The methods in url_request_unittest.h are still there,
but they're now wrappers around the ssl_test_util.h stuff.

> I thought you could do away with the sync connect code if you 
> just used the same approach taken for MakeGETRequest.

I did try, but I found it very difficult to grok the
async stuff mixed into the middle of the other code.
Breaking the async stuff out into a class with its own
unit test seemed much clearer and easier to test, and
the amount of code is the same.

Given AGL's LGTM, the only thing between me and checkin is
that the Mac side is failing; I guess I can mark the
failing tests as disabled on the Mac.

I will land and stand ready to revert.  This change will
likely run into some trouble in the real world even though
it has passed tests locally and on the trybots on Linux and Windows.

[dank, 2009-01-28 00:17:59]

Talked with Darin on the phone.  It turns out that his key
objection is that ClientSocketSyncWrapper breaks the rule
that all operations on a particular socket have to happen
on the same thread.  I'm happy to fix that, rather horrified
that I hadn't noticed myself. He's ok with me checking in and fixing
that later.  I will probably wait for WTC and Avi's reviews
first.  I'm going to be away from work for a few days so 
perhaps the commit will happen next week.  So all's well again,
I think.

[wtc, 2009-01-28 01:11:27]

I only reviewed the changes to
net/base/ssl_client_socket_nss.*,
net/base/ssl_client_socket_nss_unittest.cc
in this CL.

They look good to me.  Please review my suggested
changes and comments below.  Thank you for working
on this!

http://codereview.chromium.org/16207/diff/1248/1187
File net/base/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/16207/diff/1248/1187#newcode61
Line 61: case SEC_ERROR_UNTRUSTED_CERT:
SEC_ERROR_UNTRUSTED_CERT should be mapped to RR_CERT_AUTHORITY_INVALID.

http://codereview.chromium.org/16207/diff/1248/1187#newcode259
Line 259: X509Certificate::OSCertHandle nss_cert = SSL_PeerCertificate(nss_fd_);
I remember we asked on the NSS newsgroup and learned that
SSL_PeerCertificate only works during the SSL handshake.
So it would be nice to call SSL_PeerCertificate(nss_fd_)
at the right time and store its return value in a class
member server_cert_.  This would allow us to call GetSSLInfo
at any time.

http://codereview.chromium.org/16207/diff/1248/1187#newcode419
Line 419: SECStatus SSLClientSocketNSS::OwnBadCertHandler(void* arg, PRFileDesc*
socket) {
Add the comment
  // static
above this line.  We use this convention for definitions of
static methods.

http://codereview.chromium.org/16207/diff/1248/1187#newcode420
Line 420: SSLClientSocketNSS *that = reinterpret_cast<SSLClientSocketNSS
*>(arg);
Nit: use the convention Type* instead of Type * to be consistent.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Pointer_and_Re...

http://codereview.chromium.org/16207/diff/1248/1188
File net/base/ssl_client_socket_nss.h (right):

http://codereview.chromium.org/16207/diff/1248/1188#newcode11
Line 11: #include <nspr.h>
Nit: <nspr.h> is a convenience header that includes all
NSPR headers.  So including <nspr.h> makes it unnecessary
to include <prio.h>.

Note: If we're going for including only the absolutely
necessary headers, <nspr.h> should not be included.  But
I think it is fine to use <nspr.h>.

http://codereview.chromium.org/16207/diff/1248/1188#newcode81
Line 81: int server_cert_net_err_;
Nit: how about server_cert_net_error_ or server_cert_error_?

It seems that you only need to store the net error, and can
compute the cert status on demand (inside GetSSLInfo).

http://codereview.chromium.org/16207/diff/1248/1188#newcode105
Line 105: static SECStatus OwnBadCertHandler(void* arg, PRFileDesc* socket);
Nit: the style guide says methods should be listed before
data members.
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order

http://codereview.chromium.org/16207/diff/1248/1189
File net/base/ssl_client_socket_unittest.cc (right):

http://codereview.chromium.org/16207/diff/1248/1189#newcode36
Line 36: void StartWrongServer() {
Nit: Add a short comment to explain "Wrong" means "wrong host name".

We usually call this certificate problem "name mismatch".

http://codereview.chromium.org/16207/diff/1248/1189#newcode108
Line 108: if (rv != net::OK) {
In this test and the ConnectWrong test below, we also
need to assert that sock->Connect doesn't return net::OK.

http://codereview.chromium.org/16207/diff/1248/1189#newcode115
Line 115: EXPECT_TRUE(sock->IsConnected());
As you explained to me in private email, this is a property
of the current Windows implementation, and is not required
by our SSLCLientSocket API.  Would be nice to note this with
a short comment.

http://codereview.chromium.org/16207/diff/1248/1189#newcode131
Line 131: server_.kWrongHostName, kSSLConfig));
Is this line longer than 80 characters?

http://codereview.chromium.org/16207/diff/1248/1189#newcode146
Line 146: // bug 1354783
Remove this comment, which notes why these tests were
disabled.

http://codereview.chromium.org/16207/diff/1248/1189#newcode164
Line 164: kSSLConfig));
Nit: can this fit on the previous line?

http://codereview.chromium.org/16207/diff/1248/1189#newcode198
Line 198: // bug 1354783
Remove this comment about why this test was disabled.

http://codereview.chromium.org/16207/diff/1248/1189#newcode245
Line 245: // bug 1354783
Remove this comment.

[dank, 2009-02-10 22:43:20]

Sorry this is taking so dang long.

Avi: please review mac portion.  (It would be nice to enable these tests on the
Mac, but I couldn't figure out how to import the cert manually.)

Darin: please check TCPPinger at least, that's the
part you always object to, I think I've addressed
your comments.  Not sure what to do about the race 
there.

Wan-Teh: please check ssl_client_socket_nss*, I think
I've addressed your comments.

[Avi (use Gerrit), 2009-02-10 22:56:23]

http://codereview.chromium.org/16207/diff/3164/2153
File net/base/client_socket_test_helper_unittest.cc (right):

http://codereview.chromium.org/16207/diff/3164/2153#newcode27
Line 27: // to accept a connection until the client actually writes!
That's surprising; I've thought in the past that I'd found bugs like that only
to find the problem in my own code. You sure?

http://codereview.chromium.org/16207/diff/3164/2162
File net/net.xcodeproj/project.pbxproj (right):

http://codereview.chromium.org/16207/diff/3164/2162#newcode1559
Line 1559: 07B79D4A0F4221D7001EA432 /* ssl_test_util.cc in Sources */,
This is not alphabetized, and is added twice (!)

http://codereview.chromium.org/16207/diff/3164/2162#newcode1615
Line 1615: 07B79D490F4221D7001EA432 /* ssl_test_util.cc in Sources */,
Alphabetize.

http://codereview.chromium.org/16207/diff/3164/2164
File net/url_request/url_request_unittest.cc (right):

http://codereview.chromium.org/16207/diff/3164/2164#newcode133
Line 133: // ssl_client_socket_mac.cc doesn't handle bad certs well yet
Why indent?

http://codereview.chromium.org/16207/diff/3164/2164#newcode133
Line 133: // ssl_client_socket_mac.cc doesn't handle bad certs well yet
The old comment seems clearer. What about certs on the Mac isn't "well"?

[wtc, 2009-02-11 00:17:35]

LGTM on ssl_client_socket_nss*.

I'll try to review some of the other files.

[Avi (use Gerrit), 2009-02-11 00:18:31]

lgtm.

The crash is really disappointing; we need to look into that.

http://codereview.chromium.org/16207/diff/3180/2209
File net/net.xcodeproj/project.pbxproj (right):

http://codereview.chromium.org/16207/diff/3180/2209#newcode894
Line 894: 07FE37F00F424D9F00049AB8 /* client_socket_test_helper_unittest.cc */,
alphabetical?

[dank, 2009-02-12 00:21:34]

Took care of Avi's issue.  Waiting for Darin's review with bated breath.

[darin (slow to review), 2009-02-12 03:58:03]

http://codereview.chromium.org/16207/diff/3192/3208
File base/platform_thread.h (right):

http://codereview.chromium.org/16207/diff/3192/3208#newcode47
Line 47: static void Sleep(base::TimeDelta duration) {
IIRC, paulg tried this once, and it caused windows build problems.  It added a
dependency on timeGetTime, which means a new dependency on winmm.dll for
chrome.exe.  In an effort to keep chrome.exe fast, we decided not to take on
that dependency.

I think you would only notice a problem after checking in.  There's a buildbot
step that complains if chrome.exe takes on new implicit DLL dependencies IIRC.

http://codereview.chromium.org/16207/diff/3192/3193
File net/base/client_socket_test_helper.h (right):

http://codereview.chromium.org/16207/diff/3192/3193#newcode27
Line 27: sock_(new TCPClientSocket(addr)),
this still has the same issue i mentioned previously.  the TCPClientSocket is
being allocated on one thread, but then used on another.  you should just
allocate sock_ inside of DoConnect, and then destroy it inside DoDisconnnect.

http://codereview.chromium.org/16207/diff/3192/3193#newcode50
Line 50: event_.Wait();
doesn't this block forever?  what if Connect is slow?  i guess you rely on the
buildbot killing the unit test executable if it is too slow?  another option
would be to use TimedWait.

http://codereview.chromium.org/16207/diff/3192/3193#newcode51
Line 51: if (net_error_ == ERR_IO_PENDING)
i don't understand how net_error_ would ever equal ERR_IO_PENDING :(

maybe you meant to sleep if net_error_ is not OK?  i.e., if we had a connect
failure, then we should sleep before trying again, right?

http://codereview.chromium.org/16207/diff/3192/3193#newcode56
Line 56: event_.Wait();
this one is fine.

http://codereview.chromium.org/16207/diff/3192/3193#newcode64
Line 64: // Valgrind on laptop, 100ms enough for core 2 duo)
one solution is to use a custom Task subclass.  because you are going to call
event_.Wait() on this thread, you don't need the Task to have a reference to
|this|.  it can just be an ordinary back pointer.

i'm not sure what DCHECK you are hitting, but the usual pattern for a class like
this is to make the reference counted bit be an inner class.  that way we can
have control over when the references are released.  using an inner class also
means that the class your consumer deals with can just be a normally stack
allocated class.

class TCPPinger {
 public:
  ...

 private:
  class Context : public RefCountedThreadSafe<Context> {
  };
  Context* context_;  // holds a strong reference
  Thread io_thread_;
};

The nice thing about this design is that you can do things like use ReleaseLater
to ensure that TCPPinger's reference to context_ gets released on the io thread.
 That allows you to know that Context will not be destroyed on a random thread,
which usually helps make everything else more predictable.

You'll see this pattern used a lot in chrome.

http://codereview.chromium.org/16207/diff/3192/3193#newcode76
Line 76: if (rv != ERR_IO_PENDING) {
it is actually possible for Connect to complete synchronously, although it is
rather unlikely.  it seems like you should just call ConnectDone(rv) if rv !=
ERR_IO_PENDING.  that covers all cases nicely.

[dank, 2009-02-12 22:21:28]

OK, all issues addressed (one just by a comment)
except for ReleaseLater, which I couldn't find; 
do I need that?

As I was adding the Context class into TCPPinger,
I realized it was really a bit more than just
data, so I moved some methods in there and called
it a Worker.  I like it, thanks for the suggestion.

[darin (slow to review), 2009-02-13 02:48:15]

much better... some minor issues:

http://codereview.chromium.org/16207/diff/2296/3267
File net/base/client_socket_test_helper.h (right):

http://codereview.chromium.org/16207/diff/2296/3267#newcode48
Line 48: // Timeout here only to handle rare case of async connect blocking.
async connect never "blocks" (meaning never blocks the calling thread).  rather
it could just take a long time to complete in some unfortunate cases.

http://codereview.chromium.org/16207/diff/2296/3267#newcode67
Line 67: class Worker : public base::RefCountedThreadSafe<Worker> {
this inner class can be private right?

http://codereview.chromium.org/16207/diff/2296/3267#newcode78
Line 78: if (sock_.get() != NULL)
this explicit call to Disconnect is unnecessary.  sock_.reset(...) will destroy
the old socket, which will implicitly disconnect it.

http://codereview.chromium.org/16207/diff/2296/3267#newcode85
Line 85: sock_->Disconnect();
the contents of this block are basically identical to ConnectDone.  why not just
call ConnectDone to reuse code?

it is probably a slight bug that the two are not identical since Connect
returning something other than ERR_IO_PENDING means that Connect completed
synchronously.

http://codereview.chromium.org/16207/diff/2296/3267#newcode102
Line 102: base::WaitableEvent event_;
nit: google style says that these should be private.  there should be public
const getters, event() and net_error(), instead.

http://codereview.chromium.org/16207/diff/2296/3267#newcode114
Line 114: scoped_refptr<Worker> worker_;
you should probably change this to a raw pointer, and then add a ~TCPPinger that
uses io_thread_->message_loop()->ReleaseSoon(worker_).  that way the worker_
will get destroyed on the IO thread, and as a result, ~TCPClientSocket will run
on the IO thread.

it is also sometimes nice to keep the scoped_refptr so you don't have to
explicitly call AddRef.  in that case, the swap method is handy.

this code could go in your ~TCPPinger:

  Worker* doomed = NULL;
  worker_.swap(&doomed);
  io_thread_->message_loop()->ReleaseSoon(FROM_HERE, doomed);

[dank, 2009-02-13 21:43:03]

Sorry 'bout that TimedWait bit, I was obviously insane.
All should be fixed, please re-re-re-hic-re-review.

The windows trybot failed in PrintingLayoutTestHidden.*
but I can't reproduce the problem locally, maybe
I'll ask the trybot to rerun.

[darin (slow to review), 2009-02-13 23:43:15]

LGTM

http://codereview.chromium.org/16207/diff/2317/3332
File net/base/client_socket_test_helper.h (right):

http://codereview.chromium.org/16207/diff/2317/3332#newcode23
Line 23: class TCPPinger {
looks good.  please change this file to be named tcp_pinger.h to match the name
of the class :)

http://codereview.chromium.org/16207/diff/2317/3332#newcode110
Line 110: int WaitForResult() {
you never use this return value, but that doesn't seem to be a problem.