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 kBufferSizeIncrement = 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),
@@ skipping 128 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;
+  }

[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();
   }
 
   OnRequestCanceled();
 }
 
 void URLRequestAdapter::Destroy() {
   context_->PostTaskToNetworkThread(
       FROM_HERE, base::Bind(&URLRequestAdapter::OnDestroyRequest, this));
@@ skipping 23 matching lines @@
     http_status_text_ = headers->GetStatusText();
 
   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() {

[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);
-    }
+  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;
-      }
+  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";
-      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()) {
     OnRequestFailed();
     return;
   } else if (bytes_read == 0) {
     OnRequestSucceeded();
     return;
   }
 
   OnBytesRead(bytes_read);
   Read();

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

Recommend against tail recursion here.  Ideally should be optimized out, but
that won't happen in debug builds, and it's unclear how deep the stack could end
up, in practice, in the worst case.

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

Ack. I've tried to post the read to network thread, but that brings up
"interesting" interactions with cancel posted from another thread. I'd leave it
as is unless you feel strongly about it.
Should I just post OnReadCompleted from Read instead?

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

I'd suggest splitting the logic into 3 methods:

Read() {
  while (read completes synchronously) {
    if (HandleReadResult(read_result) < 0)
      // break / return on error.
  }
}

// Does what OnReadCompleted did.  Returns error code on error, OK (Or bytes
read - doesn't matter) on success.
HandleReadResult(int bytes_read);

OnReadCompleted(int bytes_read) {
  if (HandleReadResult(read_result) < 0)
    // early return on error
  Read();
}

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

Done.

 }
 
 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) {

[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() {
   DCHECK(OnNetworkThread());
   if (canceled_) {
     return;
   }
 
   VLOG(1) << "Request completed with HTTP status: " << http_status_code_
           << ". Total bytes read: " << total_bytes_read_;
 
   OnRequestCompleted();
 }
 
 void URLRequestAdapter::OnRequestFailed() {
   DCHECK(OnNetworkThread());
   if (canceled_) {
     return;
   }
 
   error_code_ = url_request_->status().error();
   VLOG(1) << "Request failed with status: " << url_request_->status().status()
           << " and error: " << net::ErrorToString(error_code_);
   OnRequestCompleted();
 }
 
 void URLRequestAdapter::OnRequestCanceled() {

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

Having OnCancelRequest and OnRequestCanceled seems like a bad idea.  Can just
inline this, anyways.  Only called in one place, I believe, and just calls into
another method, making it rather pointless.

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

Done.

   DCHECK(OnNetworkThread());
   OnRequestCompleted();
 }
 
 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();
 }
 
 unsigned char* URLRequestAdapter::Data() const {
   DCHECK(OnNetworkThread());
   return reinterpret_cast<unsigned char*>(read_buffer_->StartOfBuffer());
 }
 
 bool URLRequestAdapter::OnNetworkThread() const {
   return context_->GetNetworkTaskRunner()->BelongsToCurrentThread();
 }
 
 }  // namespace cronet