Use supported Android network handles in Android M and newer.

Use supported Android network handles in Android M and newer.

In Android N loading system libraries is banned so use the supported mechanism
for Android M and newer releases.  For Android L we still need to manually load
the system library.

BUG=625188
Committed: https://crrev.com/479143c5186dc2559f645cf25f9aebc006e0f3a2
Cr-Commit-Position: refs/heads/master@{#407139}

[pauljensen, 2016-07-19 18:24:10]

pauljensen@chromium.org changed reviewers:
+ xunjieli@chromium.org

[pauljensen, 2016-07-19 18:24:10]

Helen, PTAL.

[xunjieli, 2016-07-20 13:49:35]

The change looks good! How about adding a unit test in
net/udp/udp_socket_unittest.cc to test calling BindToNetwork() before Connect()
on L and above? In case something change on the platform, we would know
immediately.

https://codereview.chromium.org/2160263002/diff/40001/net/android/java/src/or...
File net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java
(right):

https://codereview.chromium.org/2160263002/diff/40001/net/android/java/src/or...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:928:
return Integer.parseInt(network.toString());
 Long.parseLong?

[pauljensen, 2016-07-20 17:31:25]

The CQ bit was checked by pauljensen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-20 17:32:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pauljensen, 2016-07-20 17:33:51]

On 2016/07/20 13:49:35, xunjieli wrote:
> The change looks good! How about adding a unit test in
> net/udp/udp_socket_unittest.cc to test calling BindToNetwork() before
Connect()
> on L and above? In case something change on the platform, we would know
> immediately.

I added a unit-test, PTAL.  This is currently tested by the QUIC Cronet tests
also, see linked bug.

https://codereview.chromium.org/2160263002/diff/40001/net/android/java/src/or...
File net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java
(right):

https://codereview.chromium.org/2160263002/diff/40001/net/android/java/src/or...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:928:
return Integer.parseInt(network.toString());
On 2016/07/20 13:49:35, xunjieli wrote:
>  Long.parseLong?

It should just be producing an int (and it always will because this is only used
for Lollipop and Lollipop is long since released).  I'd rather not make this
more permissive.  If this changes to something that isn't an int we're going to
have big problems.  The long produced by getNetworkHandle() is quite different,
it's munged a fair bit:
http://androidxref.com/6.0.0_r1/xref/frameworks/base/core/java/android/net/Ne...

[xunjieli, 2016-07-20 17:44:01]

> I added a unit-test, PTAL.  This is currently tested by the QUIC Cronet tests
> also, see linked bug.

LGTM. Yea, i know the QUIC test caught this bug. It is rather accidental
considering that the connection migration is just happened to be enabled in that
test. It is not the main goal of that java test. So I'd rather have
BindToNetwork() tested on the API level. Anyway thanks for adding the test.

https://codereview.chromium.org/2160263002/diff/40001/net/android/java/src/or...
File net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java
(right):

https://codereview.chromium.org/2160263002/diff/40001/net/android/java/src/or...
net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java:928:
return Integer.parseInt(network.toString());
On 2016/07/20 17:33:51, pauljensen wrote:
> On 2016/07/20 13:49:35, xunjieli wrote:
> >  Long.parseLong?
> 
> It should just be producing an int (and it always will because this is only
used
> for Lollipop and Lollipop is long since released).  I'd rather not make this
> more permissive.  If this changes to something that isn't an int we're going
to
> have big problems.  The long produced by getNetworkHandle() is quite
different,
> it's munged a fair bit:
>
http://androidxref.com/6.0.0_r1/xref/frameworks/base/core/java/android/net/Ne...

Acknowledged. SGTM. Please add a comment here on why Integer.parseInt is used
even though this method returns long.

[xunjieli, 2016-07-20 17:55:49]

https://codereview.chromium.org/2160263002/diff/60001/net/udp/udp_socket_posi...
File net/udp/udp_socket_posix.cc (right):

https://codereview.chromium.org/2160263002/diff/60001/net/udp/udp_socket_posi...
net/udp/udp_socket_posix.cc:360: // This is racy, but all racers should come out
with the same answer so it
One last question: why is this racy? If all socket connects happen on the
network thread, isn't this single threaded?

[pauljensen, 2016-07-20 18:02:16]

https://codereview.chromium.org/2160263002/diff/60001/net/udp/udp_socket_posi...
File net/udp/udp_socket_posix.cc (right):

https://codereview.chromium.org/2160263002/diff/60001/net/udp/udp_socket_posi...
net/udp/udp_socket_posix.cc:360: // This is racy, but all racers should come out
with the same answer so it
On 2016/07/20 17:55:49, xunjieli wrote:
> One last question: why is this racy? If all socket connects happen on the
> network thread, isn't this single threaded?

There are multiple network threads but only one static value being read and
written here.

[xunjieli, 2016-07-20 18:08:03]

https://codereview.chromium.org/2160263002/diff/60001/net/udp/udp_socket_posi...
File net/udp/udp_socket_posix.cc (right):

https://codereview.chromium.org/2160263002/diff/60001/net/udp/udp_socket_posi...
net/udp/udp_socket_posix.cc:360: // This is racy, but all racers should come out
with the same answer so it
On 2016/07/20 18:02:16, pauljensen wrote:
> On 2016/07/20 17:55:49, xunjieli wrote:
> > One last question: why is this racy? If all socket connects happen on the
> > network thread, isn't this single threaded?
> 
> There are multiple network threads but only one static value being read and
> written here.

Ah, i see. I always forget Cronet can have multiple network threads.

[pauljensen, 2016-07-20 18:20:13]

pauljensen@chromium.org changed reviewers:
+ jkarlin@chromium.org, qinmin@chromium.org

[pauljensen, 2016-07-20 18:20:14]

jkarlin PTAL @ BackgroundSyncNetworkObserver.java
qinmin PTAL @ DownloadManagerService.java

[jkarlin, 2016-07-20 18:21:44]

BackgroundSyncNetworkObserver.java lgtm

[pauljensen, 2016-07-21 18:23:15]

Min, ping.  This should be a very quick review.

[qinmin, 2016-07-21 20:47:47]

lgtm

[pauljensen, 2016-07-22 11:23:17]

The CQ bit was checked by pauljensen@chromium.org

[pauljensen, 2016-07-22 11:23:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
xunjieli@chromium.org
Link to the patchset: https://codereview.chromium.org/2160263002/#ps80001
(title: "comment parseInt")

[commit-bot: I haz the power, 2016-07-22 11:23:35]

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, 2016-07-22 12:35:23]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-07-22 12:37:28]

Description was changed from

==========
Use supported Android network handles in Android M and newer.

In Android N loading system libraries is banned so use the supported mechanism
for Android M and newer releases.  For Android L we still need to manually load
the system library.

BUG=625188
==========

to

==========
Use supported Android network handles in Android M and newer.

In Android N loading system libraries is banned so use the supported mechanism
for Android M and newer releases.  For Android L we still need to manually load
the system library.

BUG=625188

Committed: https://crrev.com/479143c5186dc2559f645cf25f9aebc006e0f3a2
Cr-Commit-Position: refs/heads/master@{#407139}
==========

[commit-bot: I haz the power, 2016-07-22 12:37:29]

Patchset 5 (id:??) landed as
https://crrev.com/479143c5186dc2559f645cf25f9aebc006e0f3a2
Cr-Commit-Position: refs/heads/master@{#407139}