Cronet Fix Channel Write after Close when request is canceled after success.

components/cronet/android/url_request_adapter.cc

Index: components/cronet/android/url_request_adapter.cc
diff --git a/components/cronet/android/url_request_adapter.cc b/components/cronet/android/url_request_adapter.cc
index 5d8f84965fc9c59dc616399aa77d4637234728f8..5d8bdd758ac0591bf5c96125715bf3072bf432a8 100644
--- a/components/cronet/android/url_request_adapter.cc
+++ b/components/cronet/android/url_request_adapter.cc
@@ -22,15 +22,13 @@
 
 namespace cronet {
 
-static const size_t kBufferSizeIncrement = 8192;
+static const size_t kReadBufferSize = 32768;
 
 URLRequestAdapter::URLRequestAdapter(URLRequestContextAdapter* context,
                                      URLRequestAdapterDelegate* delegate,
                                      GURL url,
                                      net::RequestPriority priority)
     : method_("GET"),
-      read_buffer_(new net::GrowableIOBuffer()),
-      bytes_read_(0),
       total_bytes_read_(0),
       error_code_(0),
       http_status_code_(0),
@@ -171,12 +169,6 @@ void URLRequestAdapter::OnInitiateConnection() {
 }
 
 void URLRequestAdapter::Cancel() {
-  if (canceled_) {
-    return;
-  }
-
-  canceled_ = true;
-
   context_->PostTaskToNetworkThread(
       FROM_HERE,
       base::Bind(&URLRequestAdapter::OnCancelRequest, base::Unretained(this)));
@@ -185,12 +177,15 @@ void URLRequestAdapter::Cancel() {
 void URLRequestAdapter::OnCancelRequest() {
   DCHECK(OnNetworkThread());
   VLOG(1) << "Canceling chromium request: " << url_.possibly_invalid_spec();
+  if (canceled_)
+    return;

[mmenke, 2014/12/10 20:59:14]

Can we just DCHECK here?  Looks like we make sure we call Cancel() only once in
ChromiumUrlRequest.

[mef, 2014/12/10 23:23:35]

That check happens on Java thread, which is not the same as network thread.

[mmenke, 2014/12/11 01:09:19]

Right....But it happens behind a lock, before calling into native code.  I see
no possible way for canceled_ to be true here.

If it can be true, we should have a unit test that tests it.

+  canceled_ = true;
+  // Check whether request has already completed.
+  if (url_request_ == nullptr)
+    return;
 
-  if (url_request_ != NULL) {
-    url_request_->Cancel();
-  }
-
-  OnRequestCanceled();
+  url_request_->Cancel();
+  OnRequestCompleted();
 }
 
 void URLRequestAdapter::Destroy() {
@@ -231,54 +226,41 @@ void URLRequestAdapter::OnResponseStarted(net::URLRequest* request) {
 // Reads all available data or starts an asynchronous read.
 void URLRequestAdapter::Read() {
   DCHECK(OnNetworkThread());
-  while (true) {
-    if (read_buffer_->RemainingCapacity() == 0) {
-      int new_capacity = read_buffer_->capacity() + kBufferSizeIncrement;
-      read_buffer_->SetCapacity(new_capacity);
-    }
-
-    int bytes_read;
-    if (url_request_->Read(read_buffer_.get(),
-                           read_buffer_->RemainingCapacity(),
-                           &bytes_read)) {
-      if (bytes_read == 0) {
-        OnRequestSucceeded();
-        break;
-      }
-
-      VLOG(1) << "Synchronously read: " << bytes_read << " bytes";
-      OnBytesRead(bytes_read);
-    } else if (url_request_->status().status() ==
-               net::URLRequestStatus::IO_PENDING) {
-      if (bytes_read_ != 0) {
-        VLOG(1) << "Flushing buffer: " << bytes_read_ << " bytes";
-
-        delegate_->OnBytesRead(this);
-        read_buffer_->set_offset(0);
-        bytes_read_ = 0;
-      }
-      VLOG(1) << "Started async read";
-      break;
-    } else {
-      OnRequestFailed();
-      break;
-    }
-  }
+  if (!read_buffer_.get())
+    read_buffer_ = new net::IOBufferWithSize(kReadBufferSize);
+
+  int bytes_read = 0;
+  do {

[mmenke, 2014/12/10 20:59:14]

optional:  Suggest a while loop instead of a do while loop.  Do while loops
aren't too common in chrome, and the double bytes_read initialization is a bit
ugly, and means we don't save any code by the change.

If you prefer to keep the do while, should at least get rid of the first "= 0".

[mef, 2014/12/10 23:23:36]

Done.

+    bytes_read = 0;
+    url_request_->Read(read_buffer_.get(), kReadBufferSize, &bytes_read);
+    // If IO is pending, wait for the URLRequest to call OnReadCompleted.
+    if (url_request_->status().is_io_pending())
+      return;
+  } while (HandleReadResult(url_request_.get(), bytes_read));
 }
 
-void URLRequestAdapter::OnReadCompleted(net::URLRequest* request,
-                                        int bytes_read) {
+bool URLRequestAdapter::HandleReadResult(net::URLRequest* request,
+                                         int bytes_read) {
   DCHECK(OnNetworkThread());
-  VLOG(1) << "Asynchronously read: " << bytes_read << " bytes";
-  if (bytes_read < 0) {
+  if (!url_request_->status().is_success()) {
     OnRequestFailed();
-    return;
+    return false;
   } else if (bytes_read == 0) {
     OnRequestSucceeded();
-    return;
+    return false;
   }
 
-  OnBytesRead(bytes_read);
+  total_bytes_read_ += bytes_read;
+  delegate_->OnBytesRead(this, bytes_read);
+
+  return true;
+}
+
+void URLRequestAdapter::OnReadCompleted(net::URLRequest* request,
+                                        int bytes_read) {
+  if (!HandleReadResult(request, bytes_read))
+    return;
+
   Read();
 }
 
@@ -296,13 +278,6 @@ void URLRequestAdapter::OnReceivedRedirect(net::URLRequest* request,
   }
 }
 
-void URLRequestAdapter::OnBytesRead(int bytes_read) {
-  DCHECK(OnNetworkThread());
-  read_buffer_->set_offset(read_buffer_->offset() + bytes_read);
-  bytes_read_ += bytes_read;
-  total_bytes_read_ += bytes_read;
-}
-
 void URLRequestAdapter::OnRequestSucceeded() {
   DCHECK(OnNetworkThread());
   if (canceled_) {
@@ -327,23 +302,19 @@ void URLRequestAdapter::OnRequestFailed() {
   OnRequestCompleted();
 }
 
-void URLRequestAdapter::OnRequestCanceled() {
-  DCHECK(OnNetworkThread());
-  OnRequestCompleted();
-}
-
 void URLRequestAdapter::OnRequestCompleted() {
   DCHECK(OnNetworkThread());
   VLOG(1) << "Completed: " << url_.possibly_invalid_spec();
 
-  delegate_->OnBytesRead(this);
+  DCHECK(url_request_ != nullptr);
+
   delegate_->OnRequestFinished(this);
   url_request_.reset();
 }
 
 unsigned char* URLRequestAdapter::Data() const {
   DCHECK(OnNetworkThread());
-  return reinterpret_cast<unsigned char*>(read_buffer_->StartOfBuffer());
+  return reinterpret_cast<unsigned char*>(read_buffer_->data());
 }
 
 bool URLRequestAdapter::OnNetworkThread() const {