Resource Load Statistics (experimental): Clear non-cookie website data for sites that have been navigated to, with link decoration, by a prevalent resource

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[John Wilander]

Cross-site trackers abuse link query parameters to transport user identifiers and then store them in first-party storage space.

https://bugs.webkit.org/show_bug.cgi?id=189933 capped all persistent client-side cookies to seven days of storage.
https://bugs.webkit.org/show_bug.cgi?id=195196 capped persistent client-side cookies to one day of storage for navigations with link decoration from prevalent resources.
https://bugs.webkit.org/show_bug.cgi?id=195301 added logging of navigations with link decoration from prevalent resources.

We should clear out non-cookie website data for sites that have been navigated to, with link decoration, by a prevalent resource. This makes sure tracker scripts cannot force first-party sites to store cross-site tracking data transferred in such navigations.

[Radar WebKit Bug Importer]

<rdar://problem/49001272>

[John Wilander]

Created attachment 365096 [details]
Patch

[Alex Christensen]

Comment on attachment 365096 [details]
Patch

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

> Source/WebCore/page/RuntimeEnabledFeatures.h:543
> +    bool m_isITPFirstPartyWebsiteDataRemovalEnabled { true };

DEFAULT_EXPERIMENTAL_FEATURES_ENABLED here?

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.h:163
> +    void registrableDomainsToRemoveWebsiteDataFor(HashMap<RegistrableDomain, WebsiteDataToRemove>&) override;

This should return a value instead of returning by reference.

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.cpp:66
> +static String domainsToString(HashMap<RegistrableDomain, WebsiteDataToRemove>& domainsToRemoveWebsiteDataFor)

const HashMap&

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.h:75
> +constexpr unsigned operatingDatesWindowLong { 30 };
> +constexpr unsigned operatingDatesWindowShort { 7 };

I think operatingDatesLongWindow operatingDatesShortWindow would be better names for these.  Long and short are adjectives that modify Window.  If they're at the end, it looks to me like the type (unsigned long vs unsigned short).

Even better, I think, would be to have this be an enum class:
enum class OperatingDatesWindow : bool { Long, Short };

Then most places would just pass one or the other and in one place we would need constants for how many days the enum values mean.

> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.h:63
> +enum class WebsiteDataToRemove {

: uint8_t

[John Wilander]

(In reply to Alex Christensen from comment #3)
> Comment on attachment 365096 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=365096&action=review
> 
> > Source/WebCore/page/RuntimeEnabledFeatures.h:543
> > +    bool m_isITPFirstPartyWebsiteDataRemovalEnabled { true };
> 
> DEFAULT_EXPERIMENTAL_FEATURES_ENABLED here?

You're right. Should be false. I wrote the code in the wrong order.

> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.h:163
> > +    void registrableDomainsToRemoveWebsiteDataFor(HashMap<RegistrableDomain, WebsiteDataToRemove>&) override;
> 
> This should return a value instead of returning by reference.

I assume you mean for all instances of this function.

> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.cpp:66
> > +static String domainsToString(HashMap<RegistrableDomain, WebsiteDataToRemove>& domainsToRemoveWebsiteDataFor)
> 
> const HashMap&

Got it.

> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.h:75
> > +constexpr unsigned operatingDatesWindowLong { 30 };
> > +constexpr unsigned operatingDatesWindowShort { 7 };
> 
> I think operatingDatesLongWindow operatingDatesShortWindow would be better
> names for these.  Long and short are adjectives that modify Window.  If
> they're at the end, it looks to me like the type (unsigned long vs unsigned
> short).

Agreed.

> Even better, I think, would be to have this be an enum class:
> enum class OperatingDatesWindow : bool { Long, Short };

Ah. I'll take a stab at this.

> Then most places would just pass one or the other and in one place we would
> need constants for how many days the enum values mean.
> 
> > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.h:63
> > +enum class WebsiteDataToRemove {
> 
> : uint8_t

Got it.

Thanks, Alex!

[John Wilander]

(In reply to John Wilander from comment #4)
> (In reply to Alex Christensen from comment #3)
> > Comment on attachment 365096 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=365096&action=review
> > 
> > > Source/WebCore/page/RuntimeEnabledFeatures.h:543
> > > +    bool m_isITPFirstPartyWebsiteDataRemovalEnabled { true };
> > 
> > DEFAULT_EXPERIMENTAL_FEATURES_ENABLED here?
> 
> You're right. Should be false. I wrote the code in the wrong order.
> 
> > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.h:163
> > > +    void registrableDomainsToRemoveWebsiteDataFor(HashMap<RegistrableDomain, WebsiteDataToRemove>&) override;
> > 
> > This should return a value instead of returning by reference.
> 
> I assume you mean for all instances of this function.

Looking at it now I realized why I ended up where I did. I started out with two vectors – one for remove all data and one for all but cookies. Since I couldn't return two vectors I did it by reference. But then everything became much more complicated and I ran the risk of mixing up the two vectors. Finally, I realized I want to support three different filters for data removal. That's why I switched to a HashMap.

Thanks for pointing this out.

> > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.cpp:66
> > > +static String domainsToString(HashMap<RegistrableDomain, WebsiteDataToRemove>& domainsToRemoveWebsiteDataFor)
> > 
> > const HashMap&
> 
> Got it.
> 
> > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.h:75
> > > +constexpr unsigned operatingDatesWindowLong { 30 };
> > > +constexpr unsigned operatingDatesWindowShort { 7 };
> > 
> > I think operatingDatesLongWindow operatingDatesShortWindow would be better
> > names for these.  Long and short are adjectives that modify Window.  If
> > they're at the end, it looks to me like the type (unsigned long vs unsigned
> > short).
> 
> Agreed.
> 
> > Even better, I think, would be to have this be an enum class:
> > enum class OperatingDatesWindow : bool { Long, Short };
> 
> Ah. I'll take a stab at this.
> 
> > Then most places would just pass one or the other and in one place we would
> > need constants for how many days the enum values mean.
> > 
> > > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.h:63
> > > +enum class WebsiteDataToRemove {
> > 
> > : uint8_t
> 
> Got it.
> 
> Thanks, Alex!

[John Wilander]

Created attachment 365203 [details]
Patch

[Alex Christensen]

Comment on attachment 365203 [details]
Patch

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

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:1519
> +    HashMap<RegistrableDomain, WebsiteDataToRemove> domainsToRemoveWebsiteDataFor;

If we used a struct that contained 3 Vector<RegistrableDomain> instead of the enum, it would be more memory efficient.

> Tools/WebKitTestRunner/TestInvocation.cpp:1311
> +        WKRetainPtr<WKStringRef> fromHostKey(AdoptWK, WKStringCreateWithUTF8CString("FromHost"));

auto fromHostKey = adoptWK(WKStringCreateWithUTF8CString("FromHost"));

[John Wilander]

Created attachment 365246 [details]
Patch for landing

[John Wilander]

(In reply to Alex Christensen from comment #7)
> Comment on attachment 365203 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=365203&action=review
> 
> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:1519
> > +    HashMap<RegistrableDomain, WebsiteDataToRemove> domainsToRemoveWebsiteDataFor;
> 
> If we used a struct that contained 3 Vector<RegistrableDomain> instead of
> the enum, it would be more memory efficient.

Good idea. Will do in a follow up, tracked in rdar://problem/49038934.

> > Tools/WebKitTestRunner/TestInvocation.cpp:1311
> > +        WKRetainPtr<WKStringRef> fromHostKey(AdoptWK, WKStringCreateWithUTF8CString("FromHost"));
> 
> auto fromHostKey = adoptWK(WKStringCreateWithUTF8CString("FromHost"));

Fixed.

Thanks for the review, Alex!

[John Wilander]

Test failure is unrelated.

[WebKit Commit Bot]

Comment on attachment 365246 [details]
Patch for landing

Clearing flags on attachment: 365246

Committed r243181: <https://trac.webkit.org/changeset/243181>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Truitt Savell]

Looks like the new test http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-link-decoration.html

added in https://trac.webkit.org/changeset/243181/webkit

is failing constantly on WK1. History:
http://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2FresourceLoadStatistics%2Fwebsite-data-removal-for-site-navigated-to-with-link-decoration.html

Diff:
--- /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-link-decoration-expected.txt
+++ /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-link-decoration-actual.txt
@@ -10,7 +10,7 @@
 After deletion: Client-side cookie exists.
 After deletion: Regular server-side cookie exists.
 
-After deletion: IDB entry does not exist.
+After deletion: IDB entry does exist.
 
 
 Resource load statistics:
@@ -30,6 +30,6 @@
     grandfathered: No
     topFrameLinkDecorationsFrom:
         localhost
-    gotLinkDecorationFromPrevalentResource: No    isPrevalentResource: No
+    gotLinkDecorationFromPrevalentResource: Yes    isPrevalentResource: No
     isVeryPrevalentResource: No
-    dataRecordsRemoved: 1
+    dataRecordsRemoved: 0

[Truitt Savell]

Apologies, Failing constantly on WK2

[John Wilander]

OK, looking at this.

[John Wilander]

I found the bug. ITP refuses (and should refuse) to remove website data repeatedly because of the minimumTimeBetweenDataRecordsRemoval setting. I just need to add an exception to this rule for parameters().isRunningTest.

[John Wilander]

(In reply to John Wilander from comment #16)
> I found the bug. ITP refuses (and should refuse) to remove website data
> repeatedly because of the minimumTimeBetweenDataRecordsRemoval setting. I
> just need to add an exception to this rule for parameters().isRunningTest.

Fix is on the commit queue: https://bugs.webkit.org/show_bug.cgi?id=196017