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.
<rdar://problem/35803309>
Created attachment 332440 [details] Patch
Comment on attachment 332440 [details] Patch WIP for EWS testing
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.
Created attachment 332501 [details] Patch
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 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 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 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 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.
Created attachment 332551 [details] Patch
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 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!
Committed r227755: <https://trac.webkit.org/changeset/227755>
Reopened for part 2.
Created attachment 332610 [details] Patch (Part 2)
Created attachment 332656 [details] Patch
Committed r227790: <https://trac.webkit.org/changeset/227790>
Reopening for Part 2 (Storage Access API)
Created attachment 332672 [details] Patch
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 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.
Created attachment 332689 [details] Patch
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 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.
Created attachment 332695 [details] Patch
(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.
Committed r227860: <https://trac.webkit.org/changeset/227860>
It looks like mac-wk2 is complaining about a test failure in trunk. Not related to this patch.
(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. :/