Compile src/net for PNaCl

Compile src/net for PNaCl

The new net_nacl target builds net code for PNaCl. It will be used for
remoting client, and so it excludes parts that are not used in the
remoting, e.g. net/disk_cache, net/proxy, net/spdy, net/quic.

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

[Ryan Sleevi, 2014-03-06 22:02:16]

Ok, having seen how much #ifdef soup this introduces in to net/, I'm convinced
this is not a real maintainable long-term solution.

I'm especially not fond of the continued exclusions for the nacl target.

How many more targets does remoting/ pull in? I almost wonder if this should be
discussed on chromium-dev, because this is to the level of supporting an entire
additional platform, and that's a rather heavy request. I can only presume this
will be a tree-breaker, and while that's good, it is also going to create some
degree of headache.

https://codereview.chromium.org/188873004/diff/20001/net/base/net_util.cc
File net/base/net_util.cc (right):

https://codereview.chromium.org/188873004/diff/20001/net/base/net_util.cc#new...
net/base/net_util.cc:27: #include <netinet/in.h>
Seems like this should be reworded as

#elif defined(OS_POSIX)
#include <fcntl.h>
#include <netdb.h>
#include <netinet/in.h>
#if !defined(OS_NACL)
#include <net/if.h>
#if !defined(OS_ANDROID)
#include <ifaddrs.h>
#endif
#endif
#endif

Unless I've missed somewhere that order matters.

This is to match
http://www.chromium.org/developers/coding-style#TOC-Platform-specific-code

https://codereview.chromium.org/188873004/diff/20001/net/base/net_util.cc#new...
net/base/net_util.cc:59: #endif
Likewise, it seems like this, lines 64-66, and 72-74 should all be moved to line
83, per
http://www.chromium.org/developers/coding-style#TOC-Platform-specific-code

https://codereview.chromium.org/188873004/diff/20001/net/cert/crl_set.cc
File net/cert/crl_set.cc (right):

https://codereview.chromium.org/188873004/diff/20001/net/cert/crl_set.cc#newc...
net/cert/crl_set.cc:49: #endif  // !defined(OS_NACL)
This approach is really ugly here. CRLSets won't work without this Zlib. Why
bother compiling this file at all? Why not provide a stub or exclude it?

[Sergey Ulanov, 2014-03-07 00:19:15]

On 2014/03/06 22:02:16, Ryan Sleevi wrote:
> Ok, having seen how much #ifdef soup this introduces in to net/, I'm convinced
> this is not a real maintainable long-term solution.
> 
> I'm especially not fond of the continued exclusions for the nacl target.

As I mentioned in the e-mail an alternative is to have a separate list of files
that are used in remoting. That approach would be more error-prone when files
added or removed. 
Also note that some of the exclusions (particularly net/spdy and net/quic)
compile just fine on NaCl - but I excluded them anyway because they are not
useful in a plugin and it doesn't make sense to waste time compiling unused
code.

> 
> How many more targets does remoting/ pull in? I almost wonder if this should
be
> discussed on chromium-dev, because this is to the level of supporting an
entire
> additional platform, and that's a rather heavy request. I can only presume
this
> will be a tree-breaker, and while that's good, it is also going to create some
> degree of headache.

Here is full list of external dependencies.
        '../base/base_nacl.gyp:base_i18n_nacl',
        '../base/base_nacl.gyp:base_nacl',
        '../crypto/crypto_nacl.gyp:crypto_nacl',
        '../jingle/jingle_nacl.gyp:jingle_glue_nacl',
        '../media/media_nacl.gyp:media_yuv_nacl',
        '../net/net_nacl.gyp:net_nacl',
        '../third_party/expat/expat_nacl.gyp:expat_nacl',
        '../third_party/icu/icu_nacl.gyp:icudata_nacl',
        '../third_party/icu/icu_nacl.gyp:icui18n_nacl',
        '../third_party/icu/icu_nacl.gyp:icuuc_nacl',
        '../third_party/libjingle/libjingle_nacl.gyp:libjingle_nacl',
        '../third_party/libwebm/libwebm_nacl.gyp:libwebm_nacl',
        '../third_party/libyuv/libyuv_nacl.gyp:libyuv_nacl',
        '../third_party/modp_b64/modp_b64_nacl.gyp:modp_b64_nacl',
        '../third_party/openssl/openssl_nacl.gyp:openssl_nacl',
        '../url/url_nacl.gyp:url_nacl',

I landed all of these already, except net_nacl, crypto_nacl and
jingle_glue_nacl. net_nacl is the ugliest one as there are more
platform-specific parts (beside maybe base_nacl, but we already had base_nacl
for other things, particularly for NaCl IRT).

FWIW, this not an entirely new platform in chromium - there are already many
existing targets for NaCl, which are used for NaCl tests and NaCl IRT.

[Sergey Ulanov, 2014-03-07 00:19:47]

https://codereview.chromium.org/188873004/diff/20001/net/cert/crl_set.cc
File net/cert/crl_set.cc (right):

https://codereview.chromium.org/188873004/diff/20001/net/cert/crl_set.cc#newc...
net/cert/crl_set.cc:49: #endif  // !defined(OS_NACL)
On 2014/03/06 22:02:17, Ryan Sleevi wrote:
> This approach is really ugly here. CRLSets won't work without this Zlib. Why
> bother compiling this file at all? Why not provide a stub or exclude it?

SSLClientSocket depends on CertVerifier and CertVerifier depends on CRLSet. This
can be solved by making CRLSet a virtual interface and moving and moving CRL
delta parsing to CRLSetImpl. WDYT?

[Ryan Sleevi, 2014-03-07 00:26:28]

On 2014/03/07 00:19:47, Sergey Ulanov wrote:
> https://codereview.chromium.org/188873004/diff/20001/net/cert/crl_set.cc
> File net/cert/crl_set.cc (right):
> 
>
https://codereview.chromium.org/188873004/diff/20001/net/cert/crl_set.cc#newc...
> net/cert/crl_set.cc:49: #endif  // !defined(OS_NACL)
> On 2014/03/06 22:02:17, Ryan Sleevi wrote:
> > This approach is really ugly here. CRLSets won't work without this Zlib. Why
> > bother compiling this file at all? Why not provide a stub or exclude it?
> 
> SSLClientSocket depends on CertVerifier and CertVerifier depends on CRLSet.
This
> can be solved by making CRLSet a virtual interface and moving and moving CRL
> delta parsing to CRLSetImpl. WDYT?

I'm not a fan of virtual inheritance to solve compile-time differences. It only
really makes sense to solve run-time differences.

You shouldn't need to link in CRLSet - neither cert_verifier.h nor
cert_verifier.cc require complete definitions of it. Only cert_verify_proc does,
and in PNaCl, there's no way you can be using CertVerifyProcOpenSSL.

If you do a CertVerifyProcNaCl, you should never need to compile crl_set.cc, and
can leave it as a forward declared abstract type, since it's never instantiated.

[Ryan Sleevi, 2014-03-07 00:28:31]

On 2014/03/07 00:19:15, Sergey Ulanov wrote:
> On 2014/03/06 22:02:16, Ryan Sleevi wrote:
> > Ok, having seen how much #ifdef soup this introduces in to net/, I'm
convinced
> > this is not a real maintainable long-term solution.
> > 
> > I'm especially not fond of the continued exclusions for the nacl target.
> 
> As I mentioned in the e-mail an alternative is to have a separate list of
files
> that are used in remoting. That approach would be more error-prone when files
> added or removed. 
> Also note that some of the exclusions (particularly net/spdy and net/quic)
> compile just fine on NaCl - but I excluded them anyway because they are not
> useful in a plugin and it doesn't make sense to waste time compiling unused
> code.

I'm wondering about whether or not it does make more sense to use a whitelist
approach. At least that discourages additions to net_nacl by definition.

> 
> > 
> > How many more targets does remoting/ pull in? I almost wonder if this should
> be
> > discussed on chromium-dev, because this is to the level of supporting an
> entire
> > additional platform, and that's a rather heavy request. I can only presume
> this
> > will be a tree-breaker, and while that's good, it is also going to create
some
> > degree of headache.
> 
> Here is full list of external dependencies.
>         '../base/base_nacl.gyp:base_i18n_nacl',
>         '../base/base_nacl.gyp:base_nacl',
>         '../crypto/crypto_nacl.gyp:crypto_nacl',
>         '../jingle/jingle_nacl.gyp:jingle_glue_nacl',
>         '../media/media_nacl.gyp:media_yuv_nacl',
>         '../net/net_nacl.gyp:net_nacl',
>         '../third_party/expat/expat_nacl.gyp:expat_nacl',
>         '../third_party/icu/icu_nacl.gyp:icudata_nacl',
>         '../third_party/icu/icu_nacl.gyp:icui18n_nacl',
>         '../third_party/icu/icu_nacl.gyp:icuuc_nacl',
>         '../third_party/libjingle/libjingle_nacl.gyp:libjingle_nacl',
>         '../third_party/libwebm/libwebm_nacl.gyp:libwebm_nacl',
>         '../third_party/libyuv/libyuv_nacl.gyp:libyuv_nacl',
>         '../third_party/modp_b64/modp_b64_nacl.gyp:modp_b64_nacl',
>         '../third_party/openssl/openssl_nacl.gyp:openssl_nacl',
>         '../url/url_nacl.gyp:url_nacl',
> 
> I landed all of these already, except net_nacl, crypto_nacl and
> jingle_glue_nacl. net_nacl is the ugliest one as there are more
> platform-specific parts (beside maybe base_nacl, but we already had base_nacl
> for other things, particularly for NaCl IRT).
> 
> FWIW, this not an entirely new platform in chromium - there are already many
> existing targets for NaCl, which are used for NaCl tests and NaCl IRT.

From the perspective of net/ code - which heavily relies on sockets, it might as
well be. That's where the concern comes from. You're slicing and dicing net/ up
(and I admittedly just encouraged more of said slicing and dicing)

[Sergey Ulanov, 2014-03-21 19:34:40]

PTAL. This change depends on https://codereview.chromium.org/208713004/
In the latest patch-set I've split the list of sources in net.gypi into two
parts: 
  net_sources_common - common parts compiled for NaCl,
  net_sources - everything else.

net_sources_common includes net/base, net/http, net/ssl, net/socket, net/udp,
and parts of net/cert for X509Certificate (and excludes net/cert code related to
cert verification). net_sources contains everything else. I'm hesitant of
slicing these directories further without moving files around, as it will only
make it more confusing and harder to maintain.

I still don't think this approach is easier to maintain that then one I had
implemented previously, with a single list of files and exclusions in
net_nacl.gyp.

An alternative solution is to fork SSLClientSocketOpenSSL in src/remoting and
clean up the dependencies on http stack and X509Certificate. That way the client
plugin would depend only on some parts of net/base, but will not need anything
else. WDYT?

https://codereview.chromium.org/188873004/diff/20001/net/base/net_util.cc
File net/base/net_util.cc (right):

https://codereview.chromium.org/188873004/diff/20001/net/base/net_util.cc#new...
net/base/net_util.cc:27: #include <netinet/in.h>
On 2014/03/06 22:02:17, Ryan Sleevi wrote:
> Seems like this should be reworded as
> 
> #elif defined(OS_POSIX)
> #include <fcntl.h>
> #include <netdb.h>
> #include <netinet/in.h>
> #if !defined(OS_NACL)
> #include <net/if.h>
> #if !defined(OS_ANDROID)
> #include <ifaddrs.h>
> #endif
> #endif
> #endif
> 
> Unless I've missed somewhere that order matters.
> 
> This is to match
> http://www.chromium.org/developers/coding-style#TOC-Platform-specific-code

Done.

https://codereview.chromium.org/188873004/diff/20001/net/base/net_util.cc#new...
net/base/net_util.cc:59: #endif
On 2014/03/06 22:02:17, Ryan Sleevi wrote:
> Likewise, it seems like this, lines 64-66, and 72-74 should all be moved to
line
> 83, per
> http://www.chromium.org/developers/coding-style#TOC-Platform-specific-code

Done.

[Sergey Ulanov, 2014-03-26 17:26:48]

rsleevi: ping

[Ryan Sleevi, 2014-03-27 01:04:40]

Correct me if I'm wrong, but there are no unittests that actually test the
NaCl'd implementation, are there? net_unittests is going to continue to run w/o
NaCl, and so it's up to the integration tests of remoting to catch if any
changes in net/ break something?

This is... Less than ideal.

I guess I still have (some small) issue with this, since we've set "Chromium
isn't a set of separable libraries", except we're doing just that here. The only
reason I'm not frothing here is that at least remoting/ is part of the Chromium
tree.

I do like the split of the target lists, as it avoids my primary concern of
making it too easy to spurious include code into the NaCl target. This at least
requires a conscious choice to add it to NaCl ( as opposed to a conscious choice
to remove it, when using source exclusions)

https://codereview.chromium.org/188873004/diff/40001/net/base/net_util.cc
File net/base/net_util.cc (right):

https://codereview.chromium.org/188873004/diff/40001/net/base/net_util.cc#new...
net/base/net_util.cc:1006: #if !defined(OS_NACL)
What part of this is broken under NACL?

https://codereview.chromium.org/188873004/diff/40001/net/base/net_util.cc#new...
net/base/net_util.cc:1102: #if !defined(OS_NACL)
Does this really need to be excluded - eg: if you packaged your resources with
your NaCL-netification?

https://codereview.chromium.org/188873004/diff/40001/net/base/net_util.cc#new...
net/base/net_util.cc:1970: NOTIMPLEMENTED();
Here, you intentionally NOTIMPLEMENTED(), but in GetNetworkList(), you don't.
What's the logic between the two failure modes?

https://codereview.chromium.org/188873004/diff/40001/net/net.gypi
File net/net.gypi (right):

https://codereview.chromium.org/188873004/diff/40001/net/net.gypi#newcode124
net/net.gypi:124: # TODO(tc): gnome-vfs? xdgmime? /etc/mime.types?
comment out of date here? Seems like it should just be nuked.

https://codereview.chromium.org/188873004/diff/40001/net/net_nacl.gyp
File net/net_nacl.gyp (right):

https://codereview.chromium.org/188873004/diff/40001/net/net_nacl.gyp#newcode4
net/net_nacl.gyp:4: 
Curious, why a separate GYP file for this? In other cases (eg: the 32-bit/64-bit
targets on Win), we've kept everything in a single .gyp file.

If it's for wildcard exclusions, you can just do that on a per-target basis.

https://codereview.chromium.org/188873004/diff/40001/net/net_nacl.gyp#newcode55
net/net_nacl.gyp:55: ],
Why include these in _common if they aren't _common?

[Sergey Ulanov, 2014-04-01 00:30:14]

PTAL.

Once the PNaCl build of remoting client plugin lands I'll try to make related
unittests from the remoting_unittest bundle run under PNaCl too - these tests
should be able to catch most regression that can be introduced in remoting by
changes in src/net.

https://codereview.chromium.org/188873004/diff/40001/net/base/net_util.cc
File net/base/net_util.cc (right):

https://codereview.chromium.org/188873004/diff/40001/net/base/net_util.cc#new...
net/base/net_util.cc:1006: #if !defined(OS_NACL)
On 2014/03/27 01:04:41, Ryan Sleevi wrote:
> What part of this is broken under NACL?

base_nacl doesn't compile PathService.

https://codereview.chromium.org/188873004/diff/40001/net/base/net_util.cc#new...
net/base/net_util.cc:1102: #if !defined(OS_NACL)
On 2014/03/27 01:04:41, Ryan Sleevi wrote:
> Does this really need to be excluded - eg: if you packaged your resources with
> your NaCL-netification?

Done.

https://codereview.chromium.org/188873004/diff/40001/net/base/net_util.cc#new...
net/base/net_util.cc:1970: NOTIMPLEMENTED();
On 2014/03/27 01:04:41, Ryan Sleevi wrote:
> Here, you intentionally NOTIMPLEMENTED(), but in GetNetworkList(), you don't.
> What's the logic between the two failure modes?

Added NOTIMPLEMENTED in GetNetworkList() too.

https://codereview.chromium.org/188873004/diff/40001/net/net.gypi
File net/net.gypi (right):

https://codereview.chromium.org/188873004/diff/40001/net/net.gypi#newcode124
net/net.gypi:124: # TODO(tc): gnome-vfs? xdgmime? /etc/mime.types?
On 2014/03/27 01:04:41, Ryan Sleevi wrote:
> comment out of date here? Seems like it should just be nuked.

Done.

https://codereview.chromium.org/188873004/diff/40001/net/net_nacl.gyp
File net/net_nacl.gyp (right):

https://codereview.chromium.org/188873004/diff/40001/net/net_nacl.gyp#newcode4
net/net_nacl.gyp:4: 
On 2014/03/27 01:04:41, Ryan Sleevi wrote:
> Curious, why a separate GYP file for this? In other cases (eg: the
32-bit/64-bit
> targets on Win), we've kept everything in a single .gyp file.
> 
> If it's for wildcard exclusions, you can just do that on a per-target basis.

It's because src/native_client may not always be present and this gyp file
depends on gypi files in src/native_client. Gyp will only need to process this
file when there are targets that depend on it and such target should be disabled
when disable_nacl_untrusted is set. So this file would not need to processed
when NaCl is disabled.

https://codereview.chromium.org/188873004/diff/40001/net/net_nacl.gyp#newcode55
net/net_nacl.gyp:55: ],
On 2014/03/27 01:04:41, Ryan Sleevi wrote:
> Why include these in _common if they aren't _common?

Moved these and bunch of other files to the non-common list now. Still keeping
some files with a platform-specific suffix in the _common list - they are
excluded by filename rules (e.g. tcp_socket_win.cc is still in the _common list,
because tcp_socket.h is there). Moving these to net_sources will make managing
the list of sources more confusing IMO.

[Sergey Ulanov, 2014-04-01 00:31:13]

Patch Set 4 depends on my change for filename_util.h (see
https://codereview.chromium.org/220253002/)

[Sergey Ulanov, 2014-04-10 01:09:18]

rsleevi: PTAL.
Patch Set 6 is much cleaner/smaller than the previous versions.

[Ryan Sleevi, 2014-04-10 04:33:22]

lgtm

[Sergey Ulanov, 2014-04-10 18:00:07]

The CQ bit was checked by sergeyu@chromium.org

[commit-bot: I haz the power, 2014-04-10 18:00:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/188873004/160001

[commit-bot: I haz the power, 2014-04-10 20:31:38]

Change committed as 263069