Bug 221909 - Add support for non app-bound requests
Summary: Add support for non app-bound requests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kate Cheney
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-02-15 10:21 PST by Kate Cheney
Modified: 2021-02-18 13:22 PST (History)
5 users (show)

See Also:


Attachments
Patch (36.50 KB, patch)
2021-02-15 11:05 PST, Kate Cheney
no flags Details | Formatted Diff | Diff
Patch (36.15 KB, patch)
2021-02-15 13:10 PST, Kate Cheney
no flags Details | Formatted Diff | Diff
Patch (35.60 KB, patch)
2021-02-15 14:39 PST, Kate Cheney
no flags Details | Formatted Diff | Diff
Patch (37.06 KB, patch)
2021-02-16 11:12 PST, Kate Cheney
no flags Details | Formatted Diff | Diff
Patch (37.09 KB, patch)
2021-02-16 11:18 PST, Kate Cheney
no flags Details | Formatted Diff | Diff
Patch (35.76 KB, patch)
2021-02-17 21:31 PST, Kate Cheney
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kate Cheney 2021-02-15 10:21:49 PST
We should add support for specifying non app-bound requests, mainly populating sub resource loads with this value.
Comment 1 Kate Cheney 2021-02-15 10:22:20 PST
<rdar://problem/73512988>
Comment 2 Kate Cheney 2021-02-15 11:05:30 PST
Created attachment 420337 [details]
Patch
Comment 3 Chris Dumez 2021-02-15 11:11:03 PST
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?
Comment 4 Kate Cheney 2021-02-15 12:10:25 PST
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.
Comment 5 Kate Cheney 2021-02-15 13:00:50 PST
(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.
Comment 6 Alex Christensen 2021-02-15 13:01:41 PST
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.
Comment 7 Kate Cheney 2021-02-15 13:10:36 PST
Created attachment 420354 [details]
Patch
Comment 8 Kate Cheney 2021-02-15 13:36:36 PST
(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
Comment 9 Chris Dumez 2021-02-15 14:11:35 PST
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.
Comment 10 Chris Dumez 2021-02-15 14:12:15 PST
(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.
Comment 11 Kate Cheney 2021-02-15 14:33:01 PST
(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.
Comment 12 Kate Cheney 2021-02-15 14:39:37 PST
Created attachment 420375 [details]
Patch
Comment 13 Chris Dumez 2021-02-15 14:48:58 PST
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.
Comment 14 Kate Cheney 2021-02-15 15:12:44 PST
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?
Comment 15 Kate Cheney 2021-02-15 15:26:38 PST
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.
Comment 16 Chris Dumez 2021-02-15 16:39:22 PST
(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.
Comment 17 Kate Cheney 2021-02-16 11:12:55 PST
Created attachment 420501 [details]
Patch
Comment 18 Kate Cheney 2021-02-16 11:18:50 PST
Created attachment 420505 [details]
Patch
Comment 19 Kate Cheney 2021-02-16 11:19:25 PST
(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.
Comment 20 Alex Christensen 2021-02-17 09:58:19 PST
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.
Comment 21 Kate Cheney 2021-02-17 10:49:11 PST
(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?
Comment 22 Brent Fulgham 2021-02-17 11:04:18 PST
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.
Comment 23 Kate Cheney 2021-02-17 21:31:48 PST
Created attachment 420794 [details]
Patch
Comment 24 Kate Cheney 2021-02-17 21:32:21 PST
(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.
Comment 25 Brent Fulgham 2021-02-18 12:09:06 PST
Comment on attachment 420794 [details]
Patch

Excellent! r=me
Comment 26 EWS 2021-02-18 13:22:37 PST
Committed r273102: <https://commits.webkit.org/r273102>

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