Make SSLClientSocketMac full-duplex

Make SSLClientSocketMac full-duplex

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

[wtc, 2009-10-16 00:54:24]

avi, hawk: do you have time to look at this CL?

ukai: sorry about the delay in reviewing.  I'll review this
CL tomorrow.

[wtc, 2009-10-16 02:15:50]

Hi Fumitoshi,

I made a quick pass through your CL.  It seems good.
I have some questions for you below.

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

http://codereview.chromium.org/266078/diff/1/2#newcode476
Line 476: int rv = DoPayloadRead();
In the original code, Read calls DoPayloadRead in a loop.
Now we just call it once.  Why?  (Same question for Write
below.)

http://codereview.chromium.org/266078/diff/1/2#newcode541
Line 541: next_handshake_state_ = STATE_NONE;
DoConnectCallback shouldn't need to set next_handshake_state_
to STATE_NONE.  This should be set by DoHandshakeLoop.

http://codereview.chromium.org/266078/diff/1/2#newcode551
Line 551: // since Run may result in Read being called, clear
user_read_callback_ up
Nit: capitalize "Since".  Similarly for line 564 below.

http://codereview.chromium.org/266078/diff/1/2#newcode595
Line 595: if (user_read_buf_) {
The bottom half of OnTransportReadComplete (lines 595-603)
is quite different from the bottom half of
OnTransportWriteComplete (lines 621-634).

Could you explain the asymmetry?

Why don't we check user_write_buf_ here?

http://codereview.chromium.org/266078/diff/1/2#newcode622
Line 622: DoReadCallback(result);
Why do we call the user read callback here?

http://codereview.chromium.org/266078/diff/1/3
File net/socket/ssl_client_socket_mac.h (left):

http://codereview.chromium.org/266078/diff/1/3#oldcode96
Line 96: State next_io_state_;
I'm not that familiar with the original code's use of
next_io_state_.  Could you explain how you were able to
get rid of next_io_state_?

[ukai, 2009-10-16 07:58:31]

Thanks for review!

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

http://codereview.chromium.org/266078/diff/1/2#newcode476
Line 476: int rv = DoPayloadRead();
On 2009/10/16 02:15:50, wtc wrote:
> In the original code, Read calls DoPayloadRead in a loop.
> Now we just call it once.  Why?  (Same question for Write
> below.)

In the original code, 

 if DoPayloadRead returns ERR_IO_PENDING, no iteration.

 if DoPayloadRead returns other, it must be next_state_ is STATE_NONE
(next_state_ is set to STATE_PAYLOAD_READ only if it returns ERR_IO_PENDING),
thus no iteration.

So, I don't think we need to call DoPayloadRead in a loop.

Same for Write.

http://codereview.chromium.org/266078/diff/1/2#newcode541
Line 541: next_handshake_state_ = STATE_NONE;
On 2009/10/16 02:15:50, wtc wrote:
> DoConnectCallback shouldn't need to set next_handshake_state_
> to STATE_NONE.  This should be set by DoHandshakeLoop.

Yes. DCHECK for it.

http://codereview.chromium.org/266078/diff/1/2#newcode551
Line 551: // since Run may result in Read being called, clear
user_read_callback_ up
On 2009/10/16 02:15:50, wtc wrote:
> Nit: capitalize "Since".  Similarly for line 564 below.

Done.

http://codereview.chromium.org/266078/diff/1/2#newcode595
Line 595: if (user_read_buf_) {
On 2009/10/16 02:15:50, wtc wrote:
> The bottom half of OnTransportReadComplete (lines 595-603)
> is quite different from the bottom half of
> OnTransportWriteComplete (lines 621-634).
> 
> Could you explain the asymmetry?
> 
> Why don't we check user_write_buf_ here?

I think we don't need for Read as you pointed out in ssl_client_socket_nss
OnSendComplete and OnRecvComplete.
Do we need to make symmetric these methods in SSLClientSocketNSS?

http://codereview.chromium.org/266078/diff/1/2#newcode622
Line 622: DoReadCallback(result);
On 2009/10/16 02:15:50, wtc wrote:
> Why do we call the user read callback here?

I just thought it needs to call callback when some error happened in transport
layer.
Call DoWriteCallback if user_write_buf_, and call DoReadCallback if
user_read_buf_.

http://codereview.chromium.org/266078/diff/1/3
File net/socket/ssl_client_socket_mac.h (left):

http://codereview.chromium.org/266078/diff/1/3#oldcode96
Line 96: State next_io_state_;
On 2009/10/16 02:15:50, wtc wrote:
> I'm not that familiar with the original code's use of
> next_io_state_.  Could you explain how you were able to
> get rid of next_io_state_?

next_io_state_ is used only for STATE_READ_COMPLETE and I don't think we need
it.

[wtc, 2009-10-20 02:33:59]

Hi Fumitoshi,

On Monday - Wednesday, I'm attending a conference.  So I may not be able
to review this CL again until Thursday (which is Friday in your time zone).

I was going to give your CL LGTM, but I think you haven't compiled Patch Set 2.
So I can't.  Other than that, the CL looks correct.

Please compile and test this CL.  I have some nits and questions below.  I will
try to review this CL again tomorrow (Tuesday) night.

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

http://codereview.chromium.org/266078/diff/1/2#newcode595
Line 595: if (user_read_buf_) {
On 2009/10/16 07:58:32, ukai wrote:
>
> I think we don't need for Read as you pointed out in ssl_client_socket_nss
> OnSendComplete and OnRecvComplete.
> Do we need to make symmetric these methods in SSLClientSocketNSS?

Sorry, I was confused and asked an incorrect question.

No, these methods don't need to be completely symmetric:
- OnTransportReadComplete only needs to check if (user_read_buf_).
- OnTransportWriteComplete needs to check both if (user_read_buf_) and
  if (user_write_buf_).

But the if (user_read_buf_) code and the if (user_write_buf_) code should
look symmetric.   In Patch Set 2, they look symmetric (except that you
incorrectly use 'rv' instead of 'result').

http://codereview.chromium.org/266078/diff/5001/6001#newcode469
Line 469: DCHECK(next_handshake_state_ == STATE_NONE);
Do you think this DCHECK and the same DCHECK in Write on line 489 are
still useful, now that the state machine only covers the handshake?  It's
harmless to have these two DCHECKs.  I'm just wondering if they're useful.

http://codereview.chromium.org/266078/diff/5001/6001#newcode583
Line 583: &recv_buffer_[recv_buffer_.size() - recv_buffer_tail_slop_];
Nit: Indent this line with two more spaces.

http://codereview.chromium.org/266078/diff/5001/6001#newcode607
Line 607: if (result > 0) {
Should we also execute this block of code (lines 608-611) if result is 0?

http://codereview.chromium.org/266078/diff/5001/6001#newcode613
Line 613: pending_send_error_ = result;
In the original code (OnWriteComplete), we set pending_send_error_ = result
only if result < 0, and then we return immediately.  Are you sure the new code
is correct?

http://codereview.chromium.org/266078/diff/5001/6001#newcode622
Line 622: DoReadCallback(result);
Did you compile this code?  rv is not declared at this point.  We should
be using 'result' in lines 622-623 here and lines 631-632 below.

We should be smarter and figure out whether a transport write 'result' < 0
applies to Read or Write.  Otherwise we may call the wrong user callback.

Except during a renegotiation handshake, a transport write is only caused
by Write, so the order in which you check user_write_buf_ and user_read_buf_
is good, because renegotiation handshakes are less common.

http://codereview.chromium.org/266078/diff/5001/6001#newcode796
Line 796: // (which is what we fake for Secure Transport). When Secure Transport
calls us
Please review this big comment block to see if it needs updating.  For example,
line 829 still refers to the now nonexistent DoReadComplete() method.

http://codereview.chromium.org/266078/diff/5001/6002
File net/socket/ssl_client_socket_mac.h (right):

http://codereview.chromium.org/266078/diff/5001/6002#newcode67
Line 67: int DoReadComplete(int result);
Remove the declarations of the DoReadComplete and OnWriteComplete
methods because these two methods have been removed.

[ukai, 2009-10-20 09:34:54]

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

http://codereview.chromium.org/266078/diff/1/2#newcode595
Line 595: if (user_read_buf_) {
On 2009/10/20 02:34:00, wtc wrote:
> On 2009/10/16 07:58:32, ukai wrote:
> >
> > I think we don't need for Read as you pointed out in ssl_client_socket_nss
> > OnSendComplete and OnRecvComplete.
> > Do we need to make symmetric these methods in SSLClientSocketNSS?
> 
> Sorry, I was confused and asked an incorrect question.
> 
> No, these methods don't need to be completely symmetric:
> - OnTransportReadComplete only needs to check if (user_read_buf_).
> - OnTransportWriteComplete needs to check both if (user_read_buf_) and
>   if (user_write_buf_).
> 
> But the if (user_read_buf_) code and the if (user_write_buf_) code should
> look symmetric.   In Patch Set 2, they look symmetric (except that you
> incorrectly use 'rv' instead of 'result').

Thanks. Fixed some 'rv' to 'result'.

http://codereview.chromium.org/266078/diff/5001/6001#newcode469
Line 469: DCHECK(next_handshake_state_ == STATE_NONE);
On 2009/10/20 02:34:00, wtc wrote:
> Do you think this DCHECK and the same DCHECK in Write on line 489 are
> still useful, now that the state machine only covers the handshake?  It's
> harmless to have these two DCHECKs.  I'm just wondering if they're useful.

Hmm, maybe not so useful. Removed.

http://codereview.chromium.org/266078/diff/5001/6001#newcode583
Line 583: &recv_buffer_[recv_buffer_.size() - recv_buffer_tail_slop_];
On 2009/10/20 02:34:00, wtc wrote:
> Nit: Indent this line with two more spaces.

Done.

http://codereview.chromium.org/266078/diff/5001/6001#newcode607
Line 607: if (result > 0) {
On 2009/10/20 02:34:00, wtc wrote:
> Should we also execute this block of code (lines 608-611) if result is 0?

Hmm, yes we might need to call SSLWriteCallback again.
Fixed.

http://codereview.chromium.org/266078/diff/5001/6001#newcode613
Line 613: pending_send_error_ = result;
On 2009/10/20 02:34:00, wtc wrote:
> In the original code (OnWriteComplete), we set pending_send_error_ = result
> only if result < 0, and then we return immediately.  Are you sure the new code
> is correct?

I think original code would be correct here. Fixed.

http://codereview.chromium.org/266078/diff/5001/6001#newcode622
Line 622: DoReadCallback(result);
On 2009/10/20 02:34:00, wtc wrote:
> Did you compile this code?  rv is not declared at this point.  We should
> be using 'result' in lines 622-623 here and lines 631-632 below.

Sorry. Fixed.

> 
> We should be smarter and figure out whether a transport write 'result' < 0
> applies to Read or Write.  Otherwise we may call the wrong user callback.
> 
> Except during a renegotiation handshake, a transport write is only caused
> by Write, so the order in which you check user_write_buf_ and user_read_buf_
> is good, because renegotiation handshakes are less common.

http://codereview.chromium.org/266078/diff/5001/6001#newcode796
Line 796: // (which is what we fake for Secure Transport). When Secure Transport
calls us
On 2009/10/20 02:34:00, wtc wrote:
> Please review this big comment block to see if it needs updating.  For
example,
> line 829 still refers to the now nonexistent DoReadComplete() method.

I think basic buffering mechanism is the same as original, so just update the
nonexistent DoReadComplete() method with OnTransportReadComplete() method.

[wtc, 2009-10-20 21:57:32]

http://codereview.chromium.org/266078/diff/9001/10001
File net/socket/ssl_client_socket_mac.cc (right):

http://codereview.chromium.org/266078/diff/9001/10001#newcode611
Line 611: pending_send_error_ = result;
If we do this, we should remove the if (result < 0) code below on
lines 622-625 and lines 631-633, because if (result < 0) cannot be
true at those two places.

Nit: Rewrite this if-else block (lines 605-613) like the original
OnWriteComplete:

  if (result < 0) {
    pending_send_error_ = result;
    return;
  }

  send_buffer_.erase(...);
  if (!send_buffer_.empty())
    SSLWriteCallback(this, NULL, NULL);

Now I am confused why OnTransportWriteComplete has much more code
than the original OnWriteComplete.  Could you explain it?

(I am going to dinner now.  I will review this again in 3 hours.)

[wtc, 2009-10-21 02:50:18]

LGTM, with the following caveat.

I studied the original OnWriteComplete carefully.  It doesn't call
the user callback for Write.  So I think it is wrong.  This means
we don't execute that code path in practice, because transport_->Write()
usually succeeds synchronously.

Could you please verify this in the new code?  Please set a
breakpoint in OnTransportWriteComplete, and see if it ever gets hit.

http://codereview.chromium.org/266078/diff/9001/10002
File net/socket/ssl_client_socket_mac.h (right):

http://codereview.chromium.org/266078/diff/9001/10002#newcode67
Line 67: int DoReadComplete(int result);
Please remove the declarations of DoReadComplete and OnWriteComplete.

[wtc, 2009-10-21 02:56:06]

I understand why now.  Please see below.

Please review the CL again before you check it in.  Thanks!

http://codereview.chromium.org/266078/diff/9001/10001
File net/socket/ssl_client_socket_mac.cc (right):

http://codereview.chromium.org/266078/diff/9001/10001#newcode986
Line 986: // always lie to our caller
Aha, this is the answer!!!  Here transport_->Write() returns ERR_IO_PENDING,
but we lie and return noErr.

Given this new finding, the new OnTransportWriteComplete should look
like the original OnWriteComplete.

[ukai, 2009-10-21 10:43:10]

Thanks for review!

http://codereview.chromium.org/266078/diff/9001/10001
File net/socket/ssl_client_socket_mac.cc (right):

http://codereview.chromium.org/266078/diff/9001/10001#newcode611
Line 611: pending_send_error_ = result;
On 2009/10/20 21:57:33, wtc wrote:
> If we do this, we should remove the if (result < 0) code below on
> lines 622-625 and lines 631-633, because if (result < 0) cannot be
> true at those two places.
> 
> Nit: Rewrite this if-else block (lines 605-613) like the original
> OnWriteComplete:
> 
>   if (result < 0) {
>     pending_send_error_ = result;
>     return;
>   }
> 
>   send_buffer_.erase(...);
>   if (!send_buffer_.empty())
>     SSLWriteCallback(this, NULL, NULL);
> 
> Now I am confused why OnTransportWriteComplete has much more code
> than the original OnWriteComplete.  Could you explain it?
> 
> (I am going to dinner now.  I will review this again in 3 hours.)

Done.

http://codereview.chromium.org/266078/diff/9001/10001#newcode986
Line 986: // always lie to our caller
On 2009/10/21 02:56:06, wtc wrote:
> Aha, this is the answer!!!  Here transport_->Write() returns ERR_IO_PENDING,
> but we lie and return noErr.
> 
> Given this new finding, the new OnTransportWriteComplete should look
> like the original OnWriteComplete.

Ah, I see.
Fixed in OnTransportWriteComplete.

http://codereview.chromium.org/266078/diff/9001/10002
File net/socket/ssl_client_socket_mac.h (right):

http://codereview.chromium.org/266078/diff/9001/10002#newcode67
Line 67: int DoReadComplete(int result);
On 2009/10/21 02:50:18, wtc wrote:
> Please remove the declarations of DoReadComplete and OnWriteComplete.

Done.

[wtc, 2009-10-22 22:06:44]

LGTM.  Please check this in!

http://codereview.chromium.org/266078/diff/14001/11002
File net/socket/ssl_client_socket_mac.cc (right):

http://codereview.chromium.org/266078/diff/14001/11002#newcode615
Line 615: // returns ERR_IO_PENDING, we don't need to any callbacks here.
Nit: add "call" or "do" after "need to".