Bug 197397 – Add SPI to set a list of hosts to which to send custom header fields cross-origin

• Component: New Bugs
• Status: RESOLVED FIXED

[Alex Christensen]

Add SPI to set a list of hosts to which to send custom header fields cross-origin

[Alex Christensen]

Created attachment 368523 [details]
Patch

[Alex Christensen]

Created attachment 368538 [details]
Patch

[Alex Christensen]

Created attachment 368698 [details]
Patch

[Geoffrey Garen]

Looks like mac-wk2 is crashing on every test.

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Alex Christensen]

Created attachment 368724 [details]
Patch

[Alex Christensen]

Created attachment 368738 [details]
Patch

[Alex Christensen]

Created attachment 368774 [details]
Patch

[Alex Christensen]

Created attachment 368780 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Darin Adler]

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.

[Alex Christensen]

(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.

[Alex Christensen]

Created attachment 369179 [details]
Patch

[Alex Christensen]

Created attachment 369182 [details]
Patch

[Alex Christensen]

Created attachment 369187 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Alex Christensen]

ping review

[Geoffrey Garen]

> > > 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 {

[Geoffrey Garen]

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

[Alex Christensen]

Created attachment 369325 [details]
Patch

[Alex Christensen]

Created attachment 369328 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 369328 [details]
Patch

Clearing flags on attachment: 369328

Committed r245038: <https://trac.webkit.org/changeset/245038>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/50556833>

[Ryan Haddad]

Reverted r245038 for reason:

Breaks internal builds.

Committed r245044: <https://trac.webkit.org/changeset/245044>

[Ryan Haddad]

(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.

[Alex Christensen]

Created attachment 370005 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 370005 [details]
Patch

Clearing flags on attachment: 370005

Committed r245401: <https://trac.webkit.org/changeset/245401>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 197990

[Shawn Roberts]

Rolled out in https://trac.webkit.org/changeset/245470/webkit

Causing internal build failures

[Alex Christensen]

(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.

[Ryan Haddad]

(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.

[Alex Christensen]

Created attachment 370155 [details]
Patch

[Alex Christensen]

http://trac.webkit.org/r245481