Disable HTTP/2 over NPN (with OpenSSL).

Disable HTTP/2 over NPN (with OpenSSL).

* Split SSLConfig.next_proto into two members: one for ALPN, one for NPN.
* Remove HTTP/2 from NPN.
* In OpenSSL, use alpn_protos for |ALPN|, and npn_protos for |NPN|.
* In NSS, use |alpn_protos| for both.
* In NSS, disable NPN if |npn_protos| is empty.

BUG=527066
Committed: https://crrev.com/1f295377e8de70ba03ce73d9f7929e614d23df68
Cr-Commit-Position: refs/heads/master@{#355427}

[Bence, 2015-10-13 16:08:44]

bnc@chromium.org changed reviewers:
+ davidben@chromium.org

[Bence, 2015-10-13 16:08:44]

David: PTAL.  Thanks.

[davidben, 2015-10-13 20:55:59]

Thanks! I think this looks good. Just some nits below.

https://codereview.chromium.org/1387363004/diff/80001/net/log/net_log_util.cc
File net/log/net_log_util.cc (right):

https://codereview.chromium.org/1387363004/diff/80001/net/log/net_log_util.cc...
net/log/net_log_util.cc:427: for (const NextProto proto : alpn_protos) {
Either const NextProto& proto or NextProto. Since it's just an enu, I'd do the
latter.

https://codereview.chromium.org/1387363004/diff/80001/net/log/net_log_util.cc...
net/log/net_log_util.cc:439: for (const NextProto proto : npn_protos) {
Ditto.

https://codereview.chromium.org/1387363004/diff/80001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/1387363004/diff/80001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_nss.cc:867: }
We should have a comment somewhere in this block describing the
alpn_protos/npn_protos limitation so it's clear this is intentional.

https://codereview.chromium.org/1387363004/diff/80001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/1387363004/diff/80001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_unittest.cc:3432: // to be non-empty, otherwise NSS
disables NPN.
Optional: Wrap the alpn_protos bits under #ifdef to make sure we don't read
alpn_protos in USE_OPENSSL ports.

https://codereview.chromium.org/1387363004/diff/80001/net/ssl/ssl_config.h
File net/ssl/ssl_config.h (right):

https://codereview.chromium.org/1387363004/diff/80001/net/ssl/ssl_config.h#ne...
net/ssl/ssl_config.h:166: // TODO(bnc): Deprecate NPN, see
https://crbug.com/526713.
I would add in a new paragraph.

// Note: Due to NSS limitations, ports which use NSS (only iOS) will use
|alpn_protos| for both NPN and ALPN. However, if |npn_protos| is empty, NPN will
still be disabled.

[Bence, 2015-10-14 14:55:59]

PTAL.  Thanks.

https://codereview.chromium.org/1387363004/diff/80001/net/log/net_log_util.cc
File net/log/net_log_util.cc (right):

https://codereview.chromium.org/1387363004/diff/80001/net/log/net_log_util.cc...
net/log/net_log_util.cc:427: for (const NextProto proto : alpn_protos) {
On 2015/10/13 20:55:59, David Benjamin wrote:
> Either const NextProto& proto or NextProto. Since it's just an enu, I'd do the
> latter.

Done.

https://codereview.chromium.org/1387363004/diff/80001/net/log/net_log_util.cc...
net/log/net_log_util.cc:439: for (const NextProto proto : npn_protos) {
On 2015/10/13 20:55:59, David Benjamin wrote:
> Ditto.

Done.

https://codereview.chromium.org/1387363004/diff/80001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/1387363004/diff/80001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_nss.cc:867: }
On 2015/10/13 20:55:59, David Benjamin wrote:
> We should have a comment somewhere in this block describing the
> alpn_protos/npn_protos limitation so it's clear this is intentional.

Done.

https://codereview.chromium.org/1387363004/diff/80001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/1387363004/diff/80001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_unittest.cc:3432: // to be non-empty, otherwise NSS
disables NPN.
On 2015/10/13 20:55:59, David Benjamin wrote:
> Optional: Wrap the alpn_protos bits under #ifdef to make sure we don't read
> alpn_protos in USE_OPENSSL ports.

Done.

https://codereview.chromium.org/1387363004/diff/80001/net/ssl/ssl_config.h
File net/ssl/ssl_config.h (right):

https://codereview.chromium.org/1387363004/diff/80001/net/ssl/ssl_config.h#ne...
net/ssl/ssl_config.h:166: // TODO(bnc): Deprecate NPN, see
https://crbug.com/526713.
On 2015/10/13 20:55:59, David Benjamin wrote:
> I would add in a new paragraph.
> 
> // Note: Due to NSS limitations, ports which use NSS (only iOS) will use
> |alpn_protos| for both NPN and ALPN. However, if |npn_protos| is empty, NPN
will
> still be disabled.

Done.

[Bence, 2015-10-14 14:57:02]

bnc@chromium.org changed reviewers:
+ eroman@chromium.org

[Bence, 2015-10-14 14:57:02]

eroman: PTAL at chrome/browser/*.  Thank you.

[davidben, 2015-10-14 16:36:46]

lgtm

https://codereview.chromium.org/1387363004/diff/120001/net/socket/ssl_client_...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/1387363004/diff/120001/net/socket/ssl_client_...
net/socket/ssl_client_socket_unittest.cc:3143:
client_config.alpn_protos.push_back(kProtoHTTP11);
The comment and #ifdef apply to all of these too, right? They only do NPN. (Hrm.
Before we remove NPN, we probably need to switch these tests to triggering with
ALPN, so we'll have to implement ALPN in the test server.)

[Bence, 2015-10-14 18:39:02]

It sounds like ALPN should be implemented in the test server, and then all of
these tests should be doubled: one for ALPN only, one for ALPN and NPN.

https://codereview.chromium.org/1387363004/diff/120001/net/socket/ssl_client_...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/1387363004/diff/120001/net/socket/ssl_client_...
net/socket/ssl_client_socket_unittest.cc:3143:
client_config.alpn_protos.push_back(kProtoHTTP11);
On 2015/10/14 16:36:46, David Benjamin wrote:
> The comment and #ifdef apply to all of these too, right? They only do NPN.
(Hrm.
> Before we remove NPN, we probably need to switch these tests to triggering
with
> ALPN, so we'll have to implement ALPN in the test server.)

Done.

[davidben, 2015-10-14 18:40:59]

On 2015/10/14 18:39:02, Bence wrote:
> It sounds like ALPN should be implemented in the test server, and then all of
> these tests should be doubled: one for ALPN only, one for ALPN and NPN.

Yeah, something of the sort. "Want to do it?" :-)

ALPN is a pretty easy extension. It shouldn't be too bad, horrible as tlslite
is. You only need to implement it on the client half, and it need only be as
featureful or reasonable as needed to drive the tests.

[eroman, 2015-10-21 21:51:39]

lgtm

[Bence, 2015-10-21 22:05:30]

The CQ bit was checked by bnc@chromium.org

[Bence, 2015-10-21 22:05:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
davidben@chromium.org
Link to the patchset: https://codereview.chromium.org/1387363004/#ps140001
(title: "Re: #7.")

[commit-bot: I haz the power, 2015-10-21 22:06:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1387363004/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1387363004/140001

[commit-bot: I haz the power, 2015-10-21 23:24:31]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-10-21 23:25:15]

Patchset 8 (id:??) landed as
https://crrev.com/1f295377e8de70ba03ce73d9f7929e614d23df68
Cr-Commit-Position: refs/heads/master@{#355427}