Switched to new versions of netty and tcnative

Switched to new versions of netty and tcnative

Related CLs:
https://codereview.chromium.org/2842333002/
https://codereview.chromium.org/2843223002/

BUG=712738
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2843293002
Cr-Commit-Position: refs/heads/master@{#467848}
Committed: https://chromium.googlesource.com/chromium/src/+/ff836a3e8ea62f70165a57dd5f0e831acaaca7d8

[kapishnikov, 2017-04-27 01:35:40]

Description was changed from

==========
Switched to new versions of netty and tcnative

Related CLs:
https://codereview.chromium.org/2842333002/
https://codereview.chromium.org/2843223002/

BUG=712738
==========

to

==========
Switched to new versions of netty and tcnative

Related CLs:
https://codereview.chromium.org/2842333002/
https://codereview.chromium.org/2843223002/

BUG=712738
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[kapishnikov, 2017-04-27 01:36:51]

kapishnikov@chromium.org changed reviewers:
+ davidben@chromium.org, mef@chromium.org

[kapishnikov, 2017-04-27 01:39:11]

https://codereview.chromium.org/2843293002/diff/1/components/cronet/android/B...
File components/cronet/android/BUILD.gn (left):

https://codereview.chromium.org/2843293002/diff/1/components/cronet/android/B...
components/cronet/android/BUILD.gn:590:
"//third_party/netty-tcnative:netty-tcnative_java",
The tcnative java classes are now included in
"//third_party/netty4:netty_all_java".

[mef, 2017-04-27 15:49:15]

https://codereview.chromium.org/2843293002/diff/1/DEPS
File DEPS (right):

https://codereview.chromium.org/2843293002/diff/1/DEPS#newcode496
DEPS:496: Var('chromium_git') + '/external/netty-tcnative.git' + '@' +
'5b46a8ef4a39c39c576fcdaaf718b585d75df463',
Do we need to land other 2 CLs before this one?

https://codereview.chromium.org/2843293002/diff/1/components/cronet/android/t...
File components/cronet/android/test/src/org/chromium/net/Http2TestServer.java
(right):

https://codereview.chromium.org/2843293002/diff/1/components/cronet/android/t...
components/cronet/android/test/src/org/chromium/net/Http2TestServer.java:107:
new File(CertTestUtil.CERTS_DIRECTORY + keyFileName + ".pkcs8.pem")))
Is this new format needed because Netty doesn't like the normal one?
Should we change caller to pass file name with ".pkcs8.pem"?

https://codereview.chromium.org/2843293002/diff/1/third_party/netty-tcnative/...
File third_party/netty-tcnative/README.chromium (right):

https://codereview.chromium.org/2843293002/diff/1/third_party/netty-tcnative/...
third_party/netty-tcnative/README.chromium:31: -    TCN_ASSERT(verify_confg !=
NULL);
Hrm, was it always compiled out upstream?

[davidben, 2017-04-27 16:24:49]

https://codereview.chromium.org/2843293002/diff/1/third_party/netty-tcnative/...
File third_party/netty-tcnative/README.chromium (right):

https://codereview.chromium.org/2843293002/diff/1/third_party/netty-tcnative/...
third_party/netty-tcnative/README.chromium:31: -    TCN_ASSERT(verify_confg !=
NULL);
On 2017/04/27 15:49:15, mef wrote:
> Hrm, was it always compiled out upstream?

Yeah, apparently they've never built it in debug mode! I've sent a patch
upstream so we won't have to carry this diff next time around.
https://github.com/netty/netty-tcnative/pull/267

[kapishnikov, 2017-04-27 16:40:09]

https://codereview.chromium.org/2843293002/diff/1/DEPS
File DEPS (right):

https://codereview.chromium.org/2843293002/diff/1/DEPS#newcode496
DEPS:496: Var('chromium_git') + '/external/netty-tcnative.git' + '@' +
'5b46a8ef4a39c39c576fcdaaf718b585d75df463',
On 2017/04/27 15:49:15, mef wrote:
> Do we need to land other 2 CLs before this one?

Yes, otherwise the build will fail. I will run the tests before submitting this
CL. The tests pass locally.

https://codereview.chromium.org/2843293002/diff/1/components/cronet/android/t...
File components/cronet/android/test/src/org/chromium/net/Http2TestServer.java
(right):

https://codereview.chromium.org/2843293002/diff/1/components/cronet/android/t...
components/cronet/android/test/src/org/chromium/net/Http2TestServer.java:107:
new File(CertTestUtil.CERTS_DIRECTORY + keyFileName + ".pkcs8.pem")))
On 2017/04/27 15:49:15, mef wrote:
> Is this new format needed because Netty doesn't like the normal one?
> Should we change caller to pass file name with ".pkcs8.pem"?

Yes, netty requires PEM pkcs#8 format. It doesn't like pkcs#1. I think it is a
good idea to change the callers. There are 4 callers currently. All of them rely
on QuicTestServer.getServerCert(). I don't want to change QuicTestServer because
it has nothing to do with that.

Currently QuicTestServer defines a constant with the cert file name. We could do
something similar in Http2TestServer but it removes the flexibility of passing a
custom cert, which may be useful in the future.

[mef, 2017-04-27 18:54:19]

https://codereview.chromium.org/2843293002/diff/20001/components/cronet/andro...
File components/cronet/android/test/proguard.cfg (right):

https://codereview.chromium.org/2843293002/diff/20001/components/cronet/andro...
components/cronet/android/test/proguard.cfg:25: -keep class
org.apache.tomcat.jni.CertificateRequestedCallback$KeyMaterial { *; }
nit: maybe move it up to io.netty classes and add comment?

https://codereview.chromium.org/2843293002/diff/20001/components/cronet/andro...
File
components/cronet/android/test/smoketests/src/org/chromium/net/smoke/ChromiumNativeTestSupport.java
(right):

https://codereview.chromium.org/2843293002/diff/20001/components/cronet/andro...
components/cronet/android/test/smoketests/src/org/chromium/net/smoke/ChromiumNativeTestSupport.java:29:
private  static final String SERVER_KEY_PKCS8_PEM =
"quic_test.example.com.key.pkcs8.pem";
nit: spurious space.

[kapishnikov, 2017-04-27 19:45:08]

https://codereview.chromium.org/2843293002/diff/20001/components/cronet/andro...
File components/cronet/android/test/proguard.cfg (right):

https://codereview.chromium.org/2843293002/diff/20001/components/cronet/andro...
components/cronet/android/test/proguard.cfg:25: -keep class
org.apache.tomcat.jni.CertificateRequestedCallback$KeyMaterial { *; }
On 2017/04/27 18:54:19, mef wrote:
> nit: maybe move it up to io.netty classes and add comment?

I re-ran the tests and it looks that this "keep" was only needed for tc-native
1.1.13.fork26. It works fine with tc-native 2.0.0.

https://codereview.chromium.org/2843293002/diff/20001/components/cronet/andro...
File
components/cronet/android/test/smoketests/src/org/chromium/net/smoke/ChromiumNativeTestSupport.java
(right):

https://codereview.chromium.org/2843293002/diff/20001/components/cronet/andro...
components/cronet/android/test/smoketests/src/org/chromium/net/smoke/ChromiumNativeTestSupport.java:29:
private  static final String SERVER_KEY_PKCS8_PEM =
"quic_test.example.com.key.pkcs8.pem";
On 2017/04/27 18:54:19, mef wrote:
> nit: spurious space.

Done.

[mef, 2017-04-27 21:15:44]

lgtm

[kapishnikov, 2017-04-27 22:10:36]

The CQ bit was checked by kapishnikov@chromium.org

[kapishnikov, 2017-04-27 22:10:37]

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/2843293002/#ps60001
(title: "Rebase + code formatting")

[commit-bot: I haz the power, 2017-04-27 22:29:48]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-28 02:19:20]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493331036540140,
"parent_rev": "a005bf30441ddefd842a48dd841a532e71af9c4e", "commit_rev":
"ff836a3e8ea62f70165a57dd5f0e831acaaca7d8"}

[commit-bot: I haz the power, 2017-04-28 02:19:27]

Description was changed from

==========
Switched to new versions of netty and tcnative

Related CLs:
https://codereview.chromium.org/2842333002/
https://codereview.chromium.org/2843223002/

BUG=712738
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
Switched to new versions of netty and tcnative

Related CLs:
https://codereview.chromium.org/2842333002/
https://codereview.chromium.org/2843223002/

BUG=712738
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2843293002
Cr-Commit-Position: refs/heads/master@{#467848}
Committed:
https://chromium.googlesource.com/chromium/src/+/ff836a3e8ea62f70165a57dd5f0e...
==========

[commit-bot: I haz the power, 2017-04-28 02:19:28]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/ff836a3e8ea62f70165a57dd5f0e...