Bug 74802 – Add timeout support to XMLHttpRequest

• Component: XML
• Status: RESOLVED FIXED

[Jarred Nicholls]

As described in XHR-2 W3C working draft and editor's draft, give XHR a configurable timeout ability that will abort the request and throw the timeout event.

Latest editor's version: http://dvcs.w3.org/hg/xhr/raw-file/tip/Overview.html#the-timeout-attribute

[Jarred Nicholls]

Bug #72154 should land before adding in the timeout functionality.  XHR.timeout is restricted to async requests in the same way that responseType and withCredentials is; once bug #72154 lands I will submit a patch for the timeout support, with the appropriate async restriction.

[Alexey Proskuryakov]

> XHR.timeout is restricted to async requests

This is surprising. A special API for timeout is not strictly required for async requests, but it's very important for sync ones, like them or not (we all don't).

[Jarred Nicholls]

(In reply to comment #2)
> > XHR.timeout is restricted to async requests
> 
> This is surprising. A special API for timeout is not strictly required for async requests, but it's very important for sync ones, like them or not (we all don't).

It's restricted to async only in the window context, not in workers.  The trend happening is to discourage use of sync requests in the window context altogether.  I agree, timeouts are (the most) useful on sync requests for workers, but sync requests in window context are an abomination, period, and no one should be doing them in the first place.  Firefox and Opera agree.  If you disagree, I suggest hitting up public-webapps.

Besides, I think most of the ports' network stacks timeout on the sync requests, e.g. Mac times out in 10 sec by default.

[Jarred Nicholls]

Created attachment 120131 [details]
Work in progress

[Jarred Nicholls]

(In reply to comment #4)
> Created an attachment (id=120131) [details]
> Work in progress

Alexey, what do you think about this approach so far for async requests?  Wondering if timeout behavior is better suited at a lower tier (e.g. ThreadableLoader).

A single shot timer on the caller thread works well for async requests.  It would seem that the easiest solution for sync requests in worker contexts would be to augment WorkerThreadableLoader with timeout capability (bearing in mind only worker contexts can use timeout with sync requests) and post a timeout task to the main thread loader.  Any thoughts on that?

Thanks for your time!

[Alexey Proskuryakov]

I have a number of trivial nits and some things I'll need to carefully check. The only one that seems important at this point is that this case will be working counter-intuitively:

var r = new XMLHttpRequest;
r.timeout = 1000;
r.open(method, url, false /* sync */);

The rest of XHR API doesn't allow setting properties that won't be honored.

As you mentioned, implementing timeout at a lower level makes good sense. In particular, that would:
1. Make implementation for workers more sensible.
2. Make it possible to use support for setting timeouts in networking library.

> sync requests in window context are an abomination, period, and no one should be doing them in the first place.

No disagreement here.

We need to carefully consider how much our work discourages people from using sync requests. If all it does it create more complexity for us, while a significant proportion of authors will keep using sync requests (with even poorer results for end users than would be achievable with timeout support), then maybe the current trend is not so meaningful. Ditto for features that will only make library authors write a few more lines of code, while application authors won't even notice.

Speaking of the timeout property specifically, I have three concerns.

1. It's not entirely clear what the use case is. Will be be just as good if we don't implement the timeout property? Will WebKit miss a chance to better please users in some situations?

2. Commonly in networking world, timeout occurs when no data is received for a while. This is not what this API asks for. Why don't we have a conventional API that times out when loading stalls?

3. In fact, limiting the total time makes it feel that the API is heavily skewed towards sync requests. This still doesn't make complete sense to me, even if we think that scripts in workers will be the only ones to specify timeouts. Why should the worker be given a chance to misbehave on a slow connection?

[Jarred Nicholls]

(In reply to comment #6)
> I have a number of trivial nits and some things I'll need to carefully check. The only one that seems important at this point is that this case will be working counter-intuitively:
> 
> var r = new XMLHttpRequest;
> r.timeout = 1000;
> r.open(method, url, false /* sync */);
> 
> The rest of XHR API doesn't allow setting properties that won't be honored.

Well that's because the sync timeout support isn't in the attached WIP patch, hence why I queried you for some advice.  The goal is to support it of course all together, or otherwise that line of code in setTimeout() will be changed to only check the m_async flag until sync support was added in later on.

> 
> As you mentioned, implementing timeout at a lower level makes good sense. In particular, that would:
> 1. Make implementation for workers more sensible.
> 2. Make it possible to use support for setting timeouts in networking library.

Agreed.  I think lower would be best too.  More coherent and robust.

> 
> > sync requests in window context are an abomination, period, and no one should be doing them in the first place.
> 
> No disagreement here.
> 
> We need to carefully consider how much our work discourages people from using sync requests. If all it does it create more complexity for us, while a significant proportion of authors will keep using sync requests (with even poorer results for end users than would be achievable with timeout support), then maybe the current trend is not so meaningful. Ditto for features that will only make library authors write a few more lines of code, while application authors won't even notice.

See bug #72154 :)  If we implement timeout support the way it's specified and agreed upon in the community, it would only be acceptable for synchronous requests in worker contexts - sync requests in the worker context is an acceptable use case for the web platform.  Sync requests in the window context  is completely discouraged, and has been by library vendors *long before* we, Mozilla, or Opera decided to take initiative in turning off its bells and whistles.

> 
> Speaking of the timeout property specifically, I have three concerns.
> 
> 1. It's not entirely clear what the use case is. Will be be just as good if we don't implement the timeout property? Will WebKit miss a chance to better please users in some situations?

User specified/determined timeouts for sync requests in worker contexts is a very good use case worth supporting.  However, the API would be completely unintuitive if timeouts didn't also work for async requests, though not as important.

> 
> 2. Commonly in networking world, timeout occurs when no data is received for a while. This is not what this API asks for. Why don't we have a conventional API that times out when loading stalls?

That's right.  There are good use cases for making sync requests in worker contexts that deterministically fail after a set amount of time.  If the timeout is a sliding window based on data coming over the wire, then the timeout has now become indeterministic.  But with that said, I also think the user should have the ability to specify a timeout value for data stalls, e.g., dataTimeout.  That's another discussion I suppose, but one that is also useful for async requests that cannot easily be replicated with timers + calls to abort().

> 
> 3. In fact, limiting the total time makes it feel that the API is heavily skewed towards sync requests. This still doesn't make complete sense to me, even if we think that scripts in workers will be the only ones to specify timeouts. Why should the worker be given a chance to misbehave on a slow connection?

I'll let the authors figure out how or why they'd use this to satisfy their use case, but I can see it as being useful.  I personally wouldn't use it ;)

[Jarred Nicholls]

(In reply to comment #7)
> (In reply to comment #6)
> > I have a number of trivial nits and some things I'll need to carefully check. The only one that seems important at this point is that this case will be working counter-intuitively:
> > 
> > var r = new XMLHttpRequest;
> > r.timeout = 1000;
> > r.open(method, url, false /* sync */);
> > 
> > The rest of XHR API doesn't allow setting properties that won't be honored.
> 
> Well that's because the sync timeout support isn't in the attached WIP patch, hence why I queried you for some advice.  The goal is to support it of course all together, or otherwise that line of code in setTimeout() will be changed to only check the m_async flag until sync support was added in later on.

P.S. Also the check in open() from bug #72154 would be amended to also include a check for m_timeout > 0, which is why I have this bug being dependent upon bug #72154.  So, the above code snippet you showed would in fact throw an exception and would not be permissible in the window context, only in worker contexts.

[Alexey Proskuryakov]

> User specified/determined timeouts for sync requests in worker contexts is a very good use case worth supporting.

What specific use cases re there? I agree that it sounds like a reasonable feature to have, but I'm interested in more concrete examples, which you probably have in mind.

[Jarred Nicholls]

(In reply to comment #9)
> > User specified/determined timeouts for sync requests in worker contexts is a very good use case worth supporting.
> 
> What specific use cases re there? I agree that it sounds like a reasonable feature to have, but I'm interested in more concrete examples, which you probably have in mind.

Great question.  I also posed the question publicly: http://lists.w3.org/Archives/Public/public-webapps/2011OctDec/1667.html in the context of a possible new attribute, "dataTimeout".  Because I have to assume if "timeout" made it into the spec that there are already documented use cases which would similarly apply to a data-based timeout.  Most of my thoughts stem from technical scenarios, e.g. background tabs/pages, rather than "application" use cases.

I'd encourage you to hop into the conversation if you think of any good points or use cases!  Thanks.

[Alexey Proskuryakov]

*** Bug 94461 has been marked as a duplicate of this bug. ***

[Dominik Röttsches (drott)]

Created attachment 159904 [details]
LayoutTest suggestion.

I wrote this one (http/tests/xmlhttprequest/ontimeout-event-async.html) when I started looking into the bug 94461 that had now been marked as duplicate. Feel free to use it if it's useful.

[Dominik Röttsches (drott)]

Alexey, Jarred - what's your opinion now that the spec forbids sync, has-Document case. 

Do you think we could implement and land this in two steps, first for the has-Document, async case, then for sync+async worker case (as a separate bug)?

The sync case is difficult at least for the soup backend since it preempts timers , so that's why I am suggesting to solve that separately.

That's also how Mozilla did it:
https://bugzilla.mozilla.org/show_bug.cgi?id=525816
https://bugzilla.mozilla.org/show_bug.cgi?id=498998

[Alexey Proskuryakov]

There is a setTimeoutInterval method in cross-platform ResourceRequestBase.h, which should be just used by XMLHttpRequest. It might need to be implemented for more ports if it's not there yet.

So, I don't think that there is much difference between sync and async cases.

[Dominik Röttsches (drott)]

Jarred, if you don't mind, I'll take this one since you didn't comment in almost a year. I have a work in progress here where I am implementing this based on setTimeoutInterval on ResourceHandle. Please let me know if you have any objections.

[Jarred Nicholls]

(In reply to comment #15)
> Jarred, if you don't mind, I'll take this one since you didn't comment in almost a year. I have a work in progress here where I am implementing this based on setTimeoutInterval on ResourceHandle. Please let me know if you have any objections.

No problem.

[Dominik Röttsches (drott)]

Created attachment 168716 [details]
Patch

[Dominik Röttsches (drott)]

Created attachment 168717 [details]
Patch v2, stray test expectation removed.

[Dominik Röttsches (drott)]

Alexey, fyi, I adapted the *-aborted test cases in a way that they tolerate a secondary abort event after a timeout event.

I'd appreciate if you'd have time to take a look. Thanks very much.

[Build Bot]

Comment on attachment 168717 [details]
Patch v2, stray test expectation removed.

Attachment 168717 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14291699

New failing tests:
http/tests/xmlhttprequest/timeout/xmlhttprequest-timeout-worker-overridesexpires.html
http/tests/xmlhttprequest/timeout/xmlhttprequest-timeout-aborted.html
http/tests/xmlhttprequest/timeout/xmlhttprequest-timeout-worker-simple.html
http/tests/xmlhttprequest/timeout/xmlhttprequest-timeout-worker-aborted.html
http/tests/xmlhttprequest/timeout/xmlhttprequest-timeout-twice.html
http/tests/xmlhttprequest/timeout/xmlhttprequest-timeout-worker-twice.html
http/tests/xmlhttprequest/timeout/xmlhttprequest-timeout-overridesexpires.html
http/tests/xmlhttprequest/timeout/xmlhttprequest-timeout-simple.html

[Alexey Proskuryakov]

Dominik, I presume that you would like to address the test failures first, is that correct?

[Dominik Röttsches (drott)]

(In reply to comment #21)
> Dominik, I presume that you would like to address the test failures first, is that correct?

Yes, I'll take a look at the mac test failures tomorrow morning - however, if you'd have any objections or concerns about the general approach for implementation and testing - it'd be good to hear those early.

[Alexey Proskuryakov]

Comment on attachment 168717 [details]
Patch v2, stray test expectation removed.

View in context: https://bugs.webkit.org/attachment.cgi?id=168717&action=review

> Source/WebCore/loader/cache/CachedResource.h:93
>          LoadError,
> +        TimeoutError,

What is the reason why cached resources need to distinguish the two?

> Source/WebCore/xml/XMLHttpRequest.cpp:336
> +    if (scriptExecutionContext()->isDocument() && !m_async) {
> +        ec = INVALID_ACCESS_ERR;

Shouldn't we be logging an explanation to console here, too? We're raising an exception that could break the whole control flow, and likely wouldn't ever get to open() that has logging now.

> Source/WebCore/xml/XMLHttpRequest.cpp:767
> +    if (m_timeout)
> +        request.setTimeoutInterval(m_timeout / 1000.0);

FWIW, I like to name these variables with measurement unit to avoid confusion (e.g. m_timeoutMilliseconds).

[Dominik Röttsches (drott)]

Created attachment 169009 [details]
Patch v3, review comments addressed.

[Dominik Röttsches (drott)]

(In reply to comment #23)

Thanks for your review, Alexey. Here's an updated patch, the tests are passing now on mac for me, when I have the patch from bug 99478 applied.

> (From update of attachment 168717 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168717&action=review
> 
> > Source/WebCore/loader/cache/CachedResource.h:93
> >          LoadError,
> > +        TimeoutError,
> 
> What is the reason why cached resources need to distinguish the two?

XHR uses a ThreadableLoader and still expects ResourceHandleClient style callbacks, but the ThreadableLoader only deals with CachedResources and gets informed about them using CachedResourceClient style callbacks. So the ThreadableLoader has to synthesize the didFail callback when it gets informed about the CachedResource progress in notifyFinished() - here it needs to query the failure type from CachedResource - cancellation or timeout - in order to set the correct flag on the synthesized ResourceError.

> > Source/WebCore/xml/XMLHttpRequest.cpp:336
> > +    if (scriptExecutionContext()->isDocument() && !m_async) {
> > +        ec = INVALID_ACCESS_ERR;
> 
> Shouldn't we be logging an explanation to console here, too? We're raising an exception that could break the whole control flow, and likely wouldn't ever get to open() that has logging now.

Added.

> > Source/WebCore/xml/XMLHttpRequest.cpp:767
> > +    if (m_timeout)
> > +        request.setTimeoutInterval(m_timeout / 1000.0);
> 
> FWIW, I like to name these variables with measurement unit to avoid confusion (e.g. m_timeoutMilliseconds).

Renamed.

[Alexey Proskuryakov]

> XHR uses a ThreadableLoader and still expects ResourceHandleClient style callbacks, but the ThreadableLoader only deals with CachedResources and gets informed about them using CachedResourceClient style callbacks.

It makes sense that a XHR works with CachedResource, now that it is actually cached. But synthesizing a ResourceError from a few bits of information is unfortunate.

Perhaps we should get rid of XHR::m_error, because it's not really used.

[Jarred Nicholls]

(In reply to comment #26)
> Perhaps we should get rid of XHR::m_error, because it's not really used.

That's true, it is a redundant check in several areas that is not necessary, and would still be spec compliant on the surface.  A thorough check to make sure that is the case would be nice.

[Alexey Proskuryakov]

I didn't realize at first that m_error was already a boolean, not a ResourceError. This makes passing a ResourceError to didFail even less meaningful.

[Dominik Röttsches (drott)]

(In reply to comment #26)
(In reply to comment #27)

> It makes sense that a XHR works with CachedResource, now that it is actually cached. But synthesizing a ResourceError from a few bits of information is unfortunate.

> Perhaps we should get rid of XHR::m_error, because it's not really used.

Jarred, Alexey - I understand you have two proposals for refactoring:

1) "Removing m_error"
   My 2 cents: I actually think it makes the XHR code nicely hackable if we have an equivalent of "the error flag" that's used in the specification.

2) Refactoring ThreadableLoaderClient's didFail(const ResourceError&)
   By for example creating different callback functions for the different cases like didFail, didCancel, didTimeout or replacing the argument by a failure type enumeration or similar. DocumentThreadable loader synthesizes ResourceErrors all over the place, this is not a new thing with my patch. So that would be a bigger change if done comprehensively.

Hence my question - should any of these two refactorings really be part of this patch or would this part of my implementation be acceptable for now and we'd handle the refactoring separately?

[Alexey Proskuryakov]

>    My 2 cents: I actually think it makes the XHR code nicely hackable if we have an equivalent of "the error flag" that's used in the specification.

I agree. My initial reaction was due to thinking that we kept a ResourceError there.

Nate, you are the most active contributor in this area recently. Would you like to review this?

[Nate Chapin]

Comment on attachment 169009 [details]
Patch v3, review comments addressed.

View in context: https://bugs.webkit.org/attachment.cgi?id=169009&action=review

> Source/WebCore/loader/SubresourceLoader.cpp:318
> -    m_resource->error(CachedResource::LoadError);
> +    if (error.isTimeout())
> +        m_resource->error(CachedResource::TimeoutError);
> +    else
> +        m_resource->error(CachedResource::LoadError);

This is ok in the short term, but hopefully we can clean this up soon. I'm planning on adding a m_resource->setResourceError() call here that will hopefully allow us to simplify the code here (see https://bugs.webkit.org/show_bug.cgi?id=99864).

> Source/WebCore/xml/XMLHttpRequest.cpp:1221
> +    clearResponse(); // FIXME: clearResponseBuffers()?

Why the FIXME?

> Source/WebCore/xml/XMLHttpRequest.cpp:1231
> +    if (!m_async) {
> +        m_state = DONE;
> +        m_exceptionCode = TIMEOUT_ERR;
> +        return;
> +    }

Just to make sure I follow, this is because XHRs in workers can use timeouts in sync requests, right?

> Source/WebCore/xml/XMLHttpRequest.cpp:1240
> +    if (!m_uploadComplete) {
> +        m_uploadComplete = true;
> +        if (m_upload && m_uploadEventsAllowed)
> +            m_upload->dispatchEventAndLoadEnd(XMLHttpRequestProgressEvent::create(eventNames().timeoutEvent));
> +    }
> +    m_progressEventThrottle.dispatchEventAndLoadEnd(XMLHttpRequestProgressEvent::create(eventNames().timeoutEvent));

Pedantic nit: Any reason we do these dispatches in the opposite order as everywhere else in the file?

[Dominik Röttsches (drott)]

Created attachment 169858 [details]
Patch v4, FIXME removed.

[Dominik Röttsches (drott)]

(In reply to comment #31)

Thanks for the review, Nate. I removed the FIXME in an updated patch.

> (From update of attachment 169009 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169009&action=review
> 
> > Source/WebCore/loader/SubresourceLoader.cpp:318
> > -    m_resource->error(CachedResource::LoadError);
> > +    if (error.isTimeout())
> > +        m_resource->error(CachedResource::TimeoutError);
> > +    else
> > +        m_resource->error(CachedResource::LoadError);
> 
> This is ok in the short term, but hopefully we can clean this up soon. I'm planning on adding a m_resource->setResourceError() call here that will hopefully allow us to simplify the code here (see https://bugs.webkit.org/show_bug.cgi?id=99864).

Thanks, CC'ed myself there.

> > Source/WebCore/xml/XMLHttpRequest.cpp:1221
> > +    clearResponse(); // FIXME: clearResponseBuffers()?
> 
> Why the FIXME?

That was a leftover before uploading. clearResponse() calls clearResponseBuffers(), so we're resetting to a sane state here.

> > Source/WebCore/xml/XMLHttpRequest.cpp:1231
> > +    if (!m_async) {
> > +        m_state = DONE;
> > +        m_exceptionCode = TIMEOUT_ERR;
> > +        return;
> > +    }
> 
> Just to make sure I follow, this is because XHRs in workers can use timeouts in sync requests, right?

Yes, in the synchronous worker case, the send() call needs to return with the right exception. This corresponds to line 794 in XMLHttpRequest.cpp:
    if (!m_exceptionCode && m_error)
        m_exceptionCode = XMLHttpRequestException::NETWORK_ERR;


> > Source/WebCore/xml/XMLHttpRequest.cpp:1240
> > +    if (!m_uploadComplete) {
> > +        m_uploadComplete = true;
> > +        if (m_upload && m_uploadEventsAllowed)
> > +            m_upload->dispatchEventAndLoadEnd(XMLHttpRequestProgressEvent::create(eventNames().timeoutEvent));
> > +    }
> > +    m_progressEventThrottle.dispatchEventAndLoadEnd(XMLHttpRequestProgressEvent::create(eventNames().timeoutEvent));
> 
> Pedantic nit: Any reason we do these dispatches in the opposite order as everywhere else in the file?

I think it's not pedantic but a valid question. I implemented the order here according to steps 7, 8, and 9 of
http://www.w3.org/TR/XMLHttpRequest/#timeout-error
In the spec the upload events are dispatched first, the progress events follow.

[Alexey Proskuryakov]

Comment on attachment 169858 [details]
Patch v4, FIXME removed.

View in context: https://bugs.webkit.org/attachment.cgi?id=169858&action=review

> LayoutTests/http/tests/xmlhttprequest/timeout/xmlhttprequest-timeout-synconmain.html:17
> +    <script src="/w3c/resources/testharness.js"></script>
> +    <script src="/w3c/resources/testharnessreport.js"></script>
> +    <script src="xmlhttprequest-timeout.js"></script>
> +    <script src="xmlhttprequest-timeout-runner.js"></script>
> +    <script>
> +        function testXhr() {
> +            window.addEventListener("message", testResultCallbackHandler);
> +            var runTests = { "type": "start", "group": groupFromLocation() };
> +            window.postMessage(runTests, "*");
> +        };

FWIW, I can't figure out what a test like this does without excessive amount of effort. I just wanted to make sure if the code about which Nate earlier commented "Just to make sure I follow" was actually covered by regression tests.

[Dominik Röttsches (drott)]

Created attachment 169957 [details]
Patch v5, synconworker case enabled.

[Dominik Röttsches (drott)]

Created attachment 169958 [details]
Difference between v4 and v5

[Dominik Röttsches (drott)]

Comment on attachment 169858 [details]
Patch v4, FIXME removed.

View in context: https://bugs.webkit.org/attachment.cgi?id=169858&action=review

>> LayoutTests/http/tests/xmlhttprequest/timeout/xmlhttprequest-timeout-synconmain.html:17
>> +        };
> 
> FWIW, I can't figure out what a test like this does without excessive amount of effort. I just wanted to make sure if the code about which Nate earlier commented "Just to make sure I follow" was actually covered by regression tests.

Thanks for emphasizing that. I actually commented one test out with the wrong observation. I thought it was a late update case as well but it wasn't. I modified the patch so that this test is run, which ensures that the synchronous timeout codepath that Nate talked about is covered in the tests. "Difference between v4 and v5" shows what I did to enable that test.

> LayoutTests/http/tests/xmlhttprequest/timeout/xmlhttprequest-timeout.js:330
> +    // new RequestTracker(false, "timeout hit before load", 2000),

This case is now enabled and passes, which exercises the synchronous timeout exception code path.

[Nate Chapin]

Comment on attachment 169957 [details]
Patch v5, synconworker case enabled.

View in context: https://bugs.webkit.org/attachment.cgi?id=169957&action=review

> LayoutTests/http/tests/xmlhttprequest/timeout/xmlhttprequest-timeout.js:308
> +  // FIXME: http://webkit.org/b/98156 - Late updates are not supported yet, these tests are not run.
> +  "overrides" : [
> +    new RequestTracker(true, "timeout disabled after initially set", 5000, 2000, 0),
> +    new RequestTracker(true, "timeout overrides load after a delay", 5000, 1000, 2000),
> +    new RequestTracker(true, "timeout enabled after initially disabled", 0, 2000, 5000)
> +  ],

Should these be commented out if they're not being run?

[Nate Chapin]

Comment on attachment 169957 [details]
Patch v5, synconworker case enabled.

Also, we may want to consider putting these tests in a directory that indicates that they are imported.

I'm not clear from the ChangeLog, are these tests from the W3C, or just adapted to their test harness?

[Dominik Röttsches (drott)]

Created attachment 170075 [details]
Patch v6, ChangeLog clarification.

[Dominik Röttsches (drott)]

(In reply to comment #38)
> (From update of attachment 169957 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169957&action=review
> 
> > LayoutTests/http/tests/xmlhttprequest/timeout/xmlhttprequest-timeout.js:308
> > +  // FIXME: http://webkit.org/b/98156 - Late updates are not supported yet, these tests are not run.
> > +  "overrides" : [
> > +    new RequestTracker(true, "timeout disabled after initially set", 5000, 2000, 0),
> > +    new RequestTracker(true, "timeout overrides load after a delay", 5000, 1000, 2000),
> > +    new RequestTracker(true, "timeout enabled after initially disabled", 0, 2000, 5000)
> > +  ],
> 
> Should these be commented out if they're not being run?

The entries in this array, like "twice" or "synconmain" are executed if there is a corresponding .html file like, xmlhttprequest-timeout-twice.html or xmlhttprequest-timeout-synconmain.html. The "overrides" group is currently not being run because there is no such corresponding file. I clarified that in the LayoutTests/ChangeLog now. So because the whole group is not run, they don't need to be commented.

(In reply to comment #39)
> (From update of attachment 169957 [details])
> Also, we may want to consider putting these tests in a directory that indicates that they are imported.
> 
> I'm not clear from the ChangeLog, are these tests from the W3C, or just adapted to their test harness?

They were adapted from a set of tests from Mozilla. They are not from W3C, just adapted to their testharness.js so that I can work on contributing them to W3C later. So, in that sense, it's not an import and I'd say we do not need a folder that marks them as imported. I tried to clarify this in the ChangeLogs now as well.

[Nate Chapin]

Comment on attachment 170075 [details]
Patch v6, ChangeLog clarification.

View in context: https://bugs.webkit.org/attachment.cgi?id=170075&action=review

> LayoutTests/ChangeLog:13
> +        I adapted them for W3C testharness.js and split them into groups with shorter test running time
> +        so that they can be used as WebKit layout tests. Each individual test should
> +        complete in less than 20 seconds.

I assume the timeout durations are the same as the mozilla version of these tests?

It's still unfortunate that the tests run quite so long, but I suppose it is necessary to given what's being tested.

[Dominik Röttsches (drott)]

(In reply to comment #42)
> (From update of attachment 170075 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=170075&action=review
> 
> > LayoutTests/ChangeLog:13
> > +        I adapted them for W3C testharness.js and split them into groups with shorter test running time
> > +        so that they can be used as WebKit layout tests. Each individual test should
> > +        complete in less than 20 seconds.
> 
> I assume the timeout durations are the same as the mozilla version of these tests?

Yes, they are identical.

> It's still unfortunate that the tests run quite so long, but I suppose it is necessary to given what's being tested.

Yes, we don't want these intervals too narrow in order to avoid fluctuations on the bots.

[WebKit Review Bot]

Comment on attachment 170075 [details]
Patch v6, ChangeLog clarification.

Clearing flags on attachment: 170075

Committed r132252: <http://trac.webkit.org/changeset/132252>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Barth]

These tests all appear to time out.

[Dominik Röttsches (drott)]

(In reply to comment #46)
> These tests all appear to time out.

Levi has skipped them in r132263, support for setTimeoutInterval in Chromium's http backend is tracked in bug 98397.

[Adam Barth]

> Levi has skipped them in r132263, support for setTimeoutInterval in Chromium's http backend is tracked in bug 98397.

Should we disable this feature on ports that don't support it in their network backend?  If we don't, web sites will have busted feature detection.

[Dominik Röttsches (drott)]

(In reply to comment #48)
> > Levi has skipped them in r132263, support for setTimeoutInterval in Chromium's http backend is tracked in bug 98397.
> 
> Should we disable this feature on ports that don't support it in their network backend?  If we don't, web sites will have busted feature detection.

It's only Chromium & Qt left. This is working on Mac, Gtk, EFL. 
It didn't take me very long to support setTimeoutInterval in the libsoup backend. (cmp. bug 94796) I suppose it's not gonna take very long to do that in the Chromium http backend.  Can't we look into just fixing the two remaining network backends? What do you think?

If you still think it should be disabled for other ports, how would you wanna do it, just #ifdef'ing the timeout property and ontimeout event handler in the XHR idl to hide it?

[Roger Fong]

(In reply to comment #49)

> It's only Chromium & Qt left. This is working on Mac, Gtk, EFL. 
And Apple Windows

[Dominik Röttsches (drott)]

(In reply to comment #50)
> (In reply to comment #49)
> 
> > It's only Chromium & Qt left. This is working on Mac, Gtk, EFL. 
> And Apple Windows

If CF networking on Windows supports setTimeoutInterval, I can look into getting this to work (tomorrow EEST).

[Adam Barth]

(In reply to comment #49)
> (In reply to comment #48)
> > > Levi has skipped them in r132263, support for setTimeoutInterval in Chromium's http backend is tracked in bug 98397.
> > 
> > Should we disable this feature on ports that don't support it in their network backend?  If we don't, web sites will have busted feature detection.
> 
> It's only Chromium & Qt left. This is working on Mac, Gtk, EFL. 
> It didn't take me very long to support setTimeoutInterval in the libsoup backend. (cmp. bug 94796) I suppose it's not gonna take very long to do that in the Chromium http backend.  Can't we look into just fixing the two remaining network backends? What do you think?

I think you should disable the feature on ports where it doesn't work until the feature works on those ports.

> If you still think it should be disabled for other ports, how would you wanna do it, just #ifdef'ing the timeout property and ontimeout event handler in the XHR idl to hide it?

Please use an ENABLE macro an features.gypi, like we use for many other features.

[Dominik Röttsches (drott)]

(In reply to comment #50)
> (In reply to comment #49)
> 
> > It's only Chromium & Qt left. This is working on Mac, Gtk, EFL. 
> And Apple Windows

Fix attempt for this in bug 100349.

[Dominik Röttsches (drott)]

(In reply to comment #52)

> Please use an ENABLE macro an features.gypi, like we use for many other features.

Patch in bug 100356. Posted on webkit-dev as well.