Bug 103251 – [Chromium] Implement XHR2 timeout for chromium port

• Component: New Bugs
• Status: RESOLVED WONTFIX

[Wei James (wistoch)]

[Chromium] Implement XHR2 timeout for chromium port

[Wei James (wistoch)]

Created attachment 175990 [details]
Patch

[Adam Barth]

Comment on attachment 175990 [details]
Patch

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

> Source/WebCore/platform/network/chromium/ResourceHandle.cpp:82
> +    m_owner->ref();

Please don't call ref and deref manually.

[Wei James (wistoch)]

Created attachment 176187 [details]
Patch

[Wei James (wistoch)]

(In reply to comment #2)
> (From update of attachment 175990 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=175990&action=review
> 
> > Source/WebCore/platform/network/chromium/ResourceHandle.cpp:82
> > +    m_owner->ref();
> 
> Please don't call ref and deref manually.

fixed. thanks

[Adam Barth]

Comment on attachment 176187 [details]
Patch

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

It's not clear to me what we want to implement this feature in Chromium.

> Source/WebCore/platform/network/chromium/ResourceHandle.cpp:180
> +    // Use the same value as in NSURLError.h
> +    static const int timeoutError = -1001;
> +    static const char* const errorDomain = "WebKitNetworkError";

Why should Chromium code use values based on the Mac OS X APIs?

> Source/WebKit/chromium/features.gypi:127
> -      'ENABLE_XHR_TIMEOUT=0',
> +      'ENABLE_XHR_TIMEOUT=1',

Please don't flip this bit unless this feature has been approved for inclusion in the Chromium port.

[Wei James (wistoch)]

Comment on attachment 176187 [details]
Patch

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

it is to fix crbug.com/157421 that chromium doesn't support XMLHttpRequest Level 2 timeout property. 
http://dvcs.w3.org/hg/xhr/raw-file/tip/Overview.html#the-timeout-attribute

When the timeout of XHR is set, chromium should cancel the request when that period of time passed and ontimeout callback of request will be called.

>> Source/WebCore/platform/network/chromium/ResourceHandle.cpp:180
>> +    static const char* const errorDomain = "WebKitNetworkError";
> 
> Why should Chromium code use values based on the Mac OS X APIs?

it is to be consistent with the implementation in soup backend. 
https://bugs.webkit.org/show_bug.cgi?id=94796

do you have any better proposal? thanks

>> Source/WebKit/chromium/features.gypi:127
>> +      'ENABLE_XHR_TIMEOUT=1',
> 
> Please don't flip this bit unless this feature has been approved for inclusion in the Chromium port.

fixed. thanks

[Wei James (wistoch)]

Created attachment 176189 [details]
Patch

[Allan Sandfeld Jensen]

Comment on attachment 176189 [details]
Patch

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

> Source/WebCore/platform/network/chromium/ResourceHandle.cpp:179
> +    // Use the same value as in NSURLError.h
> +    static const int timeoutError = -1001;

Note this error code is not used or even accessed anywhere. Be careful not to make its actual value seem too important.

> LayoutTests/platform/chromium/TestExpectations:1013
> +webkit.org/b/98397 http/tests/xmlhttprequest/timeout/xmlhttprequest-timeout-aborted.html [ Pass Slow ]
> +webkit.org/b/98397 http/tests/xmlhttprequest/timeout/xmlhttprequest-timeout-abortedonmain.html [ Pass Slow ]
> +webkit.org/b/98397 http/tests/xmlhttprequest/timeout/xmlhttprequest-timeout-overridesexpires.html [ Pass Slow ]
> +webkit.org/b/98397 http/tests/xmlhttprequest/timeout/xmlhttprequest-timeout-simple.html [ Pass Slow ]
> +webkit.org/b/98397 http/tests/xmlhttprequest/timeout/xmlhttprequest-timeout-synconmain.html [ Pass Slow ]
> +webkit.org/b/98397 http/tests/xmlhttprequest/timeout/xmlhttprequest-timeout-twice.html [ Pass Slow ]
> +webkit.org/b/98397 http/tests/xmlhttprequest/timeout/xmlhttprequest-timeout-worker-aborted.html [ Pass Slow ]
> +webkit.org/b/98397 http/tests/xmlhttprequest/timeout/xmlhttprequest-timeout-worker-overridesexpires.html [ Pass Slow ]
> +webkit.org/b/98397 http/tests/xmlhttprequest/timeout/xmlhttprequest-timeout-worker-simple.html [ Pass Slow ]
> +webkit.org/b/98397 http/tests/xmlhttprequest/timeout/xmlhttprequest-timeout-worker-synconworker.html [ Pass Slow ]
> +webkit.org/b/98397 http/tests/xmlhttprequest/timeout/xmlhttprequest-timeout-worker-twice.html [ Pass Slow ]

Slow might not be needed soon. See bug #102184

[Adam Barth]

Comment on attachment 176189 [details]
Patch

Just because we have a bug open doesn't mean we actually want to implement this feature.  I don't see any evidence that Chromium wants to implement this feature.

[Adam Barth]

Mr. Beverloo asked around, and apparently we do want to implement this feature.

[Adam Barth]

This patch appears to have caused WebTestFrame.ChromePageNoJavaScript to hang on the EWS bots.

[Wei James (wistoch)]

Created attachment 176615 [details]
Patch

[Wei James (wistoch)]

Created attachment 176626 [details]
Patch

[WebKit Review Bot]

Comment on attachment 176626 [details]
Patch

Attachment 176626 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15025576

New failing tests:
http/tests/xmlhttprequest/timeout/xmlhttprequest-timeout-worker-overridesexpires.html
inspector-protocol/debugger-terminate-dedicated-worker-while-paused.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-overridesexpires.html
http/tests/xmlhttprequest/timeout/xmlhttprequest-timeout-synconmain.html
http/tests/xmlhttprequest/timeout/xmlhttprequest-timeout-simple.html
http/tests/xmlhttprequest/timeout/xmlhttprequest-timeout-worker-twice.html
http/tests/xmlhttprequest/timeout/xmlhttprequest-timeout-worker-synconworker.html

[Wei James (wistoch)]

Created attachment 176640 [details]
Patch

[Adam Barth]

Comment on attachment 176640 [details]
Patch

I believe you've hung all the EWS bots again.

[Adam Barth]

(In reply to comment #11)
> This patch appears to have caused WebTestFrame.ChromePageNoJavaScript to hang on the EWS bots.

Obviously it shouldn't be possible for a patch to hang the bots, but please consider the WebTestFrame.ChromePageNoJavaScript issue again before uploading a new copy of your patch to the EWS bots.

[Wei James (wistoch)]

(In reply to comment #17)
> (In reply to comment #11)
> > This patch appears to have caused WebTestFrame.ChromePageNoJavaScript to hang on the EWS bots.
> 
> Obviously it shouldn't be possible for a patch to hang the bots, but please consider the WebTestFrame.ChromePageNoJavaScript issue again before uploading a new copy of your patch to the EWS bots.

thanks and sorry for hanging the bots. 

I checked the log of the EWS and found WebTestFrame.ChromePageNoJavaScript passed. 

[ RUN      ] WebFrameTest.ChromePageNoJavascript
[       OK ] WebFrameTest.ChromePageNoJavascript (13 ms)

and found some Layout test regression: 
Regressions: Unexpected timeouts (30)
  css2.1/t140201-c534-bgreps-05-c-ag.html [ Timeout ]
  css2.1/t170602-bdr-conflct-w-07-d.html [ Timeout ]
  css2.1/t170602-bdr-conflct-w-09-d.html [ Timeout ]
  css2.1/t170602-bdr-conflct-w-11-d.html [ Timeout ]
  css2.1/t170602-bdr-conflct-w-13-d.html [ Timeout ]
  css2.1/t170602-bdr-conflct-w-15-d.html [ Timeout ]
  css2.1/t170602-bdr-conflct-w-17-d.html [ Timeout ]
  css2.1/t170602-bdr-conflct-w-19-d.html [ Timeout ]
  css2.1/t170602-bdr-conflct-w-21-d.html [ Timeout ]
  css2.1/t170602-bdr-conflct-w-23-d.html [ Timeout ]
  css2.1/t170602-bdr-conflct-w-25-d.html [ Timeout ]
  css2.1/t170602-bdr-conflct-w-38-d.html [ Timeout ]
  css2.1/t170602-bdr-conflct-w-40-d.html [ Timeout ]
  css2.1/t170602-bdr-conflct-w-42-d.html [ Timeout ]
  css2.1/t170602-bdr-conflct-w-44-d.html [ Timeout ]

I tested locally with webkit_unit_test and WebFrameTest.ChromePageNoJavascript also passed but failed in other places. 

and I ran layout test locally and also didn't observe timeout like that in EWS. 

I asked in IRC whether webkit provides some infrustructure to test patch before submitting it to bugzilla just like trybot in chromium.org and got response that just submitting the patch to EWS with --no-review. So I did so. Obviously it is not a good method to test patch. 

do you have any suggestion? thanks

[Wei James (wistoch)]

Created attachment 177673 [details]
webkit_unit_tests log when XHR_TIMEOUT disabled 

no regression found when apply the patch and disable XHR_TIMEOUT

[Wei James (wistoch)]

Created attachment 177674 [details]
webkit_unit_tests log when enable XHR_TIMEOUT

no regression when applying the patch and set ENABLE_XHR_TIMEOUT=1

[Wei James (wistoch)]

Created attachment 177675 [details]
layout test result when xhr_timeout enabled

no regression when xhr_timeout enabled.

[Wei James (wistoch)]

Created attachment 177676 [details]
layout test result when XHR_TIMEOUT disabled and skip the xhr timeout test cases

no regression when ENABLE_XHR_TIMEOUT=0 and skip xhr timeout test cases

[Wei James (wistoch)]

I set up a clean lucid 64bit chroot and build it with clean chromium/webkit source code. 

no regression found for both webkit_unit_tests and layout test with the patch. 
Both tests are checked for ENABLE_XHR_TIMEOUT=0 and ENABLE_XHR_TIMEOUT=1. 

So i would like to upload the patch to EWS for testing again.

[Wei James (wistoch)]

Created attachment 177677 [details]
Patch

[Wei James (wistoch)]

Comment on attachment 177677 [details]
Patch

no review needed

[Adam Barth]

Comment on attachment 177677 [details]
Patch

Marking r- to avoid breaking the EWS bots.

[Wei James (wistoch)]

Created attachment 177679 [details]
no review. test the patch on EWS

[WebKit Review Bot]

Comment on attachment 177679 [details]
no review. test the patch on EWS

Attachment 177679 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15154301

New failing tests:
inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html

[Wei James (wistoch)]

I saw the same regression in the previous patch in the queue: 
while I cannot access that bug entry. 

Patch 177679 from bug 103251:
5 minutes ago (gce-cr-linux-08) Fail
5 minutes ago (gce-cr-linux-08) Unable to pass tests without patch (tree is red?) [results]
20 minutes ago (gce-cr-linux-08) Patch does not pass tests [results]
34 minutes ago (gce-cr-linux-08) Patch does not pass tests [results]

Patch 177573 from bug 104007:
57 minutes ago (gce-cr-linux-08) Patch does not pass tests [results]
1 hour, 10 minutes ago (gce-cr-linux-08) Patch does not pass tests [results]
2 hours, 1 minute ago (gce-cr-linux-08) Starting Queue
2 hours, 1 minute ago (gce-cr-linux-08) Stopping Queue, reason: Delegate terminated queue.


http://queues.webkit.org/results/15152303 
Regressions: Unexpected image and text failures (4)
  fast/text/hyphenate-character.html [ Failure ]
  fast/text/hyphenate-first-word.html [ Failure ]
  fast/text/hyphenate-locale.html [ Failure ]
  fast/text/hyphens.html [ Failure ]

Regressions: Unexpected image-only failures (1)
  svg/custom/use-disappears-after-style-update.svg [ ImageOnlyFailure ]

Regressions: Unexpected crashes (1)
  inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html [ Crash ]

[Wei James (wistoch)]

Created attachment 177701 [details]
Patch for review

[Wei James (wistoch)]

Comment on attachment 177701 [details]
Patch for review

Adam, all bots green. 

could you help to review the patch? thanks

[Adam Barth]

Yeah, inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html is flaky.

[Adam Barth]

Comment on attachment 177701 [details]
Patch for review

Why are we implementing this timeout in WebCore rather than in the network stack?  Does this work correctly for synchronous requests?  I would expect there to be some negative interactions between synchronous requests and timers.

[Adam Barth]

Comment on attachment 177701 [details]
Patch for review

I suspect we should implement this in the network stack.

[Wei James (wistoch)]

(In reply to comment #34)
> (From update of attachment 177701 [details])
> I suspect we should implement this in the network stack.

there was the same discussion when implementing XHR2 timeout with soup in GTK and EFL port and at last they decided to implement it in webkit. 

https://bugzilla.gnome.org/show_bug.cgi?id=682804 

some feedback from the developer of libsoup:

Dan Winship [libsoup developer] 2012-08-28 13:12:24 UTC
so, note that this would essentially boil down to g_timeout_add() +
soup_session_cancel_message(), and there's no reason webkit couldn't just do
that itself instead...

Dan Winship [libsoup developer] 2012-08-30 12:27:00 UTC
I guess it boils down to whether or not other apps are likely to need
per-message as opposed to per-session timeouts. And that doesn't seem like a
very common use case. So maybe it's better to just do it in WebKit.

I will discuss with chromium guys whether it makes more sense to add timeout feature in chromium http network stack.

[Wei James (wistoch)]

(In reply to comment #33)
> (From update of attachment 177701 [details])
> Why are we implementing this timeout in WebCore rather than in the network stack?  Does this work correctly for synchronous requests?  I would expect there to be some negative interactions between synchronous requests and timers.

the sync and async requests run into different code path in chromium ResourceHandle.cpp, I didn't touch and change the sync one and only add Timer to the async one. So it will not impact the sync requests.

[Wei James (wistoch)]

https://groups.google.com/a/chromium.org/forum/?fromgroups=#!topic/chromium-dev/YidnX-WUwDI

[Adam Barth]

Does that mean we don't support timeout for sync XHR?  That seems like when it would be the most useful...

[Wei James (wistoch)]

(In reply to comment #38)
> Does that mean we don't support timeout for sync XHR?  That seems like when it would be the most useful...

from the spec, XHR2 doesn't support sync timeout:
 http://www.w3.org/TR/XMLHttpRequest/#the-timeout-attribute 

client . timeout
The amount of milliseconds a request can take before being terminated. Initially zero. Zero means there is no timeout.

When set: throws an "InvalidAccessError" exception if the synchronous flag is set when there is an associated XMLHttpRequest document.

[Dominik Röttsches (drott)]

(In reply to comment #38)
> Does that mean we don't support timeout for sync XHR?  That seems like when it would be the most useful...

It is supported, but only for worker contexts. As discussed elsewhere, the intention here was to support any new features only as a reward for not using sync XHR anymore.

[Wei James (wistoch)]

(In reply to comment #34)
> (From update of attachment 177701 [details])
> I suspect we should implement this in the network stack.

adam, 

from willchan's comments in chromium-dev, it should not be implemented in chromium network stack. 

and darin suggested "For Chromium, I would probably implement that in chromium/ResourceHandle.cpp."

I think it should be the solution I used in the submitted patch. 

Is this solution ok for the async XHR timeout feature for chromium? 

thanks

[Alex Vincent]

Was this marked WONTFIX because of the Blink forking?  If so, can someone point me to an equivalent Blink bug?  I'm quite interested.

[Dominik Röttsches (drott)]

(In reply to comment #42)
> Was this marked WONTFIX because of the Blink forking?  If so, can someone point me to an equivalent Blink bug?  I'm quite interested.

Did you see crbug.com/157421 in comment #6? That's a chromium bug, not a blink bug, I guess, but maybe this gives you the info you need?

[Stephen Chenney]

Looks like it's also

https://code.google.com/p/chromium/issues/detail?id=160077

[Wei James (wistoch)]

This patch was submitted several months ago and got different feedback from chromium-dev. 

if it is valuable for blink, I can rebase and submit the patch to blink for review.

[Stephen Chenney]

(In reply to comment #45)
> This patch was submitted several months ago and got different feedback from chromium-dev. 
> 
> if it is valuable for blink, I can rebase and submit the patch to blink for review.

I believe it would be valuable for Blink. You are certainly welcome to set up a Blink dev environment and bring the patch over.

The easiest way to do that is, once your Blink checkout is set up, run "webkit-patch apply-attachment <id>" with the latest attachment. It might apply if you're lucky, or you can do the merge manually.