Bug 213291 – Add support for fetching registrable domains with resource load statistics

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Carlos Garcia Campos]

WebsiteDataStore::fetchData() doesn't return anything for resource load statistics because NetworkProcess::fetchWebsiteData() doesn't handle WebsiteDataType::ResourceLoadStatistics

[Carlos Garcia Campos]

Created attachment 402105 [details]
Patch

[John Wilander]

Comment on attachment 402105 [details]
Patch

Just as a general comment, we've struggled with the term website data here because ITP data isn't website data, i.e. it isn't data that the website creates but that WebKit creates *about* websites. That is in part why this has not been folded into the reporting structure.

[Adrian Perez]

Comment on attachment 402105 [details]
Patch

Patch looks reasonable, but the API tests are failing. Could you take
check that up, please?

[Carlos Garcia Campos]

(In reply to John Wilander from comment #2)
> Comment on attachment 402105 [details]
> Patch
> 
> Just as a general comment, we've struggled with the term website data here
> because ITP data isn't website data, i.e. it isn't data that the website
> creates but that WebKit creates *about* websites. That is in part why this
> has not been folded into the reporting structure.

I see. I still think it makes sense to allow the user to remove the resource load stats, like other (website) data and caches. In epiphany we always query the data  before showing the option to remove it. In the case of itp data we don't know if there's data to remove. That's how I noticed this.

[Carlos Garcia Campos]

(In reply to Adrian Perez from comment #3)
> Comment on attachment 402105 [details]
> Patch
> 
> Patch looks reasonable, but the API tests are failing. Could you take
> check that up, please?

I'll check, tests pass for me locally. It might be flaky.

[Carlos Garcia Campos]

The API test failure could also be because of bug #213290, if there are registrations in the default location in the system, I'll run EWS again after landing that patch.

[Adrian Perez]

(In reply to Carlos Garcia Campos from comment #6)
> The API test failure could also be because of bug #213290, if there are
> registrations in the default location in the system, I'll run EWS again
> after landing that patch.

I suppose it would make sense to reset to a known state (in this case:
remove all) at the beginning of the API tests, before doing checks :)

[Adrian Perez]

Comment on attachment 402105 [details]
Patch

r=me, but let's please make sure the tests pass before landing :]

[Carlos Garcia Campos]

(In reply to Adrian Perez from comment #8)
> Comment on attachment 402105 [details]
> Patch
> 
> r=me, but let's please make sure the tests pass before landing :]

Sure, I'll also wait for WebKit2 owners to confirm they are ok with this change.

[Adrian Perez]

(In reply to Carlos Garcia Campos from comment #9)
> (In reply to Adrian Perez from comment #8)
> > Comment on attachment 402105 [details]
> > Patch
> > 
> > r=me, but let's please make sure the tests pass before landing :]
> 
> Sure, I'll also wait for WebKit2 owners to confirm they are ok with this
> change.

Perfect, thanks!

[youenn fablet]

Comment on attachment 402105 [details]
Patch

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

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.h:147
> +    Vector<RegistrableDomain> allDomains() const override;

final?

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.h:116
> +    Vector<RegistrableDomain> allDomains() const override;

final?

> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:1375
> +        postTaskReply([domains = WTFMove(domains), completionHandler = WTFMove(completionHandler)]() mutable {

Hopping to another thread, this requires isolatedCopying domains.

> Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:1378
> +    });

Can we write it with a ternary, something like:
auto domains = m_statisticsStore ? m_statisticsStore->allDomains() : { };
postTaskReply([domains = crossThreadCopy(WTFMove(domains)), completionHandler = WTFMove(completionHandler)]() mutable {
    completionHandler(WTFMove(domains));
});

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1586
> +                resourceLoadStatistics->registrableDomains([callbackAggregator = callbackAggregator.copyRef()](Vector<RegistrableDomain>&& domains) mutable {

s/Vector<RegistrableDomain>/auto

[youenn fablet]

LGTM though I'd like to understand John comment more.

(In reply to John Wilander from comment #2)
> Comment on attachment 402105 [details]
> Patch
> 
> Just as a general comment, we've struggled with the term website data here
> because ITP data isn't website data, i.e. it isn't data that the website
> creates but that WebKit creates *about* websites. That is in part why this
> has not been folded into the reporting structure.

It seems good to have a way to retrieve that information.
I guess your suggestion might be that the UI might be different and maybe the API should be different from other website data?

[Carlos Garcia Campos]

(In reply to youenn fablet from comment #11)
> Comment on attachment 402105 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=402105&action=review
> 
> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.h:147
> > +    Vector<RegistrableDomain> allDomains() const override;
> 
> final?
> 
> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.h:116
> > +    Vector<RegistrableDomain> allDomains() const override;
> 
> final?
> 
> > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:1375
> > +        postTaskReply([domains = WTFMove(domains), completionHandler = WTFMove(completionHandler)]() mutable {
> 
> Hopping to another thread, this requires isolatedCopying domains.

Also when moving?

> > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:1378
> > +    });
> 
> Can we write it with a ternary, something like:
> auto domains = m_statisticsStore ? m_statisticsStore->allDomains() : { };
> postTaskReply([domains = crossThreadCopy(WTFMove(domains)),
> completionHandler = WTFMove(completionHandler)]() mutable {
>     completionHandler(WTFMove(domains));
> });

Yes, I started with something like this but then I decided to follow what other similar methods in the file were doing.

> > Source/WebKit/NetworkProcess/NetworkProcess.cpp:1586
> > +                resourceLoadStatistics->registrableDomains([callbackAggregator = callbackAggregator.copyRef()](Vector<RegistrableDomain>&& domains) mutable {
> 
> s/Vector<RegistrableDomain>/auto

[youenn fablet]

> > > Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:1375
> > > +        postTaskReply([domains = WTFMove(domains), completionHandler = WTFMove(completionHandler)]() mutable {
> > 
> > Hopping to another thread, this requires isolatedCopying domains.
> 
> Also when moving?

Yes, moving will only prevent refing when capturing.
It is safe to hop to another thread with a String which counter is 1. But otherwise, like in the in memory case, the strings may be derefed in both threads.
There is no guarantee that this will be the case in your code (especially for in memory stores).
Also isolatedCopy tries to optimise the case of Strings that are moved and which counter is 1, see String String::isolatedCopy() &&.

We could try to optimise the case of a Vector<T>&& so as to reuse the allocated vector and call isolatedCopy() && on each item as well.

[Carlos Garcia Campos]

Created attachment 402202 [details]
Patch for landing

[Carlos Garcia Campos]

Created attachment 402206 [details]
Patch for landing

[Carlos Garcia Campos]

Committed r263258: <https://trac.webkit.org/changeset/263258>

[Radar WebKit Bug Importer]

<rdar://problem/64522896>