[Cronet] Do not call into BidirectionalStream::SendData if stream failed

components/cronet/android/cronet_bidirectional_stream_adapter.cc

Index: components/cronet/android/cronet_bidirectional_stream_adapter.cc
diff --git a/components/cronet/android/cronet_bidirectional_stream_adapter.cc b/components/cronet/android/cronet_bidirectional_stream_adapter.cc
index 8e55ac35c9109b94a637bec4edd2929976820f7c..a33e4e3627e91f4a9ac45f1e68353c9413b96ae9 100644
--- a/components/cronet/android/cronet_bidirectional_stream_adapter.cc
+++ b/components/cronet/android/cronet_bidirectional_stream_adapter.cc
@@ -58,7 +58,7 @@ CronetBidirectionalStreamAdapter::CronetBidirectionalStreamAdapter(
     CronetURLRequestContextAdapter* context,
     JNIEnv* env,
     const JavaParamRef<jobject>& jbidi_stream)
-    : context_(context), owner_(env, jbidi_stream) {}
+    : context_(context), owner_(env, jbidi_stream), stream_failed_(false) {}
 
 CronetBidirectionalStreamAdapter::~CronetBidirectionalStreamAdapter() {
   DCHECK(context_->IsOnNetworkThread());
@@ -236,6 +236,7 @@ void CronetBidirectionalStreamAdapter::OnTrailersReceived(
 
 void CronetBidirectionalStreamAdapter::OnFailed(int error) {
   DCHECK(context_->IsOnNetworkThread());
+  stream_failed_ = true;
   JNIEnv* env = base::android::AttachCurrentThread();
   cronet::Java_CronetBidirectionalStream_onError(
       env, owner_.obj(), NetErrorToUrlRequestError(error), error,
@@ -287,6 +288,14 @@ void CronetBidirectionalStreamAdapter::WriteDataOnNetworkThread(
   DCHECK(write_buffer);
   DCHECK(!write_buffer_);
 
+  if (stream_failed_) {

[kapishnikov, 2016/04/23 00:29:47]

Should we add this check to other methods like ReadDataOnNetworkThread,
DestroyOnNetworkThread?

[xunjieli, 2016/04/23 10:39:00]

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.

[kapishnikov, 2016/04/23 14:33:36]

Acknowledged.

+    // If stream failed between the time when WriteData is invoked and
+    // WriteDataOnNetworkThread is executed, do not call into |bidi_stream_|
+    // since the underlying stream might have been destroyed. Do not invoke
+    // Java callback either, since onError is posted when |stream_failed_| is
+    // set to true.
+    return;
+  }
   write_buffer_ = write_buffer;
   bidi_stream_->SendData(write_buffer_.get(), buffer_size, end_of_stream);
 }