Bug 199492

Summary: XHR CORS requests logged twice in the server
Product: WebKit Reporter: Karl Sutt <karl>
Component: Page LoadingAssignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, beidson, cdumez, cgarcia, commit-queue, ews-watchlist, karl, koivisto, mike, rojasinate, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: Safari 12   
Hardware: Mac   
OS: macOS 10.14   
See Also: https://bugs.webkit.org/show_bug.cgi?id=199644
Attachments:
Description Flags
Patch none

Karl Sutt
Reported 2019-07-04 00:31:56 PDT
Overview: A simple XHR CORS request after a page refresh — either with in `onload` handler on <body> or an `onclick` on a <button> — gets processed twice by the server. This does not seem to happen 100% of the time, sometimes it's only processed once, but after a page refresh it's processed twice. I initially opened a StackOverflow question with a bit more context: https://stackoverflow.com/questions/56877116/flask-logs-duplicate-xhr-cors-requests-from-safari A screen recording of the behaviour: https://d.pr/v/1h7iRZ Steps to reproduce: Clone https://github.com/karls/flask-safari-troubleshoot and follow the instructions in the README. Actual results: After refreshing the page and triggering the XHR request, the server logs (and processes) the request twice, but not 100% of the time. Expected results: After refreshing the page and triggering the XHR request, the server should only process the request once. Software and hardware: MacBook Pro (13-inch, 2018, Four Thunderbolt 3 Ports) OSX 10.14.5 (18F132) Safari 12.1.1 (14607.2.6.1.1) (but also happens on Technology Preview Release 86 (Safari 13.0, WebKit 14608.1.30.2)) Server software is Flask 1.0.3 (Python) Using vanilla XMLHttpRequest to perform the HTTP CORS request.
Attachments
Patch (8.71 KB, patch)
2019-07-08 16:00 PDT, youenn fablet 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Karl Sutt
Comment 1 2019-07-04 06:11:43 PDT
A possible duplicate? Behaviour looks very similar at first glance. https://bugs.webkit.org/show_bug.cgi?id=199183
Radar WebKit Bug Importer
Comment 2 2019-07-07 17:53:07 PDT
<rdar://problem/52757558>
youenn fablet
Comment 3 2019-07-08 13:29:42 PDT
*** Bug 199183 has been marked as a duplicate of this bug. ***
youenn fablet
Comment 4 2019-07-08 13:31:49 PDT
The two XHR requests might be one triggered by speculative loader, and another regular one. That would mean the speculative load is not reused for the regular one.
youenn fablet
Comment 5 2019-07-08 16:00:56 PDT
Created attachment 373676 [details] Patch
Alex Christensen
Comment 6 2019-07-08 16:05:16 PDT
Comment on attachment 373676 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=373676&action=review > Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:281 > + return request.requester() != ResourceRequest::Requester::XHR && request.requester() != ResourceRequest::Requester::Fetch; Wouldn't we ideally want all loads that use speculative revalidation to only request once, not just xhr and fetch?
youenn fablet
Comment 7 2019-07-08 16:12:13 PDT
(In reply to Alex Christensen from comment #6) > Comment on attachment 373676 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=373676&action=review > > > Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:281 > > + return request.requester() != ResourceRequest::Requester::XHR && request.requester() != ResourceRequest::Requester::Fetch; > > Wouldn't we ideally want all loads that use speculative revalidation to only > request once, not just xhr and fetch? I think we want XHR/fetch loads to fully opt-out of speculative revalidation, since some headers might be generated dynamically. For page loads which bypass the HTTP cache, we should also probably disable speculative reload since the sub resources will probably bypass the cache as well. That is another fix I believe.
Chris Dumez
Comment 8 2019-07-09 13:00:19 PDT
Comment on attachment 373676 [details] Patch This could potentially regress warm PLT but let's try it. The alternative would be to only do speculative revalidation if the request headers were identical the last 2 times the resource was loaded.
WebKit Commit Bot
Comment 9 2019-07-09 13:44:14 PDT
Comment on attachment 373676 [details] Patch Clearing flags on attachment: 373676 Committed r247276: <https://trac.webkit.org/changeset/247276>
WebKit Commit Bot
Comment 10 2019-07-09 13:44:16 PDT
All reviewed patches have been landed. Closing bug.
youenn fablet
Comment 11 2019-07-09 15:25:51 PDT
(In reply to youenn fablet from comment #7) > (In reply to Alex Christensen from comment #6) > > Comment on attachment 373676 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=373676&action=review > > > > > Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:281 > > > + return request.requester() != ResourceRequest::Requester::XHR && request.requester() != ResourceRequest::Requester::Fetch; > > > > Wouldn't we ideally want all loads that use speculative revalidation to only > > request once, not just xhr and fetch? > > I think we want XHR/fetch loads to fully opt-out of speculative > revalidation, since some headers might be generated dynamically. > > For page loads which bypass the HTTP cache, we should also probably disable > speculative reload since the sub resources will probably bypass the cache as > well. > That is another fix I believe. (In reply to youenn fablet from comment #7) > (In reply to Alex Christensen from comment #6) > > Comment on attachment 373676 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=373676&action=review > > > > > Source/WebKit/NetworkProcess/cache/NetworkCache.cpp:281 > > > + return request.requester() != ResourceRequest::Requester::XHR && request.requester() != ResourceRequest::Requester::Fetch; > > > > Wouldn't we ideally want all loads that use speculative revalidation to only > > request once, not just xhr and fetch? > > I think we want XHR/fetch loads to fully opt-out of speculative > revalidation, since some headers might be generated dynamically. > > For page loads which bypass the HTTP cache, we should also probably disable > speculative reload since the sub resources will probably bypass the cache as > well. > That is another fix I believe. I filed https://bugs.webkit.org/show_bug.cgi?id=199644
Note You need to log in before you can comment on or make changes to this bug.