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..c3829d0f45b90348103278eb9acf8ab1f75794cd 100644
--- a/components/cronet/android/url_request_adapter.cc
+++ b/components/cronet/android/url_request_adapter.cc
@@ -22,7 +22,7 @@
 
 namespace cronet {
 
-static const size_t kBufferSizeIncrement = 8192;
+static const size_t kBufferSizeIncrement = 32768;
 
 URLRequestAdapter::URLRequestAdapter(URLRequestContextAdapter* context,
                                      URLRequestAdapterDelegate* delegate,
@@ -171,12 +171,6 @@ void URLRequestAdapter::OnInitiateConnection() {
 }
 
 void URLRequestAdapter::Cancel() {
-  if (canceled_) {
-    return;
-  }
-
-  canceled_ = true;
-
   context_->PostTaskToNetworkThread(
       FROM_HERE,
       base::Bind(&URLRequestAdapter::OnCancelRequest, base::Unretained(this)));
@@ -185,7 +179,10 @@ void URLRequestAdapter::Cancel() {
 void URLRequestAdapter::OnCancelRequest() {
   DCHECK(OnNetworkThread());
   VLOG(1) << "Canceling chromium request: " << url_.possibly_invalid_spec();
-
+  if (canceled_) {
+    return;
+  }

[mmenke, 2014/12/02 17:10:24]

nit:  Shouldn't use braces on single line if's (Yea, know it was like that
before)

[mef, 2014/12/02 19:41:40]

Done.

+  canceled_ = true;
   if (url_request_ != NULL) {
     url_request_->Cancel();
   }
@@ -231,46 +228,27 @@ void URLRequestAdapter::OnResponseStarted(net::URLRequest* request) {
 // Reads all available data or starts an asynchronous read.
 void URLRequestAdapter::Read() {

[mmenke, 2014/12/02 17:10:24]

Why does all this stuff here need to be a part of this CL?

[mef, 2014/12/02 19:41:40]

Discussed. :)

   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_->RemainingCapacity() == 0) {
+    int new_capacity = read_buffer_->capacity() + kBufferSizeIncrement;
+    read_buffer_->SetCapacity(new_capacity);
   }
+
+  int bytes_read = 0;
+  url_request_->Read(
+      read_buffer_.get(), read_buffer_->RemainingCapacity(), &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";
+  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()) {
     OnRequestFailed();
     return;
   } else if (bytes_read == 0) {
@@ -298,9 +276,11 @@ void URLRequestAdapter::OnReceivedRedirect(net::URLRequest* request,
 
 void URLRequestAdapter::OnBytesRead(int bytes_read) {

[mmenke, 2014/12/02 18:15:12]

Suggest just inlining this.

[mef, 2014/12/02 19:41:39]

Done.

   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);
+  read_buffer_->set_offset(0);
+  bytes_read_ = 0;

[mmenke, 2014/12/02 18:15:12]

If we're going to refactor this logic here, should get rid of bytes_read_, and
just pass the value directly to the delegate.

[mef, 2014/12/02 19:41:40]

Done.

 }
 
 void URLRequestAdapter::OnRequestSucceeded() {
@@ -336,7 +316,9 @@ void URLRequestAdapter::OnRequestCompleted() {
   DCHECK(OnNetworkThread());
   VLOG(1) << "Completed: " << url_.possibly_invalid_spec();
 
-  delegate_->OnBytesRead(this);
+  if (url_request_ == nullptr)

[mmenke, 2014/12/02 18:15:12]

This handles the cancel after complete case?  If you just move the
OnRequestCanceled() call into the NULL request check in OnCancelRequest, then
can turn this into a DCHECK.

[mef, 2014/12/02 19:41:39]

I think it also handles complete after cancel, doesn't it?

[mmenke, 2014/12/02 20:33:47]

Don't think so.  Cancel calls into OnRequestCompleted, which deletes the
URLRequest.  As it no longer exists after cancel, it can't call
OnRequestCompleted.

[mef, 2014/12/03 20:29:54]

Done.

+    return;
+
   delegate_->OnRequestFinished(this);
   url_request_.reset();
 }