Bug 225596 – Preflight requests not properly attributed as app-bound

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Kate Cheney]

Preflight requests are not properly attributed as app-bound.

[Kate Cheney]

<rdar://problem/77664272>

[Kate Cheney]

Created attachment 428173 [details]
Patch

[Alex Christensen]

Comment on attachment 428173 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:410
> +- (void)_appBoundLoadData:(BOOL)isRequestingLoadsMarkedAppBound completionHandler:(void (^)(BOOL result))completionHandler WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));

This should be two functions instead of one function with a bool indicating which function it is calling.

[Brent Fulgham]

Comment on attachment 428173 [details]
Patch

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

> Source/WebCore/loader/CrossOriginAccessControl.cpp:92
> +    preflightRequest.setIsAppBound(request.isAppBound());

I worry that other flags in the request might be missed if we rely on manually copying them over. We should take this action today, but I think we should consider a more general way to spawn a preflight request from an existing request to retain relevant state.

>> Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:410
>> +- (void)_appBoundLoadData:(BOOL)isRequestingLoadsMarkedAppBound completionHandler:(void (^)(BOOL result))completionHandler WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));
> 
> This should be two functions instead of one function with a bool indicating which function it is calling.

Do we need to add a separate method if these are purely for testing purposes? Passing a parameter means we could parameterize the test so the same code could be used for both flavors of test.

[Kate Cheney]

(In reply to Brent Fulgham from comment #4)
> Comment on attachment 428173 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=428173&action=review
> 
> > Source/WebCore/loader/CrossOriginAccessControl.cpp:92
> > +    preflightRequest.setIsAppBound(request.isAppBound());
> 
> I worry that other flags in the request might be missed if we rely on
> manually copying them over. We should take this action today, but I think we
> should consider a more general way to spawn a preflight request from an
> existing request to retain relevant state.

Good point, I'm not exactly sure what state matters here. Marking app-bound requests is important for all network connections, but it's possible that other state isn't important for special requests like preflight checks that just check the CORS policy.

> 
> >> Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:410
> >> +- (void)_appBoundLoadData:(BOOL)isRequestingLoadsMarkedAppBound completionHandler:(void (^)(BOOL result))completionHandler WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));
> > 
> > This should be two functions instead of one function with a bool indicating which function it is calling.
> 
> Do we need to add a separate method if these are purely for testing
> purposes? Passing a parameter means we could parameterize the test so the
> same code could be used for both flavors of test.

I see Alex's point, the test function reads weirdly with a boolean. I'm not opposed to splitting it up, but it's unfortunate that it adds so much more boilerplate WebKitTestRunner code.

[Kate Cheney]

Created attachment 428190 [details]
Patch

[Brent Fulgham]

Comment on attachment 428190 [details]
Patch

Looks good. r=me

[EWS]

Committed r277301 (237562@main): <https://commits.webkit.org/237562@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 428190 [details].