Bug 221909 – Add support for non app-bound requests

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

[Kate Cheney]

We should add support for specifying non app-bound requests, mainly populating sub resource loads with this value.

[Kate Cheney]

<rdar://problem/73512988>

[Kate Cheney]

Created attachment 420337 [details]
Patch

[Chris Dumez]

Comment on attachment 420337 [details]
Patch

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

> Source/WebCore/loader/DocumentLoader.cpp:2334
> +    m_requestIsNonAppBound = requestIsNonAppBound;

Can be moved inline in the header.

> Source/WebCore/platform/network/ResourceRequestBase.cpp:773
> +bool ResourceRequestBase::isNonAppBound() const

Can be inline in header.

> Source/WebCore/platform/network/ResourceRequestBase.cpp:778
> +void ResourceRequestBase::setIsNonAppBound(bool isNonAppBound)

ditto.

> Source/WebCore/platform/network/ResourceRequestBase.h:264
> +    bool m_isNonAppBound : 1;

Should probably be moved before the Optional data member and with the other bits for better packing.

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivateForTesting.h:90
> +- (void)_isNonAppBoundRequest:(void(^)(BOOL))completionHandler WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));

Internally this is IOS_FAMILY only, so why is the SPI both macOS too?

[Kate Cheney]

Comment on attachment 420337 [details]
Patch

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

>> Source/WebCore/platform/network/ResourceRequestBase.cpp:773
>> +bool ResourceRequestBase::isNonAppBound() const
> 
> Can be inline in header.

Will fix both of these.

>> Source/WebCore/platform/network/ResourceRequestBase.h:264
>> +    bool m_isNonAppBound : 1;
> 
> Should probably be moved before the Optional data member and with the other bits for better packing.

Ok, I will move this.

>> Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivateForTesting.h:90
>> +- (void)_isNonAppBoundRequest:(void(^)(BOOL))completionHandler WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));
> 
> Internally this is IOS_FAMILY only, so why is the SPI both macOS too?

Mistake, I'll remove it.

[Kate Cheney]

(In reply to katherine_cheney from comment #4)
> Comment on attachment 420337 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=420337&action=review
> 
> >> Source/WebCore/platform/network/ResourceRequestBase.cpp:773
> >> +bool ResourceRequestBase::isNonAppBound() const
> > 
> > Can be inline in header.
> 
> Will fix both of these.
> 

I get this complaint from the style bot: Inline functions should not be annotated with WEBCORE_EXPORT. Remove the macro, or move the inline function definition out-of-line. Moving back to cpp file for this reason.

[Alex Christensen]

Comment on attachment 420337 [details]
Patch

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

>>>> Source/WebCore/platform/network/ResourceRequestBase.cpp:773
>>>> +bool ResourceRequestBase::isNonAppBound() const
>>> 
>>> Can be inline in header.
>> 
>> Will fix both of these.
> 
> I get this complaint from the style bot: Inline functions should not be annotated with WEBCORE_EXPORT. Remove the macro, or move the inline function definition out-of-line. Moving back to cpp file for this reason.

Ignore stylebot in this case.

[Kate Cheney]

Created attachment 420354 [details]
Patch

[Kate Cheney]

(In reply to Alex Christensen from comment #6)
> Comment on attachment 420337 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=420337&action=review
> 
> >>>> Source/WebCore/platform/network/ResourceRequestBase.cpp:773
> >>>> +bool ResourceRequestBase::isNonAppBound() const
> >>> 
> >>> Can be inline in header.
> >> 
> >> Will fix both of these.
> > 
> > I get this complaint from the style bot: Inline functions should not be annotated with WEBCORE_EXPORT. Remove the macro, or move the inline function definition out-of-line. Moving back to cpp file for this reason.
> 
> Ignore stylebot in this case.

I think it may be causing failures on the debug bot now:

ERROR: WebCore has a weak external symbol in it (/Volumes/Data/worker/macOS-Catalina-Debug-Build-EWS/build/WebKitBuild/Debug/WebCore.framework/Versions/A/WebCore)
ERROR: A weak external symbol is generated when a symbol is defined in multiple compilation units and is also marked as being exported from the library.
ERROR: A common cause of weak external symbols is when an inline function is listed in the linker export file.
ERROR: symbol __ZN7WebCore19ResourceRequestBase16setIsNonAppBoundEb
ERROR: symbol __ZNK7WebCore19ResourceRequestBase13isNonAppBoundEv

[Chris Dumez]

Comment on attachment 420354 [details]
Patch

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

> Source/WebCore/loader/DocumentLoader.cpp:2332
> +void DocumentLoader::setRequestIsNonAppBound(bool requestIsNonAppBound)

Please move to header.

> Source/WebCore/loader/DocumentLoader.h:425
> +    WEBCORE_EXPORT void setRequestIsNonAppBound(bool);

Drop WEBCORE_EXPORT once you've inlined in the header.

[Chris Dumez]

(In reply to Alex Christensen from comment #6)
> Comment on attachment 420337 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=420337&action=review
> 
> >>>> Source/WebCore/platform/network/ResourceRequestBase.cpp:773
> >>>> +bool ResourceRequestBase::isNonAppBound() const
> >>> 
> >>> Can be inline in header.
> >> 
> >> Will fix both of these.
> > 
> > I get this complaint from the style bot: Inline functions should not be annotated with WEBCORE_EXPORT. Remove the macro, or move the inline function definition out-of-line. Moving back to cpp file for this reason.
> 
> Ignore stylebot in this case.

I believe the correct fix is to stop the WEBCORE_EXPORT on the inline function in the header, as the style bot suggested.

[Kate Cheney]

(In reply to Chris Dumez from comment #10)
> (In reply to Alex Christensen from comment #6)
> > Comment on attachment 420337 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=420337&action=review
> > 
> > >>>> Source/WebCore/platform/network/ResourceRequestBase.cpp:773
> > >>>> +bool ResourceRequestBase::isNonAppBound() const
> > >>> 
> > >>> Can be inline in header.
> > >> 
> > >> Will fix both of these.
> > > 
> > > I get this complaint from the style bot: Inline functions should not be annotated with WEBCORE_EXPORT. Remove the macro, or move the inline function definition out-of-line. Moving back to cpp file for this reason.
> > 
> > Ignore stylebot in this case.
> 
> I believe the correct fix is to stop the WEBCORE_EXPORT on the inline
> function in the header, as the style bot suggested.

I see. I didn't realize that WEBCORE_EXPORT is not needed if the function is inlined in the header, but that makes sense.

[Kate Cheney]

Created attachment 420375 [details]
Patch

[Chris Dumez]

Comment on attachment 420375 [details]
Patch

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

> Source/WebCore/loader/DocumentLoader.h:424
> +    bool requestIsNonAppBound() const { return m_requestIsNonAppBound; }

I am no familiar with the 'app-bound' feature at all but I am wondering why we have the negative here in the API. Why aren't we using "isAppBound" instead of "isNonAppBound". Seems to be (at least) that it would be more readable.

> Source/WebKit/Shared/NavigatingToAppBoundDomain.h:30
>  enum class NavigatingToAppBoundDomain : bool { Yes, No };

Ugh. Please reverse the No and Yes here so that 0 means No and 1 means Yes.

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewTesting.mm:312
> +- (void)_isNonAppBoundRequest:(void(^)(BOOL))completionHandler

Again, I wish this was called isAppBoundRequest

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewTesting.mm:314
> +#if PLATFORM(IOS_FAMILY)

Seems like this should cover those whole _isNonAppBoundRequest method implementation now that this method is iOS only?

> Source/WebKit/UIProcess/WebPageProxy.h:2901
> +    bool m_requestIsNonAppBound { false };

It is super fishy don't have a flag like this on the WebPageProxy. A page is so far from a request. You can make many requests in a single page.

> Source/WebKit/WebProcess/WebPage/WebPage.h:2237
> +    bool m_requestIsNonAppBound { false };

Again, I don't see how a request being app bound is related to a WebPage since you can make MANY requests in a WebPage.

[Kate Cheney]

Comment on attachment 420375 [details]
Patch

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

>> Source/WebCore/loader/DocumentLoader.h:424
>> +    bool requestIsNonAppBound() const { return m_requestIsNonAppBound; }
> 
> I am no familiar with the 'app-bound' feature at all but I am wondering why we have the negative here in the API. Why aren't we using "isAppBound" instead of "isNonAppBound". Seems to be (at least) that it would be more readable.

Hmm, yes I left it in the negative to match the call in CFNetwork but I am thinking now it would be better to reverse it at those call sites only and then store it as isAppBound in WebKit.

>> Source/WebKit/Shared/NavigatingToAppBoundDomain.h:30
>>  enum class NavigatingToAppBoundDomain : bool { Yes, No };
> 
> Ugh. Please reverse the No and Yes here so that 0 means No and 1 means Yes.

I am sorry for my past self.

>> Source/WebKit/UIProcess/API/Cocoa/WKWebViewTesting.mm:314
>> +#if PLATFORM(IOS_FAMILY)
> 
> Seems like this should cover those whole _isNonAppBoundRequest method implementation now that this method is iOS only?

Right, will fix.

>> Source/WebKit/UIProcess/WebPageProxy.h:2901
>> +    bool m_requestIsNonAppBound { false };
> 
> It is super fishy don't have a flag like this on the WebPageProxy. A page is so far from a request. You can make many requests in a single page.

I use it as a holding place after WebPageProxy::loadRequest is called until it gets sent to the WebPage and stored in the main frame's document loader. Maybe "currentRequestIsNonAppBound"?

>> Source/WebKit/WebProcess/WebPage/WebPage.h:2237
>> +    bool m_requestIsNonAppBound { false };
> 
> Again, I don't see how a request being app bound is related to a WebPage since you can make MANY requests in a WebPage.

Again, maybe "currentRequestIsNonAppBound" instead?

[Kate Cheney]

Comment on attachment 420375 [details]
Patch

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

>>> Source/WebKit/UIProcess/WebPageProxy.h:2901
>>> +    bool m_requestIsNonAppBound { false };
>> 
>> It is super fishy don't have a flag like this on the WebPageProxy. A page is so far from a request. You can make many requests in a single page.
> 
> I use it as a holding place after WebPageProxy::loadRequest is called until it gets sent to the WebPage and stored in the main frame's document loader. Maybe "currentRequestIsNonAppBound"?

Or, currentRequestIsAppBound if I reverse it.

[Chris Dumez]

(In reply to katherine_cheney from comment #15)
> Comment on attachment 420375 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=420375&action=review
> 
> >>> Source/WebKit/UIProcess/WebPageProxy.h:2901
> >>> +    bool m_requestIsNonAppBound { false };
> >> 
> >> It is super fishy don't have a flag like this on the WebPageProxy. A page is so far from a request. You can make many requests in a single page.
> > 
> > I use it as a holding place after WebPageProxy::loadRequest is called until it gets sent to the WebPage and stored in the main frame's document loader. Maybe "currentRequestIsNonAppBound"?
> 
> Or, currentRequestIsAppBound if I reverse it.

Does it reset after we're done navigating? If not, it sounds like it should be lastRequestWasAppBound, no? or maybe even better: lastNavigationWasAppBound. I personally find "request" at page level to be confusing (although I understand it matches WKWebView.loadRequest API). Internally, the loadRequest results in a main frame navigation.

[Kate Cheney]

Created attachment 420501 [details]
Patch

[Kate Cheney]

Created attachment 420505 [details]
Patch

[Kate Cheney]

(In reply to Chris Dumez from comment #16)
> (In reply to katherine_cheney from comment #15)
> > Comment on attachment 420375 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=420375&action=review
> > 
> > >>> Source/WebKit/UIProcess/WebPageProxy.h:2901
> > >>> +    bool m_requestIsNonAppBound { false };
> > >> 
> > >> It is super fishy don't have a flag like this on the WebPageProxy. A page is so far from a request. You can make many requests in a single page.
> > > 
> > > I use it as a holding place after WebPageProxy::loadRequest is called until it gets sent to the WebPage and stored in the main frame's document loader. Maybe "currentRequestIsNonAppBound"?
> > 
> > Or, currentRequestIsAppBound if I reverse it.
> 
> Does it reset after we're done navigating? If not, it sounds like it should
> be lastRequestWasAppBound, no? or maybe even better:
> lastNavigationWasAppBound. I personally find "request" at page level to be
> confusing (although I understand it matches WKWebView.loadRequest API).
> Internally, the loadRequest results in a main frame navigation.

I agree this makes more sense.

[Alex Christensen]

Comment on attachment 420505 [details]
Patch

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

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:112
> +        request.setIsAppBound(frame.mainFrame().document()->loader()->lastNavigationWasAppBound());

These need null checks

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:769
> +    request.setIsAppBound(webPage.mainFrame()->document()->loader()->lastNavigationWasAppBound());

ditto

> Tools/TestWebKitAPI/Tests/WebKitCocoa/InAppBrowserPrivacy.mm:51
> +#else
> +#define APP_BOUND_REQUEST_ADDITIONS

This isn't needed.

[Kate Cheney]

(In reply to Alex Christensen from comment #20)
> Comment on attachment 420505 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=420505&action=review
> 
> > Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:112
> > +        request.setIsAppBound(frame.mainFrame().document()->loader()->lastNavigationWasAppBound());
> 
> These need null checks

Will fix.

> 
> > Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:769
> > +    request.setIsAppBound(webPage.mainFrame()->document()->loader()->lastNavigationWasAppBound());
> 
> ditto

Will fix.

> 
> > Tools/TestWebKitAPI/Tests/WebKitCocoa/InAppBrowserPrivacy.mm:51
> > +#else
> > +#define APP_BOUND_REQUEST_ADDITIONS
> 
> This isn't needed.

It will always use the Internal SDK?

[Brent Fulgham]

Comment on attachment 420505 [details]
Patch

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

I think this looks great, but I don't think we want to limit this concept to iOS-only. r- to fix that, but otherwise this is ready to go.

> Source/WebCore/loader/DocumentLoader.h:423
> +#if PLATFORM(IOS_FAMILY)

I don't think this concept is limited to IOS_FAMILY only, is it?

> Source/WebCore/loader/DocumentLoader.h:666
> +#if PLATFORM(IOS_FAMILY)

Ditto.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1363
> +    parameters.request.setIsAppBound(lastNavigationWasAppBound == LastNavigationWasAppBound::Yes ? true : false);

nit: I think you just need "lastNavigationWasAppBound == LastNavigationWasAppBound::Yes" (the additional ternary operation is redundant)

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:880
> +        redirectRequest.setIsAppBound(request.isAppBound());

I wonder if the test is really necessary before setting. Maybe just always set the app-bound state of the redirect to match the original request, since we always want the redirect to have the state of the originating request.

> Source/WebKit/Shared/WebPageCreationParameters.cpp:174
> +#if PLATFORM(IOS_FAMILY)

I don't think this is limited to IOS.

> Source/WebKit/Shared/WebPageCreationParameters.h:245
> +#if PLATFORM(IOS_FAMILY)

Ditto.

> Source/WebKit/UIProcess/WebPageProxy.cpp:1322
> +#if PLATFORM(IOS_FAMILY)

I don't think this should be iOS-only

> Source/WebKit/UIProcess/WebPageProxy.cpp:-7880
> -    

Nit: Unneeded whitespace change.

> Source/WebKit/UIProcess/WebPageProxy.h:1842
> +#if PLATFORM(IOS_FAMILY)

I don't thin this should be iOS-only

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:517
> +#if PLATFORM(IOS_FAMILY)

Not iOS-only

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:1624
> +#if PLATFORM(IOS_FAMILY)

Ditto

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:6427
> +#if PLATFORM(IOS_FAMILY)

Ditto

> Source/WebKit/WebProcess/WebPage/WebPage.h:1411
> +#if PLATFORM(IOS_FAMILY)

Ditto

> Source/WebKit/WebProcess/WebPage/WebPage.h:2236
> +#if PLATFORM(IOS_FAMILY)

Ditto

> Source/WebKit/WebProcess/WebPage/WebPage.messages.in:633
> +#if PLATFORM(IOS_FAMILY)

Ditto

>>> Tools/TestWebKitAPI/Tests/WebKitCocoa/InAppBrowserPrivacy.mm:51
>>> +#define APP_BOUND_REQUEST_ADDITIONS
>> 
>> This isn't needed.
> 
> It will always use the Internal SDK?

Your test says APPLE_INTERNAL_SDK only, so it seems like yes?

> Tools/TestWebKitAPI/cocoa/TestWKWebView.h:70
> +#if PLATFORM(IOS_FAMILY)

Not iOS-only

> Tools/TestWebKitAPI/cocoa/TestWKWebView.mm:242
> +#if PLATFORM(IOS_FAMILY)

Ditto.

[Kate Cheney]

Created attachment 420794 [details]
Patch

[Kate Cheney]

(In reply to Brent Fulgham from comment #22)
> Comment on attachment 420505 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=420505&action=review
> 
> I think this looks great, but I don't think we want to limit this concept to
> iOS-only. r- to fix that, but otherwise this is ready to go.
> 

Thanks! Latest patch makes these changes.

[Brent Fulgham]

Comment on attachment 420794 [details]
Patch

Excellent! r=me

[EWS]

Committed r273102: <https://commits.webkit.org/r273102>

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