We should exempt app-bound domains from ITP's website data deletion.
<rdar://problem/61950767>
Created attachment 396804 [details] Patch
Created attachment 396805 [details] Patch
Fixed the non-cocoa build error.
Created attachment 396807 [details] Patch
Created attachment 396810 [details] Patch
Comment on attachment 396810 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396810&action=review LGTM > Source/WebKit/UIProcess/WebProcessPool.cpp:627 > + appBoundDomains = networkSessionParameters.resourceLoadStatisticsParameters.appBoundDomains; can this use WTFMove()? > Source/WebKit/UIProcess/WebProcessPool.cpp:654 > + appBoundDomains = networkSessionParameters.resourceLoadStatisticsParameters.appBoundDomains; Ditto
Comment on attachment 396810 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396810&action=review > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.cpp:655 > +} Is there a chance this could be called before m_appBoundDomains has been set?
Created attachment 398098 [details] Patch
(In reply to katherine_cheney from comment #7) > Comment on attachment 396810 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=396810&action=review > > LGTM > > > Source/WebKit/UIProcess/WebProcessPool.cpp:627 > > + appBoundDomains = networkSessionParameters.resourceLoadStatisticsParameters.appBoundDomains; > > can this use WTFMove()? > > > Source/WebKit/UIProcess/WebProcessPool.cpp:654 > > + appBoundDomains = networkSessionParameters.resourceLoadStatisticsParameters.appBoundDomains; > > Ditto I don't know. We don't do it for any other parameters.
(In reply to katherine_cheney from comment #8) > Comment on attachment 396810 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=396810&action=review > > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.cpp:655 > > +} > > Is there a chance this could be called before m_appBoundDomains has been set? Thanks! I'll add a test that m_appBoundDomains exists.
Created attachment 398100 [details] Patch
Created attachment 398101 [details] Patch
(In reply to John Wilander from comment #11) > (In reply to katherine_cheney from comment #8) > > Comment on attachment 396810 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=396810&action=review > > > > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.cpp:655 > > > +} > > > > Is there a chance this could be called before m_appBoundDomains has been set? > > Thanks! I'll add a test that m_appBoundDomains exists. It was not happy with me testing m_appBoundDomains.
Created attachment 398104 [details] Patch
Comment on attachment 398104 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398104&action=review > Source/WebCore/platform/network/NetworkStorageSession.cpp:319 > +{ You could also add domains to the WebKitTestRunner Info.plist if you wanted to test out more than 2 domains, and get closer to testing real-functionality. This would also simplify some of the code.
Comment on attachment 398104 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398104&action=review > Source/WebCore/platform/network/NetworkStorageSession.cpp:321 > + if (m_appBoundDomains.size() >= 2) Why this restriction at code level? What prevents from testing more? > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.h:174 > + HashSet<RegistrableDomain> appBoundDomains() const { return m_appBoundDomains; } should return a "const HashSet<RegistrableDomain>&" to avoid copying > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:689 > + if (!m_statisticsStore) Given that it is a single statement, I would write: if (m_statisticsStore) m_statisticsStore->setAppBoundDomains(WTFMove(domains)); > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:700 > +void WebResourceLoadStatisticsStore::addAppBoundDomainForTesting(const WebCore::RegistrableDomain& domain, CompletionHandler<void(uint64_t)>&& completionHandler) Why cannot this use setAppBoundDomains()? Do we really need a whole other code path for testing? Doesn't it defeat the purpose since we are not testing the normal code path? > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:719 > + auto totalAppBoundDomains = m_statisticsStore->appBoundDomains().size(); See because appBoundDomains() is not returning a const ref, you're copying the whole set here, just to get its size. > Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:408 > + appBoundDomainQueue().dispatch([this, isInAppBrowserPrivacyEnabled = parameters().networkSessionParameters.isInAppBrowserPrivacyEnabled, forceReinitialization] () mutable { Why is capturing |this| safe here? It is also super weird to capture this here. initializeAppBoundDomains() is really meant to be static (and if it isn't, it is a bug). appBoundDomains are global, not per WebsiteDataStore and initializeAppBoundDomains() will happen only once for the lifetime of the app (in general), not once per store. Thus, when you're capturing a datastore here, you're just doing something for the datastore that happened the trigger the initialization (the first data store). What about all the other data stores? > Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:415 > + RunLoop::main().dispatch([this, isInAppBrowserPrivacyEnabled, forceReinitialization, domains = retainPtr(domains)] { ditto. > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1912 > + auto callbackAggregator = CallbackAggregator::create([this, domain = WebCore::RegistrableDomain { url }, completionHandler = WTFMove(completionHandler)]() mutable { Why is it safe to capture |this| here?
Comment on attachment 398104 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398104&action=review > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:350 > + bool m_appBoundITPRelaxationEnabled { false }; Boolean variable need a prefix as per WebKit coding style. > Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:2338 > + WKBundlePagePostSynchronousMessageForTesting(InjectedBundle::singleton().page()->page(), messageName.get(), messageBody.get(), &returnData); Why is this synchronous? This is going all the way to the UIProcess then down to the network process and then back..
Comment on attachment 398104 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398104&action=review > Source/WebKit/Shared/ResourceLoadStatisticsParameters.h:51 > + Vector<WebCore::RegistrableDomain> appBoundDomains { }; You have a HashSet in the UIProcess and you need a HashSet in the NetworkProcess it seems. Why are you IPC'ing a Vector and then converting back and forth?
Comment on attachment 398104 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398104&action=review > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.h:297 > + HashSet<RegistrableDomain> m_appBoundDomains; So now the app bound domains are stored in 2 places in the network process: on the network storage session and on the ITP store (which is owned by the session). Can the code be factored differently so we don't have to? It seems unnecessarily wasteful, especially considering it is only used to decide if we should delete Website data, which would eventually happen on the main thread anyway.
(In reply to katherine_cheney from comment #16) > Comment on attachment 398104 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=398104&action=review > > > Source/WebCore/platform/network/NetworkStorageSession.cpp:319 > > +{ > > You could also add domains to the WebKitTestRunner Info.plist if you wanted > to test out more than 2 domains, and get closer to testing > real-functionality. This would also simplify some of the code. Did not know about this. I'll consider it. Thanks!
(In reply to Chris Dumez from comment #17) > Comment on attachment 398104 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=398104&action=review > > > Source/WebCore/platform/network/NetworkStorageSession.cpp:321 > > + if (m_appBoundDomains.size() >= 2) > > Why this restriction at code level? What prevents from testing more? Since the test infrastructure adds a C API to set app-bound domains, I want to defend it against misuse. However, I will consider locking it to localhost and 127.0.0.1 instead which is a better defense. > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.h:174 > > + HashSet<RegistrableDomain> appBoundDomains() const { return m_appBoundDomains; } > > should return a "const HashSet<RegistrableDomain>&" to avoid copying Good catch! > > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:689 > > + if (!m_statisticsStore) > > Given that it is a single statement, I would write: > > if (m_statisticsStore) > m_statisticsStore->setAppBoundDomains(WTFMove(domains)); Will fix. > > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:700 > > +void WebResourceLoadStatisticsStore::addAppBoundDomainForTesting(const WebCore::RegistrableDomain& domain, CompletionHandler<void(uint64_t)>&& completionHandler) > > Why cannot this use setAppBoundDomains()? Do we really need a whole other > code path for testing? Doesn't it defeat the purpose since we are not > testing the normal code path? It was because I wanted to the ability to augment the set. Also my test case is mostly concerned with the ITP behavior when app-bound domains are set. But I'll try to switch to using the set functionality instead since that covers more. I might go with Kate's suggestion and split my test case in two – one with both domains set as app-bound and one with just one of them set as app-bound. > > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:719 > > + auto totalAppBoundDomains = m_statisticsStore->appBoundDomains().size(); > > See because appBoundDomains() is not returning a const ref, you're copying > the whole set here, just to get its size. Yup, will fix. > > Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:408 > > + appBoundDomainQueue().dispatch([this, isInAppBrowserPrivacyEnabled = parameters().networkSessionParameters.isInAppBrowserPrivacyEnabled, forceReinitialization] () mutable { > > Why is capturing |this| safe here? > > It is also super weird to capture this here. initializeAppBoundDomains() is > really meant to be static (and if it isn't, it is a bug). appBoundDomains > are global, not per WebsiteDataStore and initializeAppBoundDomains() will > happen only once for the lifetime of the app (in general), not once per > store. Thus, when you're capturing a datastore here, you're just doing > something for the datastore that happened the trigger the initialization > (the first data store). What about all the other data stores? Aha! Did not realize this was static. I'll try to restructure accordingly. > > Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:415 > > + RunLoop::main().dispatch([this, isInAppBrowserPrivacyEnabled, forceReinitialization, domains = retainPtr(domains)] { > > ditto. > > > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1912 > > + auto callbackAggregator = CallbackAggregator::create([this, domain = WebCore::RegistrableDomain { url }, completionHandler = WTFMove(completionHandler)]() mutable { > > Why is it safe to capture |this| here? Should I have a weak this and check it instead?
(In reply to Chris Dumez from comment #18) > Comment on attachment 398104 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=398104&action=review > > > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:350 > > + bool m_appBoundITPRelaxationEnabled { false }; > > Boolean variable need a prefix as per WebKit coding style. Will fix. > > Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:2338 > > + WKBundlePagePostSynchronousMessageForTesting(InjectedBundle::singleton().page()->page(), messageName.get(), messageBody.get(), &returnData); > > Why is this synchronous? This is going all the way to the UIProcess then > down to the network process and then back.. Other functions with return values worked this way. But I might not need it if I'm going with the plist approach.
(In reply to Chris Dumez from comment #19) > Comment on attachment 398104 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=398104&action=review > > > Source/WebKit/Shared/ResourceLoadStatisticsParameters.h:51 > > + Vector<WebCore::RegistrableDomain> appBoundDomains { }; > > You have a HashSet in the UIProcess and you need a HashSet in the > NetworkProcess it seems. Why are you IPC'ing a Vector and then converting > back and forth? The reason why I use a Vector was an earlier discussion with Alex where he recommended it for IPC. I can switch to a HashSet if that's better in your view. (In reply to Chris Dumez from comment #20) > Comment on attachment 398104 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=398104&action=review > > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.h:297 > > + HashSet<RegistrableDomain> m_appBoundDomains; > > So now the app bound domains are stored in 2 places in the network process: > on the network storage session and on the ITP store (which is owned by the > session). Can the code be factored differently so we don't have to? > It seems unnecessarily wasteful, especially considering it is only used to > decide if we should delete Website data, which would eventually happen on > the main thread anyway. It's a small set (max ten) and the two places are on two different threads. That's why there are two. I could maybe add a filter on the main thread but that'll be logic in two places deciding which domains to delete for which feels ripe for mistakes, misunderstandings, and bugs down the road. Thanks for the review! I'll rework the patch.
(In reply to Chris Dumez from comment #17) > > > Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:408 > > + appBoundDomainQueue().dispatch([this, isInAppBrowserPrivacyEnabled = parameters().networkSessionParameters.isInAppBrowserPrivacyEnabled, forceReinitialization] () mutable { > > Why is capturing |this| safe here? > > It is also super weird to capture this here. initializeAppBoundDomains() is > really meant to be static (and if it isn't, it is a bug). appBoundDomains > are global, not per WebsiteDataStore and initializeAppBoundDomains() will > happen only once for the lifetime of the app (in general), not once per > store. Thus, when you're capturing a datastore here, you're just doing > something for the datastore that happened the trigger the initialization > (the first data store). What about all the other data stores? WebsiteDataStore::initializeAppBoundDomains() is not static. It makes use of the non-static parameters().networkSessionParameters.isInAppBrowserPrivacyEnabled. My added function WebsiteDataStore::setAppBoundDomainsForResourceLoadStatistics() makes use of non-static m_thirdPartyCookieBlockingMode, processes(), and processPools(). Given this, how would like me to propagate app-bound domains and their associated cookie policy to the web processes and the network process?
(In reply to John Wilander from comment #25) > (In reply to Chris Dumez from comment #17) > > > > > Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:408 > > > + appBoundDomainQueue().dispatch([this, isInAppBrowserPrivacyEnabled = parameters().networkSessionParameters.isInAppBrowserPrivacyEnabled, forceReinitialization] () mutable { > > > > Why is capturing |this| safe here? > > > > It is also super weird to capture this here. initializeAppBoundDomains() is > > really meant to be static (and if it isn't, it is a bug). appBoundDomains > > are global, not per WebsiteDataStore and initializeAppBoundDomains() will > > happen only once for the lifetime of the app (in general), not once per > > store. Thus, when you're capturing a datastore here, you're just doing > > something for the datastore that happened the trigger the initialization > > (the first data store). What about all the other data stores? > > WebsiteDataStore::initializeAppBoundDomains() is not static. > > It makes use of the non-static > parameters().networkSessionParameters.isInAppBrowserPrivacyEnabled. Well, that seems like a bug, doesn't it? The AppBoundDomains are currently static and shared by ALL data stores. However, initialization is based on the NetworkSessionParameters.isInAppBrowserPrivacyEnabled setting of the first data store that gets created.. > > My added function > WebsiteDataStore::setAppBoundDomainsForResourceLoadStatistics() makes use of > non-static m_thirdPartyCookieBlockingMode, processes(), and processPools(). > > Given this, how would like me to propagate app-bound domains and their > associated cookie policy to the web processes and the network process?
Created attachment 398465 [details] Patch
I addressed all the concerns except: 1) I kept the capability to set app-bound domains through a TestRunner function. 2) The small sets of app-bound domains are still kept in two places in the network process – on the background thread to allow for filtering of domains to delete website data for and in the network storage session to allow for lookup in cookie blocking. Point 2 optimizes for readability and low complexity over memory usage. If you feel that's not the right balance, I do the filtering for website data deletion out of context using the network storage session's HashSet on the main thread.
Created attachment 398468 [details] Patch
Created attachment 398541 [details] Patch
Comment on attachment 398541 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398541&action=review > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:702 > + m_statisticsStore-> setThirdPartyCookieBlockingMode(ThirdPartyCookieBlockingMode::AllExceptBetweenAppBoundDomains); m_statisticsStore-> setThirdPartyCookieBlockingMode Spaces after arrow. > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:704 > + postTaskReply([completionHandler = WTFMove(completionHandler)]() mutable { Wouldn't this work? postTaskReply(completionHandler); > Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:636 > + WebKit::WebsiteDataStore::setAppBoundDomainsForTesting(WTFMove(domains), [context, completionHandler] () { () is not needed and is usually omitted > Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:1141 > + sendWithAsyncReply(Messages::NetworkProcess::SetAppBoundDomainsForResourceLoadStatistics(sessionID, appBoundDomains), WTFMove(completionHandler)); Please capture a backgroundActivity here to make sure the network process is not suspended on iOS. ITP code iterates over all pools and sends IPC to processes that are suspended because they are not involved with the current test view. > Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:491 > +Optional<HashSet<WebCore::RegistrableDomain>> WebsiteDataStore::getAppBoundDomainsIfInitialized() Our getters' name never start with 'get' > Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:502 > + if (domain.string() != "localhost"_s && domain.string() != "127.0.0.1"_s) { Should this assert instead of returning early since this is only used for testing? Seems like someone using this testing method wrong would not necessarily notice they are doing things wrong. > Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:507 > + appBoundDomains() = WTFMove(domains); What prevents WebsiteDataStore::initializeAppBoundDomains() from overriding the domains you just set for testing after reading the plist on the background thread? > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:2232 > + WebCore::RegistrableDomain standaloneApplicationDomain { }; { } is not needed. Also, where is this ever initialized to anything else? > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:2239 > WebCore::RegistrableDomain resourceLoadStatisticsManualPrevalentResource { }; ditto. > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:337 > + static Optional<HashSet<WebCore::RegistrableDomain>> getAppBoundDomainsIfInitialized(); no 'get' prefix for WebKit getters pls. > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:338 > + constexpr static const std::atomic<bool> isAppBoundITPRelaxationEnabled = false; Who ever flips this to true? what's the point of code that never runs because it is guarded by isAppBoundITPRelaxationEnabled ? > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:339 > + static void setAppBoundDomainsForITPIfInitialized(CompletionHandler<void()>&&); Bad naming I think considering this does not take any domains in parameter. > Tools/WebKitTestRunner/TestController.cpp:3784 > + runUntil(context.done, noTimeout); It does not look right to cause a hang here. I see no reason why it cannot use a completion handler. Use the callback to WKWebsiteDataStoreSetAppBoundDomainsForTesting() to call m_currentInvocation->didSetAppBoundDomains()
(In reply to Chris Dumez from comment #31) > Comment on attachment 398541 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=398541&action=review > > > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:702 > > + m_statisticsStore-> setThirdPartyCookieBlockingMode(ThirdPartyCookieBlockingMode::AllExceptBetweenAppBoundDomains); > > m_statisticsStore-> setThirdPartyCookieBlockingMode > > Spaces after arrow. Oops! Don't know where those came from. Will fix. > > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:704 > > + postTaskReply([completionHandler = WTFMove(completionHandler)]() mutable { > > Wouldn't this work? > postTaskReply(completionHandler); I seem to recall trying that and something failed. But that might have been unrelated. Will fix. > > Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:636 > > + WebKit::WebsiteDataStore::setAppBoundDomainsForTesting(WTFMove(domains), [context, completionHandler] () { > > () is not needed and is usually omitted Aha, not even when we have a capture list and body? Will fix. > > Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:1141 > > + sendWithAsyncReply(Messages::NetworkProcess::SetAppBoundDomainsForResourceLoadStatistics(sessionID, appBoundDomains), WTFMove(completionHandler)); > > Please capture a backgroundActivity here to make sure the network process is > not suspended on iOS. ITP code iterates over all pools and sends IPC to > processes that are suspended because they are not involved with the current > test view. OK, will do. > > Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:491 > > +Optional<HashSet<WebCore::RegistrableDomain>> WebsiteDataStore::getAppBoundDomainsIfInitialized() > > Our getters' name never start with 'get' I considered that but opted to match WebsiteDataStore::getAppBoundDomains() since this is a sibling function to that. I guess WebsiteDataStore::getAppBoundDomains() should change name then. > > Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:502 > > + if (domain.string() != "localhost"_s && domain.string() != "127.0.0.1"_s) { > > Should this assert instead of returning early since this is only used for > testing? Seems like someone using this testing method wrong would not > necessarily notice they are doing things wrong. Yes, a release assert would be good! This is protection against misuse of the C API. > > Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:507 > > + appBoundDomains() = WTFMove(domains); > > What prevents WebsiteDataStore::initializeAppBoundDomains() from overriding > the domains you just set for testing after reading the plist on the > background thread? hasInitializedAppBoundDomains gates all functions that set app-bound domains and WebsiteDataStore::setAppBoundDomainsForTesting() sets it to true. Are you thinking of a race where the background thread has already passed its if (hasInitializedAppBoundDomains && forceReinitialization != ForceReinitialization::Yes) check? > > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:2232 > > + WebCore::RegistrableDomain standaloneApplicationDomain { }; > > { } is not needed. Will fix. > Also, where is this ever initialized to anything else? In WebsiteDataStore::platformSetNetworkParameters() in WebsiteDataStoreCocoa.mm: parameters.networkSessionParameters.resourceLoadStatisticsParameters.standaloneApplicationDomain = WebCore::RegistrableDomain { m_configuration->standaloneApplicationURL() }; > > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:2239 > > WebCore::RegistrableDomain resourceLoadStatisticsManualPrevalentResource { }; > > ditto. Will fix. > > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:337 > > + static Optional<HashSet<WebCore::RegistrableDomain>> getAppBoundDomainsIfInitialized(); > > no 'get' prefix for WebKit getters pls. Will fix. See previous comment on why it was named like this. > > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:338 > > + constexpr static const std::atomic<bool> isAppBoundITPRelaxationEnabled = false; > > Who ever flips this to true? what's the point of code that never runs > because it is guarded by isAppBoundITPRelaxationEnabled ? This is me "putting it behind a flag" since I'm not allowed to land it on by default. I didn't bloat our list of experimental features since this is for non-browser WebViews. If we decide we want to turn it on, I'll just remove this flag. As you can see, the flag is used in a way that still allows testing. > > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:339 > > + static void setAppBoundDomainsForITPIfInitialized(CompletionHandler<void()>&&); > > Bad naming I think considering this does not take any domains in parameter. True. How about forwardAppBoundDomainsToITPIfInitialized()? > > Tools/WebKitTestRunner/TestController.cpp:3784 > > + runUntil(context.done, noTimeout); > > It does not look right to cause a hang here. I see no reason why it cannot > use a completion handler. Use the callback to > WKWebsiteDataStoreSetAppBoundDomainsForTesting() to call > m_currentInvocation->didSetAppBoundDomains() OK, will fix. Thanks, Chris!
(In reply to John Wilander from comment #32) > (In reply to Chris Dumez from comment #31) > > Comment on attachment 398541 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=398541&action=review > > > > > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:702 > > > + m_statisticsStore-> setThirdPartyCookieBlockingMode(ThirdPartyCookieBlockingMode::AllExceptBetweenAppBoundDomains); > > > > m_statisticsStore-> setThirdPartyCookieBlockingMode > > > > Spaces after arrow. > > Oops! Don't know where those came from. Will fix. > > > > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:704 > > > + postTaskReply([completionHandler = WTFMove(completionHandler)]() mutable { > > > > Wouldn't this work? > > postTaskReply(completionHandler); > > I seem to recall trying that and something failed. But that might have been > unrelated. Will fix. > > > > Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.cpp:636 > > > + WebKit::WebsiteDataStore::setAppBoundDomainsForTesting(WTFMove(domains), [context, completionHandler] () { > > > > () is not needed and is usually omitted > > Aha, not even when we have a capture list and body? Will fix. > > > > Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:1141 > > > + sendWithAsyncReply(Messages::NetworkProcess::SetAppBoundDomainsForResourceLoadStatistics(sessionID, appBoundDomains), WTFMove(completionHandler)); > > > > Please capture a backgroundActivity here to make sure the network process is > > not suspended on iOS. ITP code iterates over all pools and sends IPC to > > processes that are suspended because they are not involved with the current > > test view. > > OK, will do. > > > > Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:491 > > > +Optional<HashSet<WebCore::RegistrableDomain>> WebsiteDataStore::getAppBoundDomainsIfInitialized() > > > > Our getters' name never start with 'get' > > I considered that but opted to match WebsiteDataStore::getAppBoundDomains() > since this is a sibling function to that. I guess > WebsiteDataStore::getAppBoundDomains() should change name then. Yes, then both need fixing. > > > > Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:502 > > > + if (domain.string() != "localhost"_s && domain.string() != "127.0.0.1"_s) { > > > > Should this assert instead of returning early since this is only used for > > testing? Seems like someone using this testing method wrong would not > > necessarily notice they are doing things wrong. > > Yes, a release assert would be good! This is protection against misuse of > the C API. > > > > Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:507 > > > + appBoundDomains() = WTFMove(domains); > > > > What prevents WebsiteDataStore::initializeAppBoundDomains() from overriding > > the domains you just set for testing after reading the plist on the > > background thread? > > hasInitializedAppBoundDomains gates all functions that set app-bound domains > and WebsiteDataStore::setAppBoundDomainsForTesting() sets it to true. > > Are you thinking of a race where the background thread has already passed > its if (hasInitializedAppBoundDomains && forceReinitialization != > ForceReinitialization::Yes) check? Correct. I definitely think there is a race here. I guess you could address that by checking hasInitializedAppBoundDomains again in initializeAppBoundDomains() after returning to the main thread. > > > > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:2232 > > > + WebCore::RegistrableDomain standaloneApplicationDomain { }; > > > > { } is not needed. > > Will fix. > > > Also, where is this ever initialized to anything else? > > In WebsiteDataStore::platformSetNetworkParameters() in > WebsiteDataStoreCocoa.mm: > > parameters.networkSessionParameters.resourceLoadStatisticsParameters. > standaloneApplicationDomain = WebCore::RegistrableDomain { > m_configuration->standaloneApplicationURL() }; > > > > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:2239 > > > WebCore::RegistrableDomain resourceLoadStatisticsManualPrevalentResource { }; > > > > ditto. > > Will fix. > > > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:337 > > > + static Optional<HashSet<WebCore::RegistrableDomain>> getAppBoundDomainsIfInitialized(); > > > > no 'get' prefix for WebKit getters pls. > > Will fix. See previous comment on why it was named like this. > > > > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:338 > > > + constexpr static const std::atomic<bool> isAppBoundITPRelaxationEnabled = false; > > > > Who ever flips this to true? what's the point of code that never runs > > because it is guarded by isAppBoundITPRelaxationEnabled ? > > This is me "putting it behind a flag" since I'm not allowed to land it on by > default. I didn't bloat our list of experimental features since this is for > non-browser WebViews. If we decide we want to turn it on, I'll just remove > this flag. As you can see, the flag is used in a way that still allows > testing. > > > > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:339 > > > + static void setAppBoundDomainsForITPIfInitialized(CompletionHandler<void()>&&); > > > > Bad naming I think considering this does not take any domains in parameter. > > True. How about forwardAppBoundDomainsToITPIfInitialized()? Better. > > > Tools/WebKitTestRunner/TestController.cpp:3784 > > > + runUntil(context.done, noTimeout); > > > > It does not look right to cause a hang here. I see no reason why it cannot > > use a completion handler. Use the callback to > > WKWebsiteDataStoreSetAppBoundDomainsForTesting() to call > > m_currentInvocation->didSetAppBoundDomains() > > OK, will fix. > > Thanks, Chris!
Comment on attachment 398541 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398541&action=review >>> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:704 >>> + postTaskReply([completionHandler = WTFMove(completionHandler)]() mutable { >> >> Wouldn't this work? >> postTaskReply(completionHandler); > > I seem to recall trying that and something failed. But that might have been unrelated. Will fix. I meant postTaskReply(WTFMove(completionHandler));
Created attachment 398573 [details] Patch
Comment on attachment 398573 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398573&action=review > Source/WebKit/Shared/ResourceLoadStatisticsParameters.h:50 > WebCore::RegistrableDomain standaloneApplicationDomain { }; { } unnecessary > Source/WebKit/Shared/ResourceLoadStatisticsParameters.h:51 > + HashSet<WebCore::RegistrableDomain> appBoundDomains { }; { } unnecessary > Source/WebKit/Shared/ResourceLoadStatisticsParameters.h:52 > WebCore::RegistrableDomain manualPrevalentResource { }; { } unnecessary > Source/WebKit/UIProcess/WebProcessPool.cpp:609 > + WebCore::RegistrableDomain standaloneApplicationDomain { }; { } unnecessary > Source/WebKit/UIProcess/WebProcessPool.cpp:610 > + HashSet<WebCore::RegistrableDomain> appBoundDomains { }; { } unnecessary > Source/WebKit/UIProcess/WebProcessPool.cpp:611 > WebCore::RegistrableDomain manualPrevalentResource { }; { } unnecessary > Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:505 > + RELEASE_ASSERT(domain.string() == "localhost"_s || domain.string() == "127.0.0.1"_s); RegistrableDomain has an operator==() so you should be able to do: RELEASE_ASSERT(domain == "localhost" || domain == "127.0.0.1"); > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:2235 > + auto optionalAppBoundDomains = appBoundDomainsIfInitialized(); There is no point in calling appBoundDomainsIfInitialized() if !isAppBoundITPRelaxationEnabled so I would write it like so: if (isAppBoundITPRelaxationEnabled) appBoundDomains = appBoundDomainsIfInitialized().valueOr(HashSet<WebCore::RegistrableDomain> { });
(In reply to Chris Dumez from comment #36) > Comment on attachment 398573 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=398573&action=review > > > Source/WebKit/Shared/ResourceLoadStatisticsParameters.h:50 > > WebCore::RegistrableDomain standaloneApplicationDomain { }; > > { } unnecessary Will fix. > > Source/WebKit/Shared/ResourceLoadStatisticsParameters.h:51 > > + HashSet<WebCore::RegistrableDomain> appBoundDomains { }; > > { } unnecessary Will fix. > > Source/WebKit/Shared/ResourceLoadStatisticsParameters.h:52 > > WebCore::RegistrableDomain manualPrevalentResource { }; > > { } unnecessary Will fix. > > Source/WebKit/UIProcess/WebProcessPool.cpp:609 > > + WebCore::RegistrableDomain standaloneApplicationDomain { }; > > { } unnecessary Will fix. > > Source/WebKit/UIProcess/WebProcessPool.cpp:610 > > + HashSet<WebCore::RegistrableDomain> appBoundDomains { }; > > { } unnecessary Will fix. > > Source/WebKit/UIProcess/WebProcessPool.cpp:611 > > WebCore::RegistrableDomain manualPrevalentResource { }; > > { } unnecessary Will fix. > > Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm:505 > > + RELEASE_ASSERT(domain.string() == "localhost"_s || domain.string() == "127.0.0.1"_s); > > RegistrableDomain has an operator==() so you should be able to do: > RELEASE_ASSERT(domain == "localhost" || domain == "127.0.0.1"); Will fix. > > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:2235 > > + auto optionalAppBoundDomains = appBoundDomainsIfInitialized(); > > There is no point in calling appBoundDomainsIfInitialized() if > !isAppBoundITPRelaxationEnabled so I would write it like so: > if (isAppBoundITPRelaxationEnabled) > appBoundDomains = > appBoundDomainsIfInitialized().valueOr(HashSet<WebCore::RegistrableDomain> { > }); Actually, there is. This is why the test is able to do this while production code isn't. WebsiteDataStore::initializeAppBoundDomains() only calls WebsiteDataStore::forwardAppBoundDomainsToITPIfInitialized() if isAppBoundITPRelaxationEnabled is true whereas WebsiteDataStore::setAppBoundDomainsForTesting() calls WebsiteDataStore::forwardAppBoundDomainsToITPIfInitialized() unconditionally. If WebsiteDataStore::forwardAppBoundDomainsToITPIfInitialize() were to check isAppBoundITPRelaxationEnabled, the test wouldn't work.
Comment on attachment 398573 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398573&action=review >>> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:2235 >>> + auto optionalAppBoundDomains = appBoundDomainsIfInitialized(); >> >> There is no point in calling appBoundDomainsIfInitialized() if !isAppBoundITPRelaxationEnabled so I would write it like so: >> if (isAppBoundITPRelaxationEnabled) >> appBoundDomains = appBoundDomainsIfInitialized().valueOr(HashSet<WebCore::RegistrableDomain> { }); > > Actually, there is. This is why the test is able to do this while production code isn't. WebsiteDataStore::initializeAppBoundDomains() only calls WebsiteDataStore::forwardAppBoundDomainsToITPIfInitialized() if isAppBoundITPRelaxationEnabled is true whereas WebsiteDataStore::setAppBoundDomainsForTesting() calls WebsiteDataStore::forwardAppBoundDomainsToITPIfInitialized() unconditionally. > > If WebsiteDataStore::forwardAppBoundDomainsToITPIfInitialize() were to check isAppBoundITPRelaxationEnabled, the test wouldn't work. Could you clarify what is wrong with the code I provided as replacement? I read your comment twice and it is still not clear to me. As far as I can tell, there is no behavior change with the change I provided.
(In reply to Chris Dumez from comment #38) > Comment on attachment 398573 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=398573&action=review > > >>> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:2235 > >>> + auto optionalAppBoundDomains = appBoundDomainsIfInitialized(); > >> > >> There is no point in calling appBoundDomainsIfInitialized() if !isAppBoundITPRelaxationEnabled so I would write it like so: > >> if (isAppBoundITPRelaxationEnabled) > >> appBoundDomains = appBoundDomainsIfInitialized().valueOr(HashSet<WebCore::RegistrableDomain> { }); > > > > Actually, there is. This is why the test is able to do this while production code isn't. WebsiteDataStore::initializeAppBoundDomains() only calls WebsiteDataStore::forwardAppBoundDomainsToITPIfInitialized() if isAppBoundITPRelaxationEnabled is true whereas WebsiteDataStore::setAppBoundDomainsForTesting() calls WebsiteDataStore::forwardAppBoundDomainsToITPIfInitialized() unconditionally. > > > > If WebsiteDataStore::forwardAppBoundDomainsToITPIfInitialize() were to check isAppBoundITPRelaxationEnabled, the test wouldn't work. > > Could you clarify what is wrong with the code I provided as replacement? I > read your comment twice and it is still not clear to me. > > As far as I can tell, there is no behavior change with the change I provided. Sorry, I thought you suggested I add a check to WebsiteDataStore::forwardAppBoundDomainsToITPIfInitialized() which also does appBoundDomains = appBoundDomainsIfInitialized(). That would have crippled the test. But I see you mean in the parameters set up. Will fix! Thanks for the review, Chris! This fixed a lot of things with this code.
Created attachment 398638 [details] Patch for landing
Committed r261242: <https://trac.webkit.org/changeset/261242> All reviewed patches have been landed. Closing bug and clearing flags on attachment 398638 [details].