Bug 216266

Summary: XHR.timeout is affected by long tasks
Product: WebKit Reporter: Samuel <samuel.elg>
Component: Page LoadingAssignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, ap, beidson, cdumez, clopez, ews-watchlist, japhet, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: Safari 13   
Hardware: Mac   
OS: macOS 10.15   
See Also: https://github.com/web-platform-tests/wpt/pull/25561
https://bugs.webkit.org/show_bug.cgi?id=216961
Bug Depends on: 221906    
Bug Blocks:    
Attachments:
Description Flags
Files to easily reproduce the bug
none
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch none

Description Samuel 2020-09-08 01:18:07 PDT
Created attachment 408218 [details]
Files to easily reproduce the bug

Steps to reproduce:

A zip file with the front and back-end needed to reproduce this issue is attached.

Open the zip file, and serve that folder (in mac you can run "http-serve") and visit the index.html file.
The folder has a "server.js" file. Run it with "node server.js."
Refresh multiple times, and you will see that sometimes the timeout callback is called, even though it shouldn't (console will log "timeout!").
What the script and server reproduce:

The timeout is set to 1000ms.
The request is made.
A blocking script is running for 2000ms (a while loop).
The server responds in 100ms (concurrently to step 3).
The timeout callback is called, and the request is canceled.
Actual results:

The request is canceled and the timeout callback is called.

Expected results:

The request should not be canceled (Chrome behaves correctly, Firefox has the same issue).
It seems like the timer counting the time of the request is running on the main thread and therefore affected by long tasks, this should not be the case.
Comment 1 Radar WebKit Bug Importer 2020-09-15 01:19:15 PDT
<rdar://problem/68908150>
Comment 2 youenn fablet 2020-09-16 01:07:10 PDT
Created attachment 408903 [details]
Patch
Comment 3 EWS Watchlist 2020-09-16 01:07:57 PDT
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess
Comment 4 youenn fablet 2020-09-16 01:13:30 PDT
Submitted web-platform-tests pull request: https://github.com/web-platform-tests/wpt/pull/25561
Comment 5 youenn fablet 2020-09-16 03:20:56 PDT
Created attachment 408908 [details]
Patch
Comment 6 youenn fablet 2020-09-16 05:05:57 PDT
Created attachment 408913 [details]
Patch
Comment 7 youenn fablet 2020-09-16 07:09:30 PDT
Created attachment 408918 [details]
Patch
Comment 8 Alex Christensen 2020-09-17 11:14:44 PDT
Comment on attachment 408918 [details]
Patch

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

> Source/WebCore/loader/ThreadableLoader.h:79
> +        virtual void computeIsDone() = 0;

I think it would be much better to have a CompletionHandler<void(bool)>&& here than introduce a new function on the client that this will call as a callback.

> Source/WebCore/loader/WorkerThreadableLoader.h:63
> +        void notifyIsDone(bool isDone);

ditto
Comment 9 youenn fablet 2020-09-17 11:54:54 PDT
(In reply to Alex Christensen from comment #8)
> Comment on attachment 408918 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=408918&action=review
> 
> > Source/WebCore/loader/ThreadableLoader.h:79
> > +        virtual void computeIsDone() = 0;
> 
> I think it would be much better to have a CompletionHandler<void(bool)>&&
> here than introduce a new function on the client that this will call as a
> callback.
> 
> > Source/WebCore/loader/WorkerThreadableLoader.h:63
> > +        void notifyIsDone(bool isDone);
> 
> ditto

I initially tried completion handlers within WorkerThreadableLoader that but this does not work well given ThreadableLoader/ThreadableLoaderClientWrapper model.
The alternative is to do like other things I did in the past and keep a map of completion handlers tied to the ScriptExecutionContext.

I chose to keep the existing model but could be convinced to go with the completion handler map.
Comment 10 EWS 2020-09-18 01:15:37 PDT
Committed r267227: <https://trac.webkit.org/changeset/267227>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 408918 [details].