[Cronet] Allow direct executor for native API

This is accomplished by avoiding holding Cronet_UrlRequestImpl::lock_
while issuing callbacks. Holding the lock is only necessary while
modifying state that should be processed in an atomic fashion.
The Cronet_UrlRequestImpl will not be deleted until after final
callbacks have been run so there is no chance of a use-after-free.

Bug: 869402
Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester;luci.chromium.try:ios-simulator-cronet
Change-Id: Ie851b57296c394e6f098022a7930d65f62f7b0c6
Reviewed-on: https://chromium-review.googlesource.com/1183625Reviewed-by: Misha Efimov <mef@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587203}

components/cronet/cronet_url_request.h

@@ -102,7 +102,7 @@ class CronetURLRequest {
                          const std::string& error_string,
                          int64_t received_byte_count) = 0;
 
-    // Invoked if request was canceled via CronetURLRequest::Start().
+    // Invoked if request was canceled via CronetURLRequest::Destroy().
     virtual void OnCanceled() = 0;
 
     // Invoked when request is destroyed. Once invoked, no other Callback

components/cronet/native/test/test_url_request_callback.cc

@@ -57,8 +57,9 @@ TestUrlRequestCallback::UrlResponseInfo::UrlResponseInfo(
 
 TestUrlRequestCallback::UrlResponseInfo::~UrlResponseInfo() = default;
 
-TestUrlRequestCallback::TestUrlRequestCallback()
-    : done_(base::WaitableEvent::ResetPolicy::MANUAL,
+TestUrlRequestCallback::TestUrlRequestCallback(bool direct_executor)
+    : direct_executor_(direct_executor),
+      done_(base::WaitableEvent::ResetPolicy::MANUAL,
             base::WaitableEvent::InitialState::NOT_SIGNALED),
       step_block_(base::WaitableEvent::ResetPolicy::MANUAL,
                   base::WaitableEvent::InitialState::NOT_SIGNALED) {}
@@ -67,13 +68,10 @@ TestUrlRequestCallback::~TestUrlRequestCallback() {
   ShutdownExecutor();
 }
 
-Cronet_ExecutorPtr TestUrlRequestCallback::GetExecutor(bool direct) {
-  if (executor_) {
-    CHECK(direct == allow_direct_executor_);
+Cronet_ExecutorPtr TestUrlRequestCallback::GetExecutor() {
+  if (executor_)
     return executor_;
-  }
-  allow_direct_executor_ = direct;
-  if (direct) {
+  if (direct_executor_) {
     executor_ =
         Cronet_Executor_CreateWith(TestUrlRequestCallback::ExecuteDirect);
   } else {
@@ -228,7 +226,7 @@ void TestUrlRequestCallback::ShutdownExecutor() {
 
 void TestUrlRequestCallback::CheckExecutorThread() {
   base::AutoLock lock(executor_lock_);
-  if (executor_thread_ && !allow_direct_executor_)
+  if (executor_thread_ && !direct_executor_)
     CHECK(executor_thread_->task_runner()->BelongsToCurrentThread());
 }
 
@@ -247,10 +245,14 @@ bool TestUrlRequestCallback::MaybeCancelOrPause(Cronet_UrlRequestPtr request) {
   }
   if (failure_type_ == CANCEL_ASYNC ||
       failure_type_ == CANCEL_ASYNC_WITHOUT_PAUSE) {
-    base::AutoLock lock(executor_lock_);
-    CHECK(executor_thread_);
-    executor_thread_->task_runner()->PostTask(
-        FROM_HERE, base::BindOnce(&Cronet_UrlRequest_Cancel, request));
+    if (direct_executor_) {
+      Cronet_UrlRequest_Cancel(request);
+    } else {
+      base::AutoLock lock(executor_lock_);
+      CHECK(executor_thread_);
+      executor_thread_->task_runner()->PostTask(
+          FROM_HERE, base::BindOnce(&Cronet_UrlRequest_Cancel, request));
+    }
   }
   return failure_type_ != CANCEL_ASYNC_WITHOUT_PAUSE;
 }

components/cronet/native/test/test_url_request_callback.h

@@ -86,10 +86,10 @@ class TestUrlRequestCallback {
   int response_data_length_ = 0;
   std::string response_as_string_;
 
-  TestUrlRequestCallback();
+  explicit TestUrlRequestCallback(bool direct_executor);
   virtual ~TestUrlRequestCallback();
 
-  Cronet_ExecutorPtr GetExecutor(bool direct);
+  Cronet_ExecutorPtr GetExecutor();
 
   Cronet_UrlRequestCallbackPtr CreateUrlRequestCallback();
 
@@ -206,8 +206,8 @@ class TestUrlRequestCallback {
   // When false response data is not accuumulated for better performance.
   bool accumulate_response_data_ = true;
 
-  // Whether to permit calls on the network thread.
-  bool allow_direct_executor_ = false;
+  // Whether to create direct executors.
+  const bool direct_executor_;
 
   // Conditionally fail on certain steps.
   FailureType failure_type_ = NONE;

components/cronet/native/url_request.cc

@@ -375,14 +375,16 @@ void Cronet_UrlRequestImpl::GetStatus(
 
 void Cronet_UrlRequestImpl::OnUploadDataProviderError(
     const std::string& error_message) {
-  base::AutoLock lock(lock_);
-  // If |error_| is not nullptr, that means that another network error is
-  // already reported.
-  if (error_)
-    return;
-  error_ = CreateCronet_Error(
-      0, 0, "Failure from UploadDataProvider: " + error_message);
-  error_->error_code = Cronet_Error_ERROR_CODE_ERROR_CALLBACK;
+  {
+    base::AutoLock lock(lock_);
+    // If |error_| is not nullptr, that means that another network error is
+    // already reported.
+    if (error_)
+      return;
+    error_ = CreateCronet_Error(
+        0, 0, "Failure from UploadDataProvider: " + error_message);
+    error_->error_code = Cronet_Error_ERROR_CODE_ERROR_CALLBACK;
+  }
   // Invoke Cronet_UrlRequestCallback_OnFailed on client executor.
   PostTaskToExecutor(base::BindOnce(
       &Cronet_UrlRequestImpl::InvokeCallbackOnFailed, base::Unretained(this)));
@@ -460,11 +462,14 @@ void Cronet_UrlRequestImpl::NetworkTasks::OnReceivedRedirect(
     const std::string& proxy_server,
     int64_t received_byte_count) {
   DCHECK_CALLED_ON_VALID_THREAD(network_thread_checker_);
-  base::AutoLock lock(url_request_->lock_);
-  url_request_->waiting_on_redirect_ = true;
-  url_request_->response_info_ = CreateCronet_UrlResponseInfo(
-      url_chain_, http_status_code, http_status_text, headers, was_cached,
-      negotiated_protocol, proxy_server, received_byte_count);
+  {
+    base::AutoLock lock(url_request_->lock_);
+    url_request_->waiting_on_redirect_ = true;
+    url_request_->response_info_ = CreateCronet_UrlResponseInfo(
+        url_chain_, http_status_code, http_status_text, headers, was_cached,
+        negotiated_protocol, proxy_server, received_byte_count);
+  }
+
   // Have to do this after creating responseInfo.
   url_chain_.push_back(new_location);
 
@@ -483,11 +488,14 @@ void Cronet_UrlRequestImpl::NetworkTasks::OnResponseStarted(
     const std::string& proxy_server,
     int64_t received_byte_count) {
   DCHECK_CALLED_ON_VALID_THREAD(network_thread_checker_);
-  base::AutoLock lock(url_request_->lock_);
-  url_request_->waiting_on_read_ = true;
-  url_request_->response_info_ = CreateCronet_UrlResponseInfo(
-      url_chain_, http_status_code, http_status_text, headers, was_cached,
-      negotiated_protocol, proxy_server, received_byte_count);
+  {
+    base::AutoLock lock(url_request_->lock_);
+    url_request_->waiting_on_read_ = true;
+    url_request_->response_info_ = CreateCronet_UrlResponseInfo(
+        url_chain_, http_status_code, http_status_text, headers, was_cached,
+        negotiated_protocol, proxy_server, received_byte_count);
+  }
+
   if (url_request_->upload_data_sink_)
     url_request_->upload_data_sink_->PostCloseToExecutor();
 
@@ -505,9 +513,11 @@ void Cronet_UrlRequestImpl::NetworkTasks::OnReadCompleted(
   IOBufferWithCronet_Buffer* io_buffer =
       reinterpret_cast<IOBufferWithCronet_Buffer*>(buffer.get());
   std::unique_ptr<Cronet_Buffer> cronet_buffer(io_buffer->Release());
-  base::AutoLock lock(url_request_->lock_);
-  url_request_->waiting_on_read_ = true;
-  url_request_->response_info_->received_byte_count = received_byte_count;
+  {
+    base::AutoLock lock(url_request_->lock_);
+    url_request_->waiting_on_read_ = true;
+    url_request_->response_info_->received_byte_count = received_byte_count;
+  }
 
   // Invoke Cronet_UrlRequestCallback_OnReadCompleted on client executor.
   url_request_->PostTaskToExecutor(base::BindOnce(
@@ -518,8 +528,10 @@ void Cronet_UrlRequestImpl::NetworkTasks::OnReadCompleted(
 void Cronet_UrlRequestImpl::NetworkTasks::OnSucceeded(
     int64_t received_byte_count) {
   DCHECK_CALLED_ON_VALID_THREAD(network_thread_checker_);
-  base::AutoLock lock(url_request_->lock_);
-  url_request_->response_info_->received_byte_count = received_byte_count;
+  {
+    base::AutoLock lock(url_request_->lock_);
+    url_request_->response_info_->received_byte_count = received_byte_count;
+  }
 
   // Invoke Cronet_UrlRequestCallback_OnSucceeded on client executor.
   url_request_->PostTaskToExecutor(
@@ -533,13 +545,16 @@ void Cronet_UrlRequestImpl::NetworkTasks::OnError(
     const std::string& error_string,
     int64_t received_byte_count) {
   DCHECK_CALLED_ON_VALID_THREAD(network_thread_checker_);
-  base::AutoLock lock(url_request_->lock_);
-  if (url_request_->response_info_)
-    url_request_->response_info_->received_byte_count = received_byte_count;
+  {
+    base::AutoLock lock(url_request_->lock_);
+    if (url_request_->response_info_)
+      url_request_->response_info_->received_byte_count = received_byte_count;
+    url_request_->error_ =
+        CreateCronet_Error(net_error, quic_error, error_string);
+  }
+
   if (url_request_->upload_data_sink_)
     url_request_->upload_data_sink_->PostCloseToExecutor();
-  url_request_->error_ =
-      CreateCronet_Error(net_error, quic_error, error_string);
 
   // Invoke Cronet_UrlRequestCallback_OnFailed on client executor.
   url_request_->PostTaskToExecutor(
@@ -549,7 +564,6 @@ void Cronet_UrlRequestImpl::NetworkTasks::OnError(
 
 void Cronet_UrlRequestImpl::NetworkTasks::OnCanceled() {
   DCHECK_CALLED_ON_VALID_THREAD(network_thread_checker_);
-  base::AutoLock lock(url_request_->lock_);
   if (url_request_->upload_data_sink_)
     url_request_->upload_data_sink_->PostCloseToExecutor();