Bug 182197 – Add telemetry to track storage access API adoptions

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Brent Fulgham]

Update WebKit to capture counts of cross-origin access due to user interaction versus the new Storage Access API.

Also add internal logging when Storage Access is invoked to assist in debugging website issues.

[Brent Fulgham]

<rdar://problem/35803309>

[Brent Fulgham]

Created attachment 332440 [details]
Patch

[Brent Fulgham]

Comment on attachment 332440 [details]
Patch

WIP for EWS testing

[EWS Watchlist]

Attachment 332440 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:196:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebKit/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:198:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brent Fulgham]

Created attachment 332501 [details]
Patch

[Chris Dumez]

Comment on attachment 332501 [details]
Patch

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

> Source/WebCore/loader/ResourceLoadObserver.cpp:135
> +void ResourceLoadObserver::setTimeToLivePartitionFree(const Seconds& value)

No need for const &, this is just a double.

> Source/WebCore/loader/ResourceLoadStatistics.h:86
> +    unsigned accessedAsFirstPartyDueToUserInteraction { 0 };

The variable name does not really make it clear what the count is for. maybe timesAccessedAsFirstPartyDueToUserInteraction? (if this is what you meant).

> Source/WebCore/loader/ResourceLoadStatistics.h:87
> +    unsigned accessedAsFirstPartyDueToStorageAccessAPI { 0 };

ditto.

> Source/WebKit/Shared/WebProcessCreationParameters.h:191
> +    double cookiePartitionTimeToLive { 0 };

Why doesn't this use Seconds type?

> Source/WebKit/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:165
> +    WTF::Function<unsigned(const PrevalentResourceTelemetry& telemetry)> numberOfTimesAccessedAsFirstPartyDueToUserInteractionGetter = [] (const PrevalentResourceTelemetry& t) {

Could probably use auto& t.

> Source/WebKit/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:168
> +    WTF::Function<unsigned(const PrevalentResourceTelemetry& telemetry)> numberOfTimesAccessedAsFirstPartyDueToStorageAccessAPIGetter = [] (const PrevalentResourceTelemetry& t) {

Could probably use auto& t.

> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:284
> +    if (timeToLiveUserInteraction < 0_s || timeToLiveUserInteraction <= 24_h * 30)

So the value has to be over 30 days? Is this really what you meant? Or did you mean that it should be within 30 days?

[Brent Fulgham]

Comment on attachment 332501 [details]
Patch

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

>> Source/WebCore/loader/ResourceLoadObserver.cpp:135
>> +void ResourceLoadObserver::setTimeToLivePartitionFree(const Seconds& value)
> 
> No need for const &, this is just a double.

Will fix.

>> Source/WebCore/loader/ResourceLoadStatistics.h:86
>> +    unsigned accessedAsFirstPartyDueToUserInteraction { 0 };
> 
> The variable name does not really make it clear what the count is for. maybe timesAccessedAsFirstPartyDueToUserInteraction? (if this is what you meant).

Yes, that's a reasonable choice for the name. I'll switch to it.

>> Source/WebCore/loader/ResourceLoadStatistics.h:87
>> +    unsigned accessedAsFirstPartyDueToStorageAccessAPI { 0 };
> 
> ditto.

Ditto!

>> Source/WebKit/Shared/WebProcessCreationParameters.h:191
>> +    double cookiePartitionTimeToLive { 0 };
> 
> Why doesn't this use Seconds type?

I wasn't sure if it would serialize easily over IPC between UI and WebContent processes. I'll try using Seconds.

>> Source/WebKit/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:165
>> +    WTF::Function<unsigned(const PrevalentResourceTelemetry& telemetry)> numberOfTimesAccessedAsFirstPartyDueToUserInteractionGetter = [] (const PrevalentResourceTelemetry& t) {
> 
> Could probably use auto& t.

Will we get a WTF::Function here? I though we wanted to be careful about specifically getting our function object because of threading issues with the std::function implementation?

>> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:284
>> +    if (timeToLiveUserInteraction < 0_s || timeToLiveUserInteraction <= 24_h * 30)
> 
> So the value has to be over 30 days? Is this really what you meant? Or did you mean that it should be within 30 days?

Oh duh. Yes, I meant the opposite. I wanted to clamp it to positive values less than (or equal to) 30 days. Thanks for catching this.

[Chris Dumez]

Comment on attachment 332501 [details]
Patch

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

>>> Source/WebKit/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:165
>>> +    WTF::Function<unsigned(const PrevalentResourceTelemetry& telemetry)> numberOfTimesAccessedAsFirstPartyDueToUserInteractionGetter = [] (const PrevalentResourceTelemetry& t) {
>> 
>> Could probably use auto& t.
> 
> Will we get a WTF::Function here? I though we wanted to be careful about specifically getting our function object because of threading issues with the std::function implementation?

I suggested
WTF::Function<unsigned(const PrevalentResourceTelemetry& telemetry)> numberOfTimesAccessedAsFirstPartyDueToUserInteractionGetter = [] (auto& t) {
not
auto numberOfTimesAccessedAsFirstPartyDueToUserInteractionGetter = [] (const PrevalentResourceTelemetry& t) {

[Chris Dumez]

Comment on attachment 332501 [details]
Patch

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

>>> Source/WebKit/Shared/WebProcessCreationParameters.h:191
>>> +    double cookiePartitionTimeToLive { 0 };
>> 
>> Why doesn't this use Seconds type?
> 
> I wasn't sure if it would serialize easily over IPC between UI and WebContent processes. I'll try using Seconds.

Should serialize fine.

[Brent Fulgham]

Comment on attachment 332501 [details]
Patch

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

>>>> Source/WebKit/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:165
>>>> +    WTF::Function<unsigned(const PrevalentResourceTelemetry& telemetry)> numberOfTimesAccessedAsFirstPartyDueToUserInteractionGetter = [] (const PrevalentResourceTelemetry& t) {
>>> 
>>> Could probably use auto& t.
>> 
>> Will we get a WTF::Function here? I though we wanted to be careful about specifically getting our function object because of threading issues with the std::function implementation?
> 
> I suggested
> WTF::Function<unsigned(const PrevalentResourceTelemetry& telemetry)> numberOfTimesAccessedAsFirstPartyDueToUserInteractionGetter = [] (auto& t) {
> not
> auto numberOfTimesAccessedAsFirstPartyDueToUserInteractionGetter = [] (const PrevalentResourceTelemetry& t) {

Ah! Will do.

[Brent Fulgham]

Created attachment 332551 [details]
Patch

[Chris Dumez]

Comment on attachment 332551 [details]
Patch

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

r=me with comments.

> Source/WebCore/loader/ResourceLoadObserver.h:88
> +    Seconds timeToLiveCookiePartitionFree { 24_h };

Missing m_ prefix.

> Source/WebKit/Shared/WebProcessCreationParameters.h:191
> +    Seconds cookiePartitionTimeToLive { 0 };

Seconds default-initializes to zero, you don't need { 0 } here.

> Source/WebKit/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:178
> +    unsigned topNumberOfTimesAccessedAsFirstPartyDueToStorageAccessAP = median(sortedPrevalentResourcesWithoutUserInteraction, 0, numberOfResourcesFromTheTop - 1, numberOfTimesAccessedAsFirstPartyDueToStorageAccessAPIGetter);

typo: topNumberOfTimesAccessedAsFirstPartyDueToStorageAccessAP

missing I at the end.

[Brent Fulgham]

Comment on attachment 332551 [details]
Patch

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

>> Source/WebCore/loader/ResourceLoadObserver.h:88
>> +    Seconds timeToLiveCookiePartitionFree { 24_h };
> 
> Missing m_ prefix.

Done.

>> Source/WebKit/Shared/WebProcessCreationParameters.h:191
>> +    Seconds cookiePartitionTimeToLive { 0 };
> 
> Seconds default-initializes to zero, you don't need { 0 } here.

Done!

>> Source/WebKit/UIProcess/WebResourceLoadStatisticsTelemetry.cpp:178
>> +    unsigned topNumberOfTimesAccessedAsFirstPartyDueToStorageAccessAP = median(sortedPrevalentResourcesWithoutUserInteraction, 0, numberOfResourcesFromTheTop - 1, numberOfTimesAccessedAsFirstPartyDueToStorageAccessAPIGetter);
> 
> typo: topNumberOfTimesAccessedAsFirstPartyDueToStorageAccessAP
> 
> missing I at the end.

Doh!

[Brent Fulgham]

Committed r227755: <https://trac.webkit.org/changeset/227755>

[Brent Fulgham]

Reopened for part 2.

[Brent Fulgham]

Created attachment 332610 [details]
Patch (Part 2)

[Brent Fulgham]

Created attachment 332656 [details]
Patch

[Brent Fulgham]

Committed r227790: <https://trac.webkit.org/changeset/227790>

[Brent Fulgham]

Reopening for Part 2 (Storage Access API)

[Brent Fulgham]

Created attachment 332672 [details]
Patch

[Chris Dumez]

Comment on attachment 332672 [details]
Patch

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

> Source/WebCore/platform/network/cf/NetworkStorageSessionCFNet.cpp:205
> +bool NetworkStorageSession::hasStorageAccessForFrame(const ResourceRequest& request, std::optional<uint64_t> frameID, std::optional<uint64_t> pageID) const

Why does this take std::optionals? It looks like the call site always has valid ids (unless I am mistaken), and it looks like this method would only ever return true if those ids are provided.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:725
> +    bool hasStorageAccessForFrame = networkStorageSession->hasStorageAccessForFrame(originalRequest(), frameID(), pageID());

frameID() / pageID() always return a uint64_t, right? If so, why does hasStorageAccessForFrame take std::optionals?

[Brent Fulgham]

Comment on attachment 332672 [details]
Patch

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

>> Source/WebCore/platform/network/cf/NetworkStorageSessionCFNet.cpp:205
>> +bool NetworkStorageSession::hasStorageAccessForFrame(const ResourceRequest& request, std::optional<uint64_t> frameID, std::optional<uint64_t> pageID) const
> 
> Why does this take std::optionals? It looks like the call site always has valid ids (unless I am mistaken), and it looks like this method would only ever return true if those ids are provided.

Huh -- you are right! I was copying an existing function, but there really isn't a need for these to be optionals. I'll revise.

>> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:725
>> +    bool hasStorageAccessForFrame = networkStorageSession->hasStorageAccessForFrame(originalRequest(), frameID(), pageID());
> 
> frameID() / pageID() always return a uint64_t, right? If so, why does hasStorageAccessForFrame take std::optionals?

Yes. I'll fix that.

[Brent Fulgham]

Created attachment 332689 [details]
Patch

[John Wilander]

Comment on attachment 332689 [details]
Patch

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

> Source/WebCore/platform/network/cf/NetworkStorageSessionCFNet.cpp:206
> +{

Shouldn't this just be:

if (!cookieStoragePartitioningEnabled)
  return false;

return hasStorageAccessForFrame(getPartitioningDomain(request.url()), getPartitioningDomain(request.firstPartyForCookies(), frameID, pageID);

?

> Source/WebCore/platform/network/cf/NetworkStorageSessionCFNet.cpp:211
> +    if (!shouldPartitionCookies(resourceDomain))

I think this is wrong since this convenience function returns m_topPrivatelyControlledDomainsToPartition.contains(topPrivatelyControlledDomain) which is going to be set even if storage access has been granted.

Partitioning is set for the whole domain whereas storage access is granted per frame. Therefore we cannot remove partitioning entries when a frame is granted storage access.

[Brent Fulgham]

Comment on attachment 332689 [details]
Patch

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

>> Source/WebCore/platform/network/cf/NetworkStorageSessionCFNet.cpp:206
>> +{
> 
> Shouldn't this just be:
> 
> if (!cookieStoragePartitioningEnabled)
>   return false;
> 
> return hasStorageAccessForFrame(getPartitioningDomain(request.url()), getPartitioningDomain(request.firstPartyForCookies(), frameID, pageID);
> 
> ?

Sure -- that's reasonable.

>> Source/WebCore/platform/network/cf/NetworkStorageSessionCFNet.cpp:211
>> +    if (!shouldPartitionCookies(resourceDomain))
> 
> I think this is wrong since this convenience function returns m_topPrivatelyControlledDomainsToPartition.contains(topPrivatelyControlledDomain) which is going to be set even if storage access has been granted.
> 
> Partitioning is set for the whole domain whereas storage access is granted per frame. Therefore we cannot remove partitioning entries when a frame is granted storage access.

But you define that an origin cannot have been granted StorageAccess API if the domain is not partitioned. So we expect this to succeed and fall through to the per-frame check (below). If the request origin isn't partitioned, we immediately return false.

I think that's right?

At any rate, I've switched to calling your 'hastStorageAccessForFrame' method, which I assume behaves as you intended.

[Brent Fulgham]

Created attachment 332695 [details]
Patch

[John Wilander]

(In reply to Brent Fulgham from comment #25)
> Comment on attachment 332689 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=332689&action=review
> 
> >> Source/WebCore/platform/network/cf/NetworkStorageSessionCFNet.cpp:206
> >> +{
> > 
> > Shouldn't this just be:
> > 
> > if (!cookieStoragePartitioningEnabled)
> >   return false;
> > 
> > return hasStorageAccessForFrame(getPartitioningDomain(request.url()), getPartitioningDomain(request.firstPartyForCookies(), frameID, pageID);
> > 
> > ?
> 
> Sure -- that's reasonable.
> 
> >> Source/WebCore/platform/network/cf/NetworkStorageSessionCFNet.cpp:211
> >> +    if (!shouldPartitionCookies(resourceDomain))
> > 
> > I think this is wrong since this convenience function returns m_topPrivatelyControlledDomainsToPartition.contains(topPrivatelyControlledDomain) which is going to be set even if storage access has been granted.
> > 
> > Partitioning is set for the whole domain whereas storage access is granted per frame. Therefore we cannot remove partitioning entries when a frame is granted storage access.
> 
> But you define that an origin cannot have been granted StorageAccess API if
> the domain is not partitioned. So we expect this to succeed and fall through
> to the per-frame check (below). If the request origin isn't partitioned, we
> immediately return false.

That is correct _at_the_time_of_decision_ to grant access. :) After the decision is made, the domain is still partitioned except for an individual frame.

> I think that's right?
> 
> At any rate, I've switched to calling your 'hastStorageAccessForFrame'
> method, which I assume behaves as you intended.

Sounds good.

[Brent Fulgham]

Committed r227860: <https://trac.webkit.org/changeset/227860>

[Brent Fulgham]

It looks like mac-wk2 is complaining about a test failure in trunk. Not related to this patch.

[John Wilander]

(In reply to Brent Fulgham from comment #29)
> It looks like mac-wk2 is complaining about a test failure in trunk. Not
> related to this patch.

Yeah, imported/w3c/web-platform-tests/service-workers/service-worker/navigation-redirect.https.html has been failing for most of the day. :/