Bug 169322 – Resource Load Statistics: Communicate to the network process which domains to partition

• Component: WebKit2
• Status: RESOLVED FIXED

[John Wilander]

The UI process should let the network process know which domains it should partition cookies for.

[John Wilander]

rdar://problem/30768921

[John Wilander]

Created attachment 303770 [details]
Patch

[John Wilander]

I have a proposed test setup for this but there's something going on in partitioning between localhost and 127.0.0.1. Will investigate further.

[Brent Fulgham]

Comment on attachment 303770 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=303770&action=review

> Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:103
> +    Vector<String> prevalentResourceDomainsWithoutUserInteraction;

If 'loadedStatistics' is generally likely to be more than 16 (or whatever the default Vector size is), we should call "prevalentResourceDomainsWithoutUserInteraction.reserveCapacity(loadedStatistics.size())" to avoid having to grow the Vector multiple times during the loop.

> Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:150
> +void ResourceLoadStatisticsStore::setShouldPartitionCookiesCallback(std::function<void(const String& primaryDomain, bool value)> handler)

I think this needs to be "std::function<void(const ... etc.)>&& handler"

> Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:155
> +void ResourceLoadStatisticsStore::setShouldPartitionCookiesCallback(std::function<void(const Vector<String>& primaryDomains, bool value)> handler)

Ditto --> I think handler has to be passed by && to avoid a copy.

> Source/WebCore/platform/network/cf/NetworkStorageSessionCFNet.cpp:114
> +    for (HashSet<String>::iterator iter = m_cookiePartitioningList.begin(); iter != m_cookiePartitioningList.end(); ++iter) {

Please use a modern C++ iterator here:

for (auto& topPrivatelyOwnedDomain : m_cookiePartitioningList) {
    ...

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:213
> +    

Please undo this whitespace change.

[John Wilander]

Created attachment 303833 [details]
Patch

[John Wilander]

(In reply to comment #4)
> Comment on attachment 303770 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=303770&action=review
> 
> > Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:103
> > +    Vector<String> prevalentResourceDomainsWithoutUserInteraction;
> 
> If 'loadedStatistics' is generally likely to be more than 16 (or whatever
> the default Vector size is), we should call
> "prevalentResourceDomainsWithoutUserInteraction.
> reserveCapacity(loadedStatistics.size())" to avoid having to grow the Vector
> multiple times during the loop.

Fixed. Alex added that I can use reserveInitialCapacity() and make use of uncheckedAppend() in the population of the vector.

> > Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:150
> > +void ResourceLoadStatisticsStore::setShouldPartitionCookiesCallback(std::function<void(const String& primaryDomain, bool value)> handler)
> 
> I think this needs to be "std::function<void(const ... etc.)>&& handler"

Fixed.

> > Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:155
> > +void ResourceLoadStatisticsStore::setShouldPartitionCookiesCallback(std::function<void(const Vector<String>& primaryDomains, bool value)> handler)
> 
> Ditto --> I think handler has to be passed by && to avoid a copy.

Fixed.

> > Source/WebCore/platform/network/cf/NetworkStorageSessionCFNet.cpp:114
> > +    for (HashSet<String>::iterator iter = m_cookiePartitioningList.begin(); iter != m_cookiePartitioningList.end(); ++iter) {
> 
> Please use a modern C++ iterator here:
> 
> for (auto& topPrivatelyOwnedDomain : m_cookiePartitioningList) {
>     ...

Fixed.

> > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.cpp:213
> > +    
> 
> Please undo this whitespace change.

Fixed.

Thanks, Brent!

[John Wilander]

Created attachment 303837 [details]
Patch

[John Wilander]

Added a platform check for the network process message in WebCookieManagerProxy::setCookieStoragePartitioningEnabled() since it's only available on Cocoa platforms.

[John Wilander]

Created attachment 303844 [details]
Patch

[Alex Christensen]

Comment on attachment 303844 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=303844&action=review

> Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:151
> +void ResourceLoadStatisticsStore::setShouldPartitionCookiesCallback(std::function<void(const String& primaryDomain, bool value)>&& handler)

Function, not std::function
Maybe in followup since this is done elsewhere in this code.

> Source/WebCore/platform/network/cf/NetworkStorageSessionCFNet.cpp:147
> +    for (auto& topPrivatelyOwnedDomain : m_cookiePartitioningList) {

This is going to be quite hot code if we call this for each network request.  Is there some guarantee that the set is going to remain small?  Do we really need to iterate the set twice, or at all?  I would be ok with adding a hash lookup, but iterating a container with unlimited size will slow down browsing a lot.

> Source/WebCore/platform/network/cf/NetworkStorageSessionCFNet.cpp:152
> +    for (HashSet<String>::iterator iter = m_cookiePartitioningList.begin(); iter != m_cookiePartitioningList.end(); ++iter) {

Use c++11 for loops:
for (auto& topPrivatelyOwnedDomain : m_cookiePartitioningList) {

Also, m_cookiePartitioningList has a bad name.  I think it should be something with "hosts" in it.

> Source/WebCore/platform/network/cf/NetworkStorageSessionCFNet.cpp:164
> +void NetworkStorageSession::shouldPartitionCookiesForHost(const String& host, bool value)

setShouldPartitionCookiesForHost

> Source/WebKit2/NetworkProcess/NetworkProcess.messages.in:83
> +    ShouldPartitionCookiesForTopPrivatelyOwnedDomain(String topPrivatelyOwnedDomain, bool value)
> +    ShouldPartitionCookiesForTopPrivatelyOwnedDomains(Vector<String> topPrivatelyOwnedDomains, bool value)

Do we need both of these? Can't we just send a vector of length one for the first option?

> Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.h:66
> +    void registerSharedResourceLoadObserver(std::function<void(const String& primaryDomain, bool value)>&& shouldPartitionCookiesForDomainHandler, std::function<void(const Vector<String>& primaryDomain, bool value)>&& shouldPartitionCookiesForDomainsHandler);

Yeah, let's not add so many uses of std::function unless they really need to be copyable.

> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:1054
> +    for (auto& processPool : processPools())

This iterates just the process pools associated with this WebsiteDataStore, right?

[John Wilander]

Created attachment 303868 [details]
Patch

[John Wilander]

(In reply to comment #10)
> Comment on attachment 303844 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=303844&action=review
> 
> > Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:151
> > +void ResourceLoadStatisticsStore::setShouldPartitionCookiesCallback(std::function<void(const String& primaryDomain, bool value)>&& handler)
> 
> Function, not std::function
> Maybe in followup since this is done elsewhere in this code.

Will do in a follow-up. We have a couple as you noted.

> > Source/WebCore/platform/network/cf/NetworkStorageSessionCFNet.cpp:147
> > +    for (auto& topPrivatelyOwnedDomain : m_cookiePartitioningList) {
> 
> This is going to be quite hot code if we call this for each network request.
> Is there some guarantee that the set is going to remain small?  Do we really
> need to iterate the set twice, or at all?  I would be ok with adding a hash
> lookup, but iterating a container with unlimited size will slow down
> browsing a lot.

The double iteration was a mistake. Thanks for spotting it. After a discussion I have now changed to calculating the host's top privately controlled domain and doing a .contains() in the hash set. Should be more performant.

> > Source/WebCore/platform/network/cf/NetworkStorageSessionCFNet.cpp:152
> > +    for (HashSet<String>::iterator iter = m_cookiePartitioningList.begin(); iter != m_cookiePartitioningList.end(); ++iter) {
> 
> Use c++11 for loops:
> for (auto& topPrivatelyOwnedDomain : m_cookiePartitioningList) {

This loop is now gone.

> Also, m_cookiePartitioningList has a bad name.  I think it should be
> something with "hosts" in it.

Fixed.

> > Source/WebCore/platform/network/cf/NetworkStorageSessionCFNet.cpp:164
> > +void NetworkStorageSession::shouldPartitionCookiesForHost(const String& host, bool value)
> 
> setShouldPartitionCookiesForHost
> 
> > Source/WebKit2/NetworkProcess/NetworkProcess.messages.in:83
> > +    ShouldPartitionCookiesForTopPrivatelyOwnedDomain(String topPrivatelyOwnedDomain, bool value)
> > +    ShouldPartitionCookiesForTopPrivatelyOwnedDomains(Vector<String> topPrivatelyOwnedDomains, bool value)
> 
> Do we need both of these? Can't we just send a vector of length one for the
> first option?

I did it that way to optimize the common case which is one domain at a time. After a discussion I now have just one message type.

> > Source/WebKit2/UIProcess/WebResourceLoadStatisticsStore.h:66
> > +    void registerSharedResourceLoadObserver(std::function<void(const String& primaryDomain, bool value)>&& shouldPartitionCookiesForDomainHandler, std::function<void(const Vector<String>& primaryDomain, bool value)>&& shouldPartitionCookiesForDomainsHandler);
> 
> Yeah, let's not add so many uses of std::function unless they really need to
> be copyable.

Yup. Commented above.

> > Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:1054
> > +    for (auto& processPool : processPools())
> 
> This iterates just the process pools associated with this WebsiteDataStore,
> right?

AFAIK, yes.

[Alex Christensen]

Comment on attachment 303868 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=303868&action=review

> Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:167
> +    Vector<String> domainVector;
> +    domainVector.reserveInitialCapacity(1);
> +    domainVector.uncheckedAppend(primaryDomain);
> +    fireShouldPartitionCookiesHandler(domainVector, value);

reserveInitialCapacity isn't worth it if the capacity is less that 16 or something.  Just do an append.  Or even better, use an initializer list:
fireShouldPartitionCookiesHandler({primaryDomain}, value);

> Source/WebCore/platform/network/cf/NetworkStorageSessionCFNet.cpp:152
> +    auto domain = topPrivatelyControlledDomain(host);
> +    if (domain.isEmpty())
> +        domain = host;

Is this for localhost?  I'm not sure this is a good idea.  Maybe just return false if there's no top privately controlled domain.

> Source/WebCore/platform/network/cf/NetworkStorageSessionCFNet.cpp:155
> +    if (m_topPrivatelyControlledDomainsForCookiePartitioning.contains(domain))
> +        return true;

just return the result of contains.

> Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:1226
> +        [this] (const Vector<String>& topPrivatelyOwnedDomains, bool value) {

When the WebsiteDataStore gets destroyed, it tells the ResourceLoadStatistics that it is going to be destroyed, so this captured this pointer won't be used again, right?

> Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:1354
> +    for (size_t i = 0; i < keys.size(); ++i) {

This could be a c++11-style for loop, too

[John Wilander]

Created attachment 303880 [details]
Patch

[John Wilander]

(In reply to comment #13)
> Comment on attachment 303868 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=303868&action=review
> 
> > Source/WebCore/loader/ResourceLoadStatisticsStore.cpp:167
> > +    Vector<String> domainVector;
> > +    domainVector.reserveInitialCapacity(1);
> > +    domainVector.uncheckedAppend(primaryDomain);
> > +    fireShouldPartitionCookiesHandler(domainVector, value);
> 
> reserveInitialCapacity isn't worth it if the capacity is less that 16 or
> something.  Just do an append.  Or even better, use an initializer list:
> fireShouldPartitionCookiesHandler({primaryDomain}, value);

Doing it with an initializer list confuses at least Xcode that the function will call itself. I did it with an explicit vector.

> > Source/WebCore/platform/network/cf/NetworkStorageSessionCFNet.cpp:152
> > +    auto domain = topPrivatelyControlledDomain(host);
> > +    if (domain.isEmpty())
> > +        domain = host;
> 
> Is this for localhost?  I'm not sure this is a good idea.  Maybe just return
> false if there's no top privately controlled domain.

I can't test partitioning if localhost gets the empty partition. Instead filed follow-up bug for fixing this in ResourceRequestBase::partitionName() too: https://bugs.webkit.org/show_bug.cgi?id=169399

> > Source/WebCore/platform/network/cf/NetworkStorageSessionCFNet.cpp:155
> > +    if (m_topPrivatelyControlledDomainsForCookiePartitioning.contains(domain))
> > +        return true;
> 
> just return the result of contains.

Fixed.

> > Source/WebKit2/UIProcess/WebsiteData/WebsiteDataStore.cpp:1226
> > +        [this] (const Vector<String>& topPrivatelyOwnedDomains, bool value) {
> 
> When the WebsiteDataStore gets destroyed, it tells the
> ResourceLoadStatistics that it is going to be destroyed, so this captured
> this pointer won't be used again, right?

I cannot see any other use of it, no.

> > Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:1354
> > +    for (size_t i = 0; i < keys.size(); ++i) {
> 
> This could be a c++11-style for loop, too

Can it? I'm using the index i for two different vectors, keys and values.

Thanks for the review and all the enhancements, Alex!

[WebKit Commit Bot]

Comment on attachment 303880 [details]
Patch

Clearing flags on attachment: 303880

Committed r213623: <http://trac.webkit.org/changeset/213623>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.