Bug 210674 - Exempt app-bound domains from ITP's website data deletion and third-party cookie blocking between themselves
Summary: Exempt app-bound domains from ITP's website data deletion and third-party coo...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: John Wilander
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-04-17 14:33 PDT by John Wilander
Modified: 2020-05-06 11:44 PDT (History)
4 users (show)

See Also:


Attachments
Patch (26.96 KB, patch)
2020-04-17 14:50 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Patch (26.99 KB, patch)
2020-04-17 15:14 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Patch (28.04 KB, patch)
2020-04-17 15:28 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Patch (26.98 KB, patch)
2020-04-17 15:35 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Patch (88.06 KB, patch)
2020-04-30 15:11 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Patch (88.08 KB, patch)
2020-04-30 15:14 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Patch (88.06 KB, patch)
2020-04-30 15:18 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Patch (88.17 KB, patch)
2020-04-30 15:31 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Patch (95.87 KB, patch)
2020-05-04 20:30 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Patch (96.09 KB, patch)
2020-05-04 20:51 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Patch (95.97 KB, patch)
2020-05-05 12:36 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Patch (97.20 KB, patch)
2020-05-05 17:30 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Patch for landing (97.23 KB, patch)
2020-05-06 11:13 PDT, John Wilander
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Wilander 2020-04-17 14:33:44 PDT
We should exempt app-bound domains from ITP's website data deletion.
Comment 1 Radar WebKit Bug Importer 2020-04-17 14:34:10 PDT
<rdar://problem/61950767>
Comment 2 John Wilander 2020-04-17 14:50:08 PDT
Created attachment 396804 [details]
Patch
Comment 3 John Wilander 2020-04-17 15:14:44 PDT
Created attachment 396805 [details]
Patch
Comment 4 John Wilander 2020-04-17 15:15:15 PDT
Fixed the non-cocoa build error.
Comment 5 John Wilander 2020-04-17 15:28:47 PDT
Created attachment 396807 [details]
Patch
Comment 6 John Wilander 2020-04-17 15:35:11 PDT
Created attachment 396810 [details]
Patch
Comment 7 Kate Cheney 2020-04-17 16:27:51 PDT
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 8 Kate Cheney 2020-04-17 16:41:40 PDT
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?
Comment 9 John Wilander 2020-04-30 15:11:07 PDT
Created attachment 398098 [details]
Patch
Comment 10 John Wilander 2020-04-30 15:12:51 PDT
(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.
Comment 11 John Wilander 2020-04-30 15:14:23 PDT
(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.
Comment 12 John Wilander 2020-04-30 15:14:46 PDT
Created attachment 398100 [details]
Patch
Comment 13 John Wilander 2020-04-30 15:18:46 PDT
Created attachment 398101 [details]
Patch
Comment 14 John Wilander 2020-04-30 15:19:10 PDT
(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.
Comment 15 John Wilander 2020-04-30 15:31:05 PDT
Created attachment 398104 [details]
Patch
Comment 16 Kate Cheney 2020-04-30 15:43:24 PDT
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 17 Chris Dumez 2020-04-30 17:01:57 PDT
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 18 Chris Dumez 2020-04-30 17:05:13 PDT
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 19 Chris Dumez 2020-04-30 17:06:40 PDT
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 20 Chris Dumez 2020-04-30 17:12:15 PDT
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.
Comment 21 John Wilander 2020-05-01 12:37:53 PDT
(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!
Comment 22 John Wilander 2020-05-01 12:51:49 PDT
(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?
Comment 23 John Wilander 2020-05-01 12:52:55 PDT
(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.
Comment 24 John Wilander 2020-05-01 12:57:19 PDT
(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.
Comment 25 John Wilander 2020-05-01 14:08:01 PDT
(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?
Comment 26 Chris Dumez 2020-05-01 14:11:48 PDT
(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?
Comment 27 John Wilander 2020-05-04 20:30:52 PDT
Created attachment 398465 [details]
Patch
Comment 28 John Wilander 2020-05-04 20:34:36 PDT
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.
Comment 29 John Wilander 2020-05-04 20:51:01 PDT
Created attachment 398468 [details]
Patch
Comment 30 John Wilander 2020-05-05 12:36:34 PDT
Created attachment 398541 [details]
Patch
Comment 31 Chris Dumez 2020-05-05 13:26:33 PDT
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()
Comment 32 John Wilander 2020-05-05 14:04:07 PDT
(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!
Comment 33 Chris Dumez 2020-05-05 14:11:21 PDT
(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 34 Chris Dumez 2020-05-05 14:37:43 PDT
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));
Comment 35 John Wilander 2020-05-05 17:30:44 PDT
Created attachment 398573 [details]
Patch
Comment 36 Chris Dumez 2020-05-06 08:01:56 PDT
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> { });
Comment 37 John Wilander 2020-05-06 10:23:42 PDT
(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 38 Chris Dumez 2020-05-06 10:27:14 PDT
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.
Comment 39 John Wilander 2020-05-06 10:50:52 PDT
(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.
Comment 40 John Wilander 2020-05-06 11:13:42 PDT
Created attachment 398638 [details]
Patch for landing
Comment 41 EWS 2020-05-06 11:44:30 PDT
Committed r261242: <https://trac.webkit.org/changeset/261242>

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