Bug 175628 - Cross origin Beacon requests with a ArrayBuffer / ArrayBufferView payload should not do a CORS preflight
Summary: Cross origin Beacon requests with a ArrayBuffer / ArrayBufferView payload sho...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on: 175679
Blocks:
  Show dependency treegraph
 
Reported: 2017-08-16 08:58 PDT by Chris Dumez
Modified: 2017-08-17 12:16 PDT (History)
7 users (show)

See Also:


Attachments
Patch (59.17 KB, patch)
2017-08-16 14:29 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2017-08-16 08:58:51 PDT
Cross origin Beacon requests with a ArrayBuffer / ArrayBufferView payload should not do a CORS preflight.
Firefox and Chrome do not require a CORS preflight in this case. The reason it is not needed is because the request does not have a "Content-Type" header as per the specification.
Comment 1 Radar WebKit Bug Importer 2017-08-16 08:59:31 PDT
<rdar://problem/33919278>
Comment 2 Chris Dumez 2017-08-16 14:29:15 PDT
Created attachment 318290 [details]
Patch
Comment 3 Geoffrey Garen 2017-08-16 16:37:45 PDT
Comment on attachment 318290 [details]
Patch

r=me
Comment 4 youenn fablet 2017-08-16 17:17:38 PDT
Comment on attachment 318290 [details]
Patch

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

> Source/WebCore/loader/cache/CachedResourceRequest.cpp:44
> +    , m_originalRequestHeaders(m_resourceRequest.httpHeaderFields())

This field seems unnecessary in CachedResourceRequest, as well as the header map copy for all requests.
Why not using an optional<header-map> in CachedResource and do the copy only for keep alive and/or beacon requests at CachedResourceLoader level?
Comment 5 WebKit Commit Bot 2017-08-16 17:28:29 PDT
Comment on attachment 318290 [details]
Patch

Clearing flags on attachment: 318290

Committed r220817: <http://trac.webkit.org/changeset/220817>
Comment 6 WebKit Commit Bot 2017-08-16 17:28:31 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Chris Dumez 2017-08-17 08:09:08 PDT
(In reply to youenn fablet from comment #4)
> Comment on attachment 318290 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=318290&action=review
> 
> > Source/WebCore/loader/cache/CachedResourceRequest.cpp:44
> > +    , m_originalRequestHeaders(m_resourceRequest.httpHeaderFields())
> 
> This field seems unnecessary in CachedResourceRequest, as well as the header
> map copy for all requests.
> Why not using an optional<header-map> in CachedResource and do the copy only
> for keep alive and/or beacon requests at CachedResourceLoader level?

I hesitated to do this, I was trying to avoid special casing keepalive/beacon too much. I can follow-up to see how it looks.
Comment 8 youenn fablet 2017-08-17 08:38:04 PDT
We should try to remove the header map copy.
DTL is already doing this copy and other resources will not do preflights.

Ideally, this copy should be only visible/manipulated by CachedResourceLoader/CachedResource.
It might be easier to have CachedResourceRequest convey this information between the two but other users of CachedResourceRequest should not be tempted to start using it.
Comment 9 Chris Dumez 2017-08-17 12:16:19 PDT
(In reply to Chris Dumez from comment #7)
> (In reply to youenn fablet from comment #4)
> > Comment on attachment 318290 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=318290&action=review
> > 
> > > Source/WebCore/loader/cache/CachedResourceRequest.cpp:44
> > > +    , m_originalRequestHeaders(m_resourceRequest.httpHeaderFields())
> > 
> > This field seems unnecessary in CachedResourceRequest, as well as the header
> > map copy for all requests.
> > Why not using an optional<header-map> in CachedResource and do the copy only
> > for keep alive and/or beacon requests at CachedResourceLoader level?
> 
> I hesitated to do this, I was trying to avoid special casing
> keepalive/beacon too much. I can follow-up to see how it looks.

https://bugs.webkit.org/show_bug.cgi?id=175679