nss: support SECWouldBlock from ChannelID callback.

nss: support SECWouldBlock from ChannelID callback.

BUG=none
TEST=none


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

[wtc, 2012-06-05 19:38:48]

Patch set 1 LGTM.  Thanks!

Please add BUG=129174 to the CL's description.

https://chromiumcodereview.appspot.com/10540014/diff/1/net/third_party/nss/ss...
File net/third_party/nss/ssl/ssl.h (right):

https://chromiumcodereview.appspot.com/10540014/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl.h:965: SECKEYPrivateKey *channelID);

Nit: indent the parameters with four spaces (instead of 8).

The parameter names don't need to be aligned.

Put '*' next to the parameter names.

https://chromiumcodereview.appspot.com/10540014/diff/1/net/third_party/nss/ss...
File net/third_party/nss/ssl/ssl3con.c (right):

https://chromiumcodereview.appspot.com/10540014/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3con.c:5501: ssl3_ExtensionNegotiated(ss,
ssl_channel_id_xtn)) {

Nit: it should be sufficient to just test ssl3_ExtensionNegotiated(ss,
ssl_channel_id_xtn).
I remember a non-null ss->getChannelID is used as a cue
that the Channel ID extension is enabled.  So
ssl3_ExtensionNegotiated(ss, ssl_channel_id_xtn) should
imply a non-null ss->getChannelID.

https://chromiumcodereview.appspot.com/10540014/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3con.c:5511: }

Nit: we may want to check if both ss->ssl3.channelIDPub and
ss->ssl3.channelID are non-null.  We have such a check after
calling ss->getClientAuthData.

https://chromiumcodereview.appspot.com/10540014/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3con.c:9029:
SECKEY_DestroyPublicKey(ss->ssl3.channelIDPub);

Nit: we shouldn't need to do this because these two members
are already cleared at the beginning of ssl3_HandleServerHello.

https://chromiumcodereview.appspot.com/10540014/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3con.c:9033: ss->ssl3.channelIDPub = channelIDPub;

We may want to check if both channelID and channelIDPub are
non-null.

https://chromiumcodereview.appspot.com/10540014/diff/1/net/third_party/nss/ss...
File net/third_party/nss/ssl/sslsecur.c (right):

https://chromiumcodereview.appspot.com/10540014/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/sslsecur.c:1553: if (ss->version <
SSL_LIBRARY_VERSION_3_0) {

I think it's better to test ss->version while holding the
1stHandshakeLock.  See SSL_RestartHandshakeAfterCertReq and
SSL_AuthCertificateComplete (the new replacement of
SSL_RestartHandshakeAfterServerCert).

[agl, 2012-06-05 19:58:43]

https://chromiumcodereview.appspot.com/10540014/diff/1/net/third_party/nss/ss...
File net/third_party/nss/ssl/ssl.h (right):

https://chromiumcodereview.appspot.com/10540014/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl.h:965: SECKEYPrivateKey *channelID);
On 2012/06/05 19:38:49, wtc wrote:
> 
> Nit: indent the parameters with four spaces (instead of 8).
> 
> The parameter names don't need to be aligned.
> 
> Put '*' next to the parameter names.

Done.

https://chromiumcodereview.appspot.com/10540014/diff/1/net/third_party/nss/ss...
File net/third_party/nss/ssl/ssl3con.c (right):

https://chromiumcodereview.appspot.com/10540014/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3con.c:5501: ssl3_ExtensionNegotiated(ss,
ssl_channel_id_xtn)) {
On 2012/06/05 19:38:49, wtc wrote:
> 
> Nit: it should be sufficient to just test ssl3_ExtensionNegotiated(ss,
> ssl_channel_id_xtn).
> I remember a non-null ss->getChannelID is used as a cue
> that the Channel ID extension is enabled.  So
> ssl3_ExtensionNegotiated(ss, ssl_channel_id_xtn) should
> imply a non-null ss->getChannelID.

Done.

https://chromiumcodereview.appspot.com/10540014/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3con.c:5511: }
On 2012/06/05 19:38:49, wtc wrote:
> 
> Nit: we may want to check if both ss->ssl3.channelIDPub and
> ss->ssl3.channelID are non-null.  We have such a check after
> calling ss->getClientAuthData.

Done.

https://chromiumcodereview.appspot.com/10540014/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3con.c:9029:
SECKEY_DestroyPublicKey(ss->ssl3.channelIDPub);
On 2012/06/05 19:38:49, wtc wrote:
> Nit: we shouldn't need to do this because these two members
> are already cleared at the beginning of ssl3_HandleServerHello.

That's true but it seems more robust to free these members before assigning to
them. Otherwise I would worry that future changes could introduce a memory leak.

https://chromiumcodereview.appspot.com/10540014/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3con.c:9033: ss->ssl3.channelIDPub = channelIDPub;
On 2012/06/05 19:38:49, wtc wrote:
> 
> We may want to check if both channelID and channelIDPub are
> non-null.

Done.

https://chromiumcodereview.appspot.com/10540014/diff/1/net/third_party/nss/ss...
File net/third_party/nss/ssl/sslsecur.c (right):

https://chromiumcodereview.appspot.com/10540014/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/sslsecur.c:1553: if (ss->version <
SSL_LIBRARY_VERSION_3_0) {
On 2012/06/05 19:38:49, wtc wrote:
> 
> I think it's better to test ss->version while holding the
> 1stHandshakeLock.  See SSL_RestartHandshakeAfterCertReq and
> SSL_AuthCertificateComplete (the new replacement of
> SSL_RestartHandshakeAfterServerCert).

Done.

[commit-bot: I haz the power, 2012-06-05 19:59:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/agl@chromium.org/10540014/7001

[commit-bot: I haz the power, 2012-06-05 20:12:05]

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

[commit-bot: I haz the power, 2012-06-05 20:18:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/agl@chromium.org/10540014/7002

[commit-bot: I haz the power, 2012-06-05 21:29:39]

Change committed as 140618