Port SSLClientSocket to Linux

Port SSLClientSocket to Linux
Passes tests (once you enable them by removing DISABLED_).
Probably want to add a mock https server so we can leave those tests enabled when we check in.

Had to add full duplex support to TCPClientSocket on Linux
to avoid kludgy plumbing issues.
Also had to add dummy implementation of 
X509Certificate::~X509Certificate to prevent link error.

Rediffed to current trunk, addressed all review issues. 


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

[dank, 2008-10-06 05:35:02]

I need to go over the code once or twice more to
try to make it clear, but it is passing all its
tests now (once you enable them and set up an https
server on localhost).

[darin (slow to review), 2008-10-06 06:18:22]

http://codereview.chromium.org/4049/diff/823/628
File net/base/ssl_client_socket.h (right):

http://codereview.chromium.org/4049/diff/823/628#newcode38
Line 38: class SSLClientSocket : public ClientSocket {
ClientSocketFactory means that consumers do not allocate SSLClientSocket
directly.  perhaps it would be better to have ssl_client_socket_schannel.{h,cc}
and ssl_client_socket_nss.{h,cc}.  Then, the factory can have the #ifdefs to
choose the correct implementation per platform.

However, I think we may need to turn SSLClientSocket into an interface or we may
need to add another method to ClientSocket to provide consumers with a way to
get SSLInfo.  I think making SSLCientSocket an interface with a pure virtual
GetSSLInfo method is probably a reasonable way to go.

[Dean McNamee, 2008-10-06 09:34:24]

I realize the comments say you wrote in NSPR style on purpose, but that's sort
of a bummer because it makes it seem super foreign in our code.  I just quickly
looked things over.  I would name the memio stuff to nss_memio or something with
SSL in the name.  Also, could memio and memio_buffer just be together?

http://codereview.chromium.org/4049/diff/823/629
File net/base/memio.h (right):

http://codereview.chromium.org/4049/diff/823/629#newcode4
Line 4: // Written in NSPR style to also be suitable for adding to the NSS demo
suite
A comment here explaining what this does would be great.  Also, this looks very
NSS specific, should this be nss_memio.{h,cc} ?

[dank, 2008-10-06 22:24:27]

OK, I renamed memio.{h,c} to nss_memio.{h,c}, 
figured out how to hide memio_buffer_* behind four simpler-to-use
functions,
sucked memio_buffer.{h,c} into nss_memio.c,
added some doc at the top of memio.h,
and fixed problems in memio_close and memio_CreateIOLayer caught a while ago by
wtc.

I gather Darin would like to make nss_client_socket.h 
define just a superclass; I have to pause to do some administrivia,
but that's where

[wtc, 2008-10-07 00:07:59]

I haven't reviewed nss_memio.{h,cc} and
ssl_client_socket* yet.  I'd like to send
you these review comments today.

http://codereview.chromium.org/4049/diff/649/658
File net/base/client_socket.h (right):

http://codereview.chromium.org/4049/diff/649/658#newcode45
Line 45: // Identical to posix system call of same name
Nit: add a period (.) at the end of sentences in comments.

http://codereview.chromium.org/4049/diff/649/658#newcode47
Line 47: virtual int getpeername(struct sockaddr *name, socklen_t *namelen) {
Should we spell this method GetPeerName?

http://codereview.chromium.org/4049/diff/649/658#newcode48
Line 48: // default implementation simply fails
Nit: capitalize the first word of a sentence in comments.

http://codereview.chromium.org/4049/diff/649/658#newcode49
Line 49: return -1;
We should return a net error code instead of -1.
For example, ERR_UNEXPECTED.  -1 is ERR_IO_PENDING,
which is definitely wrong here.

http://codereview.chromium.org/4049/diff/649/655
File net/base/ssl_client_socket.h (right):

http://codereview.chromium.org/4049/diff/649/655#newcode17
Line 17: #include <nspr/prio.h>
We should figure out a way to allow us to say
#include <prio.h>.  The reason is that some Linux
distributions install the NSPR headers in
/usr/include/nspr4 or /usr/include/nspr-4, so
<nspr/prio.h> won't work.

http://codereview.chromium.org/4049/diff/649/652
File net/base/tcp_client_socket.h (right):

http://codereview.chromium.org/4049/diff/649/652#newcode56
Line 56: // duplex mode (reading and writing at the same time) is not supported.
This comment needs to be updated now.

http://codereview.chromium.org/4049/diff/649/652#newcode105
Line 105: char* write_buf_;
Should write_buf_ be const char* ?

http://codereview.chromium.org/4049/diff/649/652#newcode111
Line 111: void DoCallbackWrite(int rv);
DoWriteCallback sounds better?

http://codereview.chromium.org/4049/diff/649/652#newcode117
Line 117: CompletionCallback* callback_;
For Linux, callback_ is not used for write.

http://codereview.chromium.org/4049/diff/649/651
File net/base/tcp_client_socket_libevent.cc (right):

http://codereview.chromium.org/4049/diff/649/651#newcode73
Line 73: callback_(NULL) {
Good catch.  I will make the same fix in tcp_client_socket.cc.

http://codereview.chromium.org/4049/diff/649/651#newcode166
Line 166: DLOG(INFO) << "Read failed, errno " << errno;
Nit: lowercase 'read' to make it clear that it's the read
system call that failed.

http://codereview.chromium.org/4049/diff/649/651#newcode199
Line 199: write_buf_ = const_cast<char*>(buf);
You can remove the const_cast if you declare write_buf_
as const char*.

http://codereview.chromium.org/4049/diff/649/651#newcode231
Line 231: // since Run may result in Read being called, clear callback_ up
front.
The comment should say Write instead of Read and
write_callback_ instead of callback_.

http://codereview.chromium.org/4049/diff/649/651#newcode285
Line 285: buf_ = NULL;
Let's also reset buf_len_ to 0.  (I don't think it's
necessary to reset buf_ and buf_len_ though.)

http://codereview.chromium.org/4049/diff/649/651#newcode293
Line 293: bytes_transferred = write(socket_, buf_, buf_len_);
You should use write_buf_ and write_buf_len_ here.
I guess write() usually succeeds synchronously, which
is why you haven't discovered this bug in testing :-)

http://codereview.chromium.org/4049/diff/649/651#newcode303
Line 303: MessageLoopForIO::current()->UnwatchSocket(event_.get());
If you decide to reset buf_ and buf_len_ in DidCompleteRead,
you should reset write_buf_ and write_buf_len_ here.

http://codereview.chromium.org/4049/diff/649/651#newcode309
Line 309: // wait_state_ should be a boolean called wait_connect_, or something
I agree.  Using NOT_WAITING in this switch statement looks
strange.

http://codereview.chromium.org/4049/diff/649/651#newcode315
Line 315: if ((flags & EV_WRITE) && write_callback_)
Are EV_READ and EV_WRITE the only bits that may be
set in 'flags'?

http://codereview.chromium.org/4049/diff/649/651#newcode327
Line 327: {
This opening brace { should be at the end of the previous line.

[Avi (use Gerrit), 2008-10-07 21:42:24]

http://codereview.chromium.org/4049/diff/649/652
File net/base/tcp_client_socket.h (right):

http://codereview.chromium.org/4049/diff/649/652#newcode31
Line 31: // The Linux implementation supports full duplex because that
The Libevent implementation... (since it's not Linux specific)

[Avi (use Gerrit), 2008-10-07 21:46:45]

http://codereview.chromium.org/4049/diff/649/651
File net/base/tcp_client_socket_libevent.cc (right):

http://codereview.chromium.org/4049/diff/649/651#newcode156
Line 156: DCHECK(!callback_);
If we're going for full-duplex, we have a problem here. The other operation in
flight will likely have a callback of its own, so this DCHECK will fire.

[wtc, 2008-10-07 23:54:45]

Here are the review comments on nss_memio.{h,c}
and ssl_client_socket.h.

I will review ssl_client_socket_nss.cc and
ssl_client_socket_unittests.cc next.

http://codereview.chromium.org/4049/diff/649/657
File net/base/nss_memio.c (right):

http://codereview.chromium.org/4049/diff/649/657#newcode9
Line 9: // want to let NSS do its own networking.
These four lines should use C comment delimiters /* */
in preparation for eventual inclusion in NSS.

http://codereview.chromium.org/4049/diff/649/657#newcode11
Line 11: #include <assert.h>
In NSS/NSPR, we use PR_ASSERT(), defined in <prlog.h>, which
this file is already including.

http://codereview.chromium.org/4049/diff/649/657#newcode58
Line 58: #define min(a,b) (((a) < (b)) ? (a) : (b))
You can use PR_MIN, defined in <prtypes.h>, which every
NSPR header includes (so you don't need to include it).

http://codereview.chromium.org/4049/diff/649/657#newcode60
Line 60: #if 0
Why is this block of code commented out?

http://codereview.chromium.org/4049/diff/649/657#newcode105
Line 105: mb->buf = NULL;
I suggest resetting the other three fields also.

http://codereview.chromium.org/4049/diff/649/657#newcode116
Line 116: /* How many bytes can be read out of the buffer without wrapping */
Would be nice to use the word "contiguous" in this comment.
It took me a few seconds to figure out what "contig" means.

http://codereview.chromium.org/4049/diff/649/657#newcode155
Line 155: if (len) {
It took me a while to verify that if
memio_buffer_unused_contig(mb) is 0, the buffer is full.

http://codereview.chromium.org/4049/diff/649/657#newcode158
Line 158: if (mb->tail >= mb->bufsize)
mb->tail cannot be > mb->bufsize.

http://codereview.chromium.org/4049/diff/649/657#newcode169
Line 169: if (mb->tail >= mb->bufsize)
Same comment: mb->tail cannot be > mb->bufsize.

http://codereview.chromium.org/4049/diff/649/657#newcode228
Line 228: static int PR_CALLBACK memio_recv(PRFileDesc* fd, void* buf, PRInt32
len,
Nit: put * before the parameter names.  (All the NSPR I/O
methods in this file have this problem.)

http://codereview.chromium.org/4049/diff/649/657#newcode244
Line 244: if (rv == 0 && ! secret->eof) {
Nit: no space after "!".

http://codereview.chromium.org/4049/diff/649/657#newcode285
Line 285: memcpy(addr, &secret->peername, sizeof(PRNetAddr));
You can also just do a struct (or rather, union) copy:

    *addr = secret->peername;

http://codereview.chromium.org/4049/diff/649/657#newcode297
Line 297: {
Nit: put the { on the previous line.

http://codereview.chromium.org/4049/diff/649/657#newcode317
Line 317: memio_close,
Nit: Perhaps these IO method names should be spelled
memio_Close, memio_Read, memio_Write, memio_Shutdown,
memio_Recv, memio_Send, and memio_GetPeerName.

http://codereview.chromium.org/4049/diff/649/657#newcode354
Line 354: static PRDescIdentity memio_identity = -1;
Use PR_INVALID_IO_LAYER instead -1.  Same value, but more
descriptive.

http://codereview.chromium.org/4049/diff/649/657#newcode378
Line 378: fd->secret = (PRFilePrivate *)secret;
This (PRFilePrivate *) cast is now unnecessary.

http://codereview.chromium.org/4049/diff/649/657#newcode386
Line 386: memcpy(&secret->peername, peername, sizeof(PRNetAddr));
You can say:
    secret->peername = *peername;

http://codereview.chromium.org/4049/diff/649/657#newcode389
Line 389: void memio_SetEOF(PRFileDesc *fd, PRBool eof)
This function is not used.

http://codereview.chromium.org/4049/diff/649/657#newcode423
Line 423: }
What if bytes_read is -1?

http://codereview.chromium.org/4049/diff/649/657#newcode441
Line 441: mb->head += bytes_written;
Nit: indent by 4.

http://codereview.chromium.org/4049/diff/649/657#newcode465
Line 465: struct memio_buffer mb;
Nit: indent by 4.

http://codereview.chromium.org/4049/diff/649/656
File net/base/nss_memio.h (right):

http://codereview.chromium.org/4049/diff/649/656#newcode4
Line 4: // Written in NSPR style to also be suitable for adding to the NSS demo
suite
It is a good idea to include this file in NSS as a demo of
the state machine technique.

http://codereview.chromium.org/4049/diff/649/656#newcode20
Line 20: 2) call NSS_ImportFD to ssl-ify it the socket
Nit: remove "it".

http://codereview.chromium.org/4049/diff/649/656#newcode20
Line 20: 2) call NSS_ImportFD to ssl-ify it the socket
NSS_ImportFD => SSL_ImportFD

http://codereview.chromium.org/4049/diff/649/656#newcode24
Line 24: usual to the nspr file descriptor returned by NSS_ImportFD,
NSS_ImportFD => SSL_ImportFD

http://codereview.chromium.org/4049/diff/649/656#newcode45
Line 45: struct PRFilePrivate *memio_GetSecret(PRFileDesc *fd);
You don't need 'struct' before PRFilePrivate because of
this typedef in "prio.h":
typedef struct PRFilePrivate    PRFilePrivate;

http://codereview.chromium.org/4049/diff/649/656#newcode47
Line 47: /* Ask memio what data it has to send to the network.
Is "send to the network" wrong?  Should that be
"Ask memio where to put bytes from the network, and how many it can handle."?

http://codereview.chromium.org/4049/diff/649/656#newcode53
Line 53: /* Tell memio how many bytes were sent to the network.
"sent to the network" => "read from the network"?

http://codereview.chromium.org/4049/diff/649/656#newcode55
Line 55: * NSS when it reads from the circular buffer.
If the read from the network fails, how to you propagate
that info to NSS?

http://codereview.chromium.org/4049/diff/649/656#newcode59
Line 59: /* Ask memio where to put bytes from the network, and how many it can
handle.
Should this be "what data it has to send to the network"?

http://codereview.chromium.org/4049/diff/649/656#newcode63
Line 63: int memio_GetWriteParams(struct PRFilePrivate *secret, char **buf);
Perhaps const char **buf?

http://codereview.chromium.org/4049/diff/649/655
File net/base/ssl_client_socket.h (right):

http://codereview.chromium.org/4049/diff/649/655#newcode101
Line 101: bool completed_handshake_;
We should create platform-specific versions of
this header file.  The ifdefs make the code hard to
read.

We often list the bool members together so that the compiler
can pack them.  The platform ifdefs will get in the way for
this space optimization.

http://codereview.chromium.org/4049/diff/649/655#newcode160
Line 160: STATE_HANDSHAKE_READ,
I'm surprised that you don't need a STATE_HANDSHAKE_WRITE
state.

http://codereview.chromium.org/4049/diff/649/655#newcode167
Line 167: PRFileDesc *nss_fd_;
Nit: put * after the type.

http://codereview.chromium.org/4049/diff/649/655#newcode170
Line 170: struct PRFilePrivate *nss_bufs_;
Nit: Put * after the type.

It's better to NOT use PRFilePrivate in a header
because PRFilePrivate may have multiple definitions
in different .cc files.

http://codereview.chromium.org/4049/diff/649/655#newcode172
Line 172: static bool nss_initialized_;
Please include "base/nss_init.h" and use
base::EnsureNSSInit() instead.

[wtc, 2008-10-09 00:03:00]

OK, finally finished the review.  There are quite a few
issues, but it's impressive work.  I think it is better to
just clean it up a little and check it in with some
outstanding problems.  Once the code is in the source tree,
it'll be easier for others to review future iterations or
help fix the remaining issues.

I also suggest that we implement Darin's suggestion to
avoid the ugly ifdefs in the ssl_client_socket.h header:

- Make SSLClientSocket an interface (abstract class).
- On each platform, we implement a derived class of
  SSLClientSocket.

So the current SSLClientSocket would become SSLClientSocketWin, and the Linux
implementation
would be SSLClientSocketNSS.

http://codereview.chromium.org/4049/diff/649/654
File net/base/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/4049/diff/649/654#newcode15
Line 15: #include <nss/pk11pub.h>
These four headers should be listed right after
"net/base/ssl_client_socket.h" because these are considered
C system headers.

As I noted before, we should figure out the necessary build
system changes so that we can include <nspr.h>, <nss.h>,
etc. because in some Linux distributions, these headers
are installed in /usr/include/nspr4 and /usr/include/nss3.

http://codereview.chromium.org/4049/diff/649/654#newcode17
Line 17: namespace net {
Nit: Start the net namespace from the SSLClientSocket constructor.

http://codereview.chromium.org/4049/diff/649/654#newcode29
Line 29: //static const int kRecvBufferSize = (5 + 16*1024 + 64);
You should delete this block comment, which only makes
sense for Windows' Schannel library.

http://codereview.chromium.org/4049/diff/649/654#newcode36
Line 36: static char *ownPasswd( PK11SlotInfo *slot, PRBool retry, void *arg)
This method should be reformatted to follow Google's
coding style.

http://codereview.chromium.org/4049/diff/649/654#newcode49
Line 49: */
Nit: use C++ // comment delimiters for this block comment.

http://codereview.chromium.org/4049/diff/649/654#newcode51
Line 51: ownBadCertHandler(void * arg, PRFileDesc * socket)
This method should be reformatted to follow Google's
coding style.

http://codereview.chromium.org/4049/diff/649/654#newcode63
Line 63: buffer_send_callback_(this, &SSLClientSocket::BufferSendComplete),
Nit: This should be moved to the previous line, after the colon.

http://codereview.chromium.org/4049/diff/649/654#newcode83
Line 83: pthread_mutex_t init_mutex = PTHREAD_MUTEX_INITIALIZER;
Nit: This should be made 'static' and moved outside the net
namespace.  Alternatively, use pthread_once.

http://codereview.chromium.org/4049/diff/649/654#newcode88
Line 88: int rv = SECSuccess;
Nit: use SECStatus instead of int.

http://codereview.chromium.org/4049/diff/649/654#newcode90
Line 90: PR_Init( PR_SYSTEM_THREAD, PR_PRIORITY_NORMAL, 1);
Use PR_Init(PR_USER_THREAD, PR_PRIORITY_NORMAL, 0).
(I know you copied this from tstclnt.c.  I'll fix that.)
See http://developer.mozilla.org/en/PR_Init.

http://codereview.chromium.org/4049/diff/649/654#newcode95
Line 95: rv = NSS_Init(nssdir);
We should use NSS_NoDB_Init(NULL) for now, until we need
to implement SSL client authentication support.

http://codereview.chromium.org/4049/diff/649/654#newcode109
Line 109: SSL_OptionSetDefault(SSL_ENABLE_TLS, PR_TRUE);
If TLS is enabled, add
    SSL_OptionSetDefault(SSL_ENABLE_SESSION_TICKETS, PR_TRUE)

http://codereview.chromium.org/4049/diff/649/654#newcode118
Line 118: return ERR_UNEXPECTED;
We should add an error code to report failure to initialize
the crypto library.  At least add a TODO comment as a reminder.

http://codereview.chromium.org/4049/diff/649/654#newcode207
Line 207: */
Nit: Format this block comment using C++ //.

http://codereview.chromium.org/4049/diff/649/654#newcode209
Line 209: if (transport_send_busy_) return ERR_IO_PENDING;
Nit: put the return statement on the next line to
be consistent with the style in the net module.

http://codereview.chromium.org/4049/diff/649/654#newcode295
Line 295: int nreceived = BufferRecv();
OK, this is why you need TCPClientSocket to be full duplex.
While memio's write buffer being nonempty implies we should
send data to the transport, memio's read buffer being empty
doesn't mean we need to read data from the transport.  For
example, while we're sending an HTTP request to the server,
the memio's read buffer is empty, but we don't need to read
until we are ready to read the server's response.

The solution is to use the 'poll' method, at least during
SSL handshake, to tell whether you need to call
BufferRecv or BufferSend.

http://codereview.chromium.org/4049/diff/649/654#newcode312
Line 312: NOTREACHED() << "Couldn't initialize nss";
The NOTREACHED() assertion should be backed by
a return statement in the release build.  You can
just return Init's return value.

http://codereview.chromium.org/4049/diff/649/654#newcode319
Line 319: return 9999;  // TODO(port): real error
Since all the error codes are negative, 9999 cannot be
an error code.  You can use ERR_FAILED as a placeholder.

http://codereview.chromium.org/4049/diff/649/654#newcode339
Line 339: return ERR_SSL_PROTOCOL_ERROR;  // TODO(port): real error
ERR_SSL_PROTOCOL_ERROR is inappropriate here because
we haven't done any SSL protocol yet.

http://codereview.chromium.org/4049/diff/649/654#newcode385
Line 385: next_state_ = STATE_PAYLOAD_READ;
When you're returning ERR_IO_PENDING, you shouldn't need
to set next_state_.  I know you're setting next_state_
here because network_moved may be true in DoLoop.  I hope
we can come up with a clearer way to accomplish that.

http://codereview.chromium.org/4049/diff/649/654#newcode394
Line 394: if (rv >= 0)
You need to handle the case where rv > 0 but rv < user_buf_len_.

You need to arrange for writing the remaining bytes
in user_buf_.

http://codereview.chromium.org/4049/diff/649/654#newcode395
Line 395: return rv;
You can't just return a positive byte count here.
Suppose rv is 2048.  We return 2048 to DoLoop.
DoLoop calls BufferSend.  Suppose BufferSend returns
ERR_IO_PENDING.  DoLoop breaks out of the while loop
and returns 2048, which is wrong because the transport
is still trying to send the 2048 bytes.

http://codereview.chromium.org/4049/diff/649/653
File net/base/ssl_client_socket_unittest.cc (right):

http://codereview.chromium.org/4049/diff/649/653#newcode25
Line 25: #define HOSTNAME "localhost"
How do these tests work?  Do you run a web server
manually on the localhost?

[wtc, 2008-10-09 00:10:14]

Dan, I can create a CL to turn SSLClientSocket into
an interface, and then you merge your CL with it.  If you
think that'll be less work for you, I'll be happy to do
that.

[Avi (use Gerrit), 2008-10-10 18:53:58]

On 2008/10/09 00:10:14, wtc wrote:
> Dan, I can create a CL to turn SSLClientSocket into
> an interface, and then you merge your CL with it.  If you
> think that'll be less work for you, I'll be happy to do
> that.

Keep me in the loop here; I have an 90% complete Mac version.

[Avi (use Gerrit), 2008-10-10 19:00:58]

http://codereview.chromium.org/4049/diff/649/653
File net/base/ssl_client_socket_unittest.cc (right):

http://codereview.chromium.org/4049/diff/649/653#newcode87
Line 87: EXPECT_EQ(static_cast<int>(arraysize(request_text) - 1), rv);
Since arraysize is a size_t, can you cast to an ssize_t rather than int?

[Avi (use Gerrit), 2008-10-10 19:26:47]

http://codereview.chromium.org/4049/diff/649/654
File net/base/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/4049/diff/649/654#newcode183
Line 183: memset(ssl_info, 0, sizeof(ssl_info));
sizeof(SSLInfo) not the sizeof the pointer (thanks mment)

[dank, 2008-10-16 22:32:27]

Ready for review again.  I think I've addressed everybody's issues.  The code is
now based on the abstract SSLClientSocket.

http://codereview.chromium.org/4049/diff/649/651
File net/base/tcp_client_socket_libevent.cc (right):

http://codereview.chromium.org/4049/diff/649/651#newcode156
Line 156: DCHECK(!callback_);
The other operation in flight has its own callback variable (write_callback_),
so the DCHECK shouldn't fire... right?

[Avi (use Gerrit), 2008-10-17 14:32:00]

http://codereview.chromium.org/4049/diff/649/651
File net/base/tcp_client_socket_libevent.cc (right):

http://codereview.chromium.org/4049/diff/649/651#newcode156
Line 156: DCHECK(!callback_);
On 2008/10/16 22:32:28, dank wrote:
> The other operation in flight has its own callback variable (write_callback_),
> so the DCHECK shouldn't fire... right?

'twas a bug in my code; this is fine.

[Avi (use Gerrit), 2008-10-17 15:32:56]

http://codereview.chromium.org/4049/diff/680/685
File net/base/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/4049/diff/680/685#newcode165
Line 165: memset(ssl_info, 0, sizeof(ssl_info));
sizeof(SSLInfo) not the sizeof the pointer

[Avi (use Gerrit), 2008-10-20 15:03:00]

lgtm with comments.

http://codereview.chromium.org/4049/diff/1054/1055
File net/SConscript (right):

http://codereview.chromium.org/4049/diff/1054/1055#newcode280
Line 280: 'base/ssl_client_socket_unittest.cc',
The SSL unit tests are OK for the Mac too now. Can you do |in ('darwin',
'posix')| for this too?

http://codereview.chromium.org/4049/diff/1054/1063
File net/base/client_socket_factory.cc (right):

http://codereview.chromium.org/4049/diff/1054/1063#newcode11
Line 11: #elif defined (OS_LINUX)
nit: space

http://codereview.chromium.org/4049/diff/1054/1061
File net/base/nss_memio.c (right):

http://codereview.chromium.org/4049/diff/1054/1061#newcode48
Line 48: /* read requests are satisified from this buffer */
nit: typo (satisfied)

http://codereview.chromium.org/4049/diff/1054/1061#newcode51
Line 51: /* write requests are satisified from this buffer */
nit: typo

http://codereview.chromium.org/4049/diff/1054/1059
File net/base/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/4049/diff/1054/1059#newcode27
Line 27: * and retry with override if desired
note: "reporting to the app" is basically setting a member variable with a
condition code and returning it in the GetSSLInfo call.

http://codereview.chromium.org/4049/diff/1054/1058
File net/base/ssl_client_socket_unittest.cc (right):

http://codereview.chromium.org/4049/diff/1054/1058#newcode37
Line 37: // Test suite redirects all hostname resolution requests to 127.0.0.1
anyway.
Not after I added the host mapper above. I'd switch the comment to help whoever
wants to manually run the tests. Something like: "For manually testing SSL
connections, change the HOSTNAME define below, and also alter the host mapper
rule above in SSLClientSocketTest's constructor."

[wtc, 2008-10-20 18:19:40]

On 2008/10/10 19:00:58, Avi wrote:
>
> Line 87: EXPECT_EQ(static_cast<int>(arraysize(request_text) - 1), rv);
> Since arraysize is a size_t, can you cast to an ssize_t rather than int?

ssize_t may not be defined on non-POSIX platforms.  So it's better to cast to
int; rv (what the cast result will be compared with) is an int anyway.

[wtc, 2008-10-20 19:17:46]

LGTM.

I reviewed the latest patch set quickly.  (I plan to review the code again in
the future when we start to tackle the TODO issues.)  I also reviewed all the
comments from you and Avi since my last review.

http://codereview.chromium.org/4049/diff/1054/1064
File base/nss_init.cc (right):

http://codereview.chromium.org/4049/diff/1054/1064#newcode10
Line 10: // else base/lock.h will barf.
Sorry about this.  This is NSS bug
https://bugzilla.mozilla.org/show_bug.cgi?id=455424.  It'll be fixed in the
upcoming NSS 3.12.2 release.

http://codereview.chromium.org/4049/diff/1054/1055
File net/SConscript (right):

http://codereview.chromium.org/4049/diff/1054/1055#newcode136
Line 136: if env['PLATFORM'] == 'posix':
Just curious: in SConscript, does 'posix' include Mac OS X?

(In C++ code, OS_POSIX includes both Mac and Linux.)

http://codereview.chromium.org/4049/diff/1054/1055#newcode138
Line 138: # TODO(tc): gnome-vfs? xdgmime? /etc/mime.types?
You may want to keep this comment right above
'base/platform_mime_util_linux.cc'.  I think this comment applies to that file
only.

http://codereview.chromium.org/4049/diff/1054/1061
File net/base/nss_memio.c (right):

http://codereview.chromium.org/4049/diff/1054/1061#newcode64
Line 64: /* Forward declarations.  They don't seem to be needed -- are they? */
Remove "They don't seem to be needed -- are they?".

In C/C++, forward declarations are not needed if the functions are defined in
the right order.  But it never hurts for use forward declarations.

http://codereview.chromium.org/4049/diff/1054/1060
File net/base/nss_memio.h (right):

http://codereview.chromium.org/4049/diff/1054/1060#newcode60
Line 60: * If bytes_read is < 0, it is treated as an NSS error code.
Nit: "NSS error code" => "NSPR error code" (four occurrences in this file)

http://codereview.chromium.org/4049/diff/1054/1058
File net/base/ssl_client_socket_unittest.cc (right):

http://codereview.chromium.org/4049/diff/1054/1058#newcode37
Line 37: // Test suite redirects all hostname resolution requests to 127.0.0.1
anyway.
I agree with Avi that we should not need any of the changes to
ssl_client_socket_unittest.cc.

http://codereview.chromium.org/4049/diff/1054/1057
File net/base/tcp_client_socket.h (right):

http://codereview.chromium.org/4049/diff/1054/1057#newcode57
Line 57: // on Windows (only supported on Linux for ssl_client_socket_nss).
"only supported on Linux for ssl_client_socket_nss" is not accurate because full
duplex mode is also supported on Mac.