Clean up timeout timer handling in ThreadableLoader

* Don't take a timeout in ThreadableLoader constructor
* Rename OverrideTimeout() to SetTimeout(), use it as the only way
  to set a timeout.
* Stop using base::Optional for the timeout. The more I look at it,
  the less it adds, especially when the timeout isn't passed in the
  constructor.

TBR=peter@chromium.org,yhirano@chromium.org

Change-Id: I8d2e7f1099110399e0fe187432b04760fbc79cac
Reviewed-on: https://chromium-review.googlesource.com/1150865
Commit-Queue: Nate Chapin <japhet@chromium.org>
Reviewed-by: Hiroshige Hayashizaki <hiroshige@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578505}

third_party/blink/renderer/core/exported/web_associated_url_loader_impl.cc

@@ -441,7 +441,7 @@ void WebAssociatedURLLoaderImpl::LoadAsynchronously(
     Document* document = ToDocument(observer_->LifecycleContext());
     DCHECK(document);
     loader_ = new ThreadableLoader(*document, client_adapter_.get(),
-                                   resource_loader_options, base::nullopt);
+                                   resource_loader_options);
     loader_->Start(webcore_request);
   }
 

third_party/blink/renderer/core/fetch/fetch_manager.cc

@@ -856,8 +856,7 @@ void FetchManager::Loader::PerformHTTPFetch(ExceptionState& exception_state) {
 
   probe::willStartFetch(execution_context_, this);
   threadable_loader_ = new ThreadableLoader(*execution_context_, this,
-                                            resource_loader_options,
-                                            base::nullopt);
+                                            resource_loader_options);
   threadable_loader_->Start(request);
 }
 
@@ -884,8 +883,7 @@ void FetchManager::Loader::PerformDataFetch() {
 
   probe::willStartFetch(execution_context_, this);
   threadable_loader_ = new ThreadableLoader(*execution_context_, this,
-                                            resource_loader_options,
-                                            base::nullopt);
+                                            resource_loader_options);
   threadable_loader_->Start(request);
 }
 

third_party/blink/renderer/core/loader/threadable_loader.cc

@@ -247,11 +247,9 @@ ThreadableLoader::CreateAccessControlPreflightRequestForTesting(
 ThreadableLoader::ThreadableLoader(
     ExecutionContext& context,
     ThreadableLoaderClient* client,
-    const ResourceLoaderOptions& resource_loader_options,
-    const base::Optional<TimeDelta>& timeout)
+    const ResourceLoaderOptions& resource_loader_options)
     : client_(client),
       loading_context_(ThreadableLoadingContext::Create(context)),
-      timeout_(timeout),
       resource_loader_options_(resource_loader_options),
       out_of_blink_cors_(RuntimeEnabledFeatures::OutOfBlinkCORSEnabled()),
       cors_flag_(false),
@@ -269,8 +267,6 @@ ThreadableLoader::ThreadableLoader(
       redirect_mode_(network::mojom::FetchRedirectMode::kFollow),
       override_referrer_(false) {
   DCHECK(client);
-  // timeout_ should either be base::nullopt to indicate no timeout, or non-zero.
-  DCHECK(!timeout_ || !timeout_->is_zero());
 }
 
 void ThreadableLoader::Start(const ResourceRequest& request) {
@@ -551,27 +547,29 @@ ThreadableLoader::~ThreadableLoader() {
   DCHECK(!GetResource());
 }
 
-void ThreadableLoader::OverrideTimeout(unsigned long timeout_milliseconds) {
-  DCHECK(async_);
+void ThreadableLoader::SetTimeout(const TimeDelta& timeout) {
+  timeout_ = timeout;
 
-  // |m_requestStartedSeconds| == 0.0 indicates loading is already finished and
-  // |m_timeoutTimer| is already stopped, and thus we do nothing for such cases.
-  // See https://crbug.com/551663 for details.
-  if (request_started_ <= TimeTicks())
+  // |request_started_| <= TimeTicks() indicates loading is either not yet
+  // started or is already finished, and thus we don't need to do anything with
+  // timeout_timer_.
+  if (request_started_ <= TimeTicks()) {
+    DCHECK(!timeout_timer_.IsActive());
     return;
+  }
 
+  DCHECK(async_);
   timeout_timer_.Stop();
-  // At the time of this method's implementation, it is only ever called by
-  // XMLHttpRequest, when the timeout attribute is set after sending the
-  // request.
+
+  // At the time of this method's implementation, it is only ever called for an
+  // inflight request by XMLHttpRequest.
   //
   // The XHR request says to resolve the time relative to when the request
   // was initially sent, however other uses of this method may need to
   // behave differently, in which case this should be re-arranged somehow.
-  if (timeout_milliseconds) {
+  if (!timeout_.is_zero()) {
     TimeDelta elapsed_time = CurrentTimeTicks() - request_started_;
-    TimeDelta next_fire = TimeDelta::FromMilliseconds(timeout_milliseconds);
-    TimeDelta resolved_time = std::max(next_fire - elapsed_time, TimeDelta());
+    TimeDelta resolved_time = std::max(timeout_ - elapsed_time, TimeDelta());
     timeout_timer_.StartOneShot(resolved_time, FROM_HERE);
   }
 }
@@ -1123,13 +1121,13 @@ void ThreadableLoader::LoadRequest(
   if (!actual_request_.IsNull())
     resource_loader_options.data_buffering_policy = kBufferData;
 
-  if (timeout_) {
+  if (!timeout_.is_zero()) {
     if (!async_) {
-      request.SetTimeoutInterval(*timeout_);
+      request.SetTimeoutInterval(timeout_);
     } else if (!timeout_timer_.IsActive()) {
       // The timer can be active if this is the actual request of a
       // CORS-with-preflight request.
-      timeout_timer_.StartOneShot(*timeout_, FROM_HERE);
+      timeout_timer_.StartOneShot(timeout_, FROM_HERE);
     }
   }
 

third_party/blink/renderer/core/loader/threadable_loader.h

@@ -104,8 +104,7 @@ class CORE_EXPORT ThreadableLoader final
   // ThreadableLoaderClient methods may call cancel().
   ThreadableLoader(ExecutionContext&,
                    ThreadableLoaderClient*,
-                   const ResourceLoaderOptions&,
-                   const base::Optional<TimeDelta>& timeout);
+                   const ResourceLoaderOptions&);
   ~ThreadableLoader() override;
 
   // Exposed for testing. Code outside this class should not call this function.
@@ -119,9 +118,11 @@ class CORE_EXPORT ThreadableLoader final
   // cases, for the timeout to be overridden after the request is sent (for
   // example, XMLHttpRequests may override their timeout setting after sending).
   //
-  // Set a new timeout relative to the time the request started, in
-  // milliseconds.
-  void OverrideTimeout(unsigned long timeout_milliseconds);
+  // If the request has already started, the new timeout will be relative to the
+  // time the request started.
+  //
+  // Passing a timeout of zero means there should be no timeout.
+  void SetTimeout(const TimeDelta& timeout);
 
   void Cancel();
 
@@ -216,7 +217,7 @@ class CORE_EXPORT ThreadableLoader final
   ThreadableLoaderClient* client_;
   Member<ThreadableLoadingContext> loading_context_;
 
-  const base::Optional<TimeDelta> timeout_;
+  TimeDelta timeout_;
   // Some items may be overridden by m_forceDoNotAllowStoredCredentials and
   // m_securityOrigin. In such a case, build a ResourceLoaderOptions with
   // up-to-date values from them and this variable, and use it.

third_party/blink/renderer/core/loader/threadable_loader_test.cc

@@ -197,7 +197,7 @@ class DocumentThreadableLoaderTestHelper : public ThreadableLoaderTestHelper {
   void CreateLoader(ThreadableLoaderClient* client) override {
     ResourceLoaderOptions resource_loader_options;
     loader_ = new ThreadableLoader(GetDocument(), client,
-                                   resource_loader_options, base::nullopt);
+                                   resource_loader_options);
   }
 
   void StartLoader(const ResourceRequest& request) override {
@@ -394,7 +394,7 @@ class WorkerThreadableLoaderTestHelper : public ThreadableLoaderTestHelper {
     DCHECK(worker_thread_->GlobalScope()->IsWorkerGlobalScope());
 
     loader_ = new ThreadableLoader(*worker_thread_->GlobalScope(), client,
-                                   resource_loader_options, base::nullopt);
+                                   resource_loader_options);
     DCHECK(loader_);
     event->Signal();
   }

third_party/blink/renderer/core/workers/worker_classic_script_loader.cc

@@ -81,7 +81,7 @@ void WorkerClassicScriptLoader::LoadSynchronously(
   resource_loader_options.synchronous_policy = kRequestSynchronously;
 
   threadable_loader_ = new ThreadableLoader(
-      execution_context, this, resource_loader_options, base::nullopt);
+      execution_context, this, resource_loader_options);
   threadable_loader_->Start(request);
 }
 
@@ -118,7 +118,7 @@ void WorkerClassicScriptLoader::LoadTopLevelScriptAsynchronously(
   scoped_refptr<WorkerClassicScriptLoader> protect(this);
   need_to_cancel_ = true;
   threadable_loader_ = new ThreadableLoader(
-      execution_context, this, resource_loader_options, base::nullopt);
+      execution_context, this, resource_loader_options);
   threadable_loader_->Start(request);
   if (failed_)
     NotifyFinished();

third_party/blink/renderer/core/xmlhttprequest/xml_http_request.cc

@@ -477,10 +477,7 @@ void XMLHttpRequest::setTimeout(unsigned timeout,
     return;
   }
 
-  if (timeout)
-    timeout_ = TimeDelta::FromMilliseconds(timeout);
-  else
-    timeout_ = base::nullopt;
+  timeout_ = TimeDelta::FromMilliseconds(timeout);
 
   // From http://www.w3.org/TR/XMLHttpRequest/#the-timeout-attribute:
   // Note: This implies that the timeout attribute can be set while fetching is
@@ -489,7 +486,7 @@ void XMLHttpRequest::setTimeout(unsigned timeout,
   //
   // The timeout may be overridden after send.
   if (loader_)
-    loader_->OverrideTimeout(timeout);
+    loader_->SetTimeout(timeout_);
 }
 
 void XMLHttpRequest::setResponseType(const String& response_type,
@@ -689,7 +686,7 @@ void XMLHttpRequest::open(const AtomicString& method,
     }
 
     // Similarly, timeouts are disabled for synchronous requests as well.
-    if (timeout_) {
+    if (!timeout_.is_zero()) {
       exception_state.ThrowDOMException(
           DOMExceptionCode::kInvalidAccessError,
           "Synchronous requests must not set a timeout.");
@@ -1124,32 +1121,29 @@ void XMLHttpRequest::CreateRequest(scoped_refptr<EncodedFormData> http_body,
     // TODO(yhirano): Turn this CHECK into DCHECK: see https://crbug.com/570946.
     CHECK(!loader_);
     DCHECK(send_flag_);
-    loader_ = new ThreadableLoader(execution_context, this,
-                                   resource_loader_options, timeout_);
-    loader_->Start(request);
-
-    return;
-  }
-
-  // Use count for XHR synchronous requests.
-  UseCounter::Count(&execution_context, WebFeature::kXMLHttpRequestSynchronous);
-  if (GetExecutionContext()->IsDocument()) {
-    // Update histogram for usage of sync xhr within pagedismissal.
-    auto pagedismissal = GetDocument()->PageDismissalEventBeingDispatched();
-    if (pagedismissal != Document::kNoDismissal) {
-      UseCounter::Count(GetDocument(), WebFeature::kSyncXhrInPageDismissal);
-      DEFINE_STATIC_LOCAL(EnumerationHistogram, syncxhr_pagedismissal_histogram,
-                          ("XHR.Sync.PageDismissal", 5));
-      syncxhr_pagedismissal_histogram.Count(pagedismissal);
+  } else {
+    // Use count for XHR synchronous requests.
+    UseCounter::Count(&execution_context, WebFeature::kXMLHttpRequestSynchronous);
+    if (GetExecutionContext()->IsDocument()) {
+      // Update histogram for usage of sync xhr within pagedismissal.
+      auto pagedismissal = GetDocument()->PageDismissalEventBeingDispatched();
+      if (pagedismissal != Document::kNoDismissal) {
+        UseCounter::Count(GetDocument(), WebFeature::kSyncXhrInPageDismissal);
+        DEFINE_STATIC_LOCAL(EnumerationHistogram, syncxhr_pagedismissal_histogram,
+                            ("XHR.Sync.PageDismissal", 5));
+        syncxhr_pagedismissal_histogram.Count(pagedismissal);
+      }
     }
+    resource_loader_options.synchronous_policy = kRequestSynchronously;
   }
 
-  resource_loader_options.synchronous_policy = kRequestSynchronously;
   loader_ = new ThreadableLoader(execution_context, this,
-                                 resource_loader_options, timeout_);
+                                 resource_loader_options);
+  loader_->SetTimeout(timeout_);
   loader_->Start(request);
 
-  ThrowForLoadFailureIfNeeded(exception_state, String());
+  if (!async_)
+    ThrowForLoadFailureIfNeeded(exception_state, String());
 }
 
 void XMLHttpRequest::abort() {

third_party/blink/renderer/core/xmlhttprequest/xml_http_request.h

@@ -151,7 +151,7 @@ class XMLHttpRequest final : public XMLHttpRequestEventTarget,
   Document* responseXML(ExceptionState&);
   Blob* ResponseBlob();
   DOMArrayBuffer* ResponseArrayBuffer();
-  unsigned timeout() const { return timeout_ ? timeout_->InMilliseconds() : 0; }
+  unsigned timeout() const { return timeout_.InMilliseconds(); }
   void setTimeout(unsigned timeout, ExceptionState&);
   ResponseTypeCode GetResponseTypeCode() const { return response_type_code_; }
   String responseType();
@@ -311,7 +311,7 @@ class XMLHttpRequest final : public XMLHttpRequestEventTarget,
   // Not converted to ASCII lowercase. Must be lowered later or compared
   // using case insensitive comparison functions if needed.
   AtomicString mime_type_override_;
-  base::Optional<TimeDelta> timeout_;
+  TimeDelta timeout_;
   TraceWrapperMember<Blob> response_blob_;
 
   Member<ThreadableLoader> loader_;

third_party/blink/renderer/modules/background_fetch/background_fetch_icon_loader.cc

@@ -73,8 +73,6 @@ void BackgroundFetchIconLoader::DidGetIconDisplaySizeIfSoLoadIcon(
 
   icon_callback_ = std::move(icon_callback);
 
-  TimeDelta timeout = TimeDelta::FromMilliseconds(kIconFetchTimeoutInMs);
-
   ResourceLoaderOptions resource_loader_options;
   if (execution_context->IsWorkerGlobalScope())
     resource_loader_options.request_initiator_context = kWorkerContext;
@@ -84,9 +82,10 @@ void BackgroundFetchIconLoader::DidGetIconDisplaySizeIfSoLoadIcon(
   resource_request.SetPriority(ResourceLoadPriority::kMedium);
   resource_request.SetRequestorOrigin(execution_context->GetSecurityOrigin());
 
-  threadable_loader_ = new ThreadableLoader(*execution_context, this,
-                                            resource_loader_options, timeout);
-
+  threadable_loader_ =
+      new ThreadableLoader(*execution_context, this, resource_loader_options);
+  threadable_loader_->SetTimeout(
+      TimeDelta::FromMilliseconds(kIconFetchTimeoutInMs));
   threadable_loader_->Start(resource_request);
 }
 

third_party/blink/renderer/modules/eventsource/event_source.cc

@@ -156,8 +156,8 @@ void EventSource::Connect() {
   resource_loader_options.security_origin = origin;
 
   probe::willSendEventSourceRequest(&execution_context, this);
-  loader_ = new ThreadableLoader(execution_context, this,
-                                 resource_loader_options, base::nullopt);
+  loader_ =
+      new ThreadableLoader(execution_context, this, resource_loader_options);
   loader_->Start(request);
 }
 

third_party/blink/renderer/modules/notifications/notification_image_loader.cc

@@ -106,8 +106,6 @@ void NotificationImageLoader::Start(ExecutionContext* context,
   start_time_ = CurrentTimeTicks();
   image_callback_ = std::move(image_callback);
 
-  TimeDelta timeout = TimeDelta::FromMilliseconds(kImageFetchTimeoutInMs);
-
   // TODO(mvanouwerkerk): Add an entry for notifications to
   // FetchInitiatorTypeNames and use it.
   ResourceLoaderOptions resource_loader_options;
@@ -120,7 +118,9 @@ void NotificationImageLoader::Start(ExecutionContext* context,
   resource_request.SetRequestorOrigin(context->GetSecurityOrigin());
 
   threadable_loader_ = new ThreadableLoader(
-      *context, this, resource_loader_options, timeout);
+      *context, this, resource_loader_options);
+  threadable_loader_->SetTimeout(
+      TimeDelta::FromMilliseconds(kImageFetchTimeoutInMs));
   threadable_loader_->Start(resource_request);
 }