[GTK][WPE] Add API to configure and enable resource load statistics

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

To WebKitWebsiteDataManager.

[Carlos Garcia Campos]

Created attachment 322837 [details]
Patch

[Build Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Michael Catanzaro]

Comment on attachment 322837 [details]
Patch

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

You'll add an Ephy preference too?

> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:313
> +    configuration.resourceLoadStatisticsDirectory = processPoolconfigurarion.resourceLoadStatisticsDirectory();

You should rename it to processPoolConfiguration in a follow-up.

> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:336
> +        configuration.setResourceLoadStatisticsDirectory(WebCore::stringFromFileSystemRepresentation(webkit_website_data_manager_get_resource_load_stats_directory(priv->websiteDataManager.get())));

I know I approved this earlier, and you probably won't like me taking it back, but seeing it written out, it just doesn't feel as nice as and professional as webkit_website_data_manager_get_resource_load_statistics_directory(). I don't think it's a problem that the function name is a bit long.

> Source/WebKit/UIProcess/API/glib/WebKitWebsiteDataManager.cpp:87
> +    PROP_RESOURCE_LOAD_STATS_DIRECTORY,

...so I'd prefer to write it out as STATISTICS everywhere.

[Carlos Garcia Campos]

Committed r222967: <http://trac.webkit.org/changeset/222967>

[Carlos Garcia Campos]

Committed r232010: <https://trac.webkit.org/changeset/232010>

[Carlos Garcia Campos]

Rolled out in r232010: <https://trac.webkit.org/changeset/232010>

[Carlos Garcia Campos]

Created attachment 396270 [details]
Patch

[Michael Catanzaro]

Comment on attachment 396270 [details]
Patch

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

> Source/WebKit/UIProcess/API/glib/WebKitWebsiteDataManager.cpp:735
> + * webkit_website_data_manager_set_resource_load_statistics_enabled:

I'm OK with this, but it's important to understand that the WebsiteDataStore-level API is a bit misnamed. It controls all of ITP, not just resource load statistics. E.g. this will probably cause third-party cookies to be blocked regardless of the WebKitCookieAcceptPolicy(?). It will certainly cause Referer headers to be stripped. Etc. A less-misleading name might be: webkit_website_data_manager_set_intelligent_tracking_prevention_enabled(). But then it would feel more at home on the WebKitWebContext, rather than WebKitWebsiteDataManager. But that would be the wrong place, because internally it really is a property of the WebsiteDataStore, not the WebProcessPool.

Conclusion: *shrug*. Maybe we could keep the current name but document that it has additional effects in addition to enabling statistics tracking? I don't know, just something to think about.

I would investigate the impact on cookie policy regardless. We might want to use a g_warning() if the WebKitCookiePolicy is inconsistent with ITP?

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #8)
> Comment on attachment 396270 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=396270&action=review
> 
> > Source/WebKit/UIProcess/API/glib/WebKitWebsiteDataManager.cpp:735
> > + * webkit_website_data_manager_set_resource_load_statistics_enabled:
> 
> I'm OK with this, but it's important to understand that the
> WebsiteDataStore-level API is a bit misnamed. It controls all of ITP, not
> just resource load statistics.

That's a good point. We could move this to the WebKitCookieManager if it makes more sense. I also wonder about other public API in WebsiteDataStore like isPrevalentResource, setPrevalentResource, clearPrevalentResource, setUseITPDatabase, etc. Are those only for testing? If we are going to expose more API than just enable/disable ITP we might consider adding an ITP manager.

> E.g. this will probably cause third-party
> cookies to be blocked regardless of the WebKitCookieAcceptPolicy(?). It will
> certainly cause Referer headers to be stripped. Etc. A less-misleading name
> might be:
> webkit_website_data_manager_set_intelligent_tracking_prevention_enabled().
> But then it would feel more at home on the WebKitWebContext, rather than
> WebKitWebsiteDataManager. But that would be the wrong place, because
> internally it really is a property of the WebsiteDataStore, not the
> WebProcessPool.

I don't know much about ITP, to be honest.

> Conclusion: *shrug*. Maybe we could keep the current name but document that
> it has additional effects in addition to enabling statistics tracking? I
> don't know, just something to think about.
> 
> I would investigate the impact on cookie policy regardless. We might want to
> use a g_warning() if the WebKitCookiePolicy is inconsistent with ITP?

I don't even know how to test it apart from running layout tests.

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #9) 
> That's a good point. We could move this to the WebKitCookieManager if it
> makes more sense. I also wonder about other public API in WebsiteDataStore
> like isPrevalentResource, setPrevalentResource, clearPrevalentResource,
> setUseITPDatabase, etc. Are those only for testing? If we are going to
> expose more API than just enable/disable ITP we might consider adding an ITP
> manager.

ITP is more than just cookies, though. Besides stripping Referers and overriding the cookie policy, it will also wipe localstorage and IndexedDB after a certain number of days (one week?) when the domain is "prevalent." So WebKitCookieManager doesn't seem to be the right place either. I guess WebKitWebsiteDataManager is probably best, but we could give it a different name, e.g. webkit_website_data_manager_set_intelligent_tracking_prevention_enabled(). And then just document that this function can enable cookie policy that is more restrictive than that set by webkit_cookie_manager_set_accept_policy().

I would not expose [is,set,clear]PrevalentResource or setUseITPDatabase. We can always expose more in the future if an application ever wants to use them.
 
> > Conclusion: *shrug*. Maybe we could keep the current name but document that
> > it has additional effects in addition to enabling statistics tracking? I
> > don't know, just something to think about.
> > 
> > I would investigate the impact on cookie policy regardless. We might want to
> > use a g_warning() if the WebKitCookiePolicy is inconsistent with ITP?
> 
> I don't even know how to test it apart from running layout tests.

I could check with my bank's bill pay website... I don't use it because it breaks with third-party cookies disabled. So if ITP is enabled and cookie policy is set to always accept cookies, the always accept cookies should effectively be overridden.

I also found https://github.com/speedarius/third-party-cookie-tester, but that looks like some effort to get set up.

I wonder how we would expose this in Epiphany's preferences dialog. Currently we have a simple tri-state policy: always accept, block third party (default), never accept. I guess we could change it to a boolean enable ITP or disable ITP setting. Probably Epiphany is not the right browser to use if you're interested in disabling cookies entirely, right?

[John Wilander]

FWIW, I intend to rename ResourceLoadStatistics to IntelligentTrackingPrevention or maybe just ITP for brevity. Just haven't gotten around to it. I also hope to get rid of all or most of the compile-time stuff.

As for the effects of turning on ITP, several of the things you mention Michael, are Cocoa-implementations which need a non-Cocoa counterpart to work. Cookie blocking and referrer downgrades are such examples.

[Michael Catanzaro]

(In reply to John Wilander from comment #11)
> FWIW, I intend to rename ResourceLoadStatistics to
> IntelligentTrackingPrevention or maybe just ITP for brevity. Just haven't
> gotten around to it. I also hope to get rid of all or most of the
> compile-time stuff.

In that case, maybe webkit_website_data_manager_set_intelligent_tracking_prevention_enabled() should be OK. What about the directory on disk, is "resourceloadstatistics" still a good name for the directory? Maybe "itp" would be a better name, in case it stores other stuff there in the future?
 
> As for the effects of turning on ITP, several of the things you mention
> Michael, are Cocoa-implementations which need a non-Cocoa counterpart to
> work. Cookie blocking and referrer downgrades are such examples.

Hm, if we know what needs implemented for soup ports, then we can implement. But we can't implement what we don't know needs implemented. :) So if you know of anything else besides these that you think we should look into, it would be great to know.

That said, I assume Referer stripping and cookie blocking are both probably already implemented by Carlos's patches, because there should be layout tests that would fail otherwise, yes?

[John Wilander]

(In reply to Michael Catanzaro from comment #12)
> (In reply to John Wilander from comment #11)
> > FWIW, I intend to rename ResourceLoadStatistics to
> > IntelligentTrackingPrevention or maybe just ITP for brevity. Just haven't
> > gotten around to it. I also hope to get rid of all or most of the
> > compile-time stuff.
> 
> In that case, maybe
> webkit_website_data_manager_set_intelligent_tracking_prevention_enabled()
> should be OK. What about the directory on disk, is "resourceloadstatistics"
> still a good name for the directory? Maybe "itp" would be a better name, in
> case it stores other stuff there in the future?
>  
> > As for the effects of turning on ITP, several of the things you mention
> > Michael, are Cocoa-implementations which need a non-Cocoa counterpart to
> > work. Cookie blocking and referrer downgrades are such examples.
> 
> Hm, if we know what needs implemented for soup ports, then we can implement.
> But we can't implement what we don't know needs implemented. :) So if you
> know of anything else besides these that you think we should look into, it
> would be great to know.

Except for HSTS (as explained above), it should all be covered by tests. Pass those and you should have all the platform-specific bits.

> That said, I assume Referer stripping and cookie blocking are both probably
> already implemented by Carlos's patches, because there should be layout
> tests that would fail otherwise, yes?

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #10)
> (In reply to Carlos Garcia Campos from comment #9) 
> > That's a good point. We could move this to the WebKitCookieManager if it
> > makes more sense. I also wonder about other public API in WebsiteDataStore
> > like isPrevalentResource, setPrevalentResource, clearPrevalentResource,
> > setUseITPDatabase, etc. Are those only for testing? If we are going to
> > expose more API than just enable/disable ITP we might consider adding an ITP
> > manager.
> 
> ITP is more than just cookies, though. Besides stripping Referers and
> overriding the cookie policy, it will also wipe localstorage and IndexedDB
> after a certain number of days (one week?) when the domain is "prevalent."
> So WebKitCookieManager doesn't seem to be the right place either. I guess
> WebKitWebsiteDataManager is probably best, but we could give it a different
> name, e.g.
> webkit_website_data_manager_set_intelligent_tracking_prevention_enabled().
> And then just document that this function can enable cookie policy that is
> more restrictive than that set by webkit_cookie_manager_set_accept_policy().

webkit_website_data_manager_set_intelligent_tracking_prevention_enabled() sounds good to me.

> I would not expose [is,set,clear]PrevalentResource or setUseITPDatabase. We
> can always expose more in the future if an application ever wants to use
> them.

Those were just examples, there are a lot of public methods in WebsitedataStore related to ITP and I have no idea what they are for or if they are just there for testing.
 
> > > Conclusion: *shrug*. Maybe we could keep the current name but document that
> > > it has additional effects in addition to enabling statistics tracking? I
> > > don't know, just something to think about.
> > > 
> > > I would investigate the impact on cookie policy regardless. We might want to
> > > use a g_warning() if the WebKitCookiePolicy is inconsistent with ITP?
> > 
> > I don't even know how to test it apart from running layout tests.
> 
> I could check with my bank's bill pay website... I don't use it because it
> breaks with third-party cookies disabled. So if ITP is enabled and cookie
> policy is set to always accept cookies, the always accept cookies should
> effectively be overridden.
> 
> I also found https://github.com/speedarius/third-party-cookie-tester, but
> that looks like some effort to get set up.
> 
> I wonder how we would expose this in Epiphany's preferences dialog.
> Currently we have a simple tri-state policy: always accept, block third
> party (default), never accept. I guess we could change it to a boolean
> enable ITP or disable ITP setting. Probably Epiphany is not the right
> browser to use if you're interested in disabling cookies entirely, right?

Shouldn't we just enable it unconditionally?

[Carlos Garcia Campos]

(In reply to John Wilander from comment #13)
> (In reply to Michael Catanzaro from comment #12)
> > (In reply to John Wilander from comment #11)
> > > FWIW, I intend to rename ResourceLoadStatistics to
> > > IntelligentTrackingPrevention or maybe just ITP for brevity. Just haven't
> > > gotten around to it. I also hope to get rid of all or most of the
> > > compile-time stuff.
> > 
> > In that case, maybe
> > webkit_website_data_manager_set_intelligent_tracking_prevention_enabled()
> > should be OK. What about the directory on disk, is "resourceloadstatistics"
> > still a good name for the directory? Maybe "itp" would be a better name, in
> > case it stores other stuff there in the future?
> >  
> > > As for the effects of turning on ITP, several of the things you mention
> > > Michael, are Cocoa-implementations which need a non-Cocoa counterpart to
> > > work. Cookie blocking and referrer downgrades are such examples.
> > 
> > Hm, if we know what needs implemented for soup ports, then we can implement.
> > But we can't implement what we don't know needs implemented. :) So if you
> > know of anything else besides these that you think we should look into, it
> > would be great to know.
> 
> Except for HSTS (as explained above), it should all be covered by tests.
> Pass those and you should have all the platform-specific bits.

Yes, I think I implemented everything except isolated sessions in bug #210184 and the only tests failing are:

http/tests/resourceLoadStatistics/switch-session-on-navigation-to-prevalent-with-interaction-database.php
http/tests/resourceLoadStatistics/switch-session-on-navigation-to-prevalent-with-interaction.php

> > That said, I assume Referer stripping and cookie blocking are both probably
> > already implemented by Carlos's patches, because there should be layout
> > tests that would fail otherwise, yes?

[Adrian Perez]

(In reply to Carlos Garcia Campos from comment #14)
> (In reply to Michael Catanzaro from comment #10)
> > (In reply to Carlos Garcia Campos from comment #9) 
> > > That's a good point. We could move this to the WebKitCookieManager if it
> > > makes more sense. I also wonder about other public API in WebsiteDataStore
> > > like isPrevalentResource, setPrevalentResource, clearPrevalentResource,
> > > setUseITPDatabase, etc. Are those only for testing? If we are going to
> > > expose more API than just enable/disable ITP we might consider adding an ITP
> > > manager.
> > 
> > ITP is more than just cookies, though. Besides stripping Referers and
> > overriding the cookie policy, it will also wipe localstorage and IndexedDB
> > after a certain number of days (one week?) when the domain is "prevalent."
> > So WebKitCookieManager doesn't seem to be the right place either. I guess
> > WebKitWebsiteDataManager is probably best, but we could give it a different
> > name, e.g.
> > webkit_website_data_manager_set_intelligent_tracking_prevention_enabled().
> > And then just document that this function can enable cookie policy that is
> > more restrictive than that set by webkit_cookie_manager_set_accept_policy().
> 
> webkit_website_data_manager_set_intelligent_tracking_prevention_enabled()
> sounds good to me.

Wow, this is a mouthful! If nobody is against it, I would prefer to use
“webkit_website_data_manager_set_itp_enabled()”—and that still remains
pretty long =)
 
> > I would not expose [is,set,clear]PrevalentResource or setUseITPDatabase. We
> > can always expose more in the future if an application ever wants to use
> > them.
> 
> Those were just examples, there are a lot of public methods in
> WebsitedataStore related to ITP and I have no idea what they are for or if
> they are just there for testing.
>  
> > > > Conclusion: *shrug*. Maybe we could keep the current name but document that
> > > > it has additional effects in addition to enabling statistics tracking? I
> > > > don't know, just something to think about.
> > > > 
> > > > I would investigate the impact on cookie policy regardless. We might want to
> > > > use a g_warning() if the WebKitCookiePolicy is inconsistent with ITP?
> > > 
> > > I don't even know how to test it apart from running layout tests.
> > 
> > I could check with my bank's bill pay website... I don't use it because it
> > breaks with third-party cookies disabled. So if ITP is enabled and cookie
> > policy is set to always accept cookies, the always accept cookies should
> > effectively be overridden.
> > 
> > I also found https://github.com/speedarius/third-party-cookie-tester, but
> > that looks like some effort to get set up.
> > 
> > I wonder how we would expose this in Epiphany's preferences dialog.
> > Currently we have a simple tri-state policy: always accept, block third
> > party (default), never accept. I guess we could change it to a boolean
> > enable ITP or disable ITP setting. Probably Epiphany is not the right
> > browser to use if you're interested in disabling cookies entirely, right?
> 
> Shouldn't we just enable it unconditionally?

Do you mean in WebKit{GTK,WPE} or in Epiphany? I would have it enabled by
default on both. For WebKit, as long as ITP can be disabled using the API,
having it enabled by default sounds good to me—it's good to make defaults
lean towards the more secure or privacy-respecting settings, so developers
embedding a WebKitWebView in an application get the “best” settings without
needing additional work. Dunno if Epiphany can have the setting always-on,
but we could have a setting without checkbox in the preferences window and
try it out for a while before taking a final decision.

[Adrian Perez]

By the way, great work getting ITP to work for GTK/WPE \o/

[Carlos Garcia Campos]

(In reply to Adrian Perez from comment #16)
> (In reply to Carlos Garcia Campos from comment #14)
> > (In reply to Michael Catanzaro from comment #10)
> > > (In reply to Carlos Garcia Campos from comment #9) 
> > > > That's a good point. We could move this to the WebKitCookieManager if it
> > > > makes more sense. I also wonder about other public API in WebsiteDataStore
> > > > like isPrevalentResource, setPrevalentResource, clearPrevalentResource,
> > > > setUseITPDatabase, etc. Are those only for testing? If we are going to
> > > > expose more API than just enable/disable ITP we might consider adding an ITP
> > > > manager.
> > > 
> > > ITP is more than just cookies, though. Besides stripping Referers and
> > > overriding the cookie policy, it will also wipe localstorage and IndexedDB
> > > after a certain number of days (one week?) when the domain is "prevalent."
> > > So WebKitCookieManager doesn't seem to be the right place either. I guess
> > > WebKitWebsiteDataManager is probably best, but we could give it a different
> > > name, e.g.
> > > webkit_website_data_manager_set_intelligent_tracking_prevention_enabled().
> > > And then just document that this function can enable cookie policy that is
> > > more restrictive than that set by webkit_cookie_manager_set_accept_policy().
> > 
> > webkit_website_data_manager_set_intelligent_tracking_prevention_enabled()
> > sounds good to me.
> 
> Wow, this is a mouthful! If nobody is against it, I would prefer to use
> “webkit_website_data_manager_set_itp_enabled()”—and that still remains
> pretty long =)

I'm fine with using itp in this case.

> > > I would not expose [is,set,clear]PrevalentResource or setUseITPDatabase. We
> > > can always expose more in the future if an application ever wants to use
> > > them.
> > 
> > Those were just examples, there are a lot of public methods in
> > WebsitedataStore related to ITP and I have no idea what they are for or if
> > they are just there for testing.
> >  
> > > > > Conclusion: *shrug*. Maybe we could keep the current name but document that
> > > > > it has additional effects in addition to enabling statistics tracking? I
> > > > > don't know, just something to think about.
> > > > > 
> > > > > I would investigate the impact on cookie policy regardless. We might want to
> > > > > use a g_warning() if the WebKitCookiePolicy is inconsistent with ITP?
> > > > 
> > > > I don't even know how to test it apart from running layout tests.
> > > 
> > > I could check with my bank's bill pay website... I don't use it because it
> > > breaks with third-party cookies disabled. So if ITP is enabled and cookie
> > > policy is set to always accept cookies, the always accept cookies should
> > > effectively be overridden.
> > > 
> > > I also found https://github.com/speedarius/third-party-cookie-tester, but
> > > that looks like some effort to get set up.
> > > 
> > > I wonder how we would expose this in Epiphany's preferences dialog.
> > > Currently we have a simple tri-state policy: always accept, block third
> > > party (default), never accept. I guess we could change it to a boolean
> > > enable ITP or disable ITP setting. Probably Epiphany is not the right
> > > browser to use if you're interested in disabling cookies entirely, right?
> > 
> > Shouldn't we just enable it unconditionally?
> 
> Do you mean in WebKit{GTK,WPE} or in Epiphany? I would have it enabled by
> default on both. For WebKit, as long as ITP can be disabled using the API,
> having it enabled by default sounds good to me—it's good to make defaults
> lean towards the more secure or privacy-respecting settings, so developers
> embedding a WebKitWebView in an application get the “best” settings without
> needing additional work. Dunno if Epiphany can have the setting always-on,
> but we could have a setting without checkbox in the preferences window and
> try it out for a while before taking a final decision.

I meant epiphany, in WebKit it's probably better to keep it disabled by default to ensure backwards compatibility.

[Adrian Perez]

(In reply to Carlos Garcia Campos from comment #18)
> (In reply to Adrian Perez from comment #16)
> > (In reply to Carlos Garcia Campos from comment #14)
> > > (In reply to Michael Catanzaro from comment #10)
> > > > (In reply to Carlos Garcia Campos from comment #9) 
> > > >
> > > > > […]
> > > >
> > > > I wonder how we would expose this in Epiphany's preferences dialog.
> > > > Currently we have a simple tri-state policy: always accept, block third
> > > > party (default), never accept. I guess we could change it to a boolean
> > > > enable ITP or disable ITP setting. Probably Epiphany is not the right
> > > > browser to use if you're interested in disabling cookies entirely, right?
> > > 
> > > Shouldn't we just enable it unconditionally?
> > 
> > Do you mean in WebKit{GTK,WPE} or in Epiphany? I would have it enabled by
> > default on both. For WebKit, as long as ITP can be disabled using the API,
> > having it enabled by default sounds good to me—it's good to make defaults
> > lean towards the more secure or privacy-respecting settings, so developers
> > embedding a WebKitWebView in an application get the “best” settings without
> > needing additional work. Dunno if Epiphany can have the setting always-on,
> > but we could have a setting without checkbox in the preferences window and
> > try it out for a while before taking a final decision.
> 
> I meant epiphany, in WebKit it's probably better to keep it disabled by
> default to ensure backwards compatibility.

Right, that's true, we should keep ITP disabled for API 4.0 (GTK3); but maybe
we can make it enabled by default on the next API break 😉️

[Michael Catanzaro]

> Those were just examples, there are a lot of public methods in
> WebsitedataStore related to ITP and I have no idea what they are for or if
> they are just there for testing.

Many of them are only intended for testing. Others might be useful for building an ITP debugger feature, but for now I think it's safest to not expose any of them.
> Shouldn't we just enable it unconditionally?

Well we could, but it probably makes sense to allow users to disable it in case it breaks websites. That's why we have the cookie policy setting, after all. If users would complain about third-party cookie blocking breaking websites, they'll definitely complain about ITP.

[Michael Catanzaro]

I wouldn't enable it by default in WebKit ever. But we'll enable by default in Epiphany, of course, to match what Safari does.

[Carlos Garcia Campos]

Created attachment 401892 [details]
Patch for landing

[Carlos Garcia Campos]

Created attachment 401893 [details]
Patch for landing

[Carlos Garcia Campos]

I've started to work on the ephy side using this new API (and the storage access permission).

[Carlos Garcia Campos]

Committed r263085: <https://trac.webkit.org/changeset/263085>