Cronet Crash in BidirectionalStream |
|||||||||||||
Issue descriptionCronet 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. We sometimes will have a crash with the following callstack: 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)
,
Apr 26 2016
Request merging into M51. This CL is in Cronet's JNI wrappers, and will not affect Chrome. We need this in M51 for Cronet consumers. Thanks!
,
Apr 26 2016
Approving merge to M51 branch 2704 as change is already baked in canary per chat with xunjieli@. If possible, please merge today before 5:00 PM PST so we can take it for this week beta release. Thank you.
,
Apr 26 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4590c2d29868a1bd079e7f1dcf2dc167b11dae0c commit 4590c2d29868a1bd079e7f1dcf2dc167b11dae0c Author: xunjieli <xunjieli@chromium.org> Date: Tue Apr 26 21:03:24 2016 [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. Review URL: https://codereview.chromium.org/1911353003 Cr-Commit-Position: refs/heads/master@{#389379} (cherry picked from commit 943b9ad8edd8185defebc09d5917242122833cc4) NOTRY=true NOPRESUBMIT=true TBR=kapishnikov@chromium.org BUG= 606394 Review URL: https://codereview.chromium.org/1924623002 Cr-Commit-Position: refs/branch-heads/2704@{#252} Cr-Branched-From: 6e53600def8f60d8c632fadc70d7c1939ccea347-refs/heads/master@{#386251} [modify] https://crrev.com/4590c2d29868a1bd079e7f1dcf2dc167b11dae0c/components/cronet/android/cronet_bidirectional_stream_adapter.cc [modify] https://crrev.com/4590c2d29868a1bd079e7f1dcf2dc167b11dae0c/components/cronet/android/cronet_bidirectional_stream_adapter.h [modify] https://crrev.com/4590c2d29868a1bd079e7f1dcf2dc167b11dae0c/components/cronet/android/test/javatests/src/org/chromium/net/BidirectionalStreamQuicTest.java [modify] https://crrev.com/4590c2d29868a1bd079e7f1dcf2dc167b11dae0c/components/cronet/android/test/src/org/chromium/net/QuicTestServer.java
,
Apr 27 2016
,
Jun 1 2016
It appears that the issue is not completely fixed, reopening.
,
Jun 6 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/707f895efd414219d609533e82be1ffc60fd9788 commit 707f895efd414219d609533e82be1ffc60fd9788 Author: xunjieli <xunjieli@chromium.org> Date: Mon Jun 06 15:22:06 2016 Do not crash on null stream in writing to bidirectional streams When SendData/SendvData is called, it might be that OnClose has been invoked without an error, in which case BidirectionalStream::Delegate::OnFailed would not have been invoked. This is theoretically possible, but I am not aware if such case would exist. This CL removes the assumption that SendData/SendvData is only called when stream is alive. If stream is destroyed, propagate that as an ERR_UNEXPECTED to caller, instead of crashing with a segfault. BUG= 606394 Review-Url: https://codereview.chromium.org/2032733002 Cr-Commit-Position: refs/heads/master@{#398031} [modify] https://crrev.com/707f895efd414219d609533e82be1ffc60fd9788/net/http/bidirectional_stream.h [modify] https://crrev.com/707f895efd414219d609533e82be1ffc60fd9788/net/http/bidirectional_stream_impl.h [modify] https://crrev.com/707f895efd414219d609533e82be1ffc60fd9788/net/net.gypi [modify] https://crrev.com/707f895efd414219d609533e82be1ffc60fd9788/net/quic/bidirectional_stream_quic_impl.cc [modify] https://crrev.com/707f895efd414219d609533e82be1ffc60fd9788/net/quic/bidirectional_stream_quic_impl_unittest.cc [modify] https://crrev.com/707f895efd414219d609533e82be1ffc60fd9788/net/spdy/bidirectional_stream_spdy_impl.cc [modify] https://crrev.com/707f895efd414219d609533e82be1ffc60fd9788/net/spdy/bidirectional_stream_spdy_impl.h [add] https://crrev.com/707f895efd414219d609533e82be1ffc60fd9788/net/spdy/bidirectional_stream_spdy_impl_unittest.cc
,
Jun 6 2016
Can I have the approval for merging the CL in #7 to M52? This CL does not impact Chrome. The code is only used in Cronet. Thank you!
,
Jun 6 2016
Your change meets the bar and is auto-approved for M52 (branch: 2743)
,
Jun 6 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1f7d4841a5d98e420fdd4fc762dc63fc0f786709 commit 1f7d4841a5d98e420fdd4fc762dc63fc0f786709 Author: xunjieli <xunjieli@chromium.org> Date: Mon Jun 06 16:41:35 2016 Do not crash on null stream in writing to bidirectional streams When SendData/SendvData is called, it might be that OnClose has been invoked without an error, in which case BidirectionalStream::Delegate::OnFailed would not have been invoked. This is theoretically possible, but I am not aware if such case would exist. This CL removes the assumption that SendData/SendvData is only called when stream is alive. If stream is destroyed, propagate that as an ERR_UNEXPECTED to caller, instead of crashing with a segfault. BUG= 606394 TBR=mef@chromium.org NOTRY=true NOPRESUBMIT=true Review-Url: https://codereview.chromium.org/2032733002 Cr-Commit-Position: refs/heads/master@{#398031} (cherry picked from commit 707f895efd414219d609533e82be1ffc60fd9788) Review-Url: https://codereview.chromium.org/2046613002 Cr-Commit-Position: refs/branch-heads/2743@{#236} Cr-Branched-From: 2b3ae3b8090361f8af5a611712fc1a5ab2de53cb-refs/heads/master@{#394939} [modify] https://crrev.com/1f7d4841a5d98e420fdd4fc762dc63fc0f786709/net/http/bidirectional_stream.h [modify] https://crrev.com/1f7d4841a5d98e420fdd4fc762dc63fc0f786709/net/http/bidirectional_stream_impl.h [modify] https://crrev.com/1f7d4841a5d98e420fdd4fc762dc63fc0f786709/net/net.gypi [modify] https://crrev.com/1f7d4841a5d98e420fdd4fc762dc63fc0f786709/net/quic/bidirectional_stream_quic_impl.cc [modify] https://crrev.com/1f7d4841a5d98e420fdd4fc762dc63fc0f786709/net/quic/bidirectional_stream_quic_impl_unittest.cc [modify] https://crrev.com/1f7d4841a5d98e420fdd4fc762dc63fc0f786709/net/spdy/bidirectional_stream_spdy_impl.cc [modify] https://crrev.com/1f7d4841a5d98e420fdd4fc762dc63fc0f786709/net/spdy/bidirectional_stream_spdy_impl.h [add] https://crrev.com/1f7d4841a5d98e420fdd4fc762dc63fc0f786709/net/spdy/bidirectional_stream_spdy_impl_unittest.cc
,
Jun 6 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3b6ac70b2c8b7085bade85f46577677a45732005 commit 3b6ac70b2c8b7085bade85f46577677a45732005 Author: xunjieli <xunjieli@chromium.org> Date: Mon Jun 06 21:36:28 2016 Use WeakPtrFactory in net::BidirectionalStream when posting task This CL uses WeakPtrFactory to post task in net::BidirectionalStream. BUG= 606394 Review-Url: https://codereview.chromium.org/2043863002 Cr-Commit-Position: refs/heads/master@{#398132} [modify] https://crrev.com/3b6ac70b2c8b7085bade85f46577677a45732005/net/http/bidirectional_stream.cc [modify] https://crrev.com/3b6ac70b2c8b7085bade85f46577677a45732005/net/http/bidirectional_stream.h [modify] https://crrev.com/3b6ac70b2c8b7085bade85f46577677a45732005/net/http/bidirectional_stream_unittest.cc
,
Jun 6 2016
May I have the approval for merging the CL in #11 into M52? The code is only used in Cronet, and it doesn't impact Chrome. Thank you!
,
Jun 6 2016
Your change meets the bar and is auto-approved for M52 (branch: 2743)
,
Jun 6 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4ba34c3758a0a360bc35db4c49c41afcd9b3ac6e commit 4ba34c3758a0a360bc35db4c49c41afcd9b3ac6e Author: xunjieli <xunjieli@chromium.org> Date: Mon Jun 06 22:09:17 2016 Use WeakPtrFactory in net::BidirectionalStream when posting task This CL uses WeakPtrFactory to post task in net::BidirectionalStream. BUG= 606394 TBR=mmenke@chromium.org NOTRY=true NOPRESUBMIT=true Review-Url: https://codereview.chromium.org/2043863002 Cr-Commit-Position: refs/heads/master@{#398132} (cherry picked from commit 3b6ac70b2c8b7085bade85f46577677a45732005) Review-Url: https://codereview.chromium.org/2041153002 Cr-Commit-Position: refs/branch-heads/2743@{#252} Cr-Branched-From: 2b3ae3b8090361f8af5a611712fc1a5ab2de53cb-refs/heads/master@{#394939} [modify] https://crrev.com/4ba34c3758a0a360bc35db4c49c41afcd9b3ac6e/net/http/bidirectional_stream.cc [modify] https://crrev.com/4ba34c3758a0a360bc35db4c49c41afcd9b3ac6e/net/http/bidirectional_stream.h [modify] https://crrev.com/4ba34c3758a0a360bc35db4c49c41afcd9b3ac6e/net/http/bidirectional_stream_unittest.cc
,
Jun 6 2016
I think this is fixed. I will re-open if the crash persists.
,
Jun 15 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4ba34c3758a0a360bc35db4c49c41afcd9b3ac6e commit 4ba34c3758a0a360bc35db4c49c41afcd9b3ac6e Author: xunjieli <xunjieli@chromium.org> Date: Mon Jun 06 22:09:17 2016 Use WeakPtrFactory in net::BidirectionalStream when posting task This CL uses WeakPtrFactory to post task in net::BidirectionalStream. BUG= 606394 TBR=mmenke@chromium.org NOTRY=true NOPRESUBMIT=true Review-Url: https://codereview.chromium.org/2043863002 Cr-Commit-Position: refs/heads/master@{#398132} (cherry picked from commit 3b6ac70b2c8b7085bade85f46577677a45732005) Review-Url: https://codereview.chromium.org/2041153002 Cr-Commit-Position: refs/branch-heads/2743@{#252} Cr-Branched-From: 2b3ae3b8090361f8af5a611712fc1a5ab2de53cb-refs/heads/master@{#394939} [modify] https://crrev.com/4ba34c3758a0a360bc35db4c49c41afcd9b3ac6e/net/http/bidirectional_stream.cc [modify] https://crrev.com/4ba34c3758a0a360bc35db4c49c41afcd9b3ac6e/net/http/bidirectional_stream.h [modify] https://crrev.com/4ba34c3758a0a360bc35db4c49c41afcd9b3ac6e/net/http/bidirectional_stream_unittest.cc |
|||||||||||||
►
Sign in to add a comment |
|||||||||||||
Comment 1 by xunji...@chromium.org
, Apr 25 2016