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

components/cronet/android/cronet_bidirectional_stream_adapter.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "cronet_bidirectional_stream_adapter.h"
 
 #include <string>
 #include <vector>
 
 #include "base/bind.h"
@@ skipping 40 matching lines @@
 
 // static
 bool CronetBidirectionalStreamAdapter::RegisterJni(JNIEnv* env) {
   return RegisterNativesImpl(env);
 }
 
 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());
 }
 
 jint CronetBidirectionalStreamAdapter::Start(
     JNIEnv* env,
     const JavaParamRef<jobject>& jcaller,
     const JavaParamRef<jstring>& jurl,
     jint jpriority,
@@ skipping 157 matching lines @@
 void CronetBidirectionalStreamAdapter::OnTrailersReceived(
     const net::SpdyHeaderBlock& response_trailers) {
   DCHECK(context_->IsOnNetworkThread());
   JNIEnv* env = base::android::AttachCurrentThread();
   cronet::Java_CronetBidirectionalStream_onResponseTrailersReceived(
       env, owner_.obj(), GetHeadersArray(env, response_trailers).obj());
 }
 
 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,
       ConvertUTF8ToJavaString(env, net::ErrorToString(error)).obj(),
       bidi_stream_->GetTotalReceivedBytes());
 }
 
 void CronetBidirectionalStreamAdapter::StartOnNetworkThread(
     std::unique_ptr<net::BidirectionalStreamRequestInfo> request_info) {
   DCHECK(context_->IsOnNetworkThread());
@@ skipping 31 matching lines @@
 }
 
 void CronetBidirectionalStreamAdapter::WriteDataOnNetworkThread(
     scoped_refptr<IOBufferWithByteBuffer> write_buffer,
     int buffer_size,
     bool end_of_stream) {
   DCHECK(context_->IsOnNetworkThread());
   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);
 }
 
 void CronetBidirectionalStreamAdapter::DestroyOnNetworkThread(
     bool send_on_canceled) {
   DCHECK(context_->IsOnNetworkThread());
   if (send_on_canceled) {
     JNIEnv* env = base::android::AttachCurrentThread();
     cronet::Java_CronetBidirectionalStream_onCanceled(env, owner_.obj());
   }
   delete this;
 }
 
 base::android::ScopedJavaLocalRef<jobjectArray>
 CronetBidirectionalStreamAdapter::GetHeadersArray(
     JNIEnv* env,
     const net::SpdyHeaderBlock& header_block) {
   DCHECK(context_->IsOnNetworkThread());
 
   std::vector<std::string> headers;
   for (const auto& header : header_block) {
     headers.push_back(header.first.as_string());
     headers.push_back(header.second.as_string());
   }
   return base::android::ToJavaArrayOfStrings(env, headers);
 }
 
 }  // namespace cronet