Bug 222241 – App-bound request parameter should be passed to main resource requests not the main frame

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Kate Cheney]

We should pass app-bound information to main resources not on the main frame.

[Radar WebKit Bug Importer]

<rdar://problem/74560966>

[Kate Cheney]

Created attachment 421123 [details]
Patch

[Kate Cheney]

Created attachment 421124 [details]
Patch

[Chris Dumez]

Comment on attachment 421124 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivateForTesting.h:91
> +- (void)_appBoundNavigationData:(void(^)(NSArray<NSNumber *> *))completionHandler;

Missing WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivateForTesting.h:92
> +- (void)_clearAppBoundNavigationReports:(void(^)(void))completionHandler;

Ditto.

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewTesting.mm:336
> +- (void)_appBoundNavigationData:(void(^)(NSArray<NSNumber *> *))completionHandler

Really feels like this should pass to the lambda a struct with 2 BOOL data members. NSArray<NSNumber *> * is so opaque.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/InAppBrowserPrivacy.mm:1446
> +        EXPECT_EQ([navigationData objectAtIndex:0].intValue, 0);

I think it would read better as:
EXPECT_FALSE([navigationData[0].boolValue);

That said, I suggested using a different type in the SPI to make it even clearer.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/InAppBrowserPrivacy.mm:1447
> +        EXPECT_EQ([navigationData objectAtIndex:1].intValue, 1);

ditto

> Tools/TestWebKitAPI/Tests/WebKitCocoa/InAppBrowserPrivacy.mm:1479
> +        EXPECT_EQ([navigationData objectAtIndex:0].intValue, 1);

ditto.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/InAppBrowserPrivacy.mm:1480
> +        EXPECT_EQ([navigationData objectAtIndex:1].intValue, 0);

ditto.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/InAppBrowserPrivacy.mm:1521
> +    RetainPtr<WKWebViewConfiguration> configuration = adoptNS([[WKWebViewConfiguration alloc] init]);

auto

> Tools/TestWebKitAPI/Tests/WebKitCocoa/InAppBrowserPrivacy.mm:1524
> +    RetainPtr<TestWKWebView> webView = adoptNS([[TestWKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:configuration.get()]);

auto

> Tools/TestWebKitAPI/Tests/WebKitCocoa/InAppBrowserPrivacy.mm:1526
> +    [webView loadRequest:request];

Can use [webView loadTestPageNamed:@"page-with-csp"];

> Tools/TestWebKitAPI/Tests/WebKitCocoa/InAppBrowserPrivacy.mm:1545
> +    RetainPtr<WKWebViewConfiguration> configuration = adoptNS([[WKWebViewConfiguration alloc] init]);

auto

> Tools/TestWebKitAPI/Tests/WebKitCocoa/InAppBrowserPrivacy.mm:1548
> +    RetainPtr<TestWKWebView> webView = adoptNS([[TestWKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:configuration.get()]);

auto

> Tools/TestWebKitAPI/Tests/WebKitCocoa/InAppBrowserPrivacy.mm:1557
> +    static bool isDone = false;

Doesn't need to be static, can simply using
__block bool isDone = false;

> Tools/TestWebKitAPI/cocoa/TestWKWebView.h:71
> +- (void)appBoundNavigationData:(void(^)(NSArray<NSNumber *> *))completionHandler;

Why is this needed? TestWKWebView subclasses WKWebView so you should be able to call _appBoundNavigationData directly, no? (assuming you have included <WebKit/WKWebViewPrivate.h>)

> Tools/TestWebKitAPI/cocoa/TestWKWebView.h:72
> +- (void)clearAppBoundNavigationReports:(void(^)())completionHandler;

ditto.

[Kate Cheney]

Comment on attachment 421124 [details]
Patch

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

>> Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivateForTesting.h:91
>> +- (void)_appBoundNavigationData:(void(^)(NSArray<NSNumber *> *))completionHandler;
> 
> Missing WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));

Even for a private testing header file?

>> Source/WebKit/UIProcess/API/Cocoa/WKWebViewTesting.mm:336
>> +- (void)_appBoundNavigationData:(void(^)(NSArray<NSNumber *> *))completionHandler
> 
> Really feels like this should pass to the lambda a struct with 2 BOOL data members. NSArray<NSNumber *> * is so opaque.

Yeah, you're probably right.

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/InAppBrowserPrivacy.mm:1480
>> +        EXPECT_EQ([navigationData objectAtIndex:1].intValue, 0);
> 
> ditto.

will fix all of these.

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/InAppBrowserPrivacy.mm:1521
>> +    RetainPtr<WKWebViewConfiguration> configuration = adoptNS([[WKWebViewConfiguration alloc] init]);
> 
> auto

Will fix.

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/InAppBrowserPrivacy.mm:1524
>> +    RetainPtr<TestWKWebView> webView = adoptNS([[TestWKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:configuration.get()]);
> 
> auto

Ditto.

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/InAppBrowserPrivacy.mm:1526
>> +    [webView loadRequest:request];
> 
> Can use [webView loadTestPageNamed:@"page-with-csp"];

Ditto.

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/InAppBrowserPrivacy.mm:1557
>> +    static bool isDone = false;
> 
> Doesn't need to be static, can simply using
> __block bool isDone = false;

OKay.

>> Tools/TestWebKitAPI/cocoa/TestWKWebView.h:71
>> +- (void)appBoundNavigationData:(void(^)(NSArray<NSNumber *> *))completionHandler;
> 
> Why is this needed? TestWKWebView subclasses WKWebView so you should be able to call _appBoundNavigationData directly, no? (assuming you have included <WebKit/WKWebViewPrivate.h>)

Yes, you're right.

>> Tools/TestWebKitAPI/cocoa/TestWKWebView.h:72
>> +- (void)clearAppBoundNavigationReports:(void(^)())completionHandler;
> 
> ditto.

Will remove.

[Chris Dumez]

Comment on attachment 421124 [details]
Patch

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

>>> Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivateForTesting.h:91
>>> +- (void)_appBoundNavigationData:(void(^)(NSArray<NSNumber *> *))completionHandler;
>> 
>> Missing WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));
> 
> Even for a private testing header file?

I am no API expert but this is what I noticed. Just look at other SPIs in this file.

[Kate Cheney]

Comment on attachment 421124 [details]
Patch

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

>>>> Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivateForTesting.h:91
>>>> +- (void)_appBoundNavigationData:(void(^)(NSArray<NSNumber *> *))completionHandler;
>>> 
>>> Missing WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));
>> 
>> Even for a private testing header file?
> 
> I am no API expert but this is what I noticed. Just look at other SPIs in this file.

Some do but most in the file don't. I think it is ok to leave them out in this case.

[Kate Cheney]

Created attachment 421128 [details]
Patch

[Chris Dumez]

+Alex/Geoff/Brady to answer if we need the availability macro for SPIs or not. I personally think we should have it but this is not my area of expertise.

[Kate Cheney]

Comment on attachment 421128 [details]
Patch

removing r flag while I figure out iOS bot issues.

[Chris Dumez]

Comment on attachment 421128 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivateForTesting.h:35
> +@interface _WKAppBoundNavigationTestingData : NSObject

Why a NSObject? Can't we use a simple struct for this? I mean NSRect / CGRect are just structs.

[Kate Cheney]

(In reply to Chris Dumez from comment #11)
> Comment on attachment 421128 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=421128&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivateForTesting.h:35
> > +@interface _WKAppBoundNavigationTestingData : NSObject
> 
> Why a NSObject? Can't we use a simple struct for this? I mean NSRect /
> CGRect are just structs.

Yeah, it needs work, I also need to pass the C++ struct as an rvalue reference instead of copying. I'll upload a new patch in a bit.

[Kate Cheney]

Created attachment 421200 [details]
Patch

[Chris Dumez]

Comment on attachment 421200 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkSession.h:209
> +    bool m_hasLoadedAppBoundRequest { false };

If AppBoundNavigationTestingData did not have "Testing" in the name, then you could have a single data member of type AppBoundNavigationTestingData and a single getter / setter on the class.

[Chris Dumez]

Comment on attachment 421200 [details]
Patch

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

>> Source/WebKit/NetworkProcess/NetworkSession.h:209
>> +    bool m_hasLoadedAppBoundRequest { false };
> 
> If AppBoundNavigationTestingData did not have "Testing" in the name, then you could have a single data member of type AppBoundNavigationTestingData and a single getter / setter on the class.

Or maybe this is only used for Testing, in which case it is fine to have "Testing" in the type name. We may want to add "Testing" in the member name though.

[Chris Dumez]

Comment on attachment 421200 [details]
Patch

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

> Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:342
> +    request.isAppBound() ? m_session->setHasLoadedAppBoundRequest(true) : m_session->setHasLoadedNonAppBoundRequest(true);

I see here that you do need separate setters. That said, you can probably have a single getter to get the AppBoundNavigationTestingData data member.

[Kate Cheney]

Created attachment 421319 [details]
Patch

[Kate Cheney]

(In reply to Chris Dumez from comment #15)
> Comment on attachment 421200 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=421200&action=review
> 
> >> Source/WebKit/NetworkProcess/NetworkSession.h:209
> >> +    bool m_hasLoadedAppBoundRequest { false };
> > 
> > If AppBoundNavigationTestingData did not have "Testing" in the name, then you could have a single data member of type AppBoundNavigationTestingData and a single getter / setter on the class.
> 
> Or maybe this is only used for Testing, in which case it is fine to have
> "Testing" in the type name. We may want to add "Testing" in the member name
> though.

This is only used for testing. I updated it to have a single AppBoundNavigationTestingData data member.

[Kate Cheney]

Created attachment 421325 [details]
Patch

[Kate Cheney]

Created attachment 421333 [details]
Patch

[Chris Dumez]

Comment on attachment 421333 [details]
Patch

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

r=me with nits

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2777
> +        completionHandler({ networkSession->appBoundNavigationTestingData().hasLoadedAppBoundRequestTesting, networkSession->appBoundNavigationTestingData().hasLoadedNonAppBoundRequestTesting });

Why not:
`completionHandler(networkSession->appBoundNavigationTestingData())` ?

Am I missing something?

> Source/WebKit/Shared/NavigatingToAppBoundDomain.h:69
> +        return {{ WTFMove(*hasLoadedAppBoundRequestTesting), WTFMove(*hasLoadedNonAppBoundRequestTesting) }};

No need for the WTFMove() since those are booleans

[Kate Cheney]

(In reply to Chris Dumez from comment #21)
> Comment on attachment 421333 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=421333&action=review
> 
> r=me with nits
> 
> > Source/WebKit/NetworkProcess/NetworkProcess.cpp:2777
> > +        completionHandler({ networkSession->appBoundNavigationTestingData().hasLoadedAppBoundRequestTesting, networkSession->appBoundNavigationTestingData().hasLoadedNonAppBoundRequestTesting });
> 
> Why not:
> `completionHandler(networkSession->appBoundNavigationTestingData())` ?
> 
> Am I missing something?

Oh — I meant to ask about this. I don’t think I can, because networkSession->appBoundNavigationTestingData() returns an lvalue reference and the completion handler takes an rvalue reference. I am not sure of the right thing to do in this case, I think there needs to be a copy, which is why I built a new AppBoundNavigationTestingData struct here.

> 
> > Source/WebKit/Shared/NavigatingToAppBoundDomain.h:69
> > +        return {{ WTFMove(*hasLoadedAppBoundRequestTesting), WTFMove(*hasLoadedNonAppBoundRequestTesting) }};
> 
> No need for the WTFMove() since those are booleans

[Chris Dumez]

Comment on attachment 421333 [details]
Patch

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

>>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2777
>>> +        completionHandler({ networkSession->appBoundNavigationTestingData().hasLoadedAppBoundRequestTesting, networkSession->appBoundNavigationTestingData().hasLoadedNonAppBoundRequestTesting });
>> 
>> Why not:
>> `completionHandler(networkSession->appBoundNavigationTestingData())` ?
>> 
>> Am I missing something?
> 
> Oh — I meant to ask about this. I don’t think I can, because networkSession->appBoundNavigationTestingData() returns an lvalue reference and the completion handler takes an rvalue reference. I am not sure of the right thing to do in this case, I think there needs to be a copy, which is why I built a new AppBoundNavigationTestingData struct here.

Are you sure the CompletionHandler really needs to take a AppBoundNavigationTestingData&&. Wouldn't a const AppBoundNavigationTestingData& be enough? IPC doesn't care about getting an rvalue reference.

If it really needs to be an rvalue reference, then the following might work:
completionHandler({ networkSession->appBoundNavigationTestingData() })
or
completionHandler(AppBoundNavigationTestingData { networkSession->appBoundNavigationTestingData() })

[Kate Cheney]

(In reply to Chris Dumez from comment #23)
> Comment on attachment 421333 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=421333&action=review
> 
> >>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2777
> >>> +        completionHandler({ networkSession->appBoundNavigationTestingData().hasLoadedAppBoundRequestTesting, networkSession->appBoundNavigationTestingData().hasLoadedNonAppBoundRequestTesting });
> >> 
> >> Why not:
> >> `completionHandler(networkSession->appBoundNavigationTestingData())` ?
> >> 
> >> Am I missing something?
> > 
> > Oh — I meant to ask about this. I don’t think I can, because networkSession->appBoundNavigationTestingData() returns an lvalue reference and the completion handler takes an rvalue reference. I am not sure of the right thing to do in this case, I think there needs to be a copy, which is why I built a new AppBoundNavigationTestingData struct here.
> 
> Are you sure the CompletionHandler really needs to take a
> AppBoundNavigationTestingData&&. Wouldn't a const
> AppBoundNavigationTestingData& be enough? IPC doesn't care about getting an
> rvalue reference.
> 

Yes in that case, I think const AppBoundNavigationTestingData& is enough.

> If it really needs to be an rvalue reference, then the following might work:
> completionHandler({ networkSession->appBoundNavigationTestingData() })
> or
> completionHandler(AppBoundNavigationTestingData {
> networkSession->appBoundNavigationTestingData() })

[Kate Cheney]

Created attachment 421434 [details]
Patch

[Kate Cheney]

(In reply to katherine_cheney from comment #25)
> Created attachment 421434 [details]
> Patch

Thanks for the review. I asked Brent to take a look as well.

[Brent Fulgham]

Comment on attachment 421434 [details]
Patch

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

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:110
> +    if (resource.type() != CachedResource::Type::MainResource || !frame.isMainFrame()) {

Good catch.

[EWS]

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

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

[Ryosuke Niwa]

Comment on attachment 421434 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WebKitCocoa/InAppBrowserPrivacy.mm:1549
> +    __block bool isDone = false;

This isDone needs to be defined before NSMutableURLRequest requestWithURL...] above!
This change caused a build failure because of that.