ssl_SecureSend should not do handshake when the handshake wait state is...

ssl_SecureSend should not do handshake when the handshake wait state is
wait_finished and we are false starting.

R=agl
BUG=38103
TEST=SSLClientSocketTest.Read_FullDuplex should not hang on Ubuntu Karmic.

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

[wtc, 2010-03-13 04:29:22]

http://codereview.chromium.org/850008/diff/1/2
File net/third_party/nss/ssl/ssl3gthr.c (right):

http://codereview.chromium.org/850008/diff/1/2#newcode212
net/third_party/nss/ssl/ssl3gthr.c:212: /* TODO(wtc): need a better way to break
out of this loop for false start. */
This test is important for false start to make SSL_ForceHandshake return
SECSuccess early, after
returning from ssl3_HandleServerHelloDone.

Because of that, we don't need to allow wait_finished here.

http://codereview.chromium.org/850008/diff/1/3
File net/third_party/nss/ssl/sslsecur.c (left):

http://codereview.chromium.org/850008/diff/1/3#oldcode151
net/third_party/nss/ssl/sslsecur.c:151: if (ss->version >=
SSL_LIBRARY_VERSION_3_0 &&
I moved this test into ssl_SecureSend.  This allows
ssl_SecureRecv to call this function (ssl_Do1stHandshake)
after we have false-started but ss->firstHsDone hasn't
been set to PR_TRUE.

[agl, 2010-03-15 14:26:10]

LGTM, thanks for fixing it!

http://codereview.chromium.org/850008/diff/1/2
File net/third_party/nss/ssl/ssl3gthr.c (right):

http://codereview.chromium.org/850008/diff/1/2#newcode212
net/third_party/nss/ssl/ssl3gthr.c:212: /* TODO(wtc): need a better way to break
out of this loop for false start. */
On 2010/03/13 04:29:22, wtc wrote:
> This test is important for false start to make SSL_ForceHandshake return
> SECSuccess early, after
> returning from ssl3_HandleServerHelloDone.
> 
> Because of that, we don't need to allow wait_finished here.

TODO comments are usually best if you're clear about what's wrong and (possibly)
what might be done about it. Is grabbing the handshake lock the problematic bit
for you?

[wtc, 2010-03-15 17:02:50]

http://codereview.chromium.org/850008/diff/1/2
File net/third_party/nss/ssl/ssl3gthr.c (right):

http://codereview.chromium.org/850008/diff/1/2#newcode212
net/third_party/nss/ssl/ssl3gthr.c:212: /* TODO(wtc): need a better way to break
out of this loop for false start. */
There are two reasons I added this comment (which probably
should not be marked as a TODO).

1. Originally I thought this test here is not necessary.
After I removed it, I found during testing that this test
is necessary to make SSL_ForceHandshake return SECSuccess
early, after returning from ssl3_HandleServerHelloDone.
I should add this comment to the source code rather than
in the code review tool.

2. After I added the test back, I found that the test is
redoing a lot of checks already done in
ssl3_HandleServerHelloDone.  So I added the TODO comment
to note that we should be able to avoid repeating those
checks.

I remember you added a boolean member to sslSocket originally.  Why did you
abandon that?

[agl, 2010-03-15 17:41:26]

On Mon, Mar 15, 2010 at 1:02 PM,  <wtc@chromium.org> wrote:
> I remember you added a boolean member to sslSocket originally.  Why did
> you abandon that?

I seem to recall that I found that I could do without it and it saved
some code. No deep reason.


AGL

[wtc, 2010-03-16 01:33:32]

In Patch Set 2, I replaced the TODO comment with a comment
that merely describes what the code does.

I also removed an unrelated code cleanup, and updated
falsestart.patch (generated with CVS; I don't use git :-))

[agl, 2010-03-16 01:36:22]

LGTM

[wtc, 2010-03-16 16:53:49]

On 2010/03/15 17:41:26, agl wrote:
>
> I seem to recall that I found that I could do without it and it saved
> some code. No deep reason.

I think that boolean member could also have avoided this bug.
We need a conditional expression for the states between "have
false started" and "!ss->firstHsDone".  Without the boolean
member, we have to enumerate all the possible values of
ss->ssl3.hs.ws, which is error-prone (especially if a new
handshake message is added in the future).