Bug 222241

Summary: App-bound request parameter should be passed to main resource requests not the main frame
Product: WebKit Reporter: Kate Cheney <katherine_cheney>
Component: WebKit Misc.Assignee: Kate Cheney <katherine_cheney>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, beidson, bfulgham, cdumez, ews-watchlist, ggaren, mkwst, rniwa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
none
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
none
Patch none

Description Kate Cheney 2021-02-20 17:40:27 PST
We should pass app-bound information to main resources not on the main frame.
Comment 1 Radar WebKit Bug Importer 2021-02-20 17:41:41 PST
<rdar://problem/74560966>
Comment 2 Kate Cheney 2021-02-20 17:50:33 PST
Created attachment 421123 [details]
Patch
Comment 3 Kate Cheney 2021-02-20 18:21:15 PST
Created attachment 421124 [details]
Patch
Comment 4 Chris Dumez 2021-02-20 19:08:02 PST
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.
Comment 5 Kate Cheney 2021-02-20 19:56:52 PST
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.
Comment 6 Chris Dumez 2021-02-20 20:01:11 PST
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.
Comment 7 Kate Cheney 2021-02-20 21:53:09 PST
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.
Comment 8 Kate Cheney 2021-02-20 21:55:29 PST
Created attachment 421128 [details]
Patch
Comment 9 Chris Dumez 2021-02-20 22:18:40 PST
+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.
Comment 10 Kate Cheney 2021-02-21 07:35:32 PST
Comment on attachment 421128 [details]
Patch

removing r flag while I figure out iOS bot issues.
Comment 11 Chris Dumez 2021-02-22 08:31:57 PST
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.
Comment 12 Kate Cheney 2021-02-22 08:35:24 PST
(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.
Comment 13 Kate Cheney 2021-02-22 09:22:41 PST
Created attachment 421200 [details]
Patch
Comment 14 Chris Dumez 2021-02-22 09:27:35 PST
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.
Comment 15 Chris Dumez 2021-02-22 09:30:45 PST
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.
Comment 16 Chris Dumez 2021-02-22 09:33:16 PST
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.
Comment 17 Kate Cheney 2021-02-23 09:23:13 PST
Created attachment 421319 [details]
Patch
Comment 18 Kate Cheney 2021-02-23 09:24:33 PST
(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.
Comment 19 Kate Cheney 2021-02-23 10:30:25 PST
Created attachment 421325 [details]
Patch
Comment 20 Kate Cheney 2021-02-23 11:10:21 PST
Created attachment 421333 [details]
Patch
Comment 21 Chris Dumez 2021-02-24 10:38:20 PST
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
Comment 22 Kate Cheney 2021-02-24 10:55:33 PST
(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
Comment 23 Chris Dumez 2021-02-24 11:03:28 PST
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() })
Comment 24 Kate Cheney 2021-02-24 11:23:58 PST
(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() })
Comment 25 Kate Cheney 2021-02-24 11:39:37 PST
Created attachment 421434 [details]
Patch
Comment 26 Kate Cheney 2021-02-24 11:39:53 PST
(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.
Comment 27 Brent Fulgham 2021-02-24 14:55:40 PST
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.
Comment 28 EWS 2021-02-24 17:45:56 PST
Committed r273465: <https://commits.webkit.org/r273465>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 421434 [details].
Comment 29 Ryosuke Niwa 2021-02-24 23:29:24 PST
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.