Bug 197397

Summary: Add SPI to set a list of hosts to which to send custom header fields cross-origin
Product: WebKit Reporter: Alex Christensen <achristensen>
Component: New BugsAssignee: Alex Christensen <achristensen>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, darin, dbates, ews-watchlist, ggaren, japhet, rniwa, ryanhaddad, sroberts, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 197990    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews107 for mac-highsierra-wk2
none
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews215 for win-future
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews214 for win-future
none
Patch
none
Patch
none
Patch
none
Patch none

Description Alex Christensen 2019-04-29 17:48:43 PDT
Add SPI to set a list of hosts to which to send custom header fields cross-origin
Comment 1 Alex Christensen 2019-04-29 17:57:12 PDT
Created attachment 368523 [details]
Patch
Comment 2 Alex Christensen 2019-04-29 21:25:50 PDT
Created attachment 368538 [details]
Patch
Comment 3 Alex Christensen 2019-05-01 13:32:26 PDT
Created attachment 368698 [details]
Patch
Comment 4 Geoffrey Garen 2019-05-01 14:19:40 PDT
Looks like mac-wk2 is crashing on every test.
Comment 5 EWS Watchlist 2019-05-01 14:24:10 PDT
Comment on attachment 368698 [details]
Patch

Attachment 368698 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/12053841

Number of test failures exceeded the failure limit.
Comment 6 EWS Watchlist 2019-05-01 14:24:12 PDT
Created attachment 368711 [details]
Archive of layout-test-results from ews107 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6
Comment 7 Alex Christensen 2019-05-01 15:56:31 PDT
Created attachment 368724 [details]
Patch
Comment 8 Alex Christensen 2019-05-01 17:08:59 PDT
Created attachment 368738 [details]
Patch
Comment 9 Alex Christensen 2019-05-02 09:14:32 PDT
Created attachment 368774 [details]
Patch
Comment 10 Alex Christensen 2019-05-02 10:04:50 PDT
Created attachment 368780 [details]
Patch
Comment 11 EWS Watchlist 2019-05-02 12:10:04 PDT
Comment on attachment 368780 [details]
Patch

Attachment 368780 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12063884

New failing tests:
http/tests/css/filters-on-iframes.html
Comment 12 EWS Watchlist 2019-05-02 12:10:14 PDT
Created attachment 368801 [details]
Archive of layout-test-results from ews215 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews215  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment 13 Darin Adler 2019-05-02 15:00:54 PDT
Comment on attachment 368780 [details]
Patch

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

Started reviewing, but sadly didn't finish. But have a couple comments already

> Source/WebCore/loader/CustomHeaderFields.cpp:48
> +        if (domainOrPattern.length() > 2
> +            && domainOrPattern[0] == '*'
> +            && domainOrPattern[1] == '.'
> +            && domainOrPattern.length() > registrableDomainLength
> +            && url.host().endsWith(StringView(domainOrPattern).substring(1)))
> +            return true;

I think the first three lines of this could be replaced with this:

    if (domainOrPattern.startsWith("*.")

For greater clarity and almost the same efficiency. I don’t understand the registrableDomainLength check and why it’s needed; hope we have test cases that fail if it’s removed.

> Source/WebCore/loader/CustomHeaderFields.h:34
> +struct WEBCORE_EXPORT CustomHeaderFields {

WEBCORE_EXPORT should go before struct, not after it, as a matter of coding style.

> Source/WebKit/Platform/spi/ios/UIKitSPI.h:208
>  typedef enum {
> -    UIFontTraitPlain = 0,
> -    UIFontTraitItalic = 1 << 0,
> -    UIFontTraitBold = 1 << 1,
> -} UIFontTrait;
> -
> -@interface UIFont ()
> -+ (UIFont *)fontWithFamilyName:(NSString *)familyName traits:(UIFontTrait)traits size:(CGFloat)fontSize;
> -- (UIFontTrait)traits;
> -@end
> -
> -typedef enum {
>      UIAllCorners = 0xFF,
>  } UIRectCorners;

No mention of the changes to this file in the change log. I don’t understand the rationale. Please just roll this file back or comment about why it’s being changed.

> Source/WebKit/UIProcess/API/APIWebsitePolicies.cpp:92
> +    customHeaderFields.reserveInitialCapacity(this->customHeaderFields().size() + 1);

Could compute this correctly instead of making it one two big by checking legacyCustomHeaderFields() size in a boolean.
Comment 14 Alex Christensen 2019-05-02 22:36:59 PDT
(In reply to Darin Adler from comment #13)
> Comment on attachment 368780 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=368780&action=review
> 
> Started reviewing, but sadly didn't finish. But have a couple comments
> already
> 
> > Source/WebCore/loader/CustomHeaderFields.cpp:48
> > +        if (domainOrPattern.length() > 2
> > +            && domainOrPattern[0] == '*'
> > +            && domainOrPattern[1] == '.'
> > +            && domainOrPattern.length() > registrableDomainLength
> > +            && url.host().endsWith(StringView(domainOrPattern).substring(1)))
> > +            return true;
> 
> I think the first three lines of this could be replaced with this:
> 
>     if (domainOrPattern.startsWith("*.")
That's a great idea.

> 
> For greater clarity and almost the same efficiency. I don’t understand the
> registrableDomainLength check and why it’s needed; hope we have test cases
> that fail if it’s removed.
The registrableDomainLength check is to prevent someone from adding something like "*.com".  I'll add such a test.

> > Source/WebCore/loader/CustomHeaderFields.h:34
> > +struct WEBCORE_EXPORT CustomHeaderFields {
> 
> WEBCORE_EXPORT should go before struct, not after it, as a matter of coding
> style.
I disagree.  It is needed exactly where it is to make Windows link successfully.  We have 3 existing instances of "struct WEBCORE_EXPORT" and 0 of "WEBCORE_EXPORT struct".  Same with 90 "class WEBCORE_EXPORT" and 0 "WEBCORE_EXPORT class".

> > Source/WebKit/Platform/spi/ios/UIKitSPI.h:208
> >  typedef enum {
> > -    UIFontTraitPlain = 0,
> > -    UIFontTraitItalic = 1 << 0,
> > -    UIFontTraitBold = 1 << 1,
> > -} UIFontTrait;
> > -
> > -@interface UIFont ()
> > -+ (UIFont *)fontWithFamilyName:(NSString *)familyName traits:(UIFontTrait)traits size:(CGFloat)fontSize;
> > -- (UIFontTrait)traits;
> > -@end
> > -
> > -typedef enum {
> >      UIAllCorners = 0xFF,
> >  } UIRectCorners;
> 
> No mention of the changes to this file in the change log. I don’t understand
> the rationale. Please just roll this file back or comment about why it’s
> being changed.
These changes are needed because our code hygiene has been lacking ever since we started compiling multiple cpp files together.  Such strange unrelated-looking build fixes are needed almost every time we add a new file because someone has assumed a file will always be included before this one.  That's what's held me back and caused me to look into https://bugs.webkit.org/show_bug.cgi?id=197534 so the iOS build doesn't have so many unrelated fixes.
Comment 15 Alex Christensen 2019-05-06 15:28:17 PDT
Created attachment 369179 [details]
Patch
Comment 16 Alex Christensen 2019-05-06 15:36:51 PDT
Created attachment 369182 [details]
Patch
Comment 17 Alex Christensen 2019-05-06 15:48:29 PDT
Created attachment 369187 [details]
Patch
Comment 18 EWS Watchlist 2019-05-06 22:54:21 PDT
Comment on attachment 369187 [details]
Patch

Attachment 369187 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12119745

New failing tests:
svg/repaint/remove-border-property-on-root.html
security/contentSecurityPolicy/video-with-file-url-allowed-by-media-src-star.html
Comment 19 EWS Watchlist 2019-05-06 22:54:23 PDT
Created attachment 369238 [details]
Archive of layout-test-results from ews214 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews214  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment 20 Alex Christensen 2019-05-07 12:05:51 PDT
ping review
Comment 21 Geoffrey Garen 2019-05-07 13:54:21 PDT
> > > Source/WebCore/loader/CustomHeaderFields.h:34
> > > +struct WEBCORE_EXPORT CustomHeaderFields {
> > 
> > WEBCORE_EXPORT should go before struct, not after it, as a matter of coding
> > style.
> I disagree.  It is needed exactly where it is to make Windows link
> successfully.  We have 3 existing instances of "struct WEBCORE_EXPORT" and 0
> of "WEBCORE_EXPORT struct".  Same with 90 "class WEBCORE_EXPORT" and 0
> "WEBCORE_EXPORT class".

Seems accurate:

scratch.cc:3:16: warning: attribute 'visibility' is ignored, place it after "struct" to apply attribute to type declaration [-Wignored-attributes]
__attribute__((visibility("default"))) struct S {
Comment 22 Geoffrey Garen 2019-05-07 14:02:17 PDT
Comment on attachment 369187 [details]
Patch

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

r=me

> Source/WebKit/UIProcess/API/Cocoa/_WKCustomHeaderFields.mm:52
> +    NSMutableDictionary<NSString *, NSString *> *dictionary = [[[NSMutableDictionary alloc] initWithCapacity:vector.size()] autorelease];

You can use +dictionaryWithCapacity here.

> Source/WebKit/UIProcess/API/Cocoa/_WKCustomHeaderFields.mm:72
> +    NSMutableArray *array = [[[NSMutableArray alloc] initWithCapacity:domains.size()] autorelease];

+arrayWithCapacity
Comment 23 Alex Christensen 2019-05-07 14:08:24 PDT
Created attachment 369325 [details]
Patch
Comment 24 Alex Christensen 2019-05-07 14:15:10 PDT
Created attachment 369328 [details]
Patch
Comment 25 WebKit Commit Bot 2019-05-07 14:51:50 PDT
Comment on attachment 369328 [details]
Patch

Clearing flags on attachment: 369328

Committed r245038: <https://trac.webkit.org/changeset/245038>
Comment 26 WebKit Commit Bot 2019-05-07 14:51:52 PDT
All reviewed patches have been landed.  Closing bug.
Comment 27 Radar WebKit Bug Importer 2019-05-07 14:54:48 PDT
<rdar://problem/50556833>
Comment 28 Ryan Haddad 2019-05-07 16:44:58 PDT
Reverted r245038 for reason:

Breaks internal builds.

Committed r245044: <https://trac.webkit.org/changeset/245044>
Comment 29 Ryan Haddad 2019-05-07 16:45:22 PDT
(In reply to Ryan Haddad from comment #28)
> Reverted r245038 for reason:
> 
> Breaks internal builds.
> 
> Committed r245044: <https://trac.webkit.org/changeset/245044>
Details in radar.
Comment 30 Alex Christensen 2019-05-15 15:58:56 PDT
Created attachment 370005 [details]
Patch
Comment 31 WebKit Commit Bot 2019-05-16 11:08:37 PDT
Comment on attachment 370005 [details]
Patch

Clearing flags on attachment: 370005

Committed r245401: <https://trac.webkit.org/changeset/245401>
Comment 32 WebKit Commit Bot 2019-05-16 11:08:39 PDT
All reviewed patches have been landed.  Closing bug.
Comment 33 WebKit Commit Bot 2019-05-17 10:27:19 PDT
Re-opened since this is blocked by bug 197990
Comment 34 Shawn Roberts 2019-05-17 11:22:23 PDT
Rolled out in https://trac.webkit.org/changeset/245470/webkit

Causing internal build failures
Comment 35 Alex Christensen 2019-05-17 13:53:03 PDT
(In reply to Shawn Roberts from comment #34)
> Rolled out in https://trac.webkit.org/changeset/245470/webkit
> 
> Causing internal build failures

Could you please put a link to such failures in the radar?  It worked for all internal builds I tested.
Comment 36 Ryan Haddad 2019-05-17 13:58:49 PDT
(In reply to Alex Christensen from comment #35)
> (In reply to Shawn Roberts from comment #34)
> Could you please put a link to such failures in the radar?  It worked for
> all internal builds I tested.
He emailed links to you. I added them to the radar.
Comment 37 Alex Christensen 2019-05-17 14:43:48 PDT
Created attachment 370155 [details]
Patch
Comment 38 Alex Christensen 2019-05-17 14:50:49 PDT
http://trac.webkit.org/r245481