[Chromium] Implement XHR2 timeout for chromium port
Created attachment 175990 [details] Patch
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.
Created attachment 176187 [details] Patch
(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
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.
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
Created attachment 176189 [details] Patch
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
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.
Mr. Beverloo asked around, and apparently we do want to implement this feature.
This patch appears to have caused WebTestFrame.ChromePageNoJavaScript to hang on the EWS bots.
Created attachment 176615 [details] Patch
Created attachment 176626 [details] Patch
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
Created attachment 176640 [details] Patch
Comment on attachment 176640 [details] Patch I believe you've hung all the EWS bots again.
(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.
(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
Created attachment 177673 [details] webkit_unit_tests log when XHR_TIMEOUT disabled no regression found when apply the patch and disable XHR_TIMEOUT
Created attachment 177674 [details] webkit_unit_tests log when enable XHR_TIMEOUT no regression when applying the patch and set ENABLE_XHR_TIMEOUT=1
Created attachment 177675 [details] layout test result when xhr_timeout enabled no regression when xhr_timeout enabled.
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
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.
Created attachment 177677 [details] Patch
Comment on attachment 177677 [details] Patch no review needed
Comment on attachment 177677 [details] Patch Marking r- to avoid breaking the EWS bots.
Created attachment 177679 [details] no review. test the patch on EWS
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
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 ]
Created attachment 177701 [details] Patch for review
Comment on attachment 177701 [details] Patch for review Adam, all bots green. could you help to review the patch? thanks
Yeah, inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html is flaky.
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.
Comment on attachment 177701 [details] Patch for review I suspect we should implement this in the network stack.
(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.
(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.
https://groups.google.com/a/chromium.org/forum/?fromgroups=#!topic/chromium-dev/YidnX-WUwDI
Does that mean we don't support timeout for sync XHR? That seems like when it would be the most useful...
(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.
(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.
(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
Was this marked WONTFIX because of the Blink forking? If so, can someone point me to an equivalent Blink bug? I'm quite interested.
(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?
Looks like it's also https://code.google.com/p/chromium/issues/detail?id=160077
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.
(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.