[Cronet] In JavaUrlRequest avoid thread-unsafe access to HttpURLConnection

[Cronet] In JavaUrlRequest avoid thread-unsafe access to HttpURLConnection

Executor passed into JavaUrlRequest is backed by a thread-pool so it can
concurrently run tasks which can lead to crashes in underlying
HttpURLConnection.  Prevent this by serializing taskes passed to
underlying Executor.

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

Review-Url: https://codereview.chromium.org/2613393002
Cr-Commit-Position: refs/heads/master@{#443307}
Committed: https://chromium.googlesource.com/chromium/src/+/16325c0ba632bc13701728830fe14a873ef8ab53

[pauljensen, 2017-01-07 02:20:34]

Description was changed from

==========
[Cronet] In JavaUrlRequest avoid thread-unsafe access to HttpURLConnection

Executor passed into JavaUrlRequest is backed by a thread-pool so it can
concurrently run tasks which can lead to crashes in underlying
HttpURLConnection.  Prevent this by serializing taskes passed to
underlying Executor.

BUG=664872
==========

to

==========
[Cronet] In JavaUrlRequest avoid thread-unsafe access to HttpURLConnection

Executor passed into JavaUrlRequest is backed by a thread-pool so it can
concurrently run tasks which can lead to crashes in underlying
HttpURLConnection.  Prevent this by serializing taskes passed to
underlying Executor.

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

[pauljensen, 2017-01-07 03:33:05]

pauljensen@chromium.org changed reviewers:
+ clm@google.com

[pauljensen, 2017-01-07 03:33:05]

Charles, PTAL, thank you!

[Charles, 2017-01-07 03:48:06]

https://codereview.chromium.org/2613393002/diff/1/components/cronet/android/j...
File
components/cronet/android/java/src/org/chromium/net/impl/JavaUrlRequest.java
(right):

https://codereview.chromium.org/2613393002/diff/1/components/cronet/android/j...
components/cronet/android/java/src/org/chromium/net/impl/JavaUrlRequest.java:135:
private final List<Runnable> mTaskQueue = new LinkedList<>();
Use an ArrayDeque instead - faster, offers methods named for queues, and
allocates less memory.

https://codereview.chromium.org/2613393002/diff/1/components/cronet/android/j...
components/cronet/android/java/src/org/chromium/net/impl/JavaUrlRequest.java:135:
private final List<Runnable> mTaskQueue = new LinkedList<>();
Also @GuardedBy

https://codereview.chromium.org/2613393002/diff/1/components/cronet/android/j...
components/cronet/android/java/src/org/chromium/net/impl/JavaUrlRequest.java:155:
task.run();
If this throws an exception, other submitted tasks won't run until another task
is submitted - you could fix that by changing this code to submit a new runnable
to the underlying executor for each runnable. 

The reason this isn't done in the Guava implementation is that someone could
wrap a direct executor, and that would cause stack overflow. That's not a risk
here, because we control the executor we're wrapping.

[pauljensen, 2017-01-11 15:29:59]

https://codereview.chromium.org/2613393002/diff/1/components/cronet/android/j...
File
components/cronet/android/java/src/org/chromium/net/impl/JavaUrlRequest.java
(right):

https://codereview.chromium.org/2613393002/diff/1/components/cronet/android/j...
components/cronet/android/java/src/org/chromium/net/impl/JavaUrlRequest.java:135:
private final List<Runnable> mTaskQueue = new LinkedList<>();
On 2017/01/07 03:48:06, Charles wrote:
> Also @GuardedBy

Done.

https://codereview.chromium.org/2613393002/diff/1/components/cronet/android/j...
components/cronet/android/java/src/org/chromium/net/impl/JavaUrlRequest.java:135:
private final List<Runnable> mTaskQueue = new LinkedList<>();
On 2017/01/07 03:48:06, Charles wrote:
> Use an ArrayDeque instead - faster, offers methods named for queues, and
> allocates less memory.

Done.

https://codereview.chromium.org/2613393002/diff/1/components/cronet/android/j...
components/cronet/android/java/src/org/chromium/net/impl/JavaUrlRequest.java:155:
task.run();
On 2017/01/07 03:48:06, Charles wrote:
> If this throws an exception, other submitted tasks won't run until another
task
> is submitted - you could fix that by changing this code to submit a new
runnable
> to the underlying executor for each runnable. 
> 
> The reason this isn't done in the Guava implementation is that someone could
> wrap a direct executor, and that would cause stack overflow. That's not a risk
> here, because we control the executor we're wrapping.

Done, note that the Runnables can only be submitted after each task has run.

[Charles, 2017-01-11 16:50:05]

https://codereview.chromium.org/2613393002/diff/20001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/impl/JavaUrlRequest.java
(right):

https://codereview.chromium.org/2613393002/diff/20001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/impl/JavaUrlRequest.java:133:
private class SeriaizingExecutor implements Executor {
private final class

https://codereview.chromium.org/2613393002/diff/20001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/impl/JavaUrlRequest.java:140:
public SeriaizingExecutor(Executor underlyingExecutor) {
No need for public

[pauljensen, 2017-01-11 16:53:50]

https://codereview.chromium.org/2613393002/diff/20001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/impl/JavaUrlRequest.java
(right):

https://codereview.chromium.org/2613393002/diff/20001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/impl/JavaUrlRequest.java:133:
private class SeriaizingExecutor implements Executor {
On 2017/01/11 16:50:05, Charles wrote:
> private final class

Done.

https://codereview.chromium.org/2613393002/diff/20001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/impl/JavaUrlRequest.java:140:
public SeriaizingExecutor(Executor underlyingExecutor) {
On 2017/01/11 16:50:05, Charles wrote:
> No need for public

Done.

[Charles, 2017-01-11 16:54:21]

lgtm

[pauljensen, 2017-01-12 02:53:01]

Charles, PTAL, I had to handle RejectedExecutionException when
JavaCronetEngine.shutdown() was called.

[Charles, 2017-01-12 17:07:37]

lgtm

[pauljensen, 2017-01-12 17:13:58]

The CQ bit was checked by pauljensen@chromium.org

[commit-bot: I haz the power, 2017-01-12 17:14:40]

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-01-12 18:53:09]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484241238284570,
"parent_rev": "080b78c46001c096c9029d24e781e0b78aface5f", "commit_rev":
"16325c0ba632bc13701728830fe14a873ef8ab53"}

[commit-bot: I haz the power, 2017-01-12 18:53:40]

Description was changed from

==========
[Cronet] In JavaUrlRequest avoid thread-unsafe access to HttpURLConnection

Executor passed into JavaUrlRequest is backed by a thread-pool so it can
concurrently run tasks which can lead to crashes in underlying
HttpURLConnection.  Prevent this by serializing taskes passed to
underlying Executor.

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

to

==========
[Cronet] In JavaUrlRequest avoid thread-unsafe access to HttpURLConnection

Executor passed into JavaUrlRequest is backed by a thread-pool so it can
concurrently run tasks which can lead to crashes in underlying
HttpURLConnection.  Prevent this by serializing taskes passed to
underlying Executor.

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

Review-Url: https://codereview.chromium.org/2613393002
Cr-Commit-Position: refs/heads/master@{#443307}
Committed:
https://chromium.googlesource.com/chromium/src/+/16325c0ba632bc13701728830fe1...
==========

[commit-bot: I haz the power, 2017-01-12 18:53:41]

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