Bug 199492 - XHR CORS requests logged twice in the server
Summary: XHR CORS requests logged twice in the server
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: Safari 12
Hardware: Macintosh macOS 10.14
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
: 199183 (view as bug list)
Depends on:
Blocks:
 
Reported: 2019-07-04 00:31 PDT by Karl Sutt
Modified: 2019-07-09 15:25 PDT (History)
12 users (show)

See Also:


Attachments
Patch (8.71 KB, patch)
2019-07-08 16:00 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 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