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 9178bf34ce1c40e379cd2c719ee9164ed9f858f8..eed2bce980a3b135cfb667e3e7aeae59afd14502 100644
--- a/Source/core/loader/DocumentThreadableLoader.cpp
+++ b/Source/core/loader/DocumentThreadableLoader.cpp
@@ -79,11 +79,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();
@@ -147,6 +150,26 @@ DocumentThreadableLoader::~DocumentThreadableLoader()
 {
 }
 
+void DocumentThreadableLoader::overrideTimeout(unsigned long timeoutMilliseconds)
+{
+    if (!m_async)
+        return;
+    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) {
+        double elapsedTime = monotonicallyIncreasingTime() - m_requestStartedSeconds;
+        double nextFire = timeoutMilliseconds / 1000.0;
+        double resolvedTime = std::max(nextFire - elapsedTime, 0.0);
+        m_timeoutTimer.startOneShot(resolvedTime, FROM_HERE);
+    }
+}
+
 void DocumentThreadableLoader::cancel()
 {
     cancelWithError(ResourceError());
@@ -168,6 +191,7 @@ void DocumentThreadableLoader::cancelWithError(const ResourceError& error)
     }
     clearResource();
     m_client = 0;
+    m_requestStartedSeconds = 0.0;
 }
 
 void DocumentThreadableLoader::setDefersLoading(bool value)
@@ -185,6 +209,7 @@ void DocumentThreadableLoader::redirectReceived(Resource* resource, ResourceRequ
     if (!isAllowedByPolicy(request.url())) {
         m_client->didFailRedirectCheck();
         request = ResourceRequest();
+        m_requestStartedSeconds = 0.0;
         return;
     }
 
@@ -251,6 +276,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)
@@ -362,8 +388,10 @@ void DocumentThreadableLoader::handleSuccessfulFinish(unsigned long identifier,
         ASSERT(!m_sameOriginRequest);
         ASSERT(m_options.crossOriginRequestPolicy == UseAccessControl);
         loadActualRequest();
-    } else
+    } else {
         m_client->didFinishLoading(identifier, finishTime);
+        m_requestStartedSeconds = 0.0;

[abarth-chromium, 2014/06/22 00:22:48]

There's no reason to believe |this| is still alive at this point.

Reading the review thread, folks asked for me to take a look at this CL before
it landed, which didn't happen.

[caitp (gmail), 2014/06/22 00:56:14]

Given that this is a non-static method, there probably is a good reason to
assume |this| is still alive. I see that it's not, but it's not clear that
expecting 'this' to exist in an instance method is the root cause. Especially
since we're referencing m_client.

[caitp (gmail), 2014/06/22 00:58:19]

Unless you mean that `didFinishLoading()` might kill 'this', okay.

[tyoshino (SeeGerritForStatus), 2014/06/23 08:00:02]

Sorry that I didn't catch this. Right, didFinishLoading() may kill this. You can
just put m_requestStartedSeconds = 0.0 before calling didFinishLoading().

[caitp (gmail), 2014/06/23 15:55:44]

It would have been hard to notice =)

I'm not sure there's a good reason to reset m_requestStarted if there's a
guarantee the loader will die (I guess it could be done in the destructor to
handle all of these cases?) --- but it may be useful for `didFinishLoading()`,
etc, to know when the entire request was started. Or it might not, I'm not sure.
I could see that potentially being useful later on, though.

[tyoshino (SeeGerritForStatus), 2014/06/24 05:55:34]

Feel free to postpone this and leave FIXME explaining discussion we had. I.e.
I'm fine with not having this line here and L431 for now.

It's not guaranteed that the loader dies on didFinishLoading, but it's not the
right way to call overrideTimeout on the ThreadableLoader after receiving
didFinishLoading callback and I guess no one does it.

+    }
 }
 
 void DocumentThreadableLoader::didTimeout(Timer<DocumentThreadableLoader>* timer)
@@ -400,6 +428,7 @@ void DocumentThreadableLoader::handlePreflightFailure(const String& url, const S
     m_actualRequest = nullptr;
 
     m_client->didFailAccessControlCheck(error);
+    m_requestStartedSeconds = 0.0;

[tyoshino (SeeGerritForStatus), 2014/06/23 08:00:02]

ditto

 }
 
 void DocumentThreadableLoader::loadRequest(const ResourceRequest& request, ResourceLoaderOptions resourceLoaderOptions)
@@ -445,6 +474,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.
         return;
     }
 
@@ -460,6 +490,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.
         return;
     }