net: False Start only for NPN capable servers.

net: False Start only for NPN capable servers.

This change causes NSS only to False Start with NPN capable servers. It also
removes the False Start blacklist and this has the effect of enabling 1/n-1
record splitting for those hosts that were previously on the blacklist.
However, those hosts have been getting 1/n-1 from Opera, Firefox and IE for a
few months now.

BUG=none
TEST=net_unittests


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

[Ryan Sleevi, 2012-04-09 22:42:56]

Two questions below, but otherwise looks fine.

https://chromiumcodereview.appspot.com/10014010/diff/1/net/socket/ssl_client_...
File net/socket/ssl_client_socket_nss.cc (left):

https://chromiumcodereview.appspot.com/10014010/diff/1/net/socket/ssl_client_...
net/socket/ssl_client_socket_nss.cc:123: static const int kCorkTimeoutMs = 200;
This seems unrelated to the description. That is, I can see the cork being
useful for false-start servers, based on how this is implemented within NSS and
perceived by the socket / HttpStream. Intentional?

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

https://chromiumcodereview.appspot.com/10014010/diff/1/net/socket/ssl_client_...
net/socket/ssl_client_socket_nss.cc:939: ssl_config_.false_start_enabled);
Did you mean to make this unconditionally true? Or perhaps contingent on the
policy flag?

Otherwise, it would seem like you're still equating the 1/n-1 fix with false
start enable/disable (even if now it's most-always-on)

[agl, 2012-04-09 23:05:30]

https://chromiumcodereview.appspot.com/10014010/diff/1/net/socket/ssl_client_...
File net/socket/ssl_client_socket_nss.cc (left):

https://chromiumcodereview.appspot.com/10014010/diff/1/net/socket/ssl_client_...
net/socket/ssl_client_socket_nss.cc:123: static const int kCorkTimeoutMs = 200;
On 2012/04/09 22:42:56, Ryan Sleevi wrote:
> Intentional?

The cork aims to put the CKX, CCS, Finished and Application Data in a single TCP
packet. This, it was hoped, would make False Start intolerance more
deterministic. It doesn't do anything for False Start capable servers except,
possibly save a packet. I don't think it's worth the code complexity for that.

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

https://chromiumcodereview.appspot.com/10014010/diff/1/net/socket/ssl_client_...
net/socket/ssl_client_socket_nss.cc:939: ssl_config_.false_start_enabled);
On 2012/04/09 22:42:56, Ryan Sleevi wrote:
> Otherwise, it would seem like you're still equating the 1/n-1 fix with false
> start enable/disable (even if now it's most-always-on)

|false_start_enabled| now means that we *can* do False Start if the server
supports NPN. It's also the switch that the policy for
kDisableSSLRecordSplitting toggles, so we do still want to tie them together.
Untangling them would be a good thing, but something for a different CL I feel.

[Ryan Sleevi, 2012-04-09 23:07:59]

Great, thanks.

LGTM.

[willchan no longer on Chromium, 2012-04-10 06:39:18]

On 2012/04/09 23:07:59, Ryan Sleevi wrote:
> Great, thanks.
> 
> LGTM.

This change makes me sad...I thought we were going to just revert to the old
blacklist and hang steady for awhile? That would also have made me sad, but less
sad than pretty much turning it off =/

[agl2, 2012-04-10 14:48:17]

On Tue, Apr 10, 2012 at 2:39 AM,  <willchan@chromium.org> wrote:
> This change makes me sad...I thought we were going to just revert to the old
> blacklist and hang steady for awhile? That would also have made me sad, but
> less
> sad than pretty much turning it off =/

I think that keeping the current blacklist around in a zombie state is
the worst answer of all. So I think it's either the case that we
continue to be pretty aggressive in killing it (although working with
sites where we can), or we go with this answer. ifette: I cc'ed you
for this reason :)


Cheers

AGL

[willchan no longer on Chromium, 2012-04-10 15:03:12]

Let me preface this by saying that I fully expect to be unhappy no matter
what we do, so don't be too concerned when I say it bothers me :) And don't
let me block you either, if everyone else agrees on this, then let's do it.
And I definitely defer to ian@ here since he has a broader vision of Chrome
use than I do.

That said, I think False Start is a nice step forward for SSL performance.
Given the current size of the blacklist, aren't we able to use False Start
for like 9X% of websites? Despite the aesthetic / philosophical
deplorability of keeping a blacklist around in a zombie state, allowing our
users to shave off an RTT off the SSL handshake for the vast (99%?)
majority of https URLs seems like a nice win to me. I'm concerned that we
are acting out of a very vague fear (do we have any good #s) about broken
websites (which admittedly have a horrible failure mode), when we do know
pretty well the cost of disabling False Start for all those websites.

I told Ian most of that in person so I suspect he already knows how I feel
:P

On Tue, Apr 10, 2012 at 3:48 PM, Adam Langley <agl@google.com> wrote:

> On Tue, Apr 10, 2012 at 2:39 AM,  <willchan@chromium.org> wrote:
> > This change makes me sad...I thought we were going to just revert to the
> old
> > blacklist and hang steady for awhile? That would also have made me sad,
> but
> > less
> > sad than pretty much turning it off =/
>
> I think that keeping the current blacklist around in a zombie state is
> the worst answer of all. So I think it's either the case that we
> continue to be pretty aggressive in killing it (although working with
> sites where we can), or we go with this answer. ifette: I cc'ed you
> for this reason :)
>
>
> Cheers
>
> AGL
>

[ian fette, 2012-04-10 17:12:14]

lgtm

I think all of us are somewhat disappointed; at the end of the day I think the
most important thing is correctness. I don't think we'd take regressions in
other parts of the product that caused sporadic issues for people.

I like that this gives sites (not just Google but any site) a way to still get
the speed advantage back, reduces the probability of user pain, and think it's a
good way to go. I had this discussion with Will in a random cafe in Berlin, and
I think we both understand each other's positions. I agree False Start is a
great benefit, I just think we need to figure out how to do it safely. NPN seems
like a good conservative start, maybe we can further broaden it out (or it will
also hopefully automatically broaden out over time as NPN support spreads.)

On 2012/04/10 15:03:12, willchan wrote:
> Let me preface this by saying that I fully expect to be unhappy no matter
> what we do, so don't be too concerned when I say it bothers me :) And don't
> let me block you either, if everyone else agrees on this, then let's do it.
> And I definitely defer to ian@ here since he has a broader vision of Chrome
> use than I do.
> 
> That said, I think False Start is a nice step forward for SSL performance.
> Given the current size of the blacklist, aren't we able to use False Start
> for like 9X% of websites? Despite the aesthetic / philosophical
> deplorability of keeping a blacklist around in a zombie state, allowing our
> users to shave off an RTT off the SSL handshake for the vast (99%?)
> majority of https URLs seems like a nice win to me. I'm concerned that we
> are acting out of a very vague fear (do we have any good #s) about broken
> websites (which admittedly have a horrible failure mode), when we do know
> pretty well the cost of disabling False Start for all those websites.
> 
> I told Ian most of that in person so I suspect he already knows how I feel
> :P
> 
> On Tue, Apr 10, 2012 at 3:48 PM, Adam Langley <mailto:agl@google.com> wrote:
> 
> > On Tue, Apr 10, 2012 at 2:39 AM,  <mailto:willchan@chromium.org> wrote:
> > > This change makes me sad...I thought we were going to just revert to the
> > old
> > > blacklist and hang steady for awhile? That would also have made me sad,
> > but
> > > less
> > > sad than pretty much turning it off =/
> >
> > I think that keeping the current blacklist around in a zombie state is
> > the worst answer of all. So I think it's either the case that we
> > continue to be pretty aggressive in killing it (although working with
> > sites where we can), or we go with this answer. ifette: I cc'ed you
> > for this reason :)
> >
> >
> > Cheers
> >
> > AGL
> >

[commit-bot: I haz the power, 2012-04-10 20:04:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/agl@google.com/10014010/1

[commit-bot: I haz the power, 2012-04-10 22:34:39]

Change committed as 131649

[wtc, 2012-04-17 22:46:09]

Drive-by review comments:

https://chromiumcodereview.appspot.com/10014010/diff/1/net/socket/ssl_client_...
File net/socket/ssl_client_socket_nss.h (left):

https://chromiumcodereview.appspot.com/10014010/diff/1/net/socket/ssl_client_...
net/socket/ssl_client_socket_nss.h:150: void UncorkAfterTimeout();

It seems that the corking code is still useful to force
false-start-incompatible implementations to fail more
deterministically.

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

https://chromiumcodereview.appspot.com/10014010/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3con.c:6089: ssl3_ExtensionNegotiated(ss,
ssl_next_proto_nego_xtn) &&

If we do it this way, we need to update the documentation
of SSL_ENABLE_FALSE_START in the NSS ssl.h header file.

Alternatively, NSS can let applications control this.
Chrome would call
SSL_HandshakeNegotiatedExtension(fd, ssl_next_proto_nego_xtn)
in SSLClientSocketNSS::OwnAuthCertHandler, and call
SSL_OptionSet(fd, SSL_ENABLE_FALSE_START, PR_FALSE) to
set ss->opt.enableFalseStart to PR_FALSE.

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

https://chromiumcodereview.appspot.com/10014010/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3ext.c:570:
ss->xtnData.negotiated[ss->xtnData.numNegotiated++] = ex_type;

Nit: please add a TODO or XXX comment that we also need to
call ssl3_RegisterServerHelloExtensionSender to register a
server hello extension sender.