Port two SSLClientSocketNSS fixes to SSLClientSocketOpenSSL.

Prevent the infinite loop inside SSLClientSocketOpenSSL::OnSendComplete.

This ports the SSLClientSocketNSS fix in
https://chromiumcodereview.appspot.com/10382186 to SSLClientSocketOpenSSL.

R=joth@chromium.org,jnd@chromium.org
BUG=127822
TEST=none
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=138309

[joth, 2012-05-18 13:34:57]

LGTM

Thanks for thinking of OpenSSL for these too.

[Johnny(Jianning) Ding, 2012-05-18 15:45:58]

https://chromiumcodereview.appspot.com/10391174/diff/1/net/socket/ssl_client_...
File net/socket/ssl_client_socket_openssl.cc (right):

https://chromiumcodereview.appspot.com/10391174/diff/1/net/socket/ssl_client_...
net/socket/ssl_client_socket_openssl.cc:967: } while (rv > 0);
I don't think it's necessary to use the loop to keep writing the buffer because
SSLClientSocketOpenSSL::BufferSend already has same logic, see line 989-999,

[joth, 2012-05-18 16:02:08]

aside - in the CL description you have 'because Write() may return
synchronously.'

nit: this is slightly ambiguous (functions always 'return' synchronously!).
suggest: 'because Write() may complete the transfer synchronously.'

https://chromiumcodereview.appspot.com/10391174/diff/1/net/socket/ssl_client_...
File net/socket/ssl_client_socket_openssl.cc (right):

https://chromiumcodereview.appspot.com/10391174/diff/1/net/socket/ssl_client_...
net/socket/ssl_client_socket_openssl.cc:967: } while (rv > 0);
On 2012/05/18 15:45:58, Johnny(Jianning) Ding wrote:
> I don't think it's necessary to use the loop to keep writing the buffer
because
> SSLClientSocketOpenSSL::BufferSend already has same logic, see line 989-999,

wtc will have a better understanding, but I believe the issue is BufferSend will
only call "new DrainableIOBuffer" once. If that buffer gets fully sent (maybe
with multiple iterations of that loop) then we need to go ahead and loop right
back round and try and get another buffer and send?

[Johnny(Jianning) Ding, 2012-05-18 16:20:06]

LGTM. Adding the loop is no harm either.

On 2012/05/18 15:45:58, Johnny(Jianning) Ding wrote:
>
https://chromiumcodereview.appspot.com/10391174/diff/1/net/socket/ssl_client_...
> File net/socket/ssl_client_socket_openssl.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10391174/diff/1/net/socket/ssl_client_...
> net/socket/ssl_client_socket_openssl.cc:967: } while (rv > 0);
> I don't think it's necessary to use the loop to keep writing the buffer
because
> SSLClientSocketOpenSSL::BufferSend already has same logic, see line 989-999,

[wtc, 2012-05-18 18:37:58]

joth,jnd: thank you for the review and nice suggestions.

Please review patch set 2.

https://chromiumcodereview.appspot.com/10391174/diff/1/net/socket/ssl_client_...
File net/socket/ssl_client_socket_openssl.cc (right):

https://chromiumcodereview.appspot.com/10391174/diff/1/net/socket/ssl_client_...
net/socket/ssl_client_socket_openssl.cc:967: } while (rv > 0);

On 2012/05/18 15:45:58, Johnny(Jianning) Ding wrote:
> I don't think it's necessary to use the loop to keep writing the buffer
because
> SSLClientSocketOpenSSL::BufferSend already has same logic, see line 989-999,

jnd: you are right!  I did a naive port of the SSLClientSocketNSS
change and didn't check the code in SSLClientSocketOpenSSL::BufferSend
carefully.

SSLClientSocketNSS does not have a loop in its BufferSend
function, which is why it needs to call BufferSend in a
loop in DoTransportIO.  I agree with you that SSLClientSocketOpenSSL
does not need this change.

[Johnny(Jianning) Ding, 2012-05-19 09:41:51]

LGTM. Thanks!

On 2012/05/18 18:37:58, wtc wrote:
> joth,jnd: thank you for the review and nice suggestions.
> 
> Please review patch set 2.
> 
>
https://chromiumcodereview.appspot.com/10391174/diff/1/net/socket/ssl_client_...
> File net/socket/ssl_client_socket_openssl.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10391174/diff/1/net/socket/ssl_client_...
> net/socket/ssl_client_socket_openssl.cc:967: } while (rv > 0);
> 
> On 2012/05/18 15:45:58, Johnny(Jianning) Ding wrote:
> > I don't think it's necessary to use the loop to keep writing the buffer
> because
> > SSLClientSocketOpenSSL::BufferSend already has same logic, see line 989-999,
> 
> jnd: you are right!  I did a naive port of the SSLClientSocketNSS
> change and didn't check the code in SSLClientSocketOpenSSL::BufferSend
> carefully.
> 
> SSLClientSocketNSS does not have a loop in its BufferSend
> function, which is why it needs to call BufferSend in a
> loop in DoTransportIO.  I agree with you that SSLClientSocketOpenSSL
> does not need this change.

[commit-bot: I haz the power, 2012-05-21 22:00:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wtc@chromium.org/10391174/9001

[commit-bot: I haz the power, 2012-05-22 03:06:49]

Try job failure for 10391174-9001 (retry) on win_rel for steps "base_unittests,
sync_unit_tests" (clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-05-22 16:59:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wtc@chromium.org/10391174/9001

[commit-bot: I haz the power, 2012-05-22 17:48:27]

Try job failure for 10391174-9001 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...