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

Source/core/loader/DocumentThreadableLoader.cpp

Index: Source/core/loader/DocumentThreadableLoader.cpp
diff --git a/Source/core/loader/DocumentThreadableLoader.cpp b/Source/core/loader/DocumentThreadableLoader.cpp
index bcb00b9b67766a00380e91cad3274af94fc719e0..719d06de0b17e08e1d8b5644e677bb854cec61b3 100644
--- a/Source/core/loader/DocumentThreadableLoader.cpp
+++ b/Source/core/loader/DocumentThreadableLoader.cpp
@@ -80,11 +80,14 @@ DocumentThreadableLoader::DocumentThreadableLoader(Document& document, Threadabl
     , 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();
@@ -148,6 +151,26 @@ 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());
@@ -169,6 +192,7 @@ void DocumentThreadableLoader::cancelWithError(const ResourceError& error)
     }
     clearResource();
     m_client = 0;
+    m_requestStartedSeconds = 0.0;
 }
 
 void DocumentThreadableLoader::setDefersLoading(bool value)
@@ -186,6 +210,7 @@ void DocumentThreadableLoader::redirectReceived(Resource* resource, ResourceRequ
     if (!isAllowedByPolicy(request.url())) {
         m_client->didFailRedirectCheck();
         request = ResourceRequest();
+        m_requestStartedSeconds = 0.0;
         return;
     }
 
@@ -252,6 +277,7 @@ void DocumentThreadableLoader::redirectReceived(Resource* resource, ResourceRequ
         m_client->didFailRedirectCheck();
     }
     request = ResourceRequest();
+    m_requestStartedSeconds = 0.0;
 }
 
 void DocumentThreadableLoader::dataSent(Resource* resource, unsigned long long bytesSent, unsigned long long totalBytesToBeSent)
@@ -366,6 +392,8 @@ void DocumentThreadableLoader::handleSuccessfulFinish(unsigned long identifier,
         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);
 }
 
@@ -402,6 +430,8 @@ void DocumentThreadableLoader::handlePreflightFailure(const String& url, const S
     // 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);
 }
 
@@ -448,6 +478,7 @@ void DocumentThreadableLoader::loadRequest(const ResourceRequest& request, Resou
 
     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;
     }
 
@@ -463,6 +494,7 @@ void DocumentThreadableLoader::loadRequest(const ResourceRequest& request, Resou
     // 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;
     }