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

Issue 761397 link

Starred by 1 user

Issue metadata

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



Sign in to add a comment

JavaUrlRequest crashes with HEAD requests that fail (e.g. get 404 response)

Project Member Reported by pauljensen@chromium.org, Sep 1 2017

Issue description

java.lang.NullPointerException:
        at org.chromium.net.impl.InputStreamChannel.close(InputStreamChannel.java:24)
        at org.chromium.net.impl.JavaUrlRequest$13.run(JavaUrlRequest.java:997)
        at org.chromium.net.impl.JavaUrlRequest$SerializingExecutor$1.run(JavaUrlRequest.java:143)
        at org.chromium.net.impl.JavaUrlRequest$1$1.run(JavaUrlRequest.java:222)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1112)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:587)
        at org.chromium.net.impl.JavaCronetEngine$1$1.run(JavaCronetEngine.java)
        at java.lang.Thread.run(Thread.java:841)

For error responses like 404, JavaUrlRequest sets mResponseChannel to HttpURLConnection.getErrorStream() which is documented to return null if the response has no body.

I've made a reproducible test case: https://chromium-review.googlesource.com/647831

Internal bug b/65279777
 
Owner: pauljensen@chromium.org
Status: Started (was: Available)
Proposed a fix in the test case CL.
Project Member

Comment 2 by bugdroid1@chromium.org, Sep 1 2017

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

commit c899367d3bf8802d7e8eb3245c1ff369b038069d
Author: Paul Jensen <pauljensen@chromium.org>
Date: Fri Sep 01 21:22:17 2017

[Cronet] Fix JavaUrlRequest crash when HEAD request gets back 404

HttpURLConnection.getErrorStream() can return null when there is no
response body (e.g. HEAD request).  Avoid crashing in InputStreamChannel
by not creating an InputStreamChannel.

Bug:  761397 
Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester
Change-Id: I9a094efe1a759f87d8444da2bf745cfe40a46f5c
Reviewed-on: https://chromium-review.googlesource.com/647831
Commit-Queue: Paul Jensen <pauljensen@chromium.org>
Reviewed-by: Helen Li <xunjieli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#499303}
[modify] https://crrev.com/c899367d3bf8802d7e8eb3245c1ff369b038069d/components/cronet/android/BUILD.gn
[modify] https://crrev.com/c899367d3bf8802d7e8eb3245c1ff369b038069d/components/cronet/android/java/src/org/chromium/net/impl/InputStreamChannel.java
[modify] https://crrev.com/c899367d3bf8802d7e8eb3245c1ff369b038069d/components/cronet/android/java/src/org/chromium/net/impl/JavaUrlRequest.java
[modify] https://crrev.com/c899367d3bf8802d7e8eb3245c1ff369b038069d/components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java

Labels: Merge-Request-62
Project Member

Comment 4 by sheriffbot@chromium.org, Sep 2 2017

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

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

Comment 5 by bugdroid1@chromium.org, Sep 3 2017

Labels: -merge-approved-62 merge-merged-3202
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f9051fd3b3a824bb491303752a8c6887c479cf73

commit f9051fd3b3a824bb491303752a8c6887c479cf73
Author: Paul Jensen <pauljensen@chromium.org>
Date: Sun Sep 03 23:34:59 2017

[Cronet] Fix JavaUrlRequest crash when HEAD request gets back 404

HttpURLConnection.getErrorStream() can return null when there is no
response body (e.g. HEAD request).  Avoid crashing in InputStreamChannel
by not creating an InputStreamChannel.

TBR=pauljensen@chromium.org

(cherry picked from commit c899367d3bf8802d7e8eb3245c1ff369b038069d)

Bug:  761397 
Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester
Change-Id: I9a094efe1a759f87d8444da2bf745cfe40a46f5c
Reviewed-on: https://chromium-review.googlesource.com/647831
Commit-Queue: Paul Jensen <pauljensen@chromium.org>
Reviewed-by: Helen Li <xunjieli@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#499303}
Reviewed-on: https://chromium-review.googlesource.com/648847
Reviewed-by: Paul Jensen <pauljensen@chromium.org>
Cr-Commit-Position: refs/branch-heads/3202@{#15}
Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098}
[modify] https://crrev.com/f9051fd3b3a824bb491303752a8c6887c479cf73/components/cronet/android/BUILD.gn
[modify] https://crrev.com/f9051fd3b3a824bb491303752a8c6887c479cf73/components/cronet/android/java/src/org/chromium/net/impl/InputStreamChannel.java
[modify] https://crrev.com/f9051fd3b3a824bb491303752a8c6887c479cf73/components/cronet/android/java/src/org/chromium/net/impl/JavaUrlRequest.java
[modify] https://crrev.com/f9051fd3b3a824bb491303752a8c6887c479cf73/components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java

Status: Fixed (was: Started)
Project Member

Comment 7 by bugdroid1@chromium.org, Sep 11 2017

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

commit 4632882b430e20eb0a3e4071d9ad068386be6a1a
Author: Paul Jensen <pauljensen@chromium.org>
Date: Mon Sep 11 16:32:55 2017

[Cronet] Requires new API level for CronetUrlRequestTest.test404Head

This is a regression test for a fix that went into JavaUrlRequest
which is packaged with the API, so require the latest API level
to run this test.

Bug:  761397 
Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester
Change-Id: Ie1abb8e1fb8889da7a144a87cb59f6d6b8298e2f
Reviewed-on: https://chromium-review.googlesource.com/657947
Reviewed-by: Helen Li <xunjieli@chromium.org>
Commit-Queue: Paul Jensen <pauljensen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#500940}
[modify] https://crrev.com/4632882b430e20eb0a3e4071d9ad068386be6a1a/components/cronet/android/api_version.txt
[modify] https://crrev.com/4632882b430e20eb0a3e4071d9ad068386be6a1a/components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java

Project Member

Comment 8 by bugdroid1@chromium.org, Sep 11 2017

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

commit 4a79f6b04bcfa83c92fd1dbbe58954c7aeb866d7
Author: Paul Jensen <pauljensen@chromium.org>
Date: Mon Sep 11 16:43:03 2017

[Cronet] Requires new API level for CronetUrlRequestTest.test404Head

This is a regression test for a fix that went into JavaUrlRequest
which is packaged with the API, so require the latest API level
to run this test.

TBR=pauljensen@chromium.org

(cherry picked from commit 4632882b430e20eb0a3e4071d9ad068386be6a1a)

Bug:  761397 
Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester
Change-Id: Ie1abb8e1fb8889da7a144a87cb59f6d6b8298e2f
Reviewed-on: https://chromium-review.googlesource.com/657947
Reviewed-by: Helen Li <xunjieli@chromium.org>
Commit-Queue: Paul Jensen <pauljensen@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#500940}
Reviewed-on: https://chromium-review.googlesource.com/660386
Reviewed-by: Paul Jensen <pauljensen@chromium.org>
Cr-Commit-Position: refs/branch-heads/3202@{#129}
Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098}
[modify] https://crrev.com/4a79f6b04bcfa83c92fd1dbbe58954c7aeb866d7/components/cronet/android/api_version.txt
[modify] https://crrev.com/4a79f6b04bcfa83c92fd1dbbe58954c7aeb866d7/components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestTest.java

Sign in to add a comment