Allow XHR timeout attribute to be overridden after send(), per spec

Source/core/loader/DocumentThreadableLoader.cpp

 /*
  * Copyright (C) 2011, 2012 Google Inc. All rights reserved.
  * Copyright (C) 2013, Intel Corporation
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
@@ skipping 62 matching lines @@
     : m_client(client)
     , m_document(document)
     , m_options(options)
     , m_resourceLoaderOptions(resourceLoaderOptions)
     , m_forceDoNotAllowStoredCredentials(false)
     , m_securityOrigin(m_resourceLoaderOptions.securityOrigin)
     , m_sameOriginRequest(securityOrigin()->canRequest(request.url()))
     , m_simpleRequest(true)
     , m_async(blockingBehavior == LoadAsynchronously)
     , m_timeoutTimer(this, &DocumentThreadableLoader::didTimeout)
+    , m_requestStartedSeconds(0.0)
 {
     ASSERT(client);
     // Setting an outgoing referer is only supported in the async code path.
     ASSERT(m_async || request.httpReferrer().isEmpty());
 
+    m_requestStartedSeconds = monotonicallyIncreasingTime();
+
     // Save any CORS simple headers on the request here. If this request redirects cross-origin, we cancel the old request
     // create a new one, and copy these headers.
     const HTTPHeaderMap& headerMap = request.httpHeaderFields();
     HTTPHeaderMap::const_iterator end = headerMap.end();
     for (HTTPHeaderMap::const_iterator it = headerMap.begin(); it != end; ++it) {
         if (isOnAccessControlSimpleRequestHeaderWhitelist(it->key, it->value))
             m_simpleRequestHeaders.add(it->key, it->value);
     }
 
     if (m_sameOriginRequest || m_options.crossOriginRequestPolicy == AllowCrossOriginRequests) {
@@ skipping 43 matching lines @@
             preflightOptions.allowCredentials = DoNotAllowStoredCredentials;
             loadRequest(preflightRequest, preflightOptions);
         }
     }
 }
 
 DocumentThreadableLoader::~DocumentThreadableLoader()
 {
 }
 
+void DocumentThreadableLoader::overrideTimeout(unsigned long timeoutMilliseconds)
+{
+    if (!m_async)
+        return;

[abarth-chromium, 2014/07/31 16:08:13]

How can this be called during a synchronous request?  Seems like this ought to
be an assert

+    m_timeoutTimer.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.
+    //
+    // 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 (timeoutMilliseconds && m_requestStartedSeconds > 0.0) {

[abarth-chromium, 2014/07/31 16:08:13]

How could m_requestStartedSeconds not be > 0.0?  Should that be an assert?

+        double elapsedTime = monotonicallyIncreasingTime() - m_requestStartedSeconds;
+        double nextFire = timeoutMilliseconds / 1000.0;
+        double resolvedTime = std::max(nextFire - elapsedTime, 0.0);
+        m_timeoutTimer.startOneShot(resolvedTime, FROM_HERE);

[abarth-chromium, 2014/07/31 16:08:12]

So, if the request has been running already for 2 seconds and we set a 1 second
timeout, the value is ignored?  I would have expect us to cancel the request
instead because it has exceeded its timeout.

This case is particularly likely when making an XMLHttpRequest from a worker
because the main thread might be busy and not get around to processing the
timeout override for a while.  I think the current behavior is racy in that
case.

[caitp (gmail), 2014/07/31 16:43:29]

I am not exactly certain how startOneShot() will work, but to me, what this is
saying is that we will timeout immediately (resolvedTime === 0.0, because of the
std::max call) if we ask for a timeout which is less than the already elapsed
time

[tyoshino (SeeGerritForStatus), 2014/08/01 07:24:18]

Correct. For the abarth@'s example
o elapsedTime == 2
o nextFire - elapsedTime == -1
o resolvedTime == 0

We'll run didTimeout asynchronously but immediately.

+    }
+}
+
 void DocumentThreadableLoader::cancel()
 {
     cancelWithError(ResourceError());
 }
 
 void DocumentThreadableLoader::cancelWithError(const ResourceError& error)
 {
     RefPtr<DocumentThreadableLoader> protect(this);
 
     // Cancel can re-enter and m_resource might be null here as a result.
     if (m_client && resource()) {
         ResourceError errorForCallback = error;
         if (errorForCallback.isNull()) {
             // FIXME: This error is sent to the client in didFail(), so it should not be an internal one. Use FrameLoaderClient::cancelledError() instead.
             errorForCallback = ResourceError(errorDomainBlinkInternal, 0, resource()->url().string(), "Load cancelled");
             errorForCallback.setIsCancellation(true);
         }
         m_client->didFail(errorForCallback);
     }
     clearResource();
     m_client = 0;
+    m_requestStartedSeconds = 0.0;
 }
 
 void DocumentThreadableLoader::setDefersLoading(bool value)
 {
     if (resource())
         resource()->setDefersLoading(value);
 }
 
 void DocumentThreadableLoader::redirectReceived(Resource* resource, ResourceRequest& request, const ResourceResponse& redirectResponse)
 {
     ASSERT(m_client);
     ASSERT_UNUSED(resource, resource == this->resource());
 
     RefPtr<DocumentThreadableLoader> protect(this);
     if (!isAllowedByPolicy(request.url())) {
         m_client->didFailRedirectCheck();
         request = ResourceRequest();
+        m_requestStartedSeconds = 0.0;
         return;
     }
 
     // Allow same origin requests to continue after allowing clients to audit the redirect.
     if (isAllowedRedirect(request.url())) {
         if (m_client->isDocumentThreadableLoaderClient())
             static_cast<DocumentThreadableLoaderClient*>(m_client)->willSendRequest(request, redirectResponse);
         return;
     }
 
@@ skipping 46 matching lines @@
             makeCrossOriginAccessRequest(request);
             return;
         }
 
         ResourceError error(errorDomainBlinkInternal, 0, redirectResponse.url().string(), accessControlErrorDescription);
         m_client->didFailAccessControlCheck(error);
     } else {
         m_client->didFailRedirectCheck();
     }
     request = ResourceRequest();
+    m_requestStartedSeconds = 0.0;
 }
 
 void DocumentThreadableLoader::dataSent(Resource* resource, unsigned long long bytesSent, unsigned long long totalBytesToBeSent)
 {
     ASSERT(m_client);
     ASSERT_UNUSED(resource, resource == this->resource());
     m_client->didSendData(bytesSent, totalBytesToBeSent);
 }
 
 void DocumentThreadableLoader::dataDownloaded(Resource* resource, int dataLength)
@@ skipping 93 matching lines @@
     else
         handleSuccessfulFinish(resource->identifier(), resource->loadFinishTime());
 }
 
 void DocumentThreadableLoader::handleSuccessfulFinish(unsigned long identifier, double finishTime)
 {
     if (m_actualRequest) {
         ASSERT(!m_sameOriginRequest);
         ASSERT(m_options.crossOriginRequestPolicy == UseAccessControl);
         loadActualRequest();
     } else

[abarth-chromium, 2014/07/31 16:01:35]

Please add { }

+        // FIXME: Should prevent timeout from being overridden after finished loading, without
+        // resetting m_requestStartedSeconds to 0.0
         m_client->didFinishLoading(identifier, finishTime);
 }
 
 void DocumentThreadableLoader::didTimeout(Timer<DocumentThreadableLoader>* timer)
 {
     ASSERT_UNUSED(timer, timer == &m_timeoutTimer);
 
     // Using values from net/base/net_error_list.h ERR_TIMED_OUT,
     // Same as existing FIXME above - this error should be coming from FrameLoaderClient to be identifiable.
     static const int timeoutError = -7;
@@ skipping 16 matching lines @@
     loadRequest(*actualRequest, *actualOptions);
 }
 
 void DocumentThreadableLoader::handlePreflightFailure(const String& url, const String& errorDescription)
 {
     ResourceError error(errorDomainBlinkInternal, 0, url, errorDescription);
 
     // Prevent handleSuccessfulFinish() from bypassing access check.
     m_actualRequest = nullptr;
 
+    // FIXME: Should prevent timeout from being overridden after preflight failure, without
+    // resetting m_requestStartedSeconds to 0.0
     m_client->didFailAccessControlCheck(error);
 }
 
 void DocumentThreadableLoader::loadRequest(const ResourceRequest& request, ResourceLoaderOptions resourceLoaderOptions)
 {
     // Any credential should have been removed from the cross-site requests.
     const KURL& requestURL = request.url();
     ASSERT(m_sameOriginRequest || requestURL.user().isEmpty());
     ASSERT(m_sameOriginRequest || requestURL.pass().isEmpty());
 
@@ skipping 26 matching lines @@
     FetchRequest fetchRequest(request, m_options.initiator, resourceLoaderOptions);
     ResourcePtr<Resource> resource = m_document.fetcher()->fetchSynchronously(fetchRequest);
     ResourceResponse response = resource ? resource->response() : ResourceResponse();
     unsigned long identifier = resource ? resource->identifier() : std::numeric_limits<unsigned long>::max();
     ResourceError error = resource ? resource->resourceError() : ResourceError();
 
     InspectorInstrumentation::documentThreadableLoaderStartedLoadingForClient(&m_document, identifier, m_client);
 
     if (!resource) {
         m_client->didFail(error);
+        m_requestStartedSeconds = 0.0; // Unnecessary for synchronous request, but for API consistency.

[abarth-chromium, 2014/07/31 16:01:35]

Why doesn't this cause the same problem?  Can the client not delete us unside
the didFail callback here?

[tyoshino (SeeGerritForStatus), 2014/08/01 07:24:18]

We enter this path only when m_asyc is false. So, there should be a ref on this
DocumentThreadableLoader held by the caller.

  loader->loadResourceSynchronously(...);


It's not 100% sure that the caller doesn't clear the smart pointer holding the
ref in didFail().

  m_loader->loadResourceSynchronously(...);
  ...
  void didFail(const ResourceError error) {
    m_loader = nullptr;
  }

I don't like code like this (deleting the object before some sync call to a
method of the object), and rather want to forbid this than not expecting the
object is live until we exit from loadRequest().

But I'm also fine with removing this line and L497. As comment is saying they're
unnecessary.

         return;
     }
 
     // No exception for file:/// resources, see <rdar://problem/4962298>.
     // Also, if we have an HTTP response, then it wasn't a network error in fact.
     if (!error.isNull() && !requestURL.isLocalFile() && response.httpStatusCode() <= 0) {
         m_client->didFail(error);
         return;
     }
 
     // FIXME: A synchronous request does not tell us whether a redirect happened or not, so we guess by comparing the
     // request and response URLs. This isn't a perfect test though, since a server can serve a redirect to the same URL that was
     // requested. Also comparing the request and response URLs as strings will fail if the requestURL still has its credentials.
     if (requestURL != response.url() && (!isAllowedByPolicy(response.url()) || !isAllowedRedirect(response.url()))) {
         m_client->didFailRedirectCheck();
+        m_requestStartedSeconds = 0.0; // Unnecessary for synchronous request, but for API consistency.

[abarth-chromium, 2014/07/31 16:01:35]

ditto

         return;
     }
 
     handleResponse(identifier, response);
 
     SharedBuffer* data = resource->resourceBuffer();
     if (data)
         handleReceivedData(data->data(), data->size());
 
     handleSuccessfulFinish(identifier, 0.0);
@@ skipping 20 matching lines @@
         return DoNotAllowStoredCredentials;
     return m_resourceLoaderOptions.allowCredentials;
 }
 
 SecurityOrigin* DocumentThreadableLoader::securityOrigin() const
 {
     return m_securityOrigin ? m_securityOrigin.get() : m_document.securityOrigin();
 }
 
 } // namespace WebCore