net: rework the NPN patch.

net: rework the NPN patch.

This change moves the protocol selection logic out of NSS and into Chromium
code. This allows some things to be a little cleaner (no more wire-encoded NPN
strings) and also allows for some tricks that we have been considering for
SPDY+WebSockets.

As a consequence of this change, next protocols are now a
std::vector<std::string> rather than an encoded char*

BUG=none
TEST=SPDY still works with Google sites.


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

[wtc, 2011-10-05 23:34:25]

Review comments on Patch Set 2.

I went through the CL quickly, so I'd like to review it
again tomorrow.

It'd be nice to describe the other major changes in this
CL.  For example I noticed that on the Chrome side, there
is a new has_next_proto method, and we now use a std:;vector
of std::string instead of a single concatenated std::string.

This change seems to simplify the NSS NPN patch significantly.
I suggest providing a default NextProtoCallback below
so as to not burden all apps too much.

http://codereview.chromium.org/8156001/diff/1001/chrome/browser/ui/webui/net_...
File chrome/browser/ui/webui/net_internals_ui.cc (right):

http://codereview.chromium.org/8156001/diff/1001/chrome/browser/ui/webui/net_...
chrome/browser/ui/webui/net_internals_ui.cc:1208: }
Just wanted to doublecheck: you intentionally replace the
tertiary ? : expression with an if-else statement for better
readability, right?

http://codereview.chromium.org/8156001/diff/1001/net/socket/ssl_client_socket...
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/8156001/diff/1001/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_nss.cc:2539: }
Rather than having every application implement the callback,
can NSS provide a default NextProtoCallback appropriate for
most applications?  Then only the apps that custom protocol
selection logic need to install their own NextProtoCallbacks.

http://codereview.chromium.org/8156001/diff/1001/net/socket/ssl_client_socket...
File net/socket/ssl_client_socket_nss.h (right):

http://codereview.chromium.org/8156001/diff/1001/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_nss.h:295: // next_proto is the protocol that we
selected by NPN.
Typo: next_proto => next_proto_

http://codereview.chromium.org/8156001/diff/1001/net/third_party/nss/ssl/ssl.h
File net/third_party/nss/ssl/ssl.h (right):

http://codereview.chromium.org/8156001/diff/1001/net/third_party/nss/ssl/ssl....
net/third_party/nss/ssl/ssl.h:160: #define SSL_NEXT_PROTO_NEGOTIATION_SUPPORTED
1
Why do we need the SSL_NEXT_PROTO_NEGOTIATION_SUPPORTED
macro?

http://codereview.chromium.org/8156001/diff/1001/net/third_party/nss/ssl/ssl....
net/third_party/nss/ssl/ssl.h:174: unsigned char* protoOutLen);
Should we use 'unsigned int' for the lengths to be future-proof?

[agl, 2011-10-07 19:19:17]

http://codereview.chromium.org/8156001/diff/1001/chrome/browser/ui/webui/net_...
File chrome/browser/ui/webui/net_internals_ui.cc (right):

http://codereview.chromium.org/8156001/diff/1001/chrome/browser/ui/webui/net_...
chrome/browser/ui/webui/net_internals_ui.cc:1208: }
Yes

http://codereview.chromium.org/8156001/diff/1001/net/socket/ssl_client_socket...
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/8156001/diff/1001/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_nss.cc:2539: }
On 2011/10/05 23:34:25, wtc wrote:
> Rather than having every application implement the callback,
> can NSS provide a default NextProtoCallback appropriate for
> most applications?

I tried writing this function, but it quickly becomes messy.

Writing a function which matches the callback interface isn't nice because it
needs to return several other bits of information to the application: the status
and the matched protocol.

the |arg| value could be casted to a structure like:

struct SSLNextProtoCallbackState {
  const unsigned char* client_protos;
  unsigned client_protos_len;
  
  unsigned out_proto_len;
  unsigned char out_proto[256];
  SSLNextProtoState state;
};

But then using this is almost as hard as writing the callback oneself, since the
application is probably written in C++ these days and will have to marshal the
client_protos in any case.

http://codereview.chromium.org/8156001/diff/1001/net/socket/ssl_client_socket...
File net/socket/ssl_client_socket_nss.h (right):

http://codereview.chromium.org/8156001/diff/1001/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_nss.h:295: // next_proto is the protocol that we
selected by NPN.
On 2011/10/05 23:34:25, wtc wrote:
> Typo: next_proto => next_proto_

Done.

http://codereview.chromium.org/8156001/diff/1001/net/third_party/nss/ssl/ssl.h
File net/third_party/nss/ssl/ssl.h (right):

http://codereview.chromium.org/8156001/diff/1001/net/third_party/nss/ssl/ssl....
net/third_party/nss/ssl/ssl.h:160: #define SSL_NEXT_PROTO_NEGOTIATION_SUPPORTED
1
On 2011/10/05 23:34:25, wtc wrote:
> Why do we need the SSL_NEXT_PROTO_NEGOTIATION_SUPPORTED
> macro?

Because we need to know that we don't have the NPN functions when building with
use_system_ssl. Previously we used SSL_NEXT_PROTO_NEGOTIATED for this.

http://codereview.chromium.org/8156001/diff/1001/net/third_party/nss/ssl/ssl....
net/third_party/nss/ssl/ssl.h:174: unsigned char* protoOutLen);
On 2011/10/05 23:34:25, wtc wrote:
> Should we use 'unsigned int' for the lengths to be future-proof?

But the format of protos requires that the maximum protocol string be 255 bytes.
Changing this would require changes to the callback function itself, so making
the return length larger couldn't actually be used.

I'd be happy to make it an unsigned int just to make it clean that it's a
length. But, when I did that, it introduced the problem of what to do when the
value from the callback is too large. We would need to introduce a new error and
I wasn't sure if we can do that with the NSS/SSL split that we have on Linux.

[wtc, 2011-10-11 22:27:57]

agl: let me respond to your comments first.  I will review
Patch Set 3 next.

http://codereview.chromium.org/8156001/diff/1001/net/socket/ssl_client_socket...
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/8156001/diff/1001/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_nss.cc:2539: }

On 2011/10/07 19:19:17, agl wrote:
>
> I tried writing this function, but it quickly becomes messy.
> 
> Writing a function which matches the callback interface isn't nice because it
> needs to return several other bits of information to the application: the
status
> and the matched protocol.

Thanks a lot for looking into this suggestion.  I came to
the same conclusion that it doesn't make sense for NSS to
provide a default NextProtoCallback.  Instead, NSS should
continue to provide the SSL_SetNextProtoNego and
SSL_GetNextProto functions, which an application should
use if it does not give NSS a NextProtoCallback.

What do you think?  I think it is useful for NSS to
help most applications use NPN easily.

http://codereview.chromium.org/8156001/diff/1001/net/third_party/nss/ssl/ssl.h
File net/third_party/nss/ssl/ssl.h (right):

http://codereview.chromium.org/8156001/diff/1001/net/third_party/nss/ssl/ssl....
net/third_party/nss/ssl/ssl.h:174: unsigned char* protoOutLen);

On 2011/10/07 19:19:17, agl wrote:
>
> I'd be happy to make it an unsigned int just to make it clean that it's a
> length.

We can consider this change when we submit this patch to
NSS upstream.

> But, when I did that, it introduced the problem of what to do when the
> value from the callback is too large. We would need to introduce a new error
and
> I wasn't sure if we can do that with the NSS/SSL split that we have on Linux.

Yes, we can do that.  NSS has two error code spaces:
SEC_ERROR_xxx and SSL_ERROR_xxx.

We can add a new SSL_ERROR_xxx error code to
mozilla/security/nss/lib/ssl/sslerr.h for this condition,
but I think we can just use the existing error code
SEC_ERROR_INPUT_LEN.

[wtc, 2011-10-11 23:43:04]

Patch Set 3 LGTM.

BUT, I strongly prefer that NSS has default NPN selection
code for most applications.

http://codereview.chromium.org/8156001/diff/9001/chrome/browser/ui/webui/net_...
File chrome/browser/ui/webui/net_internals_ui.cc (right):

http://codereview.chromium.org/8156001/diff/9001/chrome/browser/ui/webui/net_...
chrome/browser/ui/webui/net_internals_ui.cc:1192:
JoinString(net::HttpStreamFactory::next_protos(), ','));

IMPORTANT: this means the protocol strings must not contain
commas.  Is this true?  The NPN spec doesn't have the
"no commas" restriction.

http://codereview.chromium.org/8156001/diff/9001/net/http/http_network_layer.cc
File net/http/http_network_layer.cc (left):

http://codereview.chromium.org/8156001/diff/9001/net/http/http_network_layer....
net/http/http_network_layer.cc:74: // This is a temporary hack to pretend we
support version 1.

Nit: it seems useful to preserve this comment in the new code.

http://codereview.chromium.org/8156001/diff/9001/net/http/http_network_transa...
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/8156001/diff/9001/net/http/http_network_transa...
net/http/http_network_transaction_unittest.cc:100: }

I suggest moving these two functions into the anonymous
namespace above and remove 'static'.

http://codereview.chromium.org/8156001/diff/9001/net/http/http_network_transa...
net/http/http_network_transaction_unittest.cc:362: static const char
kExpectedNPNString[] = "\x08http/1.1\x06spdy/2";

kExpectedNPNString can be removed now.

http://codereview.chromium.org/8156001/diff/9001/net/socket/ssl_client_socket...
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/8156001/diff/9001/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_nss.cc:2512: DCHECK_LT(j->size(), 256u);

If a DCHECK failure here indicates an NSS bug, then using a
DCHECK is fine.

Otherwise, we should replace the DCHECK with a LOG(ERROR).

http://codereview.chromium.org/8156001/diff/9001/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_nss.cc:2515: memcmp(&protos[i+1], j->data(), len)
== 0) {

Nit: our Style Guide recommend spaces around '+'.

http://codereview.chromium.org/8156001/diff/9001/net/third_party/nss/ssl/ssl.h
File net/third_party/nss/ssl/ssl.h (right):

http://codereview.chromium.org/8156001/diff/9001/net/third_party/nss/ssl/ssl....
net/third_party/nss/ssl/ssl.h:174: unsigned char* protoOutLen);

On second thought, I think we should use
    unsigned int protosLen,
and
    unsigned int* protoOutLen);
to be consistent with NSS's convention of using 'unsigned int'
for length arguments.

http://codereview.chromium.org/8156001/diff/9001/net/third_party/nss/ssl/ssl....
net/third_party/nss/ssl/ssl.h:177: * Negotiation. It causes a client to
advertise NPN. */

If you take my suggestion of keeping SSL_SetNextProtoNego
and SSL_GetNextProto, please also add a
"It causes a client to advertise NPN." comment to
SSL_SetNextProtoNego.

http://codereview.chromium.org/8156001/diff/9001/net/third_party/nss/ssl/ssli...
File net/third_party/nss/ssl/sslimpl.h (left):

http://codereview.chromium.org/8156001/diff/9001/net/third_party/nss/ssl/ssli...
net/third_party/nss/ssl/sslimpl.h:891: * Protocol Negotiation.

Why do you delete this comment about servers?
Is it because the NPN patch for NSS does't have the
server-side implementation?  (I don't see a
ssl3_ServerSendNextProtoNegoXtn function.)

http://codereview.chromium.org/8156001/diff/9001/net/third_party/nss/ssl/ssli...
File net/third_party/nss/ssl/sslimpl.h (right):

http://codereview.chromium.org/8156001/diff/9001/net/third_party/nss/ssl/ssli...
net/third_party/nss/ssl/sslimpl.h:883: * this is the protocol that was
requested.

Nit: I think "that was negotiated" or "that the client selected"
is clearer than "that was requested".

At least change this to "that the client requested".

[wtc, 2011-10-11 23:45:50]

I'd like to repeat my previous suggestion for the CL's
commit message:

It'd be nice to describe the other major changes in this
CL.  For example I noticed that on the Chrome side, there
is a new has_next_proto method, and we now use a std:;vector
of std::string instead of a single concatenated std::string.

[agl, 2011-10-17 17:37:24]

PTAL.

The only substantive changes are that I've reintroduced the old functions to
NSS, which now work by setting a standard callback function.

http://codereview.chromium.org/8156001/diff/9001/chrome/browser/ui/webui/net_...
File chrome/browser/ui/webui/net_internals_ui.cc (right):

http://codereview.chromium.org/8156001/diff/9001/chrome/browser/ui/webui/net_...
chrome/browser/ui/webui/net_internals_ui.cc:1192:
JoinString(net::HttpStreamFactory::next_protos(), ','));
On 2011/10/11 23:43:04, wtc wrote:
> 
> IMPORTANT: this means the protocol strings must not contain
> commas.  Is this true?  The NPN spec doesn't have the
> "no commas" restriction.

This is just for debugging and has a number of limitations. The spec allows the
protocols to be full, 8-bit clean strings. There are a number of ways in which
that could go wrong.

http://codereview.chromium.org/8156001/diff/9001/net/http/http_network_layer.cc
File net/http/http_network_layer.cc (left):

http://codereview.chromium.org/8156001/diff/9001/net/http/http_network_layer....
net/http/http_network_layer.cc:74: // This is a temporary hack to pretend we
support version 1.
On 2011/10/11 23:43:04, wtc wrote:
> 
> Nit: it seems useful to preserve this comment in the new code.

Done.

http://codereview.chromium.org/8156001/diff/9001/net/http/http_network_transa...
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/8156001/diff/9001/net/http/http_network_transa...
net/http/http_network_transaction_unittest.cc:100: }
On 2011/10/11 23:43:04, wtc wrote:
> 
> I suggest moving these two functions into the anonymous
> namespace above and remove 'static'.

Done.

http://codereview.chromium.org/8156001/diff/9001/net/http/http_network_transa...
net/http/http_network_transaction_unittest.cc:362: static const char
kExpectedNPNString[] = "\x08http/1.1\x06spdy/2";
On 2011/10/11 23:43:04, wtc wrote:
> 
> kExpectedNPNString can be removed now.

Done.

http://codereview.chromium.org/8156001/diff/9001/net/socket/ssl_client_socket...
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/8156001/diff/9001/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_nss.cc:2512: DCHECK_LT(j->size(), 256u);
On 2011/10/11 23:43:04, wtc wrote:
> If a DCHECK failure here indicates an NSS bug, then using a
> DCHECK is fine.

It represents a major programming error. I think that DCHECK is correct.

http://codereview.chromium.org/8156001/diff/9001/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_nss.cc:2515: memcmp(&protos[i+1], j->data(), len)
== 0) {
On 2011/10/11 23:43:04, wtc wrote:
> 
> Nit: our Style Guide recommend spaces around '+'.

Done.

http://codereview.chromium.org/8156001/diff/9001/net/third_party/nss/ssl/ssl.h
File net/third_party/nss/ssl/ssl.h (right):

http://codereview.chromium.org/8156001/diff/9001/net/third_party/nss/ssl/ssl....
net/third_party/nss/ssl/ssl.h:174: unsigned char* protoOutLen);
On 2011/10/11 23:43:04, wtc wrote:
> 
> On second thought, I think we should use
>     unsigned int protosLen,
> and
>     unsigned int* protoOutLen);
> to be consistent with NSS's convention of using 'unsigned int'
> for length arguments.

Done.

http://codereview.chromium.org/8156001/diff/9001/net/third_party/nss/ssl/ssl....
net/third_party/nss/ssl/ssl.h:177: * Negotiation. It causes a client to
advertise NPN. */
On 2011/10/11 23:43:04, wtc wrote:
> 
> If you take my suggestion of keeping SSL_SetNextProtoNego
> and SSL_GetNextProto, please also add a
> "It causes a client to advertise NPN." comment to
> SSL_SetNextProtoNego.

Done.

http://codereview.chromium.org/8156001/diff/9001/net/third_party/nss/ssl/ssli...
File net/third_party/nss/ssl/sslimpl.h (left):

http://codereview.chromium.org/8156001/diff/9001/net/third_party/nss/ssl/ssli...
net/third_party/nss/ssl/sslimpl.h:891: * Protocol Negotiation.
On 2011/10/11 23:43:04, wtc wrote:
> Why do you delete this comment about servers?
> Is it because the NPN patch for NSS does't have the
> server-side implementation?  (I don't see a
> ssl3_ServerSendNextProtoNegoXtn function.)

Right, the server side hasn't been written and the comment was misleading in
suggesting that server-side support exists.

http://codereview.chromium.org/8156001/diff/9001/net/third_party/nss/ssl/ssli...
File net/third_party/nss/ssl/sslimpl.h (right):

http://codereview.chromium.org/8156001/diff/9001/net/third_party/nss/ssl/ssli...
net/third_party/nss/ssl/sslimpl.h:883: * this is the protocol that was
requested.
On 2011/10/11 23:43:04, wtc wrote:
> 
> Nit: I think "that was negotiated" or "that the client selected"
> is clearer than "that was requested".
> 
> At least change this to "that the client requested".

Done.

[wtc, 2011-10-18 00:58:08]

Patch Set 5 LGTM!

I remember I sent my review comments earlier today, but
they're not posted.  Perhaps I forgot to push the "Publish"
button.  Sorry about that.

All of my comments below are just nits.

http://codereview.chromium.org/8156001/diff/34001/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/8156001/diff/34001/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_nss.cc:2505: unsigned int* proto_out_len) {
Question: does this callback do the same thing as the built-in
standard callback right now?

http://codereview.chromium.org/8156001/diff/34001/net/third_party/nss/ssl/ssl.h
File net/third_party/nss/ssl/ssl.h (right):

http://codereview.chromium.org/8156001/diff/34001/net/third_party/nss/ssl/ssl...
net/third_party/nss/ssl/ssl.h:164: * (the maximum allowed by the protocol)
which, on successful return, must
Nit: I think "will" or "shall" sounds better than "must".
"must" seems to imply something the caller needs to do, but
here we're describing what the function does.

We should clarify that |protoOut| is a buffer provided by
the caller, and that on successful return, |*protoOutLen|
contains the number of bytes the function copied to the
|protoOut| buffer.

http://codereview.chromium.org/8156001/diff/34001/net/third_party/nss/ssl/ssl...
net/third_party/nss/ssl/ssl.h:183: * protocol in server-preference order.
Otherwise it selects the first

Nit: it's not clear what "Otherwise" means here.  I guess you
meant "If the server's Next Protocol Negotiation extension is
empty".

http://codereview.chromium.org/8156001/diff/34001/net/third_party/nss/ssl/ssl...
net/third_party/nss/ssl/ssl.h:198: * number of bytes copied is written into
length. */
Nit: length => *length

http://codereview.chromium.org/8156001/diff/34001/net/third_party/nss/ssl/ssl...
net/third_party/nss/ssl/ssl.h:203: unsigned buf_len);

Nit: use "unsigned int" instead of just "unsigned" for
|length| and |buf_len|.

http://codereview.chromium.org/8156001/diff/34001/net/third_party/nss/ssl/ssl...
net/third_party/nss/ssl/ssl.h:207: #define SSL_NEXT_PROTO_NO_OVERLAP	2 /* No
protocol overlap found      */
It may be a good idea to define these as an enum type.  You
can just add a TODO comment about this.

http://codereview.chromium.org/8156001/diff/34001/net/third_party/nss/ssl/ssl...
File net/third_party/nss/ssl/sslerr.h (right):

http://codereview.chromium.org/8156001/diff/34001/net/third_party/nss/ssl/ssl...
net/third_party/nss/ssl/sslerr.h:208: // protocol information during a
handshake.
Use a C-style comment /* */.

This comment should be added to a different file that contains
the error messages.  On the NSS trunk (NSS 3.13), that file is
http://mxr.mozilla.org/security/source/security/nss/lib/ssl/SSLerrs.h

http://codereview.chromium.org/8156001/diff/34001/net/third_party/nss/ssl/ssl...
File net/third_party/nss/ssl/sslimpl.h (right):

http://codereview.chromium.org/8156001/diff/34001/net/third_party/nss/ssl/ssl...
net/third_party/nss/ssl/sslimpl.h:326: * list of supported protocols. */
Unless this comment would also be correct for the server side,
we should clarify that this applies to the client side only.

http://codereview.chromium.org/8156001/diff/34001/net/third_party/nss/ssl/ssl...
File net/third_party/nss/ssl/sslsock.c (right):

http://codereview.chromium.org/8156001/diff/34001/net/third_party/nss/ssl/ssl...
net/third_party/nss/ssl/sslsock.c:1357: memcmp(&protos[i+1],
&ss->opt.nextProtoNego.data[j+1], protos[i]) == 0) {
Nit: is this line longer than 80 characters?

http://codereview.chromium.org/8156001/diff/34001/net/third_party/nss/ssl/ssl...
net/third_party/nss/ssl/sslsock.c:1365: i += (unsigned int)protos[i] + 1;
Nit: since ss->opt.nextProtoNego.data[j] and protos[i] are
unsigned chars, I believe the (unsigned int) casts are
unnecessary.

Or is it necessary to avoid overflowing unsigned char after
adding 1 to it?

http://codereview.chromium.org/8156001/diff/34001/net/third_party/nss/ssl/ssl...
net/third_party/nss/ssl/sslsock.c:1372: found:
Nit: in NSS source code, labels are not indented.

[agl, 2011-10-18 16:44:43]

Thanks! Throwing the commit switch.

http://codereview.chromium.org/8156001/diff/34001/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_nss.cc (right):

http://codereview.chromium.org/8156001/diff/34001/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_nss.cc:2505: unsigned int* proto_out_len) {
On 2011/10/18 00:58:08, wtc wrote:
> Question: does this callback do the same thing as the built-in
> standard callback right now?

I believe so and the tests pass.

http://codereview.chromium.org/8156001/diff/34001/net/third_party/nss/ssl/ssl.h
File net/third_party/nss/ssl/ssl.h (right):

http://codereview.chromium.org/8156001/diff/34001/net/third_party/nss/ssl/ssl...
net/third_party/nss/ssl/ssl.h:164: * (the maximum allowed by the protocol)
which, on successful return, must
On 2011/10/18 00:58:08, wtc wrote:
> Nit: I think "will" or "shall" sounds better than "must".
> "must" seems to imply something the caller needs to do, but
> here we're describing what the function does.
> 
> We should clarify that |protoOut| is a buffer provided by
> the caller, and that on successful return, |*protoOutLen|
> contains the number of bytes the function copied to the
> |protoOut| buffer.

Done.

http://codereview.chromium.org/8156001/diff/34001/net/third_party/nss/ssl/ssl...
net/third_party/nss/ssl/ssl.h:183: * protocol in server-preference order.
Otherwise it selects the first
On 2011/10/18 00:58:08, wtc wrote:
> 
> Nit: it's not clear what "Otherwise" means here.  I guess you
> meant "If the server's Next Protocol Negotiation extension is
> empty".

Done.

http://codereview.chromium.org/8156001/diff/34001/net/third_party/nss/ssl/ssl...
net/third_party/nss/ssl/ssl.h:198: * number of bytes copied is written into
length. */
On 2011/10/18 00:58:08, wtc wrote:
> Nit: length => *length

Done.

http://codereview.chromium.org/8156001/diff/34001/net/third_party/nss/ssl/ssl...
net/third_party/nss/ssl/ssl.h:203: unsigned buf_len);
On 2011/10/18 00:58:08, wtc wrote:
> 
> Nit: use "unsigned int" instead of just "unsigned" for
> |length| and |buf_len|.

Done.

http://codereview.chromium.org/8156001/diff/34001/net/third_party/nss/ssl/ssl...
net/third_party/nss/ssl/ssl.h:207: #define SSL_NEXT_PROTO_NO_OVERLAP	2 /* No
protocol overlap found      */
On 2011/10/18 00:58:08, wtc wrote:
> It may be a good idea to define these as an enum type.  You
> can just add a TODO comment about this.

I've added a TODO because, at the moment, we use one of these defines as the
#ifdef to figure out if the NSS that we're compiling against supports NPN.

http://codereview.chromium.org/8156001/diff/34001/net/third_party/nss/ssl/ssl...
File net/third_party/nss/ssl/sslerr.h (right):

http://codereview.chromium.org/8156001/diff/34001/net/third_party/nss/ssl/ssl...
net/third_party/nss/ssl/sslerr.h:208: // protocol information during a
handshake.
On 2011/10/18 00:58:08, wtc wrote:
> Use a C-style comment /* */.
> 
> This comment should be added to a different file that contains
> the error messages.  On the NSS trunk (NSS 3.13), that file is
> http://mxr.mozilla.org/security/source/security/nss/lib/ssl/SSLerrs.h

Have removed the comment here. We don't that that trunk file yet so the addition
will have to wait for the next upstream sync.

http://codereview.chromium.org/8156001/diff/34001/net/third_party/nss/ssl/ssl...
File net/third_party/nss/ssl/sslimpl.h (right):

http://codereview.chromium.org/8156001/diff/34001/net/third_party/nss/ssl/ssl...
net/third_party/nss/ssl/sslimpl.h:326: * list of supported protocols. */
On 2011/10/18 00:58:08, wtc wrote:
> Unless this comment would also be correct for the server side,
> we should clarify that this applies to the client side only.

I believe that it would apply to the server side code.

http://codereview.chromium.org/8156001/diff/34001/net/third_party/nss/ssl/ssl...
File net/third_party/nss/ssl/sslsock.c (right):

http://codereview.chromium.org/8156001/diff/34001/net/third_party/nss/ssl/ssl...
net/third_party/nss/ssl/sslsock.c:1357: memcmp(&protos[i+1],
&ss->opt.nextProtoNego.data[j+1], protos[i]) == 0) {
On 2011/10/18 00:58:08, wtc wrote:
> Nit: is this line longer than 80 characters?

Done.

http://codereview.chromium.org/8156001/diff/34001/net/third_party/nss/ssl/ssl...
net/third_party/nss/ssl/sslsock.c:1365: i += (unsigned int)protos[i] + 1;
On 2011/10/18 00:58:08, wtc wrote:
> Or is it necessary to avoid overflowing unsigned char after
> adding 1 to it?

Right. I want the addition to occur in ℤ/2**32ℤ to avoid an overflow in the
intermediate value.

I actually still believe that the cast is unnecessary in C, but I would rather
waste some space and be clear.

http://codereview.chromium.org/8156001/diff/34001/net/third_party/nss/ssl/ssl...
net/third_party/nss/ssl/sslsock.c:1372: found:
On 2011/10/18 00:58:08, wtc wrote:
> Nit: in NSS source code, labels are not indented.

Done.

[commit-bot: I haz the power, 2011-10-18 16:45:18]

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

[commit-bot: I haz the power, 2011-10-18 18:56:36]

Change committed as 106093

[wtc, 2011-10-28 02:06:26]

agl: while inspecting the about:net-internals logs, I found
that SSL_SetNextProtoCallback always fails with the -5998
error (PR_WOULD_BLOCK_ERROR).  It turns out to be a missing
return statement.  (Brian Smith also found this bug in his
code review.)

Could you fix this (since I don't know how to regenerate the
patch)?  Thanks.

http://codereview.chromium.org/8156001/diff/43001/net/third_party/nss/ssl/ssl...
File net/third_party/nss/ssl/sslsock.c (right):

http://codereview.chromium.org/8156001/diff/43001/net/third_party/nss/ssl/ssl...
net/third_party/nss/ssl/sslsock.c:1328: ssl_ReleaseSSL3HandshakeLock(ss);

BUG: we need to return SECSuccess at the end of this function.