Make SSLClientSocketNSS full-duplex

Make SSLClientSocketNSS full-duplex

BUG=13289, 12497
TEST=visit https site and works as before.

[ukai, 2009-10-05 07:44:49]

I didn't noticed mbelshe's CL, so I might need to catch up the mbelshe's CL...

[Mike Belshe, 2009-10-05 17:23:55]

A first set of comments.  Not sure if the windows changes have much relevance -
NSS takes care of a lot of decrypt/encrypt/renegotiation work which makes the
windows code more muddled, but we wouldn't want to match that in the nss impl.

http://codereview.chromium.org/255074/diff/1/2
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/255074/diff/1/2#newcode196
Line 196: io_callback_(this, &SSLClientSocketNSS::OnIOComplete),
Can we rename OnIOComplete to OnHandshakeComplete now?  I think it is not used
for read or write anymore.

http://codereview.chromium.org/255074/diff/1/2#newcode421
Line 421: } while (nreceived > 0);
Shouldn't this do-loop be done before calling DoPayloadRead?

Also, you could rewrite it to be:
  while(BufferRecv() > 0);

http://codereview.chromium.org/255074/diff/1/2#newcode564
Line 564: }
nit: extraneous curlies

http://codereview.chromium.org/255074/diff/1/2#newcode570
Line 570: EnterFunction(result);
Add DCHECK(next_state_ == STATE_PAYLOAD_READ_WRITE)

http://codereview.chromium.org/255074/diff/1/2#newcode571
Line 571: int rv = DoLoop(result);
Instead of going back to the state machine, should we just call
DoPayloadWrite()?

http://codereview.chromium.org/255074/diff/1/2#newcode575
Line 575: }
nit: extraneous curlies here.

http://codereview.chromium.org/255074/diff/1/2#newcode581
Line 581: EnterFunction(result);
Add DCHECK(next_state_ == STATE_PAYLOAD_READ_WRITE)

http://codereview.chromium.org/255074/diff/1/2#newcode582
Line 582: int rv = DoLoop(result);
Maybe just call DoPayloadRead()?

http://codereview.chromium.org/255074/diff/1/2#newcode586
Line 586: }
nit: extraneous curlies.

http://codereview.chromium.org/255074/diff/1/2#newcode709
Line 709: if (rv == ERR_IO_PENDING && write_buf_)
This looks wrong to me; I'm not sure why rv must be ERR_IO_PENDING in order to
call DoPayloadWrite().  I don't like the way the state machine has two behaviors
in this state.  You could unfold the DoLoop for this state (by putting the logic
in the OnRecvComplete and OnSendComplete), or Wan-Teh might have good comments.

[ukai, 2009-10-06 10:01:07]

Thanks for review!

http://codereview.chromium.org/255074/diff/1/2
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/255074/diff/1/2#newcode196
Line 196: io_callback_(this, &SSLClientSocketNSS::OnIOComplete),
On 2009/10/05 17:23:55, Mike Belshe wrote:
> Can we rename OnIOComplete to OnHandshakeComplete now?  I think it is not used
> for read or write anymore.

Done.

http://codereview.chromium.org/255074/diff/1/2#newcode421
Line 421: } while (nreceived > 0);
On 2009/10/05 17:23:55, Mike Belshe wrote:
> Shouldn't this do-loop be done before calling DoPayloadRead?
> 
> Also, you could rewrite it to be:
>   while(BufferRecv() > 0);

Done.

http://codereview.chromium.org/255074/diff/1/2#newcode564
Line 564: }
On 2009/10/05 17:23:55, Mike Belshe wrote:
> nit: extraneous curlies
> 

Done.

http://codereview.chromium.org/255074/diff/1/2#newcode570
Line 570: EnterFunction(result);
On 2009/10/05 17:23:55, Mike Belshe wrote:
> Add DCHECK(next_state_ == STATE_PAYLOAD_READ_WRITE)

Done.

http://codereview.chromium.org/255074/diff/1/2#newcode571
Line 571: int rv = DoLoop(result);
On 2009/10/05 17:23:55, Mike Belshe wrote:
> Instead of going back to the state machine, should we just call
> DoPayloadWrite()?

Done.
maybe, we also need to call BufferSend() here?

http://codereview.chromium.org/255074/diff/1/2#newcode575
Line 575: }
On 2009/10/05 17:23:55, Mike Belshe wrote:
> nit: extraneous curlies here.

Done.

http://codereview.chromium.org/255074/diff/1/2#newcode581
Line 581: EnterFunction(result);
On 2009/10/05 17:23:55, Mike Belshe wrote:
> Add DCHECK(next_state_ == STATE_PAYLOAD_READ_WRITE)

Done.

http://codereview.chromium.org/255074/diff/1/2#newcode582
Line 582: int rv = DoLoop(result);
On 2009/10/05 17:23:55, Mike Belshe wrote:
> Maybe just call DoPayloadRead()?

Done.
maybe, we also need to call BufferRecv() here?

http://codereview.chromium.org/255074/diff/1/2#newcode586
Line 586: }
On 2009/10/05 17:23:55, Mike Belshe wrote:
> nit: extraneous curlies.

Done.

http://codereview.chromium.org/255074/diff/1/2#newcode709
Line 709: if (rv == ERR_IO_PENDING && write_buf_)
On 2009/10/05 17:23:55, Mike Belshe wrote:
> This looks wrong to me; I'm not sure why rv must be ERR_IO_PENDING in order to
> call DoPayloadWrite().  I don't like the way the state machine has two
behaviors
> in this state.  You could unfold the DoLoop for this state (by putting the
logic
> in the OnRecvComplete and OnSendComplete), or Wan-Teh might have good
comments.

Done.

[wtc, 2009-10-06 21:08:11]

Hi Fumitoshi,

This CL is quite tricky.  Please review the comments below
and make as many suggested changes as you can, and then
I'll review it again.  Please also ask Dan Kegel to review
the next Patch Set.  Thanks!

My main concern are the unrolling of DoLoop() for Read(),
Write(), OnRecvComplete, and OnSendComplete().
I'm not sure if the loop unrolling is done correctly
because after unrolling, we call DoPayloadRead and
DoPayloadWrite only once.

http://codereview.chromium.org/255074/diff/4001/5001
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/255074/diff/4001/5001#newcode404
Line 404: int SSLClientSocketNSS::Read(IOBuffer* buf, int buf_len,
Read and Write don't have the loop around
DoPayloadRead and DoPayloadWrite in the original code
(DoLoop).  You only kept the loop around BufferRecv() and
BufferSend().  Are you sure that's enough?

http://codereview.chromium.org/255074/diff/4001/5001#newcode418
Line 418: while (BufferRecv() > 0);
Please follow the style guide's recommendations for empty
loop bodies:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Loops_and_Swit...

http://codereview.chromium.org/255074/diff/4001/5001#newcode442
Line 442: do {
Why don't we call BufferSend() in a while loop?  This is
the only place you use a do-while loop, and it's equivalent
to
  while (BufferSend() > 0);

http://codereview.chromium.org/255074/diff/4001/5001#newcode513
Line 513: // Since Run may result in Read being called, clear |user_callback_|
up front.
Delete this stale comment, or change user_callback_ to
user_read_callback_.

http://codereview.chromium.org/255074/diff/4001/5001#newcode516
Line 516: scoped_refptr<IOBuffer> buf = read_buf_;
Why do we need to hold a reference to read_buf_ while calling
user's read callback?  Similar question for DoWriteCallback
below.

http://codereview.chromium.org/255074/diff/4001/5001#newcode527
Line 527: // Since Run may result in Write being called, clear |user_callback_|
up
Delete this stale comment, or change user_callback_ to
user_write_callback_.

http://codereview.chromium.org/255074/diff/4001/5001#newcode549
Line 549: // Since Run may result in Read being called, clear
|user_connect_callback_|
This comment is stale now.  Rather than trying to update it,
I suggest that we just remove it.

http://codereview.chromium.org/255074/diff/4001/5001#newcode560
Line 560: if ((rv != ERR_IO_PENDING) && user_connect_callback_)
Remove the user_connect_callback_ test.
user_connect_callback_ cannot be NULL at this point in the
new code.

http://codereview.chromium.org/255074/diff/4001/5001#newcode565
Line 565: void SSLClientSocketNSS::OnSendComplete(int result) {
OnSendComplete and OnRecvComplete don't have the loop around
DoPayloadWrite and DoPayloadRead in the original code
(DoLoop).  You only kept the loop around BufferSend() and
BufferRecv().  Are you sure that's enough?

http://codereview.chromium.org/255074/diff/4001/5001#newcode569
Line 569: if ((rv != ERR_IO_PENDING) && user_write_callback_)
Remove the user_write_callback_ test.

http://codereview.chromium.org/255074/diff/4001/5001#newcode572
Line 572: while (BufferSend() > 0);
See my comment above about empty loop bodies.

http://codereview.chromium.org/255074/diff/4001/5001#newcode580
Line 580: if ((rv != ERR_IO_PENDING) && user_read_callback_)
Remove the user_read_callback_ test.

http://codereview.chromium.org/255074/diff/4001/5001#newcode583
Line 583: while (BufferRecv() > 0);
See my comment above about empty loop bodies.

http://codereview.chromium.org/255074/diff/4001/5001#newcode859
Line 859: read_buf_ = 0;
Nit: use NULL instead of 0 here.

http://codereview.chromium.org/255074/diff/4001/5001#newcode879
Line 879: write_buf_ = 0;
Nit: use NULL instead of 0 here.

http://codereview.chromium.org/255074/diff/4001/5002
File net/socket/ssl_client_socket_nss.h (right):

http://codereview.chromium.org/255074/diff/4001/5002#newcode63
Line 63: void OnHandshakeComplete(int result);
Let's name this method OnHandshakeIOComplete.

OnHandshakeComplete suggests that the handshake is complete,
which is not true.

http://codereview.chromium.org/255074/diff/4001/5002#newcode93
Line 93: CompletionCallbackImpl<SSLClientSocketNSS> io_callback_;
Let's rename this member handshake_io_callback_.

http://codereview.chromium.org/255074/diff/4001/5002#newcode103
Line 103: scoped_refptr<IOBuffer> read_buf_;
Let's keep the user_ prefix for these members:
  user_read_buf_
  user_read_buf_len_
  user_write_buf_
  user_write_buf_len_
Otherwise read_buf_ would be easily confused with recv_buffer_.

http://codereview.chromium.org/255074/diff/4001/5002#newcode120
Line 120: STATE_HANDSHAKE_READ,
We should rename this state STATE_HANDSHAKE and the
corresponding function DoHandshake.

http://codereview.chromium.org/255074/diff/4001/5002#newcode123
Line 123: STATE_PAYLOAD_READ_WRITE,
next_state_ is now only applicable to the handshake.
We probably don't need STATE_PAYLOAD_READ_WRITE.

http://codereview.chromium.org/255074/diff/4001/5002#newcode125
Line 125: State next_state_;
We probably should rename this member next_handshake_state_
and DoLoop() should probably be renamed DoHandshakeLoop()
because the state machine now only applies to the handshake.

[ukai, 2009-10-07 12:25:20]

Thanks for review.

I changed a lot of code, especially factoring out DoReadLoop and DoWriteLoop.
Could you take a look it again, please?
Thanks!

http://codereview.chromium.org/255074/diff/4001/5001
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/255074/diff/4001/5001#newcode404
Line 404: int SSLClientSocketNSS::Read(IOBuffer* buf, int buf_len,
On 2009/10/06 21:08:11, wtc wrote:
> Read and Write don't have the loop around
> DoPayloadRead and DoPayloadWrite in the original code
> (DoLoop).  You only kept the loop around BufferRecv() and
> BufferSend().  Are you sure that's enough?

I think once DoPayloadRead and DoPayloadWrite returns !ERR_IO_PENDING, then user
requested read/write finished, so we should not call DoPayloadRead and
DoPayloadWrite again.
Anyway, I've factor out DoReadLoop() and DoWriteLoop().

http://codereview.chromium.org/255074/diff/4001/5001#newcode418
Line 418: while (BufferRecv() > 0);
On 2009/10/06 21:08:11, wtc wrote:
> Please follow the style guide's recommendations for empty
> loop bodies:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Loops_and_Swit...

Done.

http://codereview.chromium.org/255074/diff/4001/5001#newcode442
Line 442: do {
On 2009/10/06 21:08:11, wtc wrote:
> Why don't we call BufferSend() in a while loop?  This is
> the only place you use a do-while loop, and it's equivalent
> to
>   while (BufferSend() > 0);

Done.

http://codereview.chromium.org/255074/diff/4001/5001#newcode513
Line 513: // Since Run may result in Read being called, clear |user_callback_|
up front.
On 2009/10/06 21:08:11, wtc wrote:
> Delete this stale comment, or change user_callback_ to
> user_read_callback_.

Done.

http://codereview.chromium.org/255074/diff/4001/5001#newcode516
Line 516: scoped_refptr<IOBuffer> buf = read_buf_;
On 2009/10/06 21:08:11, wtc wrote:
> Why do we need to hold a reference to read_buf_ while calling
> user's read callback?  Similar question for DoWriteCallback
> below.

not necessary. fixed.

http://codereview.chromium.org/255074/diff/4001/5001#newcode527
Line 527: // Since Run may result in Write being called, clear |user_callback_|
up
On 2009/10/06 21:08:11, wtc wrote:
> Delete this stale comment, or change user_callback_ to
> user_write_callback_.

Done.

http://codereview.chromium.org/255074/diff/4001/5001#newcode549
Line 549: // Since Run may result in Read being called, clear
|user_connect_callback_|
On 2009/10/06 21:08:11, wtc wrote:
> This comment is stale now.  Rather than trying to update it,
> I suggest that we just remove it.

Done.

http://codereview.chromium.org/255074/diff/4001/5001#newcode560
Line 560: if ((rv != ERR_IO_PENDING) && user_connect_callback_)
On 2009/10/06 21:08:11, wtc wrote:
> Remove the user_connect_callback_ test.
> user_connect_callback_ cannot be NULL at this point in the
> new code.

Done.

http://codereview.chromium.org/255074/diff/4001/5001#newcode565
Line 565: void SSLClientSocketNSS::OnSendComplete(int result) {
On 2009/10/06 21:08:11, wtc wrote:
> OnSendComplete and OnRecvComplete don't have the loop around
> DoPayloadWrite and DoPayloadRead in the original code
> (DoLoop).  You only kept the loop around BufferSend() and
> BufferRecv().  Are you sure that's enough?

I think once DoPayloadRead and DoPayloadWrite returns !ERR_IO_PENDING, then user
requested read/write finished, so we should not call DoPayloadRead and
DoPayloadWrite again.
Anyway, I've factor out DoReadLoop() and DoWriteLoop().

http://codereview.chromium.org/255074/diff/4001/5001#newcode569
Line 569: if ((rv != ERR_IO_PENDING) && user_write_callback_)
On 2009/10/06 21:08:11, wtc wrote:
> Remove the user_write_callback_ test.

Done.

http://codereview.chromium.org/255074/diff/4001/5001#newcode572
Line 572: while (BufferSend() > 0);
On 2009/10/06 21:08:11, wtc wrote:
> See my comment above about empty loop bodies.

Done.

http://codereview.chromium.org/255074/diff/4001/5001#newcode580
Line 580: if ((rv != ERR_IO_PENDING) && user_read_callback_)
On 2009/10/06 21:08:11, wtc wrote:
> Remove the user_read_callback_ test.

Done.

http://codereview.chromium.org/255074/diff/4001/5001#newcode583
Line 583: while (BufferRecv() > 0);
On 2009/10/06 21:08:11, wtc wrote:
> See my comment above about empty loop bodies.

Done.

http://codereview.chromium.org/255074/diff/4001/5001#newcode859
Line 859: read_buf_ = 0;
On 2009/10/06 21:08:11, wtc wrote:
> Nit: use NULL instead of 0 here.

Done.

http://codereview.chromium.org/255074/diff/4001/5001#newcode879
Line 879: write_buf_ = 0;
On 2009/10/06 21:08:11, wtc wrote:
> Nit: use NULL instead of 0 here.

Done.

http://codereview.chromium.org/255074/diff/4001/5002
File net/socket/ssl_client_socket_nss.h (right):

http://codereview.chromium.org/255074/diff/4001/5002#newcode63
Line 63: void OnHandshakeComplete(int result);
On 2009/10/06 21:08:11, wtc wrote:
> Let's name this method OnHandshakeIOComplete.
> 
> OnHandshakeComplete suggests that the handshake is complete,
> which is not true.

Done.

http://codereview.chromium.org/255074/diff/4001/5002#newcode93
Line 93: CompletionCallbackImpl<SSLClientSocketNSS> io_callback_;
On 2009/10/06 21:08:11, wtc wrote:
> Let's rename this member handshake_io_callback_.

Done.

http://codereview.chromium.org/255074/diff/4001/5002#newcode103
Line 103: scoped_refptr<IOBuffer> read_buf_;
On 2009/10/06 21:08:11, wtc wrote:
> Let's keep the user_ prefix for these members:
>   user_read_buf_
>   user_read_buf_len_
>   user_write_buf_
>   user_write_buf_len_
> Otherwise read_buf_ would be easily confused with recv_buffer_.

Done.

http://codereview.chromium.org/255074/diff/4001/5002#newcode120
Line 120: STATE_HANDSHAKE_READ,
On 2009/10/06 21:08:11, wtc wrote:
> We should rename this state STATE_HANDSHAKE and the
> corresponding function DoHandshake.

Done.

http://codereview.chromium.org/255074/diff/4001/5002#newcode123
Line 123: STATE_PAYLOAD_READ_WRITE,
On 2009/10/06 21:08:11, wtc wrote:
> next_state_ is now only applicable to the handshake.
> We probably don't need STATE_PAYLOAD_READ_WRITE.

Done.

http://codereview.chromium.org/255074/diff/4001/5002#newcode125
Line 125: State next_state_;
On 2009/10/06 21:08:11, wtc wrote:
> We probably should rename this member next_handshake_state_
> and DoLoop() should probably be renamed DoHandshakeLoop()
> because the state machine now only applies to the handshake.

Done.

[wtc, 2009-10-08 22:55:30]

LGTM.

http://codereview.chromium.org/255074/diff/6001/6002
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/255074/diff/6001/6002#newcode89
Line 89: "; next_state " << next_handshake_state_
I wonder if we should change next_state to next_handshake_state
in the strings on lines 89 and 92.

http://codereview.chromium.org/255074/diff/6001/6002#newcode559
Line 559: void SSLClientSocketNSS::OnSendComplete(int result) {
General comment about OnSendComplete and OnRecvComplete:

Rather than matching OnSendComplete with DoWriteLoop and
OnRecvComplete with DoReadLoop, we may need to try both
DoWriteLoop and DoReadLoop, depending on whether the client
asked us to write or read.

The reason is that during certain phases of the SSL
protocol, we may need to write to the transport even
though the client asked us to read, and vice versa.
This can happen during a read (the server may
ask us to redo the handshake, called a renegotiation,
when we're trying to read).  Also, NSS may have buffered
some data that it's still trying to write to the transport,
and it may do that when you ask NSS to read.

So it seems that at least OnSendComplete should try to
call both DoWriteLoop and DoReadLoop.

http://codereview.chromium.org/255074/diff/6001/6002#newcode563
Line 563: int rv = DoHandshakeLoop(result);
If it were not for the EnterFunction/LeaveFunction, we
can just call OnHandshakeIOComplete(result) here.

http://codereview.chromium.org/255074/diff/6001/6002#newcode565
Line 565: if (user_connect_callback_)
We shouldn't need to test user_connect_callback_ here.
Same comment for the user_connect_callback_ test on line
589 below.

http://codereview.chromium.org/255074/diff/6001/6002#newcode572
Line 572: // Network layer sent some data, check client requested to write
Nit: add "if" after "check".  Same comment for line 596 below.

http://codereview.chromium.org/255074/diff/6001/6002#newcode575
Line 575: return;
We also need
  LeaveFunction("");
here.

Same comment for the return statement on line 599 below.

http://codereview.chromium.org/255074/diff/6001/6002#newcode577
Line 577: int rv = DoWriteLoop();
Why don't we need to pass 'result' to DoWriteLoop()?

Should we return early, without calling DoWriteLoop(),
if 'result' is < 0?

http://codereview.chromium.org/255074/diff/6001/6002#newcode740
Line 740: int SSLClientSocketNSS::DoReadLoop() {
General comment about DoReadLoop and DoWriteLoop:

We may need to call both BufferSend() and BufferRecv()
in DoReadLoop and DoWriteLoop, because NSS may need to do
transport IO "in the opposite direction".

For example, when we try to read payload data, NSS may
need to write to the transport for some reason, for
example, when a renegotiation handshake is in progress
or when NSS has buffered data that it's still trying to
write to the transport.

http://codereview.chromium.org/255074/diff/6001/6002#newcode746
Line 746: return ERR_FAILED;
Use ERR_UNEXPECTED, if this is a bug when we reach here.

http://codereview.chromium.org/255074/diff/6001/6002#newcode748
Line 748: while (BufferRecv() > 0) continue;
Do we need this while loop here?  We don't have a corresponding
while (BufferSend() > 0) look in DoWriteLoop.

http://codereview.chromium.org/255074/diff/6001/6002#newcode756
Line 756: while (BufferRecv() > 0)
Can this be an if instead of while?  Is "while" better?

http://codereview.chromium.org/255074/diff/6001/6002#newcode937
Line 937: GotoState(STATE_NONE);
Delete this.  We shouldn't need this now.

http://codereview.chromium.org/255074/diff/6001/6002#newcode961
Line 961: GotoState(STATE_NONE);
Delete this.  We shouldn't need this now.

[ukai, 2009-10-09 10:58:26]

Thanks for review!

http://codereview.chromium.org/255074/diff/6001/6002
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/255074/diff/6001/6002#newcode89
Line 89: "; next_state " << next_handshake_state_
On 2009/10/08 22:55:30, wtc wrote:
> I wonder if we should change next_state to next_handshake_state
> in the strings on lines 89 and 92.

Done.

http://codereview.chromium.org/255074/diff/6001/6002#newcode559
Line 559: void SSLClientSocketNSS::OnSendComplete(int result) {
On 2009/10/08 22:55:30, wtc wrote:
> General comment about OnSendComplete and OnRecvComplete:
> 
> Rather than matching OnSendComplete with DoWriteLoop and
> OnRecvComplete with DoReadLoop, we may need to try both
> DoWriteLoop and DoReadLoop, depending on whether the client
> asked us to write or read.
> 
> The reason is that during certain phases of the SSL
> protocol, we may need to write to the transport even
> though the client asked us to read, and vice versa.
> This can happen during a read (the server may
> ask us to redo the handshake, called a renegotiation,
> when we're trying to read).  Also, NSS may have buffered
> some data that it's still trying to write to the transport,
> and it may do that when you ask NSS to read.
> 
> So it seems that at least OnSendComplete should try to
> call both DoWriteLoop and DoReadLoop.

Thanks for info. I didn't know that.
I introduce DoTransportIO() and use it.
How about it?

http://codereview.chromium.org/255074/diff/6001/6002#newcode563
Line 563: int rv = DoHandshakeLoop(result);
On 2009/10/08 22:55:30, wtc wrote:
> If it were not for the EnterFunction/LeaveFunction, we
> can just call OnHandshakeIOComplete(result) here.

Done.

http://codereview.chromium.org/255074/diff/6001/6002#newcode565
Line 565: if (user_connect_callback_)
On 2009/10/08 22:55:30, wtc wrote:
> We shouldn't need to test user_connect_callback_ here.
> Same comment for the user_connect_callback_ test on line
> 589 below.

Done.

http://codereview.chromium.org/255074/diff/6001/6002#newcode572
Line 572: // Network layer sent some data, check client requested to write
On 2009/10/08 22:55:30, wtc wrote:
> Nit: add "if" after "check".  Same comment for line 596 below.

Done.

http://codereview.chromium.org/255074/diff/6001/6002#newcode575
Line 575: return;
On 2009/10/08 22:55:30, wtc wrote:
> We also need
>   LeaveFunction("");
> here.
> 
> Same comment for the return statement on line 599 below.

Done.

http://codereview.chromium.org/255074/diff/6001/6002#newcode577
Line 577: int rv = DoWriteLoop();
On 2009/10/08 22:55:30, wtc wrote:
> Why don't we need to pass 'result' to DoWriteLoop()?
> 
> Should we return early, without calling DoWriteLoop(),
> if 'result' is < 0?

Done.

http://codereview.chromium.org/255074/diff/6001/6002#newcode746
Line 746: return ERR_FAILED;
On 2009/10/08 22:55:30, wtc wrote:
> Use ERR_UNEXPECTED, if this is a bug when we reach here.

Done.

http://codereview.chromium.org/255074/diff/6001/6002#newcode748
Line 748: while (BufferRecv() > 0) continue;
On 2009/10/08 22:55:30, wtc wrote:
> Do we need this while loop here?  We don't have a corresponding
> while (BufferSend() > 0) look in DoWriteLoop.

Hmm, maybe unnecessary.
Removed.

http://codereview.chromium.org/255074/diff/6001/6002#newcode756
Line 756: while (BufferRecv() > 0)
On 2009/10/08 22:55:30, wtc wrote:
> Can this be an if instead of while?  Is "while" better?

Done.

http://codereview.chromium.org/255074/diff/6001/6002#newcode937
Line 937: GotoState(STATE_NONE);
On 2009/10/08 22:55:30, wtc wrote:
> Delete this.  We shouldn't need this now.

Done.

http://codereview.chromium.org/255074/diff/6001/6002#newcode961
Line 961: GotoState(STATE_NONE);
On 2009/10/08 22:55:30, wtc wrote:
> Delete this.  We shouldn't need this now.

Done.

[wtc, 2009-10-12 21:04:16]

LGTM.  Please check this in after making as many suggested
changes as possible.  It'll be easier for me to help you
resolve the remaining issues after this is checked in.

http://codereview.chromium.org/255074/diff/10001/8002
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/255074/diff/10001/8002#newcode553
Line 553: DCHECK(user_connect_callback_);
Nit: let's delete this DCHECK because it is redundant with
the DCHECK in DoConnectCallback on line 542.

http://codereview.chromium.org/255074/diff/10001/8002#newcode559
Line 559: void SSLClientSocketNSS::OnSendComplete(int result) {
This is an important issue.  My comment below may be a
little incoherent.  You may need to read it twice to
understand it.  Sorry about that.

Your code should work except when we are doing a
renegotiation handshake in the middle of a Read().  This
means when we are trying to read decrypted data, we need
to temporarily go through a new handshake (during which
we will read from and write to the transport).

So OnSendComplete may need to call DoPayloadRead while
the renegotiation handshake is in progress, something
like this:

  // This replaces the DoWriteLoop call.
  int rv_read, rv_write;
  do {
    rv_read = DoPayloadRead();
    rv_write = DoPayloadWrite();
    network_moved = DoTransportIO();
  } while (rv_read == ERR_IO_PENDING &&
           rv_write == ERR_IO_PENDING &&
           network_moved);

  if (rv_read != ERR_IO_PENDING)
    DoReadCallback(rv_read);
  if (rv_write != ERR_IO_PENDING)
    DoWriteCallback(rv_write);

I may not get all the details right, but this is the
rough idea.  We could also just call DoReadLoop and
DoWriteLoop.  What I haven't figured out is what to do
if the 'result' argument of OnSendComplete is < 0.

It is fine to leave OnRecvComplete unchanged.

http://codereview.chromium.org/255074/diff/10001/8002#newcode709
Line 709: network_moved = false;
Nit: you can delete this line now because it is now part
of the DoTransportIO() call below.

http://codereview.chromium.org/255074/diff/10001/8002#newcode759
Line 759: network_moved = false;
Remove this.

http://codereview.chromium.org/255074/diff/10001/8002#newcode761
Line 761: if (rv != ERR_IO_PENDING)
In the original code, we will still call DoTransportIO,
and then check rv.  So it may be safer to do that in the
new code, like this:

  do {
    rv = DoPayloadRead();
    network_moved = DoTransportIO();
  } while (rv == ERR_IO_PENDING && network_moved);

http://codereview.chromium.org/255074/diff/10001/8002#newcode784
Line 784: network_moved = false;
Remove this.

http://codereview.chromium.org/255074/diff/10001/8002#newcode786
Line 786: if (rv != ERR_IO_PENDING)
Same here.  It may be safer to write this do-while loop like
this:
  do {
    rv = DoPayloadWrite();
    network_moved = DoTransportIO();
  } while (rv == ERR_IO_PENDING && network_moved);

http://codereview.chromium.org/255074/diff/10001/8002#newcode937
Line 937: user_read_buf_ = NULL;
I suggest that we reset user_read_buf_ and user_read_buf_len_
in Read (at the end, for synchronous completion of Read) and
DoReadCallback (for asynchronous completion of Read).

http://codereview.chromium.org/255074/diff/10001/8002#newcode955
Line 955: if (!user_write_buf_)
I don't understand this (lines 955-956).  Why do we need
to test !user_write_buf_?  Is it correct to return OK in
this case?

http://codereview.chromium.org/255074/diff/10001/8002#newcode960
Line 960: user_write_buf_ = NULL;
I suggest that we reset user_write_buf_ and user_write_buf_len_
in Write (at the end, for synchronous completion of Wrire) and
DoWriteCallback (for asynchronous completion of Wrire).

[ukai, 2009-10-13 04:40:14]

Thanks for review.

http://codereview.chromium.org/255074/diff/10001/8002
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/255074/diff/10001/8002#newcode553
Line 553: DCHECK(user_connect_callback_);
On 2009/10/12 21:04:16, wtc wrote:
> Nit: let's delete this DCHECK because it is redundant with
> the DCHECK in DoConnectCallback on line 542.

Done.

http://codereview.chromium.org/255074/diff/10001/8002#newcode559
Line 559: void SSLClientSocketNSS::OnSendComplete(int result) {
On 2009/10/12 21:04:16, wtc wrote:
> This is an important issue.  My comment below may be a
> little incoherent.  You may need to read it twice to
> understand it.  Sorry about that.
> 
> Your code should work except when we are doing a
> renegotiation handshake in the middle of a Read().  This
> means when we are trying to read decrypted data, we need
> to temporarily go through a new handshake (during which
> we will read from and write to the transport).
> 
> So OnSendComplete may need to call DoPayloadRead while
> the renegotiation handshake is in progress, something
> like this:
> 
>   // This replaces the DoWriteLoop call.
>   int rv_read, rv_write;
>   do {
>     rv_read = DoPayloadRead();
>     rv_write = DoPayloadWrite();
>     network_moved = DoTransportIO();
>   } while (rv_read == ERR_IO_PENDING &&
>            rv_write == ERR_IO_PENDING &&
>            network_moved);
> 
>   if (rv_read != ERR_IO_PENDING)
>     DoReadCallback(rv_read);
>   if (rv_write != ERR_IO_PENDING)
>     DoWriteCallback(rv_write);
> 
> I may not get all the details right, but this is the
> rough idea.  We could also just call DoReadLoop and
> DoWriteLoop.  What I haven't figured out is what to do
> if the 'result' argument of OnSendComplete is < 0.
> 
> It is fine to leave OnRecvComplete unchanged.

Done.

http://codereview.chromium.org/255074/diff/10001/8002#newcode709
Line 709: network_moved = false;
On 2009/10/12 21:04:16, wtc wrote:
> Nit: you can delete this line now because it is now part
> of the DoTransportIO() call below.

Done.

http://codereview.chromium.org/255074/diff/10001/8002#newcode759
Line 759: network_moved = false;
On 2009/10/12 21:04:16, wtc wrote:
> Remove this.

Done.

http://codereview.chromium.org/255074/diff/10001/8002#newcode761
Line 761: if (rv != ERR_IO_PENDING)
On 2009/10/12 21:04:16, wtc wrote:
> In the original code, we will still call DoTransportIO,
> and then check rv.  So it may be safer to do that in the
> new code, like this:
> 
>   do {
>     rv = DoPayloadRead();
>     network_moved = DoTransportIO();
>   } while (rv == ERR_IO_PENDING && network_moved);

Done.

http://codereview.chromium.org/255074/diff/10001/8002#newcode784
Line 784: network_moved = false;
On 2009/10/12 21:04:16, wtc wrote:
> Remove this.

Done.

http://codereview.chromium.org/255074/diff/10001/8002#newcode786
Line 786: if (rv != ERR_IO_PENDING)
On 2009/10/12 21:04:16, wtc wrote:
> Same here.  It may be safer to write this do-while loop like
> this:
>   do {
>     rv = DoPayloadWrite();
>     network_moved = DoTransportIO();
>   } while (rv == ERR_IO_PENDING && network_moved);

Done.

http://codereview.chromium.org/255074/diff/10001/8002#newcode937
Line 937: user_read_buf_ = NULL;
On 2009/10/12 21:04:16, wtc wrote:
> I suggest that we reset user_read_buf_ and user_read_buf_len_
> in Read (at the end, for synchronous completion of Read) and
> DoReadCallback (for asynchronous completion of Read).

Done.

http://codereview.chromium.org/255074/diff/10001/8002#newcode955
Line 955: if (!user_write_buf_)
On 2009/10/12 21:04:16, wtc wrote:
> I don't understand this (lines 955-956).  Why do we need
> to test !user_write_buf_?  Is it correct to return OK in
> this case?

ah, unnecessary.

http://codereview.chromium.org/255074/diff/10001/8002#newcode960
Line 960: user_write_buf_ = NULL;
On 2009/10/12 21:04:16, wtc wrote:
> I suggest that we reset user_write_buf_ and user_write_buf_len_
> in Write (at the end, for synchronous completion of Wrire) and
> DoWriteCallback (for asynchronous completion of Wrire).

Done.

[wtc, 2009-10-14 01:54:15]

LGTM.  Thanks!  Just two style nits.

http://codereview.chromium.org/255074/diff/6005/14001
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/255074/diff/6005/14001#newcode422
Line 422: if (rv == ERR_IO_PENDING)
Please add braces {} to "if" when "else" has braces.

http://codereview.chromium.org/255074/diff/6005/14001#newcode448
Line 448: user_write_callback_ = callback;
Same here: both "if" and "else" should have braces {}.