Bug 163814 – REGRESSION (Safari 10 / r189445): WKWebView and WebView no longer allow async XMLHttpRequest timeout to exceed 60 seconds

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Dan Saunders]

WKWebView and WebView control do not allow XMLHttpRequest timeout to be greater than 60 seconds. The below code can repro, this was not an issue with the version of WebKit that was shipped in OSX 10.11.5, but it does not work in 10.12.1.
var xhr = new XMLHttpRequest();
var startTime = new Date();
xhr.open('GET', url, true);

xhr.timeout = 120000; // time in milliseconds

xhr.onload = function () {
};

xhr.ontimeout = function (e) {
  console.error((new Date() - startTime) + " milliseconds timeout."); // 60000 milliseconds timeout.
};

xhr.send(null);


I believe the relevant code in WebKit is below, for asynchronous requests m_timeoutInterval is not getting set so it is using NSUrlRequest default timeoutInterval of 60 seconds.

XMLHttpRequest::createRequest(ExceptionCode& ec)

    if (m_timeoutMilliseconds) {
        if (!m_async)
            request.setTimeoutInterval(m_timeoutMilliseconds / 1000.0);
        else {
            m_sendingTime = std::chrono::steady_clock::now();
            m_timeoutTimer.startOneShot(std::chrono::milliseconds { m_timeoutMilliseconds });
        }
    }

ResourceRequest::doUpdatePlatformRequest

    double timeoutInterval = ResourceRequestBase::timeoutInterval();
    if (timeoutInterval)
        [nsRequest setTimeoutInterval:timeoutInterval];

[Radar WebKit Bug Importer]

<rdar://problem/28917420>

[Chris Dumez]

Potential regression from Youenn's <http://trac.webkit.org/changeset/189445>.

[youenn fablet]

Right, I will fix it tomorrow.

[youenn fablet]

Created attachment 292730 [details]
Patch

[Build Bot]

Comment on attachment 292730 [details]
Patch

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

New failing tests:
http/tests/xmlhttprequest/resetting-timeout-to-zero.html

[Build Bot]

Created attachment 292733 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 292730 [details]
Patch

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

New failing tests:
http/tests/xmlhttprequest/resetting-timeout-to-zero.html

[Build Bot]

Created attachment 292735 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 292730 [details]
Patch

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

New failing tests:
http/tests/xmlhttprequest/timeout-greater-than-default-network-timeout.html
http/tests/xmlhttprequest/resetting-timeout-to-zero.html

[Build Bot]

Created attachment 292736 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[youenn fablet]

Created attachment 292739 [details]
Patch

[Build Bot]

Comment on attachment 292739 [details]
Patch

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

New failing tests:
http/tests/xmlhttprequest/timeout-greater-than-default-network-timeout.html

[Build Bot]

Created attachment 292743 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[youenn fablet]

Created attachment 292746 [details]
Fixing flaky test

[Alex Christensen]

Comment on attachment 292746 [details]
Fixing flaky test

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

ResourceRequestBase.h says 0 is a special number meaning that it should use the default timeout.  This patch seems to contradict that.  What does it do if m_timeoutMilliseconds is 0?

> LayoutTests/http/tests/xmlhttprequest/resetting-timeout-to-zero.html:17
> +        xhr.timeout = 100000; // time in milliseconds

Would 70000 work?  Adding an unnecessary 30 seconds will slow down the tests more than necessary.

[Brady Eidson]

(In reply to comment #15)
> Comment on attachment 292746 [details]
> Fixing flaky test
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=292746&action=review
> 
> ResourceRequestBase.h says 0 is a special number meaning that it should use
> the default timeout.  This patch seems to contradict that.  What does it do
> if m_timeoutMilliseconds is 0?
> 
> > LayoutTests/http/tests/xmlhttprequest/resetting-timeout-to-zero.html:17
> > +        xhr.timeout = 100000; // time in milliseconds
> 
> Would 70000 work?  Adding an unnecessary 30 seconds will slow down the tests
> more than necessary.

We're not actually adding a test that takes over a minute to run, are we?

I don't think that's okay... is it?

[youenn fablet]

(In reply to comment #15)
> Comment on attachment 292746 [details]
> Fixing flaky test
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=292746&action=review
> 
> ResourceRequestBase.h says 0 is a special number meaning that it should use
> the default timeout.  This patch seems to contradict that.  What does it do
> if m_timeoutMilliseconds is 0?
> 
> > LayoutTests/http/tests/xmlhttprequest/resetting-timeout-to-zero.html:17
> > +        xhr.timeout = 100000; // time in milliseconds
> 
> Would 70000 work?  Adding an unnecessary 30 seconds will slow down the tests
> more than necessary.

70s is fine too.
But it does not change anything since in this case this is the simulated default network timeout that is kicking in. The test, if successful, will take about 60s. Reducing to 70s would help in case of failures.

> We're not actually adding a test that takes over a minute to run, are we?
> 
> I don't think that's okay... is it?

The two tests need to last the time it takes to hit the default network timeout.
If we decide to reduce it in WTR, we could reduce the duration of the tests.
Otherwise, we need to wait 60 seconds, or not cover the changes with tests.

[Darin Adler]

Comment on attachment 292746 [details]
Fixing flaky test

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

> Source/WebCore/xml/XMLHttpRequest.cpp:248
> +    // If timeout is zero, we should use the default network timeout. But we disabled it so let's mimick this with a 60 seconds timeout value.

The word "mimic" does not have a "k".

[youenn fablet]

Created attachment 292911 [details]
Patch

[youenn fablet]

(In reply to comment #19)
> Created attachment 292911 [details]
> Patch

Reduced to 70000 for the test and fixed mimick in changelog and xhr.cpp

[Build Bot]

Comment on attachment 292911 [details]
Patch

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

New failing tests:
http/tests/security/svg-image-with-css-cross-domain.html

[Build Bot]

Created attachment 292914 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[youenn fablet]

(In reply to comment #21)
> Comment on attachment 292911 [details]
> Patch
> 
> Attachment 292911 [details] did not pass mac-debug-ews (mac):
> Output: http://webkit-queues.webkit.org/results/2374304
> 
> New failing tests:
> http/tests/security/svg-image-with-css-cross-domain.html

Test is flaky as per history and is not related to this patch.

[youenn fablet]

> Test is flaky as per history and is not related to this patch.

Test should be fixed with https://bugs.webkit.org/show_bug.cgi?id=164095.
Ping review?

[youenn fablet]

(In reply to comment #24)
> > Test is flaky as per history and is not related to this patch.
> 
> Test should be fixed with https://bugs.webkit.org/show_bug.cgi?id=164095.

I was meaning https://bugs.webkit.org/show_bug.cgi?id=163922

[Darin Adler]

Comment on attachment 292911 [details]
Patch

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

> Source/WebCore/xml/XMLHttpRequest.cpp:248
> +    // If timeout is zero, we should use the default network timeout. But we disabled it so let's mimic it with a 60 seconds timeout value.

I am really confused by this. Is the default network timeout 60 seconds? Is this a real solution or a temporary hack of some kind?

[youenn fablet]

Thanks for the review.

> > Source/WebCore/xml/XMLHttpRequest.cpp:248
> > +    // If timeout is zero, we should use the default network timeout. But we disabled it so let's mimic it with a 60 seconds timeout value.
> 
> I am really confused by this. Is the default network timeout 60 seconds? Is
> this a real solution or a temporary hack of some kind?

Default CFNetwork timeout is 60 seconds.
I think it is the same for soup as well, not sure for curl.

Timeout is an area where there should be some whatwg discussions, both for fetch API and XHR I think.

[WebKit Commit Bot]

Comment on attachment 292911 [details]
Patch

Clearing flags on attachment: 292911

Committed r208101: <http://trac.webkit.org/changeset/208101>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

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