New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 747197 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner: ----
Closed: Jul 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug



Sign in to add a comment

JavaUrlRequest doesn't shutdown properly

Project Member Reported by pauljensen@chromium.org, Jul 21 2017

Issue description

I'm seeing leaks from several tests:

CronetUrlRequestTest.testUnexpectedReads
CronetUrlRequestTest.testUploadReadFailAsync
CronetUrlRequestTest.testUnexpectedFollowRedirects 

They look like this:

07-20 04:57:57.107  6669  6678 E StrictMode: A resource was acquired at attached stack trace but never released. See java.io.Closeable for information on avoiding resource leaks.
07-20 04:57:57.107  6669  6678 E StrictMode: java.lang.Throwable: Explicit termination method 'close' not called
07-20 04:57:57.107  6669  6678 E StrictMode: 	at dalvik.system.CloseGuard.open(CloseGuard.java:180)
07-20 04:57:57.107  6669  6678 E StrictMode: 	at java.net.AbstractPlainSocketImpl.create(AbstractPlainSocketImpl.java:103)
07-20 04:57:57.107  6669  6678 E StrictMode: 	at java.net.Socket.createImpl(Socket.java:451)
07-20 04:57:57.107  6669  6678 E StrictMode: 	at java.net.Socket.getImpl(Socket.java:517)
07-20 04:57:57.107  6669  6678 E StrictMode: 	at java.net.Socket.setSoTimeout(Socket.java:1108)
07-20 04:57:57.107  6669  6678 E StrictMode: 	at com.android.okhttp.Connection.connectSocket(Connection.java:195)
07-20 04:57:57.107  6669  6678 E StrictMode: 	at com.android.okhttp.Connection.connect(Connection.java:172)
07-20 04:57:57.107  6669  6678 E StrictMode: 	at com.android.okhttp.Connection.connectAndSetOwner(Connection.java:367)
07-20 04:57:57.107  6669  6678 E StrictMode: 	at com.android.okhttp.OkHttpClient$1.connectAndSetOwner(OkHttpClient.java:130)
07-20 04:57:57.107  6669  6678 E StrictMode: 	at com.android.okhttp.internal.http.HttpEngine.connect(HttpEngine.java:329)
07-20 04:57:57.107  6669  6678 E StrictMode: 	at com.android.okhttp.internal.http.HttpEngine.sendRequest(HttpEngine.java:246)
07-20 04:57:57.107  6669  6678 E StrictMode: 	at com.android.okhttp.internal.huc.HttpURLConnectionImpl.execute(HttpURLConnectionImpl.java:457)
07-20 04:57:57.107  6669  6678 E StrictMode: 	at com.android.okhttp.internal.huc.HttpURLConnectionImpl.connect(HttpURLConnectionImpl.java:126)
07-20 04:57:57.107  6669  6678 E StrictMode: 	at org.chromium.net.impl.JavaUrlRequest$7.run(JavaUrlRequest.java:696)
07-20 04:57:57.107  6669  6678 E StrictMode: 	at org.chromium.net.impl.JavaUrlRequest$8.run(JavaUrlRequest.java:708)
07-20 04:57:57.107  6669  6678 E StrictMode: 	at org.chromium.net.impl.JavaUrlRequest$SerializingExecutor$1.run(JavaUrlRequest.java:142)
07-20 04:57:57.107  6669  6678 E StrictMode: 	at org.chromium.net.impl.JavaUrlRequest$1$1.run(JavaUrlRequest.java:205)
07-20 04:57:57.107  6669  6678 E StrictMode: 	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1133)
07-20 04:57:57.107  6669  6678 E StrictMode: 	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:607)
07-20 04:57:57.107  6669  6678 E StrictMode: 	at org.chromium.net.impl.JavaCronetEngine$1$1.run(JavaCronetEngine.java:58)
07-20 04:57:57.107  6669  6678 E StrictMode: 	at java.lang.Thread.run(Thread.java:761)

I tried running on a Nexus 6 flashed with Nougat but didn't see the failures.
I used an x86 emulator with Nougat and could see the failures.

I'm not entirely sure what's going on but I see at least one problem:
JavaUrlRequest.SerializingExecutor.runTask() only runs one task and then attempts to post the next task to the underlying executor.  This doesn't work well with ExecutorService.shutdown().  At shutdown time, all tasks in the queue should be executed, but because runTask() posts the next task it gets RejectedExecutionException and so only attempts to run one task.  This can lead to the disconnect() task being rejected, even though it was submitted to the queue prior to initiating shutdown().
 
Status: Started (was: Available)
Working on a fix: https://chromium-review.googlesource.com/c/581433/
Project Member

Comment 2 by bugdroid1@chromium.org, Jul 25 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/78a1b1adde7d141ff0cd5c0f00b6eaa9f4ba6338

commit 78a1b1adde7d141ff0cd5c0f00b6eaa9f4ba6338
Author: Paul Jensen <pauljensen@chromium.org>
Date: Tue Jul 25 15:10:14 2017

[Cronet] Fix JavaUrlRequest shutdown behavior

The SerializingExecutor was previously posting tasks from its queue to the
underlying Executor.  This fails to execute all tasks in its queue during
shutdown because the underlying Executor.execute() rejects new tasks.
This causes leaks.  This change makes the SerializingExecutor loop over
tasks in its queue without posting to the underlying Executor.

BUG= 747197 

Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester
Change-Id: I3ac1ec880f4c47e7b61a02159dce2f17f2fb7658
Reviewed-on: https://chromium-review.googlesource.com/581433
Reviewed-by: Miriam Gershenson <mgersh@chromium.org>
Commit-Queue: Miriam Gershenson <mgersh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#489302}
[modify] https://crrev.com/78a1b1adde7d141ff0cd5c0f00b6eaa9f4ba6338/components/cronet/android/java/src/org/chromium/net/impl/JavaUrlRequest.java

Labels: Merge-Request-61
Labels: -Merge-Request-61
Hmm I requested the merge before my fix went out in a canary build.  Remove tag.
Labels: Merge-Request-61
Project Member

Comment 6 by sheriffbot@chromium.org, Jul 29 2017

Labels: -Merge-Request-61 Hotlist-Merge-Approved Merge-Approved-61
Your change meets the bar and is auto-approved for M61. Please go ahead and merge the CL to branch 3163 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), ketakid @(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 7 by bugdroid1@chromium.org, Jul 31 2017

Labels: -merge-approved-61 merge-merged-3163
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/755154e69631ebf9d94c4671209c3c8156b17ca5

commit 755154e69631ebf9d94c4671209c3c8156b17ca5
Author: Paul Jensen <pauljensen@chromium.org>
Date: Mon Jul 31 13:05:56 2017

[Cronet] Fix JavaUrlRequest shutdown behavior

The SerializingExecutor was previously posting tasks from its queue to the
underlying Executor.  This fails to execute all tasks in its queue during
shutdown because the underlying Executor.execute() rejects new tasks.
This causes leaks.  This change makes the SerializingExecutor loop over
tasks in its queue without posting to the underlying Executor.

BUG= 747197 
TBR=pauljensen@chromium.org

(cherry picked from commit 78a1b1adde7d141ff0cd5c0f00b6eaa9f4ba6338)

Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester
Change-Id: I3ac1ec880f4c47e7b61a02159dce2f17f2fb7658
Reviewed-on: https://chromium-review.googlesource.com/581433
Reviewed-by: Miriam Gershenson <mgersh@chromium.org>
Commit-Queue: Miriam Gershenson <mgersh@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#489302}
Reviewed-on: https://chromium-review.googlesource.com/593771
Reviewed-by: Paul Jensen <pauljensen@chromium.org>
Cr-Commit-Position: refs/branch-heads/3163@{#147}
Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528}
[modify] https://crrev.com/755154e69631ebf9d94c4671209c3c8156b17ca5/components/cronet/android/java/src/org/chromium/net/impl/JavaUrlRequest.java

Status: Fixed (was: Started)

Sign in to add a comment