net: always enable NSS's HTTP fetches

net: always enable NSS's HTTP fetches

By tying them to revocation checking and disabling revocation checking by
default, I broke AIA chasing on Linux.

This change also renames the public functions in nss_ocsp.cc to better reflect
that HTTP fetching is used for more than just OCSP.

BUG=117832
TEST=none (yet)

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=126637

[Ryan Sleevi, 2012-03-12 19:23:57]

Thanks for the quick fix Adam.

One suggestion from the nit-picker brigade: I'm not sure "ForOCSPAndAIA" is much
better naming. After all, OCSP is part of AIA - and these functions are also
used for CRLs.

Perhaps a more general NssPkixIO as the typename? NSSUrlIO?

It also raises the question if we should move this code out of net/ocsp/,
although that's a discussion for a later date. IIRC, it's not under nss/base/
because it depends on URLRequestContext, which is part of net/url_request, which
net/base/ can't depend on.

https://chromiumcodereview.appspot.com/9693004/diff/1/net/ocsp/nss_ocsp.h
File net/ocsp/nss_ocsp.h (right):

https://chromiumcodereview.appspot.com/9693004/diff/1/net/ocsp/nss_ocsp.h#new...
net/ocsp/nss_ocsp.h:17: // control the message loop for OCSP.
Comment updates for this method and others?

Perhaps this should be

SetMessageLoopForNssPkixIO?

EnsureNssPkixIOInit()

ShutdownNssPkixIO()?

SetURLRequestContextForNssPkixIO()?

[agl, 2012-03-12 21:15:11]

On 2012/03/12 19:23:57, Ryan Sleevi wrote:
> Perhaps a more general NssPkixIO as the typename? NSSUrlIO?

Yep, you're right. That's a better name, thanks. I prefer uppercase acronyms but
NSSPKIXIO is a bit of a lump, so NssPkixIO it is.

[Ryan Sleevi, 2012-03-12 21:26:34]

lgtm

[Ryan Sleevi, 2012-03-12 21:26:58]

https://chromiumcodereview.appspot.com/9693004/diff/6004/net/ocsp/nss_ocsp.h
File net/ocsp/nss_ocsp.h (right):

https://chromiumcodereview.appspot.com/9693004/diff/6004/net/ocsp/nss_ocsp.h#...
net/ocsp/nss_ocsp.h:17: // control the message loop for OCSP.
Comment updates?

[wtc, 2012-03-12 22:38:10]

Patch set 4 LGTM.  I suggest different function names.
Also the comments in net/ocsp/nss_ocsp.h aren't fully
updated.

https://chromiumcodereview.appspot.com/9693004/diff/1037/content/common/net/u...
File content/common/net/url_fetcher_impl_unittest.cc (right):

https://chromiumcodereview.appspot.com/9693004/diff/1037/content/common/net/u...
content/common/net/url_fetcher_impl_unittest.cc:68: net::EnsureNssPkixIOInit();

We should try to spell "NSS" in all caps, otherwise these two
function calls look a little inconsistent.

https://chromiumcodereview.appspot.com/9693004/diff/1037/net/ocsp/nss_ocsp.h
File net/ocsp/nss_ocsp.h (right):

https://chromiumcodereview.appspot.com/9693004/diff/1037/net/ocsp/nss_ocsp.h#...
net/ocsp/nss_ocsp.h:15: // Sets the MessageLoop for NSS's PKIX related IO (i.e.
OCSP, AIA and CRL

Nit: PKIX related IO => HTTP client functions

This reflects the name of the NSS function
SEC_RegisterDefaultHttpClient:
http://mxr.mozilla.org/security/ident?i=SEC_RegisterDefaultHttpClient

Nit: AIA => CA certificate

https://chromiumcodereview.appspot.com/9693004/diff/1037/net/ocsp/nss_ocsp.h#...
net/ocsp/nss_ocsp.h:17: // EnsureOCSPInit() if you want to control the message
loop for OCSP.

EnsureOCSPInit() and "for OCSP" need to be updated.

https://chromiumcodereview.appspot.com/9693004/diff/1037/net/ocsp/nss_ocsp.h#...
net/ocsp/nss_ocsp.h:18: NET_EXPORT void SetMessageLoopForNssPkixIO();

Please name this NSSHttpClient or NSSHttpIO.  NSS can use it
for any HTTP operation, not limited to use by libpkix.

https://chromiumcodereview.appspot.com/9693004/diff/1037/net/ocsp/nss_ocsp.h#...
net/ocsp/nss_ocsp.h:20: // Initializes IO handlers for NSS.  This must be called
before any

IO handlers => HTTP client functions

The "OCSP handlers" on lines 21-22 should also be changed.

https://chromiumcodereview.appspot.com/9693004/diff/1037/net/ocsp/nss_ocsp.h#...
net/ocsp/nss_ocsp.h:30: // Set URLRequestContext for PKIX related fetches.
On lines 26-27 and this line, change
PKIX related fetches => HTTP requests issued by NSS

or you can say "certificate related HTTP requests".

I think "PKIX" is a little obscure.

https://chromiumcodereview.appspot.com/9693004/diff/1037/net/socket/ssl_clien...
File net/socket/ssl_client_socket_nss.cc (left):

https://chromiumcodereview.appspot.com/9693004/diff/1037/net/socket/ssl_clien...
net/socket/ssl_client_socket_nss.cc:823: if (ssl_config_.rev_checking_enabled) {

Just to confirm: the removal of this "if" is the bug fix,
right?

[agl, 2012-03-13 15:14:05]

https://chromiumcodereview.appspot.com/9693004/diff/1037/content/common/net/u...
File content/common/net/url_fetcher_impl_unittest.cc (right):

https://chromiumcodereview.appspot.com/9693004/diff/1037/content/common/net/u...
content/common/net/url_fetcher_impl_unittest.cc:68: net::EnsureNssPkixIOInit();
On 2012/03/12 22:38:11, wtc wrote:
> 
> We should try to spell "NSS" in all caps, otherwise these two
> function calls look a little inconsistent.

Done.

https://chromiumcodereview.appspot.com/9693004/diff/1037/net/ocsp/nss_ocsp.h
File net/ocsp/nss_ocsp.h (right):

https://chromiumcodereview.appspot.com/9693004/diff/1037/net/ocsp/nss_ocsp.h#...
net/ocsp/nss_ocsp.h:17: // EnsureOCSPInit() if you want to control the message
loop for OCSP.
On 2012/03/12 22:38:11, wtc wrote:
> 
> EnsureOCSPInit() and "for OCSP" need to be updated.

Done.

https://chromiumcodereview.appspot.com/9693004/diff/1037/net/ocsp/nss_ocsp.h#...
net/ocsp/nss_ocsp.h:18: NET_EXPORT void SetMessageLoopForNssPkixIO();
On 2012/03/12 22:38:11, wtc wrote:
> 
> Please name this NSSHttpClient or NSSHttpIO.  NSS can use it
> for any HTTP operation, not limited to use by libpkix.

Renamed to NSSHttpIO.

https://chromiumcodereview.appspot.com/9693004/diff/1037/net/ocsp/nss_ocsp.h#...
net/ocsp/nss_ocsp.h:20: // Initializes IO handlers for NSS.  This must be called
before any
On 2012/03/12 22:38:11, wtc wrote:
> 
> IO handlers => HTTP client functions
> 
> The "OCSP handlers" on lines 21-22 should also be changed.

Done.

https://chromiumcodereview.appspot.com/9693004/diff/1037/net/ocsp/nss_ocsp.h#...
net/ocsp/nss_ocsp.h:30: // Set URLRequestContext for PKIX related fetches.
On 2012/03/12 22:38:11, wtc wrote:
> On lines 26-27 and this line, change
> PKIX related fetches => HTTP requests issued by NSS
> 
> or you can say "certificate related HTTP requests".
> 
> I think "PKIX" is a little obscure.

Done.

https://chromiumcodereview.appspot.com/9693004/diff/1037/net/socket/ssl_clien...
File net/socket/ssl_client_socket_nss.cc (left):

https://chromiumcodereview.appspot.com/9693004/diff/1037/net/socket/ssl_clien...
net/socket/ssl_client_socket_nss.cc:823: if (ssl_config_.rev_checking_enabled) {
On 2012/03/12 22:38:11, wtc wrote:
> Just to confirm: the removal of this "if" is the bug fix,
> right?

Yep.

[commit-bot: I haz the power, 2012-03-13 15:18:28]

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

[commit-bot: I haz the power, 2012-03-13 16:04:26]

Try job failure for 9693004-51 (retry) on linux_rel for step
"remoting_unittests".
It's a second try, previously, step "remoting_unittests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...