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

components/cronet/android/url_request_adapter.cc

 // Copyright 2014 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 "url_request_adapter.h"
 
 #include <string.h>
 
 #include "base/bind.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/string_number_conversions.h"
 #include "components/cronet/android/url_request_context_adapter.h"
 #include "components/cronet/android/wrapped_channel_upload_element_reader.h"
 #include "net/base/elements_upload_data_stream.h"
 #include "net/base/load_flags.h"
 #include "net/base/net_errors.h"
 #include "net/base/upload_bytes_element_reader.h"
 #include "net/http/http_response_headers.h"
 #include "net/http/http_status_code.h"
 
 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),
       canceled_(false),
       expected_size_(0),
       chunked_upload_(false),
       disable_redirect_(false) {
   context_ = context;
   delegate_ = delegate;
@@ skipping 121 matching lines @@
   } else if (chunked_upload_) {
     url_request_->EnableChunkedUpload();
   }
 
   url_request_->SetPriority(priority_);
 
   url_request_->Start();
 }
 
 void URLRequestAdapter::Cancel() {
-  if (canceled_) {
-    return;
-  }
-
-  canceled_ = true;
-
   context_->PostTaskToNetworkThread(
       FROM_HERE,
       base::Bind(&URLRequestAdapter::OnCancelRequest, base::Unretained(this)));
 }
 
 void URLRequestAdapter::OnCancelRequest() {
   DCHECK(OnNetworkThread());
   VLOG(1) << "Canceling chromium request: " << url_.possibly_invalid_spec();
-
+  if (canceled_) {
+    return;
+  }
+  canceled_ = true;
   if (url_request_ != NULL) {
     url_request_->Cancel();
   }
 
   OnRequestCanceled();
 }
 
 void URLRequestAdapter::Destroy() {
   context_->PostTaskToNetworkThread(
       FROM_HERE, base::Bind(&URLRequestAdapter::OnDestroyRequest, this));
@@ skipping 25 matching lines @@
   request->GetResponseHeaderByName("Content-Type", &content_type_);
   expected_size_ = request->GetExpectedContentSize();
   delegate_->OnResponseStarted(this);
 
   Read();
 }
 
 // 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);
-    }
+  if (!read_buffer_.get())
+    read_buffer_ = new net::IOBufferWithSize(kReadBufferSize);
 
-    int bytes_read;
-    if (url_request_->Read(read_buffer_.get(),
-                           read_buffer_->RemainingCapacity(),
-                           &bytes_read)) {
-      if (bytes_read == 0) {
-        OnRequestSucceeded();
-        break;
-      }
+  int bytes_read = 0;
+  url_request_->Read(
+      read_buffer_.get(), read_buffer_->size(), &bytes_read);
+  // If IO is pending, wait for the URLRequest to call OnReadCompleted.
+  if (url_request_->status().is_io_pending())
+    return;
 
-      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;
-    }
-  }
+  VLOG(1) << "Synchronously read: " << bytes_read << " bytes";
+  OnReadCompleted(url_request_.get(), bytes_read);
 }
 
 void URLRequestAdapter::OnReadCompleted(net::URLRequest* request,
                                         int bytes_read) {
   DCHECK(OnNetworkThread());
-  VLOG(1) << "Asynchronously read: " << bytes_read << " bytes";
-  if (bytes_read < 0) {
+  VLOG(1) << "Completed read: " << bytes_read << " bytes";
+  if (!url_request_->status().is_success()) {

[miloslav, 2014/11/26 10:44:35]

Could url_request_->status().is_success() be false and bytes_read > 0 at the
same time? I presume no, but if it could, then maybe we should still send the
last bytes successfully read before failing the request.

[mef, 2014/11/26 14:06:21]

We use the same logic in other places and I would rather keep it consistent.
Matt, WDYT?
What are scenarios where some remaining bytes are valuable even though request
has failed?

     OnRequestFailed();
     return;
   } else if (bytes_read == 0) {
     OnRequestSucceeded();
     return;
   }
 
   OnBytesRead(bytes_read);
   Read();
 }
 
 void URLRequestAdapter::OnReceivedRedirect(net::URLRequest* request,
                                            const net::RedirectInfo& info,
                                            bool* defer_redirect) {
   DCHECK(OnNetworkThread());
   if (disable_redirect_) {
     http_status_code_ = request->GetResponseCode();
     request->CancelWithError(net::ERR_TOO_MANY_REDIRECTS);
     error_code_ = net::ERR_TOO_MANY_REDIRECTS;
     canceled_ = true;
     *defer_redirect = false;
     OnRequestCompleted();
   }
 }
 
 void URLRequestAdapter::OnBytesRead(int bytes_read) {
   DCHECK(OnNetworkThread());
-  read_buffer_->set_offset(read_buffer_->offset() + bytes_read);
-  bytes_read_ += bytes_read;
+  bytes_read_ = bytes_read;
   total_bytes_read_ += bytes_read;
+  delegate_->OnBytesRead(this);
 }
 
 void URLRequestAdapter::OnRequestSucceeded() {
   DCHECK(OnNetworkThread());
   if (canceled_) {
     return;
   }
 
   VLOG(1) << "Request completed with HTTP status: " << http_status_code_
           << ". Total bytes read: " << total_bytes_read_;
@@ skipping 15 matching lines @@
 
 void URLRequestAdapter::OnRequestCanceled() {
   DCHECK(OnNetworkThread());
   OnRequestCompleted();
 }
 
 void URLRequestAdapter::OnRequestCompleted() {
   DCHECK(OnNetworkThread());
   VLOG(1) << "Completed: " << url_.possibly_invalid_spec();
 
-  delegate_->OnBytesRead(this);

[mef, 2014/11/26 14:14:28]

I believe that calling OnBytesRead in combination with checking/setting
|canceled_| from user thread was responsible for channel write after close, but
I'm not sure that it is safe to factor out from the rest of read changes into
separate CL.

+  if (url_request_ == nullptr)

[mmenke, 2014/11/26 14:07:43]

I'm assuming this compiles?  Didn't realize scoped_ptrs could be compared to
NULL.

[mef, 2014/11/26 14:14:28]

Should I add .get() for clarity?

[mmenke, 2014/11/26 14:34:14]

If it works, I'm assuming it's supposed to work, so I think it's fine as-is.

+    return;
+
   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 {
   return context_->GetNetworkTaskRunner()->BelongsToCurrentThread();
 }
 
 }  // namespace cronet