net: don't set NSS options in a callback.

net: don't set NSS options in a callback.

This debugger traps in debug mode due to the locks held at the time that the callback is made.

BUG=125299
TEST=none

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

[wtc, 2012-04-27 20:52:37]

Patch set 1 LGTM.

By "console spam", I guess you meant NSS assertion failures?
NSS assertion failures should be fatal because they call abort().  So "console
spam" sounds like you only saw error
messages.

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

https://chromiumcodereview.appspot.com/10221018/diff/1/net/socket/ssl_client_...
net/socket/ssl_client_socket_nss.cc:2114: SSL_OptionSet(socket,
SSL_ENABLE_FALSE_START, PR_FALSE);

I guess it is the SSL_OptionSet call, not the
SSL_HandshakeNegotiatedExtension call, that triggers console
spam in debug mode?

There is another SSL_HandshakeNegotiatedExtension call below
in SSLClientSocketNSS::DomainBoundCertNegotiated(), which is
also called while NSS holds the same locks.  So calling
SSL_HandshakeNegotiatedExtension should be fine.

I wonder if we should change SSL_OptionSet and SSL_OptionGet
to only get SSL3HandshakeLock, just like
SSL_HandshakeNegotiatedExtension does.  On the other hand,
it is risky to change a handshake-related socket option
during the handshake.  We know it is safe to change
SSL_ENABLE_FALSE_START at this point because that option
only matters later in the handshake process, but in general
it is hard to determine this.

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

https://chromiumcodereview.appspot.com/10221018/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3con.c:6104: * would be called.

Nit: would be nice to add a comment about the NPN extension
check here.

[agl, 2012-04-30 16:53:31]

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

https://chromiumcodereview.appspot.com/10221018/diff/1/net/socket/ssl_client_...
net/socket/ssl_client_socket_nss.cc:2114: SSL_OptionSet(socket,
SSL_ENABLE_FALSE_START, PR_FALSE);
On 2012/04/27 20:52:37, wtc wrote:
> I guess it is the SSL_OptionSet call, not the
> SSL_HandshakeNegotiatedExtension call, that triggers console
> spam in debug mode?

rsleevi clarified that the problem was the debug assertion trapping into the
debugging rather than console spam in crbug.com/125299. I believe that
DomainBoundCertNegotiated will have the same problem, but it's much less
commonly enabled.

This suggests that a wider ranging solution might be needed. But the addition of
special, public calls that don't take a lock is a little ugly and certainly
debatable. In the mean time it seems that this is causing developer pain so I'll
go with the short term solution for now.

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

https://chromiumcodereview.appspot.com/10221018/diff/1/net/third_party/nss/ss...
net/third_party/nss/ssl/ssl3con.c:6104: * would be called.
On 2012/04/27 20:52:37, wtc wrote:
> 
> Nit: would be nice to add a comment about the NPN extension
> check here.

Done (and patch updated).

[commit-bot: I haz the power, 2012-04-30 16:53:51]

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

[commit-bot: I haz the power, 2012-04-30 20:25:33]

Change committed as 134584