Bug 182197 - Add telemetry to track storage access API adoptions
Summary: Add telemetry to track storage access API adoptions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-01-26 17:05 PST by Brent Fulgham
Modified: 2018-01-30 15:16 PST (History)
7 users (show)

See Also:


Attachments
Patch (20.50 KB, patch)
2018-01-26 17:32 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (17.33 KB, patch)
2018-01-28 17:14 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (18.70 KB, patch)
2018-01-29 10:23 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (Part 2) (11.66 KB, patch)
2018-01-29 17:36 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (11.66 KB, patch)
2018-01-30 08:22 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (7.35 KB, patch)
2018-01-30 11:19 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (7.25 KB, patch)
2018-01-30 12:51 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (7.04 KB, patch)
2018-01-30 13:31 PST, Brent Fulgham
cdumez: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2018-01-26 17:05:53 PST
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.
Comment 1 Brent Fulgham 2018-01-26 17:30:59 PST
<rdar://problem/35803309>
Comment 2 Brent Fulgham 2018-01-26 17:32:44 PST
Created attachment 332440 [details]
Patch
Comment 3 Brent Fulgham 2018-01-26 17:33:01 PST
Comment on attachment 332440 [details]
Patch

WIP for EWS testing
Comment 4 EWS Watchlist 2018-01-26 17:33:56 PST
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.
Comment 5 Brent Fulgham 2018-01-28 17:14:20 PST
Created attachment 332501 [details]
Patch
Comment 6 Chris Dumez 2018-01-29 09:27:44 PST
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?
Comment 7 Brent Fulgham 2018-01-29 09:43:05 PST
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.
Comment 8 Chris Dumez 2018-01-29 09:45:34 PST
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) {
Comment 9 Chris Dumez 2018-01-29 09:45:51 PST
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.
Comment 10 Brent Fulgham 2018-01-29 10:09:01 PST
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.
Comment 11 Brent Fulgham 2018-01-29 10:23:10 PST
Created attachment 332551 [details]
Patch
Comment 12 Chris Dumez 2018-01-29 12:01:11 PST
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.
Comment 13 Brent Fulgham 2018-01-29 12:55:47 PST
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!
Comment 14 Brent Fulgham 2018-01-29 14:00:59 PST
Committed r227755: <https://trac.webkit.org/changeset/227755>
Comment 15 Brent Fulgham 2018-01-29 14:01:27 PST
Reopened for part 2.
Comment 16 Brent Fulgham 2018-01-29 17:36:46 PST
Created attachment 332610 [details]
Patch (Part 2)
Comment 17 Brent Fulgham 2018-01-30 08:22:22 PST
Created attachment 332656 [details]
Patch
Comment 18 Brent Fulgham 2018-01-30 09:44:43 PST
Committed r227790: <https://trac.webkit.org/changeset/227790>
Comment 19 Brent Fulgham 2018-01-30 09:45:25 PST
Reopening for Part 2 (Storage Access API)
Comment 20 Brent Fulgham 2018-01-30 11:19:34 PST
Created attachment 332672 [details]
Patch
Comment 21 Chris Dumez 2018-01-30 12:40:42 PST
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?
Comment 22 Brent Fulgham 2018-01-30 12:43:53 PST
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.
Comment 23 Brent Fulgham 2018-01-30 12:51:58 PST
Created attachment 332689 [details]
Patch
Comment 24 John Wilander 2018-01-30 13:01:17 PST
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.
Comment 25 Brent Fulgham 2018-01-30 13:29:46 PST
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.
Comment 26 Brent Fulgham 2018-01-30 13:31:46 PST
Created attachment 332695 [details]
Patch
Comment 27 John Wilander 2018-01-30 13:42:06 PST
(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.
Comment 28 Brent Fulgham 2018-01-30 15:12:05 PST
Committed r227860: <https://trac.webkit.org/changeset/227860>
Comment 29 Brent Fulgham 2018-01-30 15:12:50 PST
It looks like mac-wk2 is complaining about a test failure in trunk. Not related to this patch.
Comment 30 John Wilander 2018-01-30 15:16:55 PST
(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. :/