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
Kate Cheney
2021-02-20 17:40:27 PST
Created attachment 421123 [details]
Patch
Created attachment 421124 [details]
Patch
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 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 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 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. Created attachment 421128 [details]
Patch
+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 on attachment 421128 [details]
Patch
removing r flag while I figure out iOS bot issues.
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. (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. Created attachment 421200 [details]
Patch
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 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 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. Created attachment 421319 [details]
Patch
(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. Created attachment 421325 [details]
Patch
Created attachment 421333 [details]
Patch
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 (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 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() }) (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() }) Created attachment 421434 [details]
Patch
(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 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. Committed r273465: <https://commits.webkit.org/r273465> All reviewed patches have been landed. Closing bug and clearing flags on attachment 421434 [details]. 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. |