[third-party] Netty fork of Tomcat Native

[third-party] Netty Server and dependencies

Added new libraries to the Chromium third-party library collection:
1. Netty 4 Server. See http://netty.io
2. Netty fork of Tomcat Native (netty-tcnative). See http://netty.io/wiki/forked-tomcat-native.html
and the library tc-native depends on:
3. 'Apache Portable Runtime'. See http://apr.apache.org

The server provides support for Http/2 protocol, which is needed for testing some of the Cronet features
on Android, in particular, the new Bidirectional Streaming API.

To build the netty-tcnative library call:
ninja -C out/Debug netty-tcnative

The output files can be found here:
- out/Debug/lib/libnetty-tcnative.so
- out/Debug/lib.java/netty-tcnative.jar

BUG=563732
Committed: https://crrev.com/6dd98af3db0b8e809ed95ff66620181efbf3f8f1
Cr-Commit-Position: refs/heads/master@{#370791}

Committed: https://crrev.com/810c220259edb6a0b79ca1c16e8d7fb71aa258ce
Cr-Commit-Position: refs/heads/master@{#372172}

[kapishnikov, 2015-12-16 22:26:42]

kapishnikov@chromium.org changed reviewers:
+ mef@chromium.org, pauljensen@chromium.org, xunjieli@chromium.org

[kapishnikov, 2015-12-16 22:26:46]

xunjieli@chromium.org: Please review changes in 

pauljensen@chromium.org: Please review changes in 

mef@chromium.org: Please review changes in

[kapishnikov, 2015-12-16 22:33:04]

I have added the latest netty-tcnative and apache-portable-runtime files from
the repos as-is with minimal modifications to make them compile for Android. If
the size is critical, we can try to delete some files that are not needed, e.g.
unit tests, etc.

[xunjieli, 2015-12-16 23:00:19]

On 2015/12/16 22:33:04, kapishnikov wrote:
> I have added the latest netty-tcnative and apache-portable-runtime files from
> the repos as-is with minimal modifications to make them compile for Android.
If
> the size is critical, we can try to delete some files that are not needed,
e.g.
> unit tests, etc.

This is great. Thanks, Andrei. Could you draft a design doc and send it out to
chrome-eng-review per instructions at
http://dev.chromium.org/developers/adding-3rd-party-libraries? I think we should
seek approval first.

[kapishnikov, 2015-12-17 01:36:14]

On 2015/12/16 23:00:19, xunjieli wrote:
> On 2015/12/16 22:33:04, kapishnikov wrote:
> > I have added the latest netty-tcnative and apache-portable-runtime files
from
> > the repos as-is with minimal modifications to make them compile for Android.
> If
> > the size is critical, we can try to delete some files that are not needed,
> e.g.
> > unit tests, etc.
> 
> This is great. Thanks, Andrei. Could you draft a design doc and send it out to
> chrome-eng-review per instructions at
> http://dev.chromium.org/developers/adding-3rd-party-libraries I think we
should
> seek approval first.

Yes, I will do it. Thanks for the link.

[mef, 2015-12-18 16:40:16]

On 2015/12/17 01:36:14, kapishnikov wrote:
> On 2015/12/16 23:00:19, xunjieli wrote:
> > On 2015/12/16 22:33:04, kapishnikov wrote:
> > > I have added the latest netty-tcnative and apache-portable-runtime files
> from
> > > the repos as-is with minimal modifications to make them compile for
Android.
> > If
> > > the size is critical, we can try to delete some files that are not needed,
> > e.g.
> > > unit tests, etc.
> > 
> > This is great. Thanks, Andrei. Could you draft a design doc and send it out
to
> > chrome-eng-review per instructions at
> > http://dev.chromium.org/developers/adding-3rd-party-libraries I think we
> should
> > seek approval first.
> 
> Yes, I will do it. Thanks for the link.

Quick update: I was unable to patch this using git cl (it says that patchset is
too big), but I've applied diff manually, and it builds on my machine.
We do also need netty itself in addition to this.

[kapishnikov, 2016-01-05 15:38:30]

I have filed a ticket with the infrastructure team to create two new repos on
googlesource.com:

https://code.google.com/p/chromium/issues/detail?id=574216

[kapishnikov, 2016-01-06 21:48:43]

Description was changed from

==========
[third-party] Netty fork of Tomcat Native

Adds a new library to  the third-party library collection:
1. Netty fork of Tomcat Native (tc-native). See
http://netty.io/wiki/forked-tomcat-native.html
and the library tc-native depends on:
2. 'Apache Portable Runtime'. See http://apr.apache.org

tc-native is required by the Netty server that we are planning to use for HTTP/2
and bidirectional socket testing.

To build the library call:
ninja -C out/Debug netty-tcnative

The output files can be found here:
- out/Debug/lib/libnetty-tcnative.so
- out/Debug/lib.java/netty-tcnative.jar

The files that require the review are:
- components/cronet.gypi
- third_party/apache-portable-runtime/apr.gyp
- third_party/netty-tcnative/netty-tcnative.gyp

The other files are added as-is with some modifications so that they can be
built for Android. The modified files are:
- third_party/apache-portable-runtime/include/apr.h
- third_party/apache-portable-runtime/include/arch/unix/apr_private.h
- third_party/netty-tcnative/c/ssl.c
- DEPS

This is a preliminary change list that will need the approval to add the
third-party libraries to the Chromium project.

BUG=563732
==========

to

==========
[third-party] Netty Server and dependencies

Added new libraries to the Chromium third-party library collection:
1. Netty 4 Server. See http://netty.io
2. Netty fork of Tomcat Native (netty-tcnative). See
http://netty.io/wiki/forked-tomcat-native.html
and the library tc-native depends on:
3. 'Apache Portable Runtime'. See http://apr.apache.org

The server provides support for Http/2 protocol, which is needed for testing
some of the Cronet features
on Android, in particular, the new Bidirectional Streaming API.

To build the netty-tcnative library call:
ninja -C out/Debug netty-tcnative

The output files can be found here:
- out/Debug/lib/libnetty-tcnative.so
- out/Debug/lib.java/netty-tcnative.jar

BUG=563732
==========

[kapishnikov, 2016-01-08 18:16:15]

There is also a pending request to create netty4 external repository:
https://code.google.com/p/chromium/issues/detail?id=574809

[mef, 2016-01-11 21:46:03]

https://codereview.chromium.org/1537473002/diff/50001/components/cronet.gypi
File components/cronet.gypi (right):

https://codereview.chromium.org/1537473002/diff/50001/components/cronet.gypi#...
components/cronet.gypi:392: 'run_findbugs': 1,
In order for libnetty-tcnative.so to be included into CronetTest.apk this needs
additional_bundled_libs variable:

+            'additional_bundled_libs': [
+              '<(PRODUCT_DIR)/lib/libnetty-tcnative.so',
+            ],

https://codereview.chromium.org/1537473002/diff/50001/third_party/netty-tcnat...
File third_party/netty-tcnative/netty-tcnative.gyp (right):

https://codereview.chromium.org/1537473002/diff/50001/third_party/netty-tcnat...
third_party/netty-tcnative/netty-tcnative.gyp:40:
'../boringssl/src/decrepit/bio/base64_bio.c',
It is a bit weird that it builds files from another third_party project. 
If those are not built by current boringssl build targets, then maybe we should
add another target to ../boringssl project?

https://codereview.chromium.org/1537473002/diff/50001/third_party/netty4/OWNERS
File third_party/netty4/OWNERS (right):

https://codereview.chromium.org/1537473002/diff/50001/third_party/netty4/OWNE...
third_party/netty4/OWNERS:1: kapishnikov@chromium.org
This project also needs a gyp file, similar to this:

https://codereview.chromium.org/1529643002/diff/60001/third_party/netty/netty...

[kapishnikov, 2016-01-12 15:50:26]

See the changes below. Also uploaded the nightly Netty build and changed the
hash in DEPS.

https://codereview.chromium.org/1537473002/diff/50001/components/cronet.gypi
File components/cronet.gypi (right):

https://codereview.chromium.org/1537473002/diff/50001/components/cronet.gypi#...
components/cronet.gypi:392: 'run_findbugs': 1,
On 2016/01/11 21:46:03, mef wrote:
> In order for libnetty-tcnative.so to be included into CronetTest.apk this
needs
> additional_bundled_libs variable:
> 
> +            'additional_bundled_libs': [
> +              '<(PRODUCT_DIR)/lib/libnetty-tcnative.so',
> +            ],

Done.

https://codereview.chromium.org/1537473002/diff/50001/third_party/netty-tcnat...
File third_party/netty-tcnative/netty-tcnative.gyp (right):

https://codereview.chromium.org/1537473002/diff/50001/third_party/netty-tcnat...
third_party/netty-tcnative/netty-tcnative.gyp:40:
'../boringssl/src/decrepit/bio/base64_bio.c',
On 2016/01/11 21:46:03, mef wrote:
> It is a bit weird that it builds files from another third_party project. 
> If those are not built by current boringssl build targets, then maybe we
should
> add another target to ../boringssl project?

Added a new target "boringssl-decrepit" in boringssl.gyp that compiles all
decrepit BoringSSL sources. Referencing this target from here.

https://codereview.chromium.org/1537473002/diff/50001/third_party/netty4/OWNERS
File third_party/netty4/OWNERS (right):

https://codereview.chromium.org/1537473002/diff/50001/third_party/netty4/OWNE...
third_party/netty4/OWNERS:1: kapishnikov@chromium.org
On 2016/01/11 21:46:03, mef wrote:
> This project also needs a gyp file, similar to this:
> 
>
https://codereview.chromium.org/1529643002/diff/60001/third_party/netty/netty...

Done.

[mef, 2016-01-12 20:57:55]

On 2016/01/12 15:50:26, kapishnikov wrote:
> See the changes below. Also uploaded the nightly Netty build and changed the
> hash in DEPS.
> 
> https://codereview.chromium.org/1537473002/diff/50001/components/cronet.gypi
> File components/cronet.gypi (right):
> 
>
https://codereview.chromium.org/1537473002/diff/50001/components/cronet.gypi#...
> components/cronet.gypi:392: 'run_findbugs': 1,
> On 2016/01/11 21:46:03, mef wrote:
> > In order for libnetty-tcnative.so to be included into CronetTest.apk this
> needs
> > additional_bundled_libs variable:
> > 
> > +            'additional_bundled_libs': [
> > +              '<(PRODUCT_DIR)/lib/libnetty-tcnative.so',
> > +            ],
> 
> Done.
> 
>
https://codereview.chromium.org/1537473002/diff/50001/third_party/netty-tcnat...
> File third_party/netty-tcnative/netty-tcnative.gyp (right):
> 
>
https://codereview.chromium.org/1537473002/diff/50001/third_party/netty-tcnat...
> third_party/netty-tcnative/netty-tcnative.gyp:40:
> '../boringssl/src/decrepit/bio/base64_bio.c',
> On 2016/01/11 21:46:03, mef wrote:
> > It is a bit weird that it builds files from another third_party project. 
> > If those are not built by current boringssl build targets, then maybe we
> should
> > add another target to ../boringssl project?
> 
> Added a new target "boringssl-decrepit" in boringssl.gyp that compiles all
> decrepit BoringSSL sources. Referencing this target from here.
> 
>
https://codereview.chromium.org/1537473002/diff/50001/third_party/netty4/OWNERS
> File third_party/netty4/OWNERS (right):
> 
>
https://codereview.chromium.org/1537473002/diff/50001/third_party/netty4/OWNE...
> third_party/netty4/OWNERS:1: mailto:kapishnikov@chromium.org
> On 2016/01/11 21:46:03, mef wrote:
> > This project also needs a gyp file, similar to this:
> > 
> >
>
https://codereview.chromium.org/1529643002/diff/60001/third_party/netty/netty...
> 
> Done.

I've tried this patch locally and it works! 
Might want to disable warnings and update BUILD.gn in boringssl.

[kapishnikov, 2016-01-15 18:56:24]

kapishnikov@chromium.org changed reviewers:
+ rsleevi@chromium.org

[kapishnikov, 2016-01-15 18:56:24]

Hi Ryan,

I have added a new target to third_party/boringssl/boringssl.gyp to build
decrepit sources. These are needed for our test server (see the rest of the CL).
None of the other targets have been modified. Could you please review the
change:

third_party/boringssl/boringssl.gyp
third_party/boringssl/boringssl.gypi

Thanks,
Andrei

[kapishnikov, 2016-01-20 19:38:59]

kapishnikov@chromium.org changed reviewers:
+ davidben@chromium.org, rsleevi@chrium.org
- rsleevi@chromium.org

[kapishnikov, 2016-01-20 19:38:59]

Including David to review changes in BoringSSL GYP files.

[davidben, 2016-01-20 19:43:02]

davidben@chromium.org changed reviewers:
+ agl@chromium.org

[davidben, 2016-01-20 19:43:03]

+agl

What do you need decrepit for? It's not supposed to be built in Chromium or
Android.

https://codereview.chromium.org/1537473002/diff/130001/third_party/boringssl/...
File third_party/boringssl/boringssl.gypi (right):

https://codereview.chromium.org/1537473002/diff/130001/third_party/boringssl/...
third_party/boringssl/boringssl.gypi:429: 'boringssl_decrepit_sources': [
This file is automatically generated. It should not be modified manually. (See
comment at the top of the file.)

[kapishnikov, 2016-01-20 19:53:30]

On 2016/01/20 19:43:03, davidben wrote:
> +agl
> 
> What do you need decrepit for? It's not supposed to be built in Chromium or
> Android.
> 
>
https://codereview.chromium.org/1537473002/diff/130001/third_party/boringssl/...
> File third_party/boringssl/boringssl.gypi (right):
> 
>
https://codereview.chromium.org/1537473002/diff/130001/third_party/boringssl/...
> third_party/boringssl/boringssl.gypi:429: 'boringssl_decrepit_sources': [
> This file is automatically generated. It should not be modified manually. (See
> comment at the top of the file.)

It is needed for netty-tcnative library that we are going to use for
bidirectional streaming tests. The library references couple of classes from the
decrepit folder.

[davidben, 2016-01-20 19:53:54]

On 2016/01/20 19:53:30, kapishnikov wrote:
> On 2016/01/20 19:43:03, davidben wrote:
> > +agl
> > 
> > What do you need decrepit for? It's not supposed to be built in Chromium or
> > Android.
> > 
> >
>
https://codereview.chromium.org/1537473002/diff/130001/third_party/boringssl/...
> > File third_party/boringssl/boringssl.gypi (right):
> > 
> >
>
https://codereview.chromium.org/1537473002/diff/130001/third_party/boringssl/...
> > third_party/boringssl/boringssl.gypi:429: 'boringssl_decrepit_sources': [
> > This file is automatically generated. It should not be modified manually.
(See
> > comment at the top of the file.)
> 
> It is needed for netty-tcnative library that we are going to use for
> bidirectional streaming tests. The library references couple of classes from
the
> decrepit folder.

Which functions?

[kapishnikov, 2016-01-20 20:06:21]

On 2016/01/20 19:53:54, davidben wrote:
> On 2016/01/20 19:53:30, kapishnikov wrote:
> > On 2016/01/20 19:43:03, davidben wrote:
> > > +agl
> > > 
> > > What do you need decrepit for? It's not supposed to be built in Chromium
or
> > > Android.
> > > 
> > >
> >
>
https://codereview.chromium.org/1537473002/diff/130001/third_party/boringssl/...
> > > File third_party/boringssl/boringssl.gypi (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1537473002/diff/130001/third_party/boringssl/...
> > > third_party/boringssl/boringssl.gypi:429: 'boringssl_decrepit_sources': [
> > > This file is automatically generated. It should not be modified manually.
> (See
> > > comment at the top of the file.)
> > 
> > It is needed for netty-tcnative library that we are going to use for
> > bidirectional streaming tests. The library references couple of classes from
> the
> > decrepit folder.
> 
> Which functions?

'RSA_generate_key' & 'BIO_f_base64' are referenced from 
.../third_party/netty-tcnative/src/c/ssl.c:236
.../third_party/netty-tcnative/src/c/ssl.c:627
respectively.

Here is the link to the file:
https://chromium.googlesource.com/external/netty-tcnative/+/master/c/ssl.c

[agl, 2016-01-20 20:16:37]

On 2016/01/20 20:06:21, kapishnikov wrote:
> Here is the link to the file:
> https://chromium.googlesource.com/external/netty-tcnative/+/master/c/ssl.c

I think that netty-tcnative are pondering statically linking with BoringSSL in
some cases:
https://github.com/netty/netty-tcnative/pull/82#issuecomment-170686296

Given that, would it be possible to patch netty-tcnative upstream for one of
these? RSA_generate_key is deprecated in OpenSSL already, so that change should
be easy and small. For BIO_f_base64, I'm tempted to bring that into BoringSSL
proper, although we could also patch to remove that.

Given those changes, you wouldn't need to link decrepit. Does that work for you?

[davidben, 2016-01-20 20:26:03]

On 2016/01/20 20:16:37, agl wrote:
> On 2016/01/20 20:06:21, kapishnikov wrote:
> > Here is the link to the file:
> > https://chromium.googlesource.com/external/netty-tcnative/+/master/c/ssl.c
> 
> I think that netty-tcnative are pondering statically linking with BoringSSL in
> some cases:
> https://github.com/netty/netty-tcnative/pull/82#issuecomment-170686296
> 
> Given that, would it be possible to patch netty-tcnative upstream for one of
> these? RSA_generate_key is deprecated in OpenSSL already, so that change
should
> be easy and small. For BIO_f_base64, I'm tempted to bring that into BoringSSL
> proper, although we could also patch to remove that.
> 
> Given those changes, you wouldn't need to link decrepit. Does that work for
you?

It looks like RSA_generate_key is only used (in very convoluted ways) as part of
the export cipher (and FREAK) ephemeral RSA stuff. We could just rip all that
code out altogether and ask upstream to remove it too.

BIO_f_base64 is only used in this randMake function exposed to Java but, as far
as I can tell, never used. We can perhaps also just patch that one out and
convince upstream to ditch it. Seems kind of a pointless function to expose,
going by the docs.
https://chromium.googlesource.com/external/netty-tcnative/+/master/java/src/o...

[agl, 2016-01-20 20:41:01]

On 2016/01/20 20:26:03, davidben wrote:
> On 2016/01/20 20:16:37, agl wrote:
> > On 2016/01/20 20:06:21, kapishnikov wrote:
> > > Here is the link to the file:
> > > https://chromium.googlesource.com/external/netty-tcnative/+/master/c/ssl.c
> > 
> > I think that netty-tcnative are pondering statically linking with BoringSSL
in
> > some cases:
> > https://github.com/netty/netty-tcnative/pull/82#issuecomment-170686296
> > 
> > Given that, would it be possible to patch netty-tcnative upstream for one of
> > these? RSA_generate_key is deprecated in OpenSSL already, so that change
> should
> > be easy and small. For BIO_f_base64, I'm tempted to bring that into
BoringSSL
> > proper, although we could also patch to remove that.
> > 
> > Given those changes, you wouldn't need to link decrepit. Does that work for
> you?
> 
> It looks like RSA_generate_key is only used (in very convoluted ways) as part
of
> the export cipher (and FREAK) ephemeral RSA stuff. We could just rip all that
> code out altogether and ask upstream to remove it too.

It looks like it's disabled when building with OpenSSL 1.1.0 already but
switching to _ex would be trivial as a first, easy step.

> BIO_f_base64 is only used in this randMake function exposed to Java but, as
far
> as I can tell, never used. We can perhaps also just patch that one out and
> convince upstream to ditch it. Seems kind of a pointless function to expose,
> going by the docs.
>
https://chromium.googlesource.com/external/netty-tcnative/+/master/java/src/o...

But it's exposed to Java and thus we don't know who might call it, right?

[davidben, 2016-01-20 20:48:29]

On 2016/01/20 20:41:01, agl wrote:
> On 2016/01/20 20:26:03, davidben wrote:
> > On 2016/01/20 20:16:37, agl wrote:
> > > On 2016/01/20 20:06:21, kapishnikov wrote:
> > > > Here is the link to the file:
> > > >
https://chromium.googlesource.com/external/netty-tcnative/+/master/c/ssl.c
> > > 
> > > I think that netty-tcnative are pondering statically linking with
BoringSSL
> in
> > > some cases:
> > > https://github.com/netty/netty-tcnative/pull/82#issuecomment-170686296
> > > 
> > > Given that, would it be possible to patch netty-tcnative upstream for one
of
> > > these? RSA_generate_key is deprecated in OpenSSL already, so that change
> > should
> > > be easy and small. For BIO_f_base64, I'm tempted to bring that into
> BoringSSL
> > > proper, although we could also patch to remove that.
> > > 
> > > Given those changes, you wouldn't need to link decrepit. Does that work
for
> > you?
> > 
> > It looks like RSA_generate_key is only used (in very convoluted ways) as
part
> of
> > the export cipher (and FREAK) ephemeral RSA stuff. We could just rip all
that
> > code out altogether and ask upstream to remove it too.
> 
> It looks like it's disabled when building with OpenSSL 1.1.0 already but
> switching to _ex would be trivial as a first, easy step.
> 
> > BIO_f_base64 is only used in this randMake function exposed to Java but, as
> far
> > as I can tell, never used. We can perhaps also just patch that one out and
> > convince upstream to ditch it. Seems kind of a pointless function to expose,
> > going by the docs.
> >
>
https://chromium.googlesource.com/external/netty-tcnative/+/master/java/src/o...
> 
> But it's exposed to Java and thus we don't know who might call it, right?

That's true. It's not called in anything I can find. Notably it's not called in
netty4 which is what kapishnikov@ needs this for. Whether netty-tcnative is
supposed to have a stable API or is just what the rest of netty needs, I'm not
sure. I guess we can check with upstream whether they'd be willing to delete
some of this stuff.

This Java file is made entirely of functions that take in longs that get casted
to C pointers, so I certainly hope it's not intended to be kept stable! But
perhaps I'm being too optimistic. :-)

[kapishnikov, 2016-01-20 22:36:48]

On 2016/01/20 20:48:29, davidben wrote:
> On 2016/01/20 20:41:01, agl wrote:
> > On 2016/01/20 20:26:03, davidben wrote:
> > > On 2016/01/20 20:16:37, agl wrote:
> > > > On 2016/01/20 20:06:21, kapishnikov wrote:
> > > > > Here is the link to the file:
> > > > >
> https://chromium.googlesource.com/external/netty-tcnative/+/master/c/ssl.c
> > > > 
> > > > I think that netty-tcnative are pondering statically linking with
> BoringSSL
> > in
> > > > some cases:
> > > > https://github.com/netty/netty-tcnative/pull/82#issuecomment-170686296
> > > > 
> > > > Given that, would it be possible to patch netty-tcnative upstream for
one
> of
> > > > these? RSA_generate_key is deprecated in OpenSSL already, so that change
> > > should
> > > > be easy and small. For BIO_f_base64, I'm tempted to bring that into
> > BoringSSL
> > > > proper, although we could also patch to remove that.
> > > > 
> > > > Given those changes, you wouldn't need to link decrepit. Does that work
> for
> > > you?
> > > 
> > > It looks like RSA_generate_key is only used (in very convoluted ways) as
> part
> > of
> > > the export cipher (and FREAK) ephemeral RSA stuff. We could just rip all
> that
> > > code out altogether and ask upstream to remove it too.
> > 
> > It looks like it's disabled when building with OpenSSL 1.1.0 already but
> > switching to _ex would be trivial as a first, easy step.
> > 
> > > BIO_f_base64 is only used in this randMake function exposed to Java but,
as
> > far
> > > as I can tell, never used. We can perhaps also just patch that one out and
> > > convince upstream to ditch it. Seems kind of a pointless function to
expose,
> > > going by the docs.
> > >
> >
>
https://chromium.googlesource.com/external/netty-tcnative/+/master/java/src/o...
> > 
> > But it's exposed to Java and thus we don't know who might call it, right?
> 
> That's true. It's not called in anything I can find. Notably it's not called
in
> netty4 which is what kapishnikov@ needs this for. Whether netty-tcnative is
> supposed to have a stable API or is just what the rest of netty needs, I'm not
> sure. I guess we can check with upstream whether they'd be willing to delete
> some of this stuff.
> 
> This Java file is made entirely of functions that take in longs that get
casted
> to C pointers, so I certainly hope it's not intended to be kept stable! But
> perhaps I'm being too optimistic. :-)

I have removed netty-tcnative dependency on BoringSSL decrepit files. The change
is reflected here:
https://codereview.chromium.org/1537473002/diff2/130001:150001/third_party/ne...

[davidben, 2016-01-21 02:39:31]

(Oops, hit the wrong button, sorry.)

[davidben, 2016-01-21 02:39:49]

davidben@chromium.org changed reviewers:
- davidben@chromium.org

[kapishnikov, 2016-01-21 03:53:01]

We got all required approvals:
1. Engineering proposal review.
2. Open source third party review.
3. Security review. 

David also reviewed a change in netty-tcnative to eliminate dependency on
BoringSSL decrepit files.

If all tests pass, we should be good to check the change in.

[mef, 2016-01-21 17:02:55]

lgtm with couple of nits.

https://codereview.chromium.org/1537473002/diff/170001/components/cronet.gypi
File components/cronet.gypi (right):

https://codereview.chromium.org/1537473002/diff/170001/components/cronet.gypi...
components/cronet.gypi:383: 'netty-tcnative',
Would it be enough to just depend on 
'../third_party/netty-tcnative/netty-tcnative.gyp:netty-tcnative' here and
remove netty-tcnative from cronet.gypi?

https://codereview.chromium.org/1537473002/diff/170001/third_party/netty-tcna...
File third_party/netty-tcnative/netty-tcnative.gyp (right):

https://codereview.chromium.org/1537473002/diff/170001/third_party/netty-tcna...
third_party/netty-tcnative/netty-tcnative.gyp:62: 'variables': {
maybe add dependency to netty-tcnative-so target here, so it wouldn't have to be
explicitly added by the user?

https://codereview.chromium.org/1537473002/diff/170001/third_party/netty4/net...
File third_party/netty4/netty.gyp (right):

https://codereview.chromium.org/1537473002/diff/170001/third_party/netty4/net...
third_party/netty4/netty.gyp:7: # Ask mef@ for their location.
Hrm, they are now in real external repository, so this comment is stale.

[mef, 2016-01-21 17:03:26]

On 2016/01/21 17:02:55, mef wrote:
> lgtm with couple of nits.
> 
> https://codereview.chromium.org/1537473002/diff/170001/components/cronet.gypi
> File components/cronet.gypi (right):
> 
>
https://codereview.chromium.org/1537473002/diff/170001/components/cronet.gypi...
> components/cronet.gypi:383: 'netty-tcnative',
> Would it be enough to just depend on 
> '../third_party/netty-tcnative/netty-tcnative.gyp:netty-tcnative' here and
> remove netty-tcnative from cronet.gypi?
> 
>
https://codereview.chromium.org/1537473002/diff/170001/third_party/netty-tcna...
> File third_party/netty-tcnative/netty-tcnative.gyp (right):
> 
>
https://codereview.chromium.org/1537473002/diff/170001/third_party/netty-tcna...
> third_party/netty-tcnative/netty-tcnative.gyp:62: 'variables': {
> maybe add dependency to netty-tcnative-so target here, so it wouldn't have to
be
> explicitly added by the user?
> 
>
https://codereview.chromium.org/1537473002/diff/170001/third_party/netty4/net...
> File third_party/netty4/netty.gyp (right):
> 
>
https://codereview.chromium.org/1537473002/diff/170001/third_party/netty4/net...
> third_party/netty4/netty.gyp:7: # Ask mef@ for their location.
> Hrm, they are now in real external repository, so this comment is stale.

Also the title is stale.

[agl, 2016-01-21 17:35:50]

On 2016/01/21 03:53:01, kapishnikov wrote:
> We got all required approvals:
> 1. Engineering proposal review.
> 2. Open source third party review.
> 3. Security review. 
> 
> David also reviewed a change in netty-tcnative to eliminate dependency on
> BoringSSL decrepit files.
> 
> If all tests pass, we should be good to check the change in.

R.e. the local patches[1], I've sent an email to see if we can eliminate the
need for that by upstreaming and/or BoringSSL changes.

In the mean time, at line 48 of [1]: I'm confused how that compiles. RSA_F4 is a
#define to 0x10001, but RSA_generate_key_ex takes a BIGNUM* in that position.

[1]
https://codereview.chromium.org/1537473002/diff2/130001:150001/third_party/ne...

[davidben, 2016-01-21 17:49:59]

On 2016/01/21 17:35:50, agl wrote:
> On 2016/01/21 03:53:01, kapishnikov wrote:
> > We got all required approvals:
> > 1. Engineering proposal review.
> > 2. Open source third party review.
> > 3. Security review. 
> > 
> > David also reviewed a change in netty-tcnative to eliminate dependency on
> > BoringSSL decrepit files.
> > 
> > If all tests pass, we should be good to check the change in.
> 
> R.e. the local patches[1], I've sent an email to see if we can eliminate the
> need for that by upstreaming and/or BoringSSL changes.
> 
> In the mean time, at line 48 of [1]: I'm confused how that compiles. RSA_F4 is
a
> #define to 0x10001, but RSA_generate_key_ex takes a BIGNUM* in that position.
> 
> [1]
>
https://codereview.chromium.org/1537473002/diff2/130001:150001/third_party/ne...

Bah! Apparently this is only a warning and netty.gyp sets -w. Andrei, could you
remove that line? The warning on integer to pointer cast is extremely important.
:-)

[kapishnikov, 2016-01-21 19:43:31]

https://codereview.chromium.org/1537473002/diff/170001/components/cronet.gypi
File components/cronet.gypi (right):

https://codereview.chromium.org/1537473002/diff/170001/components/cronet.gypi...
components/cronet.gypi:383: 'netty-tcnative',
On 2016/01/21 17:02:55, mef wrote:
> Would it be enough to just depend on 
> '../third_party/netty-tcnative/netty-tcnative.gyp:netty-tcnative' here and
> remove netty-tcnative from cronet.gypi?

Done.

https://codereview.chromium.org/1537473002/diff/170001/third_party/netty-tcna...
File third_party/netty-tcnative/netty-tcnative.gyp (right):

https://codereview.chromium.org/1537473002/diff/170001/third_party/netty-tcna...
third_party/netty-tcnative/netty-tcnative.gyp:62: 'variables': {
On 2016/01/21 17:02:55, mef wrote:
> maybe add dependency to netty-tcnative-so target here, so it wouldn't have to
be
> explicitly added by the user?

Done.

https://codereview.chromium.org/1537473002/diff/170001/third_party/netty4/net...
File third_party/netty4/netty.gyp (right):

https://codereview.chromium.org/1537473002/diff/170001/third_party/netty4/net...
third_party/netty4/netty.gyp:7: # Ask mef@ for their location.
On 2016/01/21 17:02:55, mef wrote:
> Hrm, they are now in real external repository, so this comment is stale.

Done.

[kapishnikov, 2016-01-21 20:01:25]

The CQ bit was checked by kapishnikov@chromium.org

[kapishnikov, 2016-01-21 20:01:25]

The patchset sent to the CQ was uploaded after l-g-t-m from mef@chromium.org
Link to the patchset: https://codereview.chromium.org/1537473002/#ps210001
(title: "Spelling fix")

[commit-bot: I haz the power, 2016-01-21 20:02:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1537473002/210001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1537473002/210001

[commit-bot: I haz the power, 2016-01-21 21:31:19]

Description was changed from

==========
[third-party] Netty Server and dependencies

Added new libraries to the Chromium third-party library collection:
1. Netty 4 Server. See http://netty.io
2. Netty fork of Tomcat Native (netty-tcnative). See
http://netty.io/wiki/forked-tomcat-native.html
and the library tc-native depends on:
3. 'Apache Portable Runtime'. See http://apr.apache.org

The server provides support for Http/2 protocol, which is needed for testing
some of the Cronet features
on Android, in particular, the new Bidirectional Streaming API.

To build the netty-tcnative library call:
ninja -C out/Debug netty-tcnative

The output files can be found here:
- out/Debug/lib/libnetty-tcnative.so
- out/Debug/lib.java/netty-tcnative.jar

BUG=563732
==========

to

==========
[third-party] Netty Server and dependencies

Added new libraries to the Chromium third-party library collection:
1. Netty 4 Server. See http://netty.io
2. Netty fork of Tomcat Native (netty-tcnative). See
http://netty.io/wiki/forked-tomcat-native.html
and the library tc-native depends on:
3. 'Apache Portable Runtime'. See http://apr.apache.org

The server provides support for Http/2 protocol, which is needed for testing
some of the Cronet features
on Android, in particular, the new Bidirectional Streaming API.

To build the netty-tcnative library call:
ninja -C out/Debug netty-tcnative

The output files can be found here:
- out/Debug/lib/libnetty-tcnative.so
- out/Debug/lib.java/netty-tcnative.jar

BUG=563732
==========

[commit-bot: I haz the power, 2016-01-21 21:31:20]

Committed patchset #12 (id:210001)

[commit-bot: I haz the power, 2016-01-21 21:32:36]

Description was changed from

==========
[third-party] Netty Server and dependencies

Added new libraries to the Chromium third-party library collection:
1. Netty 4 Server. See http://netty.io
2. Netty fork of Tomcat Native (netty-tcnative). See
http://netty.io/wiki/forked-tomcat-native.html
and the library tc-native depends on:
3. 'Apache Portable Runtime'. See http://apr.apache.org

The server provides support for Http/2 protocol, which is needed for testing
some of the Cronet features
on Android, in particular, the new Bidirectional Streaming API.

To build the netty-tcnative library call:
ninja -C out/Debug netty-tcnative

The output files can be found here:
- out/Debug/lib/libnetty-tcnative.so
- out/Debug/lib.java/netty-tcnative.jar

BUG=563732
==========

to

==========
[third-party] Netty Server and dependencies

Added new libraries to the Chromium third-party library collection:
1. Netty 4 Server. See http://netty.io
2. Netty fork of Tomcat Native (netty-tcnative). See
http://netty.io/wiki/forked-tomcat-native.html
and the library tc-native depends on:
3. 'Apache Portable Runtime'. See http://apr.apache.org

The server provides support for Http/2 protocol, which is needed for testing
some of the Cronet features
on Android, in particular, the new Bidirectional Streaming API.

To build the netty-tcnative library call:
ninja -C out/Debug netty-tcnative

The output files can be found here:
- out/Debug/lib/libnetty-tcnative.so
- out/Debug/lib.java/netty-tcnative.jar

BUG=563732

Committed: https://crrev.com/6dd98af3db0b8e809ed95ff66620181efbf3f8f1
Cr-Commit-Position: refs/heads/master@{#370791}
==========

[commit-bot: I haz the power, 2016-01-21 21:32:37]

Patchset 12 (id:??) landed as
https://crrev.com/6dd98af3db0b8e809ed95ff66620181efbf3f8f1
Cr-Commit-Position: refs/heads/master@{#370791}

[kapishnikov, 2016-01-21 22:04:58]

A revert of this CL (patchset #12 id:210001) has been created in
https://codereview.chromium.org/1612863004/ by kapishnikov@chromium.org.

The reason for reverting is: error on the clank-tot bot:
https://uberchromegw.corp.google.com/i/internal.client.clank_tot/builders/cla....

[kapishnikov, 2016-01-26 22:01:31]

Description was changed from

==========
[third-party] Netty Server and dependencies

Added new libraries to the Chromium third-party library collection:
1. Netty 4 Server. See http://netty.io
2. Netty fork of Tomcat Native (netty-tcnative). See
http://netty.io/wiki/forked-tomcat-native.html
and the library tc-native depends on:
3. 'Apache Portable Runtime'. See http://apr.apache.org

The server provides support for Http/2 protocol, which is needed for testing
some of the Cronet features
on Android, in particular, the new Bidirectional Streaming API.

To build the netty-tcnative library call:
ninja -C out/Debug netty-tcnative

The output files can be found here:
- out/Debug/lib/libnetty-tcnative.so
- out/Debug/lib.java/netty-tcnative.jar

BUG=563732

Committed: https://crrev.com/6dd98af3db0b8e809ed95ff66620181efbf3f8f1
Cr-Commit-Position: refs/heads/master@{#370791}
==========

to

==========
[third-party] Netty Server and dependencies

Added new libraries to the Chromium third-party library collection:
1. Netty 4 Server. See http://netty.io
2. Netty fork of Tomcat Native (netty-tcnative). See
http://netty.io/wiki/forked-tomcat-native.html
and the library tc-native depends on:
3. 'Apache Portable Runtime'. See http://apr.apache.org

The server provides support for Http/2 protocol, which is needed for testing
some of the Cronet features
on Android, in particular, the new Bidirectional Streaming API.

To build the netty-tcnative library call:
ninja -C out/Debug netty-tcnative

The output files can be found here:
- out/Debug/lib/libnetty-tcnative.so
- out/Debug/lib.java/netty-tcnative.jar

BUG=563732

Committed: https://crrev.com/6dd98af3db0b8e809ed95ff66620181efbf3f8f1
Cr-Commit-Position: refs/heads/master@{#370791}
==========

[mef, 2016-01-26 22:36:48]

https://codereview.chromium.org/1537473002/diff/230001/components/cronet.gypi
File components/cronet.gypi (right):

https://codereview.chromium.org/1537473002/diff/230001/components/cronet.gypi...
components/cronet.gypi:401: # libnetty-tcnative shared library should have a
specific name when it is copied to
is it possible to have this target in netty-tcnative gyp?

https://codereview.chromium.org/1537473002/diff/230001/third_party/netty-tcna...
File third_party/netty-tcnative/netty-tcnative.gyp (right):

https://codereview.chromium.org/1537473002/diff/230001/third_party/netty-tcna...
third_party/netty-tcnative/netty-tcnative.gyp:61: 'netty_tcnative_so_file_name':
'libnetty-tcnative.cr.so',
Can this do a copy and set 'additional_bundled_libs' variable instead?

[kapishnikov, 2016-01-27 00:46:07]

https://codereview.chromium.org/1537473002/diff/230001/components/cronet.gypi
File components/cronet.gypi (right):

https://codereview.chromium.org/1537473002/diff/230001/components/cronet.gypi...
components/cronet.gypi:401: # libnetty-tcnative shared library should have a
specific name when it is copied to
On 2016/01/26 22:36:48, mef wrote:
> is it possible to have this target in netty-tcnative gyp?

Done.

https://codereview.chromium.org/1537473002/diff/230001/third_party/netty-tcna...
File third_party/netty-tcnative/netty-tcnative.gyp (right):

https://codereview.chromium.org/1537473002/diff/230001/third_party/netty-tcna...
third_party/netty-tcnative/netty-tcnative.gyp:61: 'netty_tcnative_so_file_name':
'libnetty-tcnative.cr.so',
On 2016/01/26 22:36:48, mef wrote:
> Can this do a copy and set 'additional_bundled_libs' variable instead?

I moved the copying and setting the shared library location into a single target
in netty-tcnative.gyp. I still kept
'additional_bundled_libs': [
  '>(netty_tcnative_so_file_location)',
],
in cronet.gypi. This way it is clearer that the APK contains the library.

[mef, 2016-01-27 16:08:08]

lgtm

https://codereview.chromium.org/1537473002/diff/230001/third_party/netty-tcna...
File third_party/netty-tcnative/netty-tcnative.gyp (right):

https://codereview.chromium.org/1537473002/diff/230001/third_party/netty-tcna...
third_party/netty-tcnative/netty-tcnative.gyp:61: 'netty_tcnative_so_file_name':
'libnetty-tcnative.cr.so',
On 2016/01/27 00:46:07, kapishnikov wrote:
> On 2016/01/26 22:36:48, mef wrote:
> > Can this do a copy and set 'additional_bundled_libs' variable instead?
> 
> I moved the copying and setting the shared library location into a single
target
> in netty-tcnative.gyp. I still kept
> 'additional_bundled_libs': [
>   '>(netty_tcnative_so_file_location)',
> ],
> in cronet.gypi. This way it is clearer that the APK contains the library.

Acknowledged.

[kapishnikov, 2016-01-27 22:09:01]

I tested the GN build and the result netty-tcnative.jar and libnetty-tcnative.so
files with the sample Netty server. They work fine. However, when I tried to
build CronetTest.apk with bidirectional stream, the build cannot find Netty
files from the prebuilt jar. Checked that the 'cronet_test_apk' target has
"//third_party/netty4:netty_all" as the dependency and the result netty_all.jar
file has the required classes. Need to look into why it does not add them to the
build classpath.

[kapishnikov, 2016-01-28 15:26:23]

The CQ bit was checked by kapishnikov@chromium.org

[kapishnikov, 2016-01-28 15:26:24]

The patchset sent to the CQ was uploaded after l-g-t-m from mef@chromium.org
Link to the patchset: https://codereview.chromium.org/1537473002/#ps290001
(title: "Added support for GN build")

[commit-bot: I haz the power, 2016-01-28 15:26:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1537473002/290001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1537473002/290001

[commit-bot: I haz the power, 2016-01-28 15:55:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-28 15:55:20]

Try jobs failed on following builders:
  android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[mef, 2016-01-28 18:36:01]

https://codereview.chromium.org/1537473002/diff/310001/third_party/apache-por...
File third_party/apache-portable-runtime/BUILD.gn (right):

https://codereview.chromium.org/1537473002/diff/310001/third_party/apache-por...
third_party/apache-portable-runtime/BUILD.gn:5:
import("//build/config/android/config.gni")
should this be conditional on 'android' platform or somesuch?

https://codereview.chromium.org/1537473002/diff/310001/third_party/apache-por...
File third_party/apache-portable-runtime/apr.gyp (right):

https://codereview.chromium.org/1537473002/diff/310001/third_party/apache-por...
third_party/apache-portable-runtime/apr.gyp:109:
'-isystem<(android_ndk_root)/sources/android/support/include',
this is only correct for android.

[kapishnikov, 2016-01-28 19:59:40]

The CQ bit was checked by kapishnikov@chromium.org

[kapishnikov, 2016-01-28 19:59:40]

The patchset sent to the CQ was uploaded after l-g-t-m from mef@chromium.org
Link to the patchset: https://codereview.chromium.org/1537473002/#ps310001
(title: "Added cflag to include NDK support headers in APR build")

[commit-bot: I haz the power, 2016-01-28 20:00:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1537473002/310001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1537473002/310001

[commit-bot: I haz the power, 2016-01-28 21:35:10]

Description was changed from

==========
[third-party] Netty Server and dependencies

Added new libraries to the Chromium third-party library collection:
1. Netty 4 Server. See http://netty.io
2. Netty fork of Tomcat Native (netty-tcnative). See
http://netty.io/wiki/forked-tomcat-native.html
and the library tc-native depends on:
3. 'Apache Portable Runtime'. See http://apr.apache.org

The server provides support for Http/2 protocol, which is needed for testing
some of the Cronet features
on Android, in particular, the new Bidirectional Streaming API.

To build the netty-tcnative library call:
ninja -C out/Debug netty-tcnative

The output files can be found here:
- out/Debug/lib/libnetty-tcnative.so
- out/Debug/lib.java/netty-tcnative.jar

BUG=563732

Committed: https://crrev.com/6dd98af3db0b8e809ed95ff66620181efbf3f8f1
Cr-Commit-Position: refs/heads/master@{#370791}
==========

to

==========
[third-party] Netty Server and dependencies

Added new libraries to the Chromium third-party library collection:
1. Netty 4 Server. See http://netty.io
2. Netty fork of Tomcat Native (netty-tcnative). See
http://netty.io/wiki/forked-tomcat-native.html
and the library tc-native depends on:
3. 'Apache Portable Runtime'. See http://apr.apache.org

The server provides support for Http/2 protocol, which is needed for testing
some of the Cronet features
on Android, in particular, the new Bidirectional Streaming API.

To build the netty-tcnative library call:
ninja -C out/Debug netty-tcnative

The output files can be found here:
- out/Debug/lib/libnetty-tcnative.so
- out/Debug/lib.java/netty-tcnative.jar

BUG=563732

Committed: https://crrev.com/6dd98af3db0b8e809ed95ff66620181efbf3f8f1
Cr-Commit-Position: refs/heads/master@{#370791}
==========

[commit-bot: I haz the power, 2016-01-28 21:35:11]

Committed patchset #17 (id:310001)

[commit-bot: I haz the power, 2016-01-28 21:35:52]

Description was changed from

==========
[third-party] Netty Server and dependencies

Added new libraries to the Chromium third-party library collection:
1. Netty 4 Server. See http://netty.io
2. Netty fork of Tomcat Native (netty-tcnative). See
http://netty.io/wiki/forked-tomcat-native.html
and the library tc-native depends on:
3. 'Apache Portable Runtime'. See http://apr.apache.org

The server provides support for Http/2 protocol, which is needed for testing
some of the Cronet features
on Android, in particular, the new Bidirectional Streaming API.

To build the netty-tcnative library call:
ninja -C out/Debug netty-tcnative

The output files can be found here:
- out/Debug/lib/libnetty-tcnative.so
- out/Debug/lib.java/netty-tcnative.jar

BUG=563732

Committed: https://crrev.com/6dd98af3db0b8e809ed95ff66620181efbf3f8f1
Cr-Commit-Position: refs/heads/master@{#370791}
==========

to

==========
[third-party] Netty Server and dependencies

Added new libraries to the Chromium third-party library collection:
1. Netty 4 Server. See http://netty.io
2. Netty fork of Tomcat Native (netty-tcnative). See
http://netty.io/wiki/forked-tomcat-native.html
and the library tc-native depends on:
3. 'Apache Portable Runtime'. See http://apr.apache.org

The server provides support for Http/2 protocol, which is needed for testing
some of the Cronet features
on Android, in particular, the new Bidirectional Streaming API.

To build the netty-tcnative library call:
ninja -C out/Debug netty-tcnative

The output files can be found here:
- out/Debug/lib/libnetty-tcnative.so
- out/Debug/lib.java/netty-tcnative.jar

BUG=563732

Committed: https://crrev.com/6dd98af3db0b8e809ed95ff66620181efbf3f8f1
Cr-Commit-Position: refs/heads/master@{#370791}

Committed: https://crrev.com/810c220259edb6a0b79ca1c16e8d7fb71aa258ce
Cr-Commit-Position: refs/heads/master@{#372172}
==========

[commit-bot: I haz the power, 2016-01-28 21:35:54]

Patchset 17 (id:??) landed as
https://crrev.com/810c220259edb6a0b79ca1c16e8d7fb71aa258ce
Cr-Commit-Position: refs/heads/master@{#372172}