Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1911353003:
[Cronet] Do not call into BidirectionalStream::SendData if stream failed (Closed)
|
Created:
4 years, 8 months ago by xunjieli Modified:
4 years, 8 months ago Reviewers:
kapishnikov CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Cronet] Do not call into BidirectionalStream::SendData if stream failed
Cronet currently posts a task to the network thread to
write data to the stream. However, between the time when
the task is posted on Java thread and when the task is
executed on the network thread, the underlying
stream might have failed for some reason (e.g. server
terminated the connection). If that is the case,
we should not call into the stream.
Without this patch, when running the new unit test with
asan enabled, we will have a crash.
Committed: https://crrev.com/943b9ad8edd8185defebc09d5917242122833cc4
Cr-Commit-Position: refs/heads/master@{#389379}
Patch Set 1 : #
Total comments: 7
Patch Set 2 : Address comments #
Messages
Total messages: 18 (10 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== temp BUG= ========== to ========== [Cronet] Do not call into BidirectionalStream::SendData if stream failed Cronet currently posts a task to the network thread to write data to the stream. However, between the time when the task is posted on Java thread and when the task is executed on the network thread, the underlying stream might have failed for some reason (e.g. server terminated the connection). If that is the case, we should not call into the stream. ==========
Patchset #1 (id:20001) has been deleted
Description was changed from ========== [Cronet] Do not call into BidirectionalStream::SendData if stream failed Cronet currently posts a task to the network thread to write data to the stream. However, between the time when the task is posted on Java thread and when the task is executed on the network thread, the underlying stream might have failed for some reason (e.g. server terminated the connection). If that is the case, we should not call into the stream. ========== to ========== [Cronet] Do not call into BidirectionalStream::SendData if stream failed Cronet currently posts a task to the network thread to write data to the stream. However, between the time when the task is posted on Java thread and when the task is executed on the network thread, the underlying stream might have failed for some reason (e.g. server terminated the connection). If that is the case, we should not call into the stream. Without this patch, when running the unit test with asan enabled, we will have a crash. ==========
Description was changed from ========== [Cronet] Do not call into BidirectionalStream::SendData if stream failed Cronet currently posts a task to the network thread to write data to the stream. However, between the time when the task is posted on Java thread and when the task is executed on the network thread, the underlying stream might have failed for some reason (e.g. server terminated the connection). If that is the case, we should not call into the stream. Without this patch, when running the unit test with asan enabled, we will have a crash. ========== to ========== [Cronet] Do not call into BidirectionalStream::SendData if stream failed Cronet currently posts a task to the network thread to write data to the stream. However, between the time when the task is posted on Java thread and when the task is executed on the network thread, the underlying stream might have failed for some reason (e.g. server terminated the connection). If that is the case, we should not call into the stream. Without this patch, when running the new unit test with asan enabled, we will have a crash. ==========
xunjieli@chromium.org changed reviewers: + kapishnikov@chromium.org
Hi Andrei, I think what our embedder has seen is probably due to this. When I run the new test that I included in this CL with address sanitizer in release mode, I will have the exact same call stack. I/cr_ChromiumNetwork( 9023): destroyNativeStreamLocked org.chromium.net.CronetBidirectionalStream@448e3aa0 I/ ( 9023): ================================================================= I/TestRunner( 9023): finished: testOnErrorBeforeWriteIsExecutedOnNetworkThread(org.chromium.net.BidirectionalStreamQuicTest) I/TestRunner( 9023): passed: testOnErrorBeforeWriteIsExecutedOnNetworkThread(org.chromium.net.BidirectionalStreamQuicTest) I/ ( 9023): ==9023==ERROR: AddressSanitizer: SEGV on unknown address 0x0000009a (pc 0x79dec604 bp 0x7a022f5c sp 0x7a24e938 T2593) D/AndroidRuntime( 9009): Shutting down VM D/dalvikvm( 9009): GC_CONCURRENT freed 106K, 17% free 567K/676K, paused 1ms+0ms, total 3ms I/ ( 9023): #0 net::ReliableQuicStream::WriteOrBufferData(base::BasicStringPiece<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > >, bool, net::QuicAckListenerInterface*) /usr/local/google/home/xunjieli/chrome/src/out/Release/../../net/quic/reliable_quic_stream.cc:191 I/ ( 9023): #1 net::QuicChromiumClientStream::WriteStreamData(base::BasicStringPiece<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > >, bool, base::Callback<void (int), (base::internal::CopyMode)1> const&) /usr/local/google/home/xunjieli/chrome/src/out/Release/../../net/quic/quic_chromium_client_stream.cc:133 I/ ( 9023): #2 net::BidirectionalStreamQuicImpl::SendData(net::IOBuffer*, int, bool) /usr/local/google/home/xunjieli/chrome/src/out/Release/../../net/quic/bidirectional_stream_quic_impl.cc:106 I/ ( 9023): #3 net::BidirectionalStream::SendData(net::IOBuffer*, int, bool) /usr/local/google/home/xunjieli/chrome/src/out/Release/../../net/http/bidirectional_stream.cc:108 I/ ( 9023): #4 void base::internal::RunnableAdapter<void (cronet::CronetBidirectionalStreamAdapter::*)(scoped_refptr<cronet::IOBufferWithByteBuffer>, int, bool)>::Run<scoped_refptr<cronet::IOBufferWithByteBuffer> const&, int const&, unsigned char const&>(cronet::CronetBidirectionalStreamAdapter*, scoped_refptr<cronet::IOBufferWithByteBuffer> const&, int const&, unsigned char const&) /usr/local/google/home/xunjieli/chrome/src/out/Release/../../base/bind_internal.h:181 I/ ( 9023): #5 base::Callback<void (), (base::internal::CopyMode)1>::Run() const /usr/local/google/home/xunjieli/chrome/src/out/Release/../../base/callback.h:397 I/ ( 9023): #6 base::MessageLoop::RunTask(base::PendingTask const&) /usr/local/google/home/xunjieli/chrome/src/out/Release/../../base/message_loop/message_loop.cc:479 I/ ( 9023): #7 base::MessageLoop::DeferOrRunPendingTask(base::PendingTask const&) /usr/local/google/home/xunjieli/chrome/src/out/Release/../../base/message_loop/message_loop.cc:488 I/ ( 9023): #8 base::MessageLoop::DoWork() /usr/local/google/home/xunjieli/chrome/src/out/Release/../../base/message_loop/message_loop.cc:600 I/ ( 9023): #9 base::MessagePumpLibevent::Run(base::MessagePump::Delegate*) /usr/local/google/home/xunjieli/chrome/src/out/Release/../../base/message_loop/message_pump_libevent.cc:230 I/ ( 9023): #10 base::RunLoop::Run() /usr/local/google/home/xunjieli/chrome/src/out/Release/../../base/run_loop.cc:35 I/ ( 9023): #11 base::MessageLoop::Run() /usr/local/google/home/xunjieli/chrome/src/out/Release/../../base/message_loop/message_loop.cc:295 I/ ( 9023): #12 base::Thread::Run(base::MessageLoop*) /usr/local/google/home/xunjieli/chrome/src/out/Release/../../base/threading/thread.cc:202 I/ ( 9023): #13 ThreadFunc /usr/local/google/home/xunjieli/chrome/src/out/Release/../../base/threading/platform_thread_posix.cc:70 I/ ( 9023): #14 0x40633171 (/system/lib/libc.so+0xd171) I/ ( 9023): #15 0x40633309 (/system/lib/libc.so+0xd309) I/ ( 9023): #16 0x74a0e435 (<unknown module>) I/ ( 9023): AddressSanitizer can not provide additional info. I/ ( 9023): SUMMARY: AddressSanitizer: SEGV (/data/app-lib/org.chromium.net-1/libcronet_tests.so+0x67605) I/ ( 9023): Thread T2593 (network) created by T2590 here: I/ ( 9023): #0 __sanitizer_syscall_pre_impl_ioprio_set /b/build/slave/linux_upload_clang/build/src/third_party/llvm/compiler-rt/lib/asan/../sanitizer_common/sanitizer_common_syscalls.inc:2381 I/ ( 9023): #1 CreateThread /usr/local/google/home/xunjieli/chrome/src/out/Release/../../base/threading/platform_thread_posix.cc:109 I/ ( 9023): #2 0x74b00b5b (<unknown module>) I/ ( 9023): Thread T2590 created by T0 (rg.chromium.net) here: I/ ( 9023): #0 __sanitizer_syscall_pre_impl_ioprio_set /b/build/slave/linux_upload_clang/build/src/third_party/llvm/compiler-rt/lib/asan/../sanitizer_common/sanitizer_common_syscalls.inc:2381 I/ ( 9023): #1 0x42c94f71 (/system/lib/libdvm.so+0x53f71) I/ ( 9023): ==9023==ABORTING I/ActivityManager( 605): Force stopping org.chromium.net appid=10425 user=0: finished inst
Helen, thank you for diagnosing and fixing the issue. The change looks good, just couple of suggestions. https://codereview.chromium.org/1911353003/diff/60001/components/cronet/andro... File components/cronet/android/cronet_bidirectional_stream_adapter.cc (right): https://codereview.chromium.org/1911353003/diff/60001/components/cronet/andro... components/cronet/android/cronet_bidirectional_stream_adapter.cc:291: if (stream_failed_) { Should we add this check to other methods like ReadDataOnNetworkThread, DestroyOnNetworkThread? https://codereview.chromium.org/1911353003/diff/60001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamQuicTest.java (right): https://codereview.chromium.org/1911353003/diff/60001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamQuicTest.java:53: if (!mSkipServerShutdown) { To avoid adding test logic to tearDown(), I think it is better if we add a method (flag) to QuicTestServer.java to check whether the server has been already shut down or not. We can also make QuicTestServer.shutdownQuicTestServer() method idempotent and always call it in tearDown() unconditionally. Both approaches eliminate need for mSkipServerShutdown. https://codereview.chromium.org/1911353003/diff/60001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamQuicTest.java:145: assertEquals(-356, callback.mError.getCronetInternalErrorCode()); We can replace '-356' with NetError.ERR_QUIC_PROTOCOL_ERROR
Patchset #1 (id:40001) has been deleted
Thanks for the review! https://codereview.chromium.org/1911353003/diff/60001/components/cronet/andro... File components/cronet/android/cronet_bidirectional_stream_adapter.cc (right): https://codereview.chromium.org/1911353003/diff/60001/components/cronet/andro... components/cronet/android/cronet_bidirectional_stream_adapter.cc:291: if (stream_failed_) { On 2016/04/23 00:29:47, kapishnikov wrote: > Should we add this check to other methods like ReadDataOnNetworkThread, > DestroyOnNetworkThread? No that shouldn't be necessary. (1) ReadData can be called anytime even after stream is closed, since the underlying net::BidirectionalStream::ReadData returns an error code synchronously when the stream is destroyed. (We need to handle the case where Read can get called anytime after stream is closed. ) A fun fact: EOF is detected by reading 0 bytes. If the underlying stream closes successfully (it will get destroyed), and later we can do a read and synchronously return EOF. In this case, we actually want to call into bidistream and know that EOF has been reached. (2) DestroyOnNetworkThread is called when something is failed on the Java side or the stream gets canceled on Java side. In that case, Java will destroy CronetBidirectionalStreamAdapter, and CronetBidirectionalStreamAdapter won't get to call into bidstream to WriteData. Any earlier WriteDataOnNetworkThread will succeed because the Destroy task is posted later on the network thread, so Destroy will execute later. https://codereview.chromium.org/1911353003/diff/60001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamQuicTest.java (right): https://codereview.chromium.org/1911353003/diff/60001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamQuicTest.java:53: if (!mSkipServerShutdown) { On 2016/04/23 00:29:47, kapishnikov wrote: > To avoid adding test logic to tearDown(), I think it is better if we add a > method (flag) to QuicTestServer.java to check whether the server has been > already shut down or not. We can also make > QuicTestServer.shutdownQuicTestServer() method idempotent and always call it in > tearDown() unconditionally. Both approaches eliminate need for > mSkipServerShutdown. Done. https://codereview.chromium.org/1911353003/diff/60001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamQuicTest.java:145: assertEquals(-356, callback.mError.getCronetInternalErrorCode()); On 2016/04/23 00:29:47, kapishnikov wrote: > We can replace '-356' with NetError.ERR_QUIC_PROTOCOL_ERROR Done. Ah, I didn't know that there is a java class for NetError codes. Good to know! I guess we need to refactor error codes mapping in cronet/android/api/src/org/chromium/net/UrlRequestException.java.
Looks good! LGTM https://codereview.chromium.org/1911353003/diff/60001/components/cronet/andro... File components/cronet/android/cronet_bidirectional_stream_adapter.cc (right): https://codereview.chromium.org/1911353003/diff/60001/components/cronet/andro... components/cronet/android/cronet_bidirectional_stream_adapter.cc:291: if (stream_failed_) { On 2016/04/23 10:39:00, xunjieli wrote: > On 2016/04/23 00:29:47, kapishnikov wrote: > > Should we add this check to other methods like ReadDataOnNetworkThread, > > DestroyOnNetworkThread? > > No that shouldn't be necessary. > (1) ReadData can be called anytime even after stream is closed, since the > underlying net::BidirectionalStream::ReadData returns an error code > synchronously when the stream is destroyed. (We need to handle the case where > Read can get called anytime after stream is closed. ) A fun fact: EOF is > detected by reading 0 bytes. If the underlying stream closes successfully (it > will get destroyed), and later we can do a read and synchronously return EOF. In > this case, we actually want to call into bidistream and know that EOF has been > reached. > (2) DestroyOnNetworkThread is called when something is failed on the Java side > or the stream gets canceled on Java side. In that case, Java will destroy > CronetBidirectionalStreamAdapter, and CronetBidirectionalStreamAdapter won't get > to call into bidstream to WriteData. Any earlier WriteDataOnNetworkThread will > succeed because the Destroy task is posted later on the network thread, so > Destroy will execute later. Acknowledged.
Thanks, Andrei. (Looks like iOS might have the same problem, so we probably need to do the same thing there.)
The CQ bit was checked by xunjieli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1911353003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1911353003/80001
Message was sent while issue was closed.
Description was changed from ========== [Cronet] Do not call into BidirectionalStream::SendData if stream failed Cronet currently posts a task to the network thread to write data to the stream. However, between the time when the task is posted on Java thread and when the task is executed on the network thread, the underlying stream might have failed for some reason (e.g. server terminated the connection). If that is the case, we should not call into the stream. Without this patch, when running the new unit test with asan enabled, we will have a crash. ========== to ========== [Cronet] Do not call into BidirectionalStream::SendData if stream failed Cronet currently posts a task to the network thread to write data to the stream. However, between the time when the task is posted on Java thread and when the task is executed on the network thread, the underlying stream might have failed for some reason (e.g. server terminated the connection). If that is the case, we should not call into the stream. Without this patch, when running the new unit test with asan enabled, we will have a crash. ==========
Message was sent while issue was closed.
Committed patchset #2 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [Cronet] Do not call into BidirectionalStream::SendData if stream failed Cronet currently posts a task to the network thread to write data to the stream. However, between the time when the task is posted on Java thread and when the task is executed on the network thread, the underlying stream might have failed for some reason (e.g. server terminated the connection). If that is the case, we should not call into the stream. Without this patch, when running the new unit test with asan enabled, we will have a crash. ========== to ========== [Cronet] Do not call into BidirectionalStream::SendData if stream failed Cronet currently posts a task to the network thread to write data to the stream. However, between the time when the task is posted on Java thread and when the task is executed on the network thread, the underlying stream might have failed for some reason (e.g. server terminated the connection). If that is the case, we should not call into the stream. Without this patch, when running the new unit test with asan enabled, we will have a crash. Committed: https://crrev.com/943b9ad8edd8185defebc09d5917242122833cc4 Cr-Commit-Position: refs/heads/master@{#389379} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/943b9ad8edd8185defebc09d5917242122833cc4 Cr-Commit-Position: refs/heads/master@{#389379} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
