Bug 210674 – Exempt app-bound domains from ITP's website data deletion and third-party cookie blocking between themselves

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[John Wilander]

We should exempt app-bound domains from ITP's website data deletion.

[Radar WebKit Bug Importer]

<rdar://problem/61950767>

[John Wilander]

Created attachment 396804 [details]
Patch

[John Wilander]

Created attachment 396805 [details]
Patch

[John Wilander]

Fixed the non-cocoa build error.

[John Wilander]

Created attachment 396807 [details]
Patch

[John Wilander]

Created attachment 396810 [details]
Patch

[Kate Cheney]

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

[Kate Cheney]

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?

[John Wilander]

Created attachment 398098 [details]
Patch

[John Wilander]

(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.

[John Wilander]

(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.

[John Wilander]

Created attachment 398100 [details]
Patch

[John Wilander]

Created attachment 398101 [details]
Patch

[John Wilander]

(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.

[John Wilander]

Created attachment 398104 [details]
Patch

[Kate Cheney]

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.

[Chris Dumez]

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?

[Chris Dumez]

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..

[Chris Dumez]

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?

[Chris Dumez]

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.

[John Wilander]

(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!

[John Wilander]

(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?

[John Wilander]

(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.

[John Wilander]

(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.

[John Wilander]

(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?

[Chris Dumez]

(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?

[John Wilander]

Created attachment 398465 [details]
Patch

[John Wilander]

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.

[John Wilander]

Created attachment 398468 [details]
Patch

[John Wilander]

Created attachment 398541 [details]
Patch

[Chris Dumez]

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()

[John Wilander]

(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!

[Chris Dumez]

(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!

[Chris Dumez]

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));

[John Wilander]

Created attachment 398573 [details]
Patch

[Chris Dumez]

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> { });

[John Wilander]

(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.

[Chris Dumez]

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.

[John Wilander]

(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.

[John Wilander]

Created attachment 398638 [details]
Patch for landing

[EWS]

Committed r261242: <https://trac.webkit.org/changeset/261242>

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