Ensure that NSS HTTP IO is initialized on iOS.

Ensure that NSS HTTP IO is initialized on iOS.

This continues the work of these two CLs:
https://chromiumcodereview.appspot.com/10983023
https://chromiumcodereview.appspot.com/11293235

SSLServerSocketNSS does not use NSS HTTP IO, so it doesn't need
to include nss_ocsp.h.

Use defined(OS_MACOSX) && !defined(OS_IOS) to detect Mac OS X
(excluding iOS).

R=blundell@chromium.org,droger@chromium.org,rsleevi@chromium.org
BUG=none
TEST=none

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

[wtc, 2012-11-28 20:43:24]

blundell,droger: I'd appreciate it if one of you could test
this CL on iOS because I'm unable to do that. Feel free to
create your own CL based on the patch. Thanks.

Could you confirm that chrome/browser/io_thread.cc is not
used for iOS?

[Ryan Sleevi, 2012-11-28 21:56:13]

https://codereview.chromium.org/11434009/diff/1/net/url_request/url_fetcher_i...
File net/url_request/url_fetcher_impl_unittest.cc (right):

https://codereview.chromium.org/11434009/diff/1/net/url_request/url_fetcher_i...
net/url_request/url_fetcher_impl_unittest.cc:123: #endif
Broader question: Do we need these at all in the unit test?

This code doesn't make use of any NSS specific functionality. The fact that
SSLClientSocketNSS uses NSS "should be" transparent here to this test.

EnsureNSSSSLInit (called by SSLClientSocketNSS) will ensure that
crypto::EnsureNSSInit is called, and SSLClientSocketNSS will also ensure that
EnsureNSSHttpIOInit() is called on the right thread when starting up.

I can see the argument for calling EnsureNSSHttpIOInit / ShutdownNSSHttpIO as a
general test cleanup, but I think that should be by whatever sets up the
base::MessageLoop for this thread (which I can't seem to find?

https://codereview.chromium.org/11434009/diff/1/net/url_request/url_request_u...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/11434009/diff/1/net/url_request/url_request_u...
net/url_request/url_request_unittest.cc:4493: #endif
Are these tests somehow disabled on iOS? I suspect that they're falling into the
OS_MACOSX rules and thus not running.

If so, more than OS_MACOSX needs to be updated here in this file.

[droger, 2012-11-29 08:32:26]

@wtc:
io_thread.cc is used on iOS.

https://codereview.chromium.org/11434009/diff/1/net/url_request/url_fetcher_i...
File net/url_request/url_fetcher_impl_unittest.cc (right):

https://codereview.chromium.org/11434009/diff/1/net/url_request/url_fetcher_i...
net/url_request/url_fetcher_impl_unittest.cc:123: #endif
On 2012/11/28 21:56:13, Ryan Sleevi wrote:
> Broader question: Do we need these at all in the unit test?
> 
> This code doesn't make use of any NSS specific functionality. The fact that
> SSLClientSocketNSS uses NSS "should be" transparent here to this test.
> 
> EnsureNSSSSLInit (called by SSLClientSocketNSS) will ensure that
> crypto::EnsureNSSInit is called, and SSLClientSocketNSS will also ensure that
> EnsureNSSHttpIOInit() is called on the right thread when starting up.
> 
> I can see the argument for calling EnsureNSSHttpIOInit / ShutdownNSSHttpIO as
a
> general test cleanup, but I think that should be by whatever sets up the
> base::MessageLoop for this thread (which I can't seem to find?

FYI, these tests are excluded on iOS (at the gyp level) because of the test
server. I think we should still try to do the right thing, in case these tests
are enabled in the future.

https://codereview.chromium.org/11434009/diff/1/net/url_request/url_request_u...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/11434009/diff/1/net/url_request/url_request_u...
net/url_request/url_request_unittest.cc:1333: #if !defined(OS_IOS)
Disabled tests on iOS.

https://codereview.chromium.org/11434009/diff/1/net/url_request/url_request_u...
net/url_request/url_request_unittest.cc:4493: #endif
On 2012/11/28 21:56:13, Ryan Sleevi wrote:
> Are these tests somehow disabled on iOS? I suspect that they're falling into
the
> OS_MACOSX rules and thus not running.
> 
> If so, more than OS_MACOSX needs to be updated here in this file.

Some tests in this file are disabled on iOS because we don't have the test
server.
See comment above.

[droger, 2012-11-29 09:08:27]

On 2012/11/28 20:43:24, wtc wrote:
> blundell,droger: I'd appreciate it if one of you could test
> this CL on iOS because I'm unable to do that. Feel free to
> create your own CL based on the patch. Thanks.

I tried the patch on the bots and it compiled.
The unittests were not run (they will be enabled soon), however the changes in
the tests are in excluded code, so I don't think it could fail.

This change is still worth doing though (unless it requires too much effort). We
actually have an internal upstreaming bug for the url_fetcher test that your CL
fixes: b/7337371.

[blundell, 2012-11-29 09:24:50]

Looks fine to me. I patched this in downstream and it (unsurprisingly) causes no
problems.

[wtc, 2012-11-29 19:24:32]

Thank you all for your comments. Please review patch set 2.

Note: I generated this CL mechanially by searching for
"NSSHttpIO" in all the .cc files in a Chromium source tree
checked out on Linux. (All the functions declared in
net/base/nss_ocsp.h have "NSSHttpIO" in their names.)

I included io_thread.cc in the CL now that I know it is also
used for iOS.

https://codereview.chromium.org/11434009/diff/1/net/url_request/url_fetcher_i...
File net/url_request/url_fetcher_impl_unittest.cc (right):

https://codereview.chromium.org/11434009/diff/1/net/url_request/url_fetcher_i...
net/url_request/url_fetcher_impl_unittest.cc:123: #endif

On 2012/11/28 21:56:13, Ryan Sleevi wrote:
> Broader question: Do we need these at all in the unit test?

Good question. I haven't found the answer. Because of some
refactoring, it is not easy to research why these two calls
were added in SVN history.

> I can see the argument for calling EnsureNSSHttpIOInit / ShutdownNSSHttpIO as
a
> general test cleanup, but I think that should be by whatever sets up the
> base::MessageLoop for this thread (which I can't seem to find?

If you find that place, please let me know.

https://codereview.chromium.org/11434009/diff/1/net/url_request/url_request_u...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/11434009/diff/1/net/url_request/url_request_u...
net/url_request/url_request_unittest.cc:4493: #endif

On 2012/11/28 21:56:13, Ryan Sleevi wrote:
> Are these tests somehow disabled on iOS? I suspect that they're falling into
the
> OS_MACOSX rules and thus not running.
> 
> If so, more than OS_MACOSX needs to be updated here in this file.

I don't know what you meant by "the OS_MACOSX rules". I
searched for "OS_MACOSX" and "OS X" in this file and fixed
all the problems I spotted. Please take a look at patch set
2.

https://codereview.chromium.org/11434009/diff/8001/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/11434009/diff/8001/chrome/browser/io_thread.c...
chrome/browser/io_thread.cc:65: #endif  // defined(USE_NSS) || defined(OS_IOS)

Please let me know if you prefer that I simply remove these
#endif comments. I think they clutter the code when the
conditionally-compiled section of code is very short.

[Ryan Sleevi, 2012-11-29 20:35:13]

Thanks Wan-Teh.

As a mechnical change, this LGTM. We can address the layering of the test code
needing to do this in a separate CL, but I see no reason not to include these
fixes.

https://codereview.chromium.org/11434009/diff/8001/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/11434009/diff/8001/chrome/browser/io_thread.c...
chrome/browser/io_thread.cc:65: #endif  // defined(USE_NSS) || defined(OS_IOS)
On 2012/11/29 19:24:33, wtc wrote:
> 
> Please let me know if you prefer that I simply remove these
> #endif comments. I think they clutter the code when the
> conditionally-compiled section of code is very short.

Agreed

[commit-bot: I haz the power, 2012-11-30 17:47:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wtc@chromium.org/11434009/12001

[commit-bot: I haz the power, 2012-11-30 17:47:09]

Presubmit check for 11434009-12001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

[wtc, 2012-11-30 23:26:05]

thakis: could you review the changes to chrome/browser/io_thread.cc
in this CL? Thanks.

[Nico, 2012-12-01 01:41:21]

https://codereview.chromium.org/11434009/diff/7002/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/11434009/diff/7002/chrome/browser/io_thread.c...
chrome/browser/io_thread.cc:70: #if defined(USE_NSS) || defined(OS_IOS)
Is NSS used on IOS? If so, should USE_NSS be set when building on ios? That
seems simpler.

[Ryan Sleevi, 2012-12-01 02:09:12]

On 2012/12/01 01:41:21, Nico wrote:
> https://codereview.chromium.org/11434009/diff/7002/chrome/browser/io_thread.cc
> File chrome/browser/io_thread.cc (right):
> 
>
https://codereview.chromium.org/11434009/diff/7002/chrome/browser/io_thread.c...
> chrome/browser/io_thread.cc:70: #if defined(USE_NSS) || defined(OS_IOS)
> Is NSS used on IOS? If so, should USE_NSS be set when building on ios? That
> seems simpler.

USE_NSS implies a fair bit of UI functionality, as well as that NSS is being
used "as it's supposed to be" (eg: as a shared library managed by the system).

The use of the #ifdef system here to mean "NSS is being used for cert
validation" may need to change soon, as we start mixing and matching "NSS being
used for chain building, but trust coming from the OS"

Very much intentional to whitelist these ifdefs than blacklist.

[wtc, 2012-12-01 02:19:43]

https://codereview.chromium.org/11434009/diff/7002/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/11434009/diff/7002/chrome/browser/io_thread.c...
chrome/browser/io_thread.cc:70: #if defined(USE_NSS) || defined(OS_IOS)

On 2012/12/01 01:41:21, Nico wrote:
> Is NSS used on IOS? If so, should USE_NSS be set when building on ios? That
> seems simpler.

NSS is used on iOS to verify certificates. The test
    #if defined(USE_NSS) || defined(OS_IOS)
identifies the platforms where we use NSS to verify certificates.

The meaning of USE_NSS changed several times. The current
meaning of USE_NSS is roughly "use the system NSS libraries".
If USE_NSS is not defined, then we either
1. use our own copy of NSS, or
2. use OpenSSL.

iOS falls under case 1 -- Chrome for iOS contains its own copy
of NSS. So we cannot define USE_NSS for iOS.

If we want to clean up this mess, I think we can do the following.
- Rename USE_NSS to USE_SYSTEM_NSS.
- Add a new macro VERIFY_CERTS_WITH_NSS, which is defined
  if USE_SYSTEM_NSS is defined or OS_IOS is defined.

But that will be a separate (big) CL.

[Nico, 2012-12-01 03:45:52]

On 2012/12/01 02:19:43, wtc wrote:
> https://codereview.chromium.org/11434009/diff/7002/chrome/browser/io_thread.cc
> File chrome/browser/io_thread.cc (right):
> 
>
https://codereview.chromium.org/11434009/diff/7002/chrome/browser/io_thread.c...
> chrome/browser/io_thread.cc:70: #if defined(USE_NSS) || defined(OS_IOS)
> 
> On 2012/12/01 01:41:21, Nico wrote:
> > Is NSS used on IOS? If so, should USE_NSS be set when building on ios? That
> > seems simpler.
> 
> NSS is used on iOS to verify certificates. The test
>     #if defined(USE_NSS) || defined(OS_IOS)
> identifies the platforms where we use NSS to verify certificates.
> 
> The meaning of USE_NSS changed several times. The current
> meaning of USE_NSS is roughly "use the system NSS libraries".
> If USE_NSS is not defined, then we either
> 1. use our own copy of NSS, or
> 2. use OpenSSL.
> 
> iOS falls under case 1 -- Chrome for iOS contains its own copy
> of NSS. So we cannot define USE_NSS for iOS.
> 
> If we want to clean up this mess, I think we can do the following.
> - Rename USE_NSS to USE_SYSTEM_NSS.
> - Add a new macro VERIFY_CERTS_WITH_NSS, which is defined
>   if USE_SYSTEM_NSS is defined or OS_IOS is defined.
> 
> But that will be a separate (big) CL.

Ok, that makes sense, lgtm. Please file a bug to clean this up eventually
though.

[commit-bot: I haz the power, 2012-12-01 15:05:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wtc@chromium.org/11434009/7002

[commit-bot: I haz the power, 2012-12-01 18:10:31]

Change committed as 170669