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

Issue 626653 link

Starred by 3 users
Status: Fixed
Owner:
█ xunji...@chromium.org
Last visit > 30 days ago
Closed: Jul 2016
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug



Sign in to add a comment

CronetChunkedOutputStream.write() might block forever

Project Member Reported by xunji...@chromium.org, Jul 8 2016 
Client reported that CronetChunkedOutputStream.write() might block forever when write() is called after we have received onFailed(). If the request has failed, we should not run the loop to wait for the native stack to consume the bytes, since the native stack is done with this request and we will never be called back.

A similar issue was addressed in CronetInputStream.java where read() might block forever. I think we can apply the same approach in Cronet*OutputStream.java.

Comment 1 by xunji...@chromium.org, Jul 8 2016

Status: Started (was: Assigned)
CronetChunkedOutputStream.close() might unintentionally block the calling thread as well. I have an old CL, and I will land it first. UploadDataProviderImpl.read() might need a rewrite as well :\
Project Member

Comment 2 by bugdroid1@chromium.org, Jul 8 2016 

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

commit 578950706701e926c468cc1ab8128e94bd032e08
Author: xunjieli <xunjieli@chromium.org>
Date: Fri Jul 08 15:18:26 2016

Remove unnecessary loop and quit in CronetChunkedOutputStream

The MessageLoop.loop() in CronetChunkedOutputStream.close()
is potentially problematic. If the user closes the
OutputStream unintentionally when MessageLoop is not going
to be quit, it might wait forever. This removes the
corresponding unnecessary quit().

BUG= 626653 

Review-Url: https://codereview.chromium.org/1657103002
Cr-Commit-Position: refs/heads/master@{#404391}

[modify] https://crrev.com/578950706701e926c468cc1ab8128e94bd032e08/components/cronet/android/java/src/org/chromium/net/urlconnection/CronetChunkedOutputStream.java
Project Member

Comment 3 by bugdroid1@chromium.org, Jul 11 2016 

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

commit b08d71cee8ab485bc4bf6872d01be9fe8b4df296
Author: xunjieli <xunjieli@chromium.org>
Date: Mon Jul 11 14:17:12 2016

[Cronet] Check whether request is done before spinning up message loop

When client writes to OutputStream, the request might have already completed.
If that is the case, do not spin up the message loop since that will block
the calling thread forever.

This CL also adds tests.

R=kapishnikov@chromium.org
R=mef@chromium.org
BUG= 626653 

Review-Url: https://codereview.chromium.org/2131323002
Cr-Commit-Position: refs/heads/master@{#404652}

[modify] https://crrev.com/b08d71cee8ab485bc4bf6872d01be9fe8b4df296/components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java
[modify] https://crrev.com/b08d71cee8ab485bc4bf6872d01be9fe8b4df296/components/cronet/android/java/src/org/chromium/net/urlconnection/CronetChunkedOutputStream.java
[modify] https://crrev.com/b08d71cee8ab485bc4bf6872d01be9fe8b4df296/components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java
[modify] https://crrev.com/b08d71cee8ab485bc4bf6872d01be9fe8b4df296/components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java
[modify] https://crrev.com/b08d71cee8ab485bc4bf6872d01be9fe8b4df296/components/cronet/android/java/src/org/chromium/net/urlconnection/CronetOutputStream.java
[modify] https://crrev.com/b08d71cee8ab485bc4bf6872d01be9fe8b4df296/components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetChunkedOutputStreamTest.java
[modify] https://crrev.com/b08d71cee8ab485bc4bf6872d01be9fe8b4df296/components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetFixedModeOutputStreamTest.java
Project Member

Comment 4 by bugdroid1@chromium.org, Jul 11 2016 

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

commit bf9178a723b6fb899d7b73f2793d532fc4badd66
Author: xunjieli <xunjieli@chromium.org>
Date: Mon Jul 11 15:53:30 2016

[Cronet] Remove buffer.compact from CronetChunkedOutputStream

This CL unifies the buffer manipulation code in CronetChunkedOutputStream to
match the one in CronetFixedOutputStream.

BUG= 626653 
BUG= 618872 

Review-Url: https://codereview.chromium.org/2133273002
Cr-Commit-Position: refs/heads/master@{#404664}

[modify] https://crrev.com/bf9178a723b6fb899d7b73f2793d532fc4badd66/components/cronet/android/java/src/org/chromium/net/urlconnection/CronetChunkedOutputStream.java
[modify] https://crrev.com/bf9178a723b6fb899d7b73f2793d532fc4badd66/components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetChunkedOutputStreamTest.java

Comment 5 by xunji...@chromium.org, Jul 11 2016

Components: Internals>Network>Library
Labels: Merge-Request-53
Requesting merge into M53 for the above three CLs.
These three CLs do not impact Chrome. The code is only compiled in Cronet.
Thanks!

Comment 6 by dimu@google.com, Jul 11 2016

Labels: -Merge-Request-53 Merge-Approved-53 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M53 (branch: 2785)
Project Member

Comment 7 by bugdroid1@chromium.org, Jul 11 2016

Labels: -merge-approved-53 merge-merged-2785
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d299790d76cbea13398a508b8981e852df71ff08

commit d299790d76cbea13398a508b8981e852df71ff08
Author: xunjieli <xunjieli@chromium.org>
Date: Mon Jul 11 17:33:45 2016

Remove unnecessary loop and quit in CronetChunkedOutputStream

The MessageLoop.loop() in CronetChunkedOutputStream.close()
is potentially problematic. If the user closes the
OutputStream unintentionally when MessageLoop is not going
to be quit, it might wait forever. This removes the
corresponding unnecessary quit().

BUG= 626653 
TBR=pauljensen@chromium.org
NOTRY=true
NOPRESUBMIT=true

Review-Url: https://codereview.chromium.org/1657103002
Cr-Commit-Position: refs/heads/master@{#404391}
(cherry picked from commit 578950706701e926c468cc1ab8128e94bd032e08)

Review-Url: https://codereview.chromium.org/2137163002
Cr-Commit-Position: refs/branch-heads/2785@{#84}
Cr-Branched-From: 68623971be0cfc492a2cb0427d7f478e7b214c24-refs/heads/master@{#403382}

[modify] https://crrev.com/d299790d76cbea13398a508b8981e852df71ff08/components/cronet/android/java/src/org/chromium/net/urlconnection/CronetChunkedOutputStream.java
Project Member

Comment 8 by bugdroid1@chromium.org, Jul 11 2016 

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

commit 1e353500420e895b051c1d22046a3f110ac74722
Author: xunjieli <xunjieli@chromium.org>
Date: Mon Jul 11 18:42:58 2016

[Cronet] Check whether request is done before spinning up message loop

When client writes to OutputStream, the request might have already completed.
If that is the case, do not spin up the message loop since that will block
the calling thread forever.

This CL also adds tests.

TBR=mef@chromium.org
BUG= 626653 

NOTRY=true
NOPRESUBMIT=true
Review-Url: https://codereview.chromium.org/2131323002
Cr-Commit-Position: refs/heads/master@{#404652}
(cherry picked from commit b08d71cee8ab485bc4bf6872d01be9fe8b4df296)

Review-Url: https://codereview.chromium.org/2141663002
Cr-Commit-Position: refs/branch-heads/2785@{#87}
Cr-Branched-From: 68623971be0cfc492a2cb0427d7f478e7b214c24-refs/heads/master@{#403382}

[modify] https://crrev.com/1e353500420e895b051c1d22046a3f110ac74722/components/cronet/android/java/src/org/chromium/net/urlconnection/CronetBufferedOutputStream.java
[modify] https://crrev.com/1e353500420e895b051c1d22046a3f110ac74722/components/cronet/android/java/src/org/chromium/net/urlconnection/CronetChunkedOutputStream.java
[modify] https://crrev.com/1e353500420e895b051c1d22046a3f110ac74722/components/cronet/android/java/src/org/chromium/net/urlconnection/CronetFixedModeOutputStream.java
[modify] https://crrev.com/1e353500420e895b051c1d22046a3f110ac74722/components/cronet/android/java/src/org/chromium/net/urlconnection/CronetHttpURLConnection.java
[modify] https://crrev.com/1e353500420e895b051c1d22046a3f110ac74722/components/cronet/android/java/src/org/chromium/net/urlconnection/CronetOutputStream.java
[modify] https://crrev.com/1e353500420e895b051c1d22046a3f110ac74722/components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetChunkedOutputStreamTest.java
[modify] https://crrev.com/1e353500420e895b051c1d22046a3f110ac74722/components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetFixedModeOutputStreamTest.java
Project Member

Comment 9 by bugdroid1@chromium.org, Jul 11 2016 

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

commit 6ebc21fe28de20e525ba5d8a8f700ea4e6702769
Author: xunjieli <xunjieli@chromium.org>
Date: Mon Jul 11 19:20:59 2016

[Cronet] Remove buffer.compact from CronetChunkedOutputStream

This CL unifies the buffer manipulation code in CronetChunkedOutputStream to
match the one in CronetFixedOutputStream.

BUG= 626653 
TBR=kapishnikov@chromium.org
TBR=mef@chromium.org

NOTRY=true
NOPRESUBMIT=true

Review-Url: https://codereview.chromium.org/2133273002
Cr-Commit-Position: refs/heads/master@{#404664}
(cherry picked from commit bf9178a723b6fb899d7b73f2793d532fc4badd66)

Review-Url: https://codereview.chromium.org/2136163002
Cr-Commit-Position: refs/branch-heads/2785@{#88}
Cr-Branched-From: 68623971be0cfc492a2cb0427d7f478e7b214c24-refs/heads/master@{#403382}

[modify] https://crrev.com/6ebc21fe28de20e525ba5d8a8f700ea4e6702769/components/cronet/android/java/src/org/chromium/net/urlconnection/CronetChunkedOutputStream.java
[modify] https://crrev.com/6ebc21fe28de20e525ba5d8a8f700ea4e6702769/components/cronet/android/test/javatests/src/org/chromium/net/urlconnection/CronetChunkedOutputStreamTest.java

Comment 10 by xunji...@chromium.org, Jul 11 2016

Status: Fixed (was: Started)
►

Sign in to add a comment