Bug 216266 - XHR.timeout is affected by long tasks
Summary: XHR.timeout is affected by long tasks
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: Safari 13
Hardware: Macintosh macOS 10.15
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-09-08 01:18 PDT by Samuel
Modified: 2020-09-24 22:45 PDT (History)
9 users (show)

See Also:


Attachments
Files to easily reproduce the bug (10.92 MB, application/zip)
2020-09-08 01:18 PDT, Samuel
no flags Details
Patch (22.67 KB, patch)
2020-09-16 01:07 PDT, youenn fablet
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (23.67 KB, patch)
2020-09-16 03:20 PDT, youenn fablet
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (23.67 KB, patch)
2020-09-16 05:05 PDT, youenn fablet
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (25.14 KB, patch)
2020-09-16 07:09 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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].