Bug 101731 – [Qt] Support ResourceRequest's setTimeoutInterval

• Component: Page Loading
• Status: RESOLVED FIXED

[Allan Sandfeld Jensen]

The Qt port currently does not support timeoutInterval on ResourceRequest, for more information see bug #98397 and bug #74802

[Allan Sandfeld Jensen]

Created attachment 173246 [details]
Patch

[Jocelyn Turcotte]

Comment on attachment 173246 [details]
Patch

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

> Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:43
> +static const int gTimeoutError = -1001;

I'm not sure if that's necessary, XMLHttpRequest::didFail relies on ResourceError::isTimeout(), and this error code is converted only in ResourceErrorMac.mm which we don't use.
I'm also wondering if we should setIsTimeout(true) for QNetworkReply::TimeoutError errors in QNetworkReplyHandler::errorForReply. That's why this value seemed to be necessary on the Mac port.

> Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:357
> +        m_timeoutTimer.setSingleShot(true);
> +        m_timeoutTimer.start(ms);

Should we support the note there? http://www.w3.org/TR/XMLHttpRequest/#dom-xmlhttprequest-timeout
"Note: This implies that the timeout attribute can be set while fetching is in progress. If that occurs it will still be measured relative to the start of fetching."

> Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:368
> +void QNetworkReplyWrapper::didTimeout()
> +{
> +    QueueLocker lock(m_queue);
> +    m_timeoutError = true;
> +    resetConnections();
> +    setFinished();
> +    m_queue->push(&QNetworkReplyHandler::finish);
> +}

Why not put the timer in QNetworkReplyWrapper instead?

[Allan Sandfeld Jensen]

(In reply to comment #2)
> > Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:357
> > +        m_timeoutTimer.setSingleShot(true);
> > +        m_timeoutTimer.start(ms);
> 
> Should we support the note there? http://www.w3.org/TR/XMLHttpRequest/#dom-xmlhttprequest-timeout
> "Note: This implies that the timeout attribute can be set while fetching is in progress. If that occurs it will still be measured relative to the start of fetching."
> 
We currently do not support updating a ResourceRequest once it has been started. I think this would be implemented in ResourceRequest::doUpdatePlatformRequest, but we have an empty implementation of that. Also currently I don't think any code calls doUpdatePlatformRequest, it seems to be designed to be triggered by other platform code.

[Allan Sandfeld Jensen]

Created attachment 173266 [details]
Patch

Moved timer to QNetworkReplyHandler, and made the error more Qt standard, though it is not really used anywhere

[Allan Sandfeld Jensen]

Created attachment 173267 [details]
Patch

Removed left-over line in header

[Allan Sandfeld Jensen]

(In reply to comment #3)
> (In reply to comment #2)
> > > Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:357
> > > +        m_timeoutTimer.setSingleShot(true);
> > > +        m_timeoutTimer.start(ms);
> > 
> > Should we support the note there? http://www.w3.org/TR/XMLHttpRequest/#dom-xmlhttprequest-timeout
> > "Note: This implies that the timeout attribute can be set while fetching is in progress. If that occurs it will still be measured relative to the start of fetching."
> > 
> We currently do not support updating a ResourceRequest once it has been started. I think this would be implemented in ResourceRequest::doUpdatePlatformRequest, but we have an empty implementation of that. Also currently I don't think any code calls doUpdatePlatformRequest, it seems to be designed to be triggered by other platform code.

Btw, this is not currently possible anyway in XHR, see bug #98156

[Jocelyn Turcotte]

Comment on attachment 173267 [details]
Patch

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

> Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:479
> +    , m_timeoutTimer(this)

I think it's better not to set the parent for inline members.

> Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:575
> +    if (m_replyWrapper->isFinished())

Just in case somebody forgets a m_timeoutTimer.stop() some time in the future, I would remove m_replyWrapper != 0 from the assert above, move the assert after the following return and also return early here if m_replyWrapper == 0.
It's easy to forget and alsmost impossible to test every code path.

> Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:584
> +    ResourceError timeoutError("QtNetwork", QNetworkReply::TimeoutError, m_replyWrapper->reply()->url().toString(), "Request timed out");

I think the QNetworkReply::TimeoutError adds noise here, future readers might believe that it's useful.

> Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:814
> +    double timeoutInSeconds = d->m_firstRequest.timeoutInterval();
> +    if (timeoutInSeconds > 0 && timeoutInSeconds < (INT_MAX / 1000))
> +        m_timeoutTimer.start(timeoutInSeconds * 1000);

Just in case sendNetworkRequest completes the request synchronously (local files or resources in cache), it might be better to start the timer before so that finish() doesn't call stop before it's started.

[Allan Sandfeld Jensen]

(In reply to comment #7)
> (From update of attachment 173267 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=173267&action=review
> 
> > Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:479
> > +    , m_timeoutTimer(this)
> 
> I think it's better not to set the parent for inline members.
> 
Right.

> > Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:575
> > +    if (m_replyWrapper->isFinished())
> 
> Just in case somebody forgets a m_timeoutTimer.stop() some time in the future, I would remove m_replyWrapper != 0 from the assert above, move the assert after the following return and also return early here if m_replyWrapper == 0.
> It's easy to forget and alsmost impossible to test every code path.
> 
I think that it is easy to forget is a good reason to keep it as an assert (so that debug bots turn red), but you right, it would be good to have a safe return path for release builds as well. I will see if I can make it more error-safe.

> > Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:814
> > +    double timeoutInSeconds = d->m_firstRequest.timeoutInterval();
> > +    if (timeoutInSeconds > 0 && timeoutInSeconds < (INT_MAX / 1000))
> > +        m_timeoutTimer.start(timeoutInSeconds * 1000);
> 
> Just in case sendNetworkRequest completes the request synchronously (local files or resources in cache), it might be better to start the timer before so that finish() doesn't call stop before it's started.

A good thing about having it in start() is that start() is not called directly but inserted into the queue from QNetworkReplyWrapper and so are finish() calls, which should guarantee us that finish() will execute after start() . We could have a problem with abort() though, because abort() is called directly, I will need to take a second look at that case.

[Dominik Röttsches (drott)]

Nice to see another port supporting the feature!

[Allan Sandfeld Jensen]

Created attachment 173594 [details]
Patch

Protect timeout() better against misuse of API, and drop incorrect Timer initialization

[Simon Hausmann]

Comment on attachment 173594 [details]
Patch

r=me, but the timer should use QBasicTimer. Please fix before landing :)

[Allan Sandfeld Jensen]

Committed r134243: <http://trac.webkit.org/changeset/134243>

[Csaba Osztrogonác]

(In reply to comment #12)
> Committed r134243: <http://trac.webkit.org/changeset/134243>

These http/tests/xmlhttprequest/timeout/* tests are very very very sloooow. :-/ These tests made layout testing time from 20 mins to 24 mins ... It is nonsense if only a couple of tests takes the 17% of the runtime of 30.000 tests.

Any idea how can we fix them?

[Allan Sandfeld Jensen]

(In reply to comment #13)
> (In reply to comment #12)
> > Committed r134243: <http://trac.webkit.org/changeset/134243>
> 
> These http/tests/xmlhttprequest/timeout/* tests are very very very sloooow. :-/ These tests made layout testing time from 20 mins to 24 mins ... It is nonsense if only a couple of tests takes the 17% of the runtime of 30.000 tests.
> 
> Any idea how can we fix them?

Well, there is no reason the timeouts have to be 2 or 5 seconds. The files are local, so perhaps dividing the timeouts by 10 would work?

[Dominik Röttsches (drott)]

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > Committed r134243: <http://trac.webkit.org/changeset/134243>
> > 
> > These http/tests/xmlhttprequest/timeout/* tests are very very very sloooow. :-/ These tests made layout testing time from 20 mins to 24 mins ... It is nonsense if only a couple of tests takes the 17% of the runtime of 30.000 tests.
> > 
> > Any idea how can we fix them?
> 
> Well, there is no reason the timeouts have to be 2 or 5 seconds. The files are local, so perhaps dividing the timeouts by 10 would work?

I can look into tuning those values, bug 102184. Might take a few days though.