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

Description Karl Sutt 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.
Comment 1 Karl Sutt 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
Comment 2 Radar WebKit Bug Importer 2019-07-07 17:53:07 PDT 
<rdar://problem/52757558>
Comment 3 youenn fablet 2019-07-08 13:29:42 PDT 
*** Bug 199183 has been marked as a duplicate of this bug. ***
Comment 4 youenn fablet 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.
Comment 5 youenn fablet 2019-07-08 16:00:56 PDT 
Created attachment 373676 [details]
Patch
Comment 6 Alex Christensen 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?
Comment 7 youenn fablet 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.
Comment 8 Chris Dumez 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.
Comment 9 WebKit Commit Bot 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>
Comment 10 WebKit Commit Bot 2019-07-09 13:44:16 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 11 youenn fablet 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