Bug 188109 – Resource Load Statistics: Remove partitioned cookies for reduced complexity, lower memory footprint, and ability to support more platforms

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

[John Wilander]

We should remove partitioned cookies to achieve the following:

1. Simplify the model. Third-parties now have a way to get to their first-party cookies through the Storage Access API. Juggling multiple cookie jars is hard for developers. With just one cookie jar and the Storage Access API, things will be much simpler.

2. Lower the memory footprint. By not accepting multiple sets of cookies for one domain, we lower our memory use.

3. Partitioned cookies requires support in the HTTP layer below WebKit. By removing that requirement we increase the flexibility of the feature and can make Resource Load Statistics work on more platforms.

[John Wilander]

<rdar://problem/42664391>

[John Wilander]

Created attachment 345946 [details]
Patch

[EWS Watchlist]

Comment on attachment 345946 [details]
Patch

Attachment 345946 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/8676594

New failing tests:
imported/w3c/web-platform-tests/fetch/api/cors/cors-cookies-redirect.any.html
http/tests/cookies/third-party-cookie-relaxing.html
imported/w3c/web-platform-tests/fetch/api/cors/cors-cookies-redirect.any.worker.html

[EWS Watchlist]

Created attachment 345954 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6

[John Wilander]

The failing cookie tests are related. I'll have to look at them on macOS 10.12 because they are not failing on my machine.

[EWS Watchlist]

Comment on attachment 345946 [details]
Patch

Attachment 345946 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/8676580

New failing tests:
imported/w3c/web-platform-tests/fetch/api/cors/cors-cookies-redirect.any.html
http/tests/cookies/third-party-cookie-relaxing.html
imported/w3c/web-platform-tests/fetch/api/cors/cors-cookies-redirect.any.worker.html

[EWS Watchlist]

Created attachment 345960 [details]
Archive of layout-test-results from ews117 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-sierra  Platform: Mac OS X 10.12.6

[John Wilander]

After analyzing the code paths and discussing with Dan Bates, I believe I've found the reason for the test failures on Sierra.

[Alex Christensen]

Comment on attachment 345946 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKProcessPoolPrivate.h:116
> +// FIXME: Deprecated. Left here until a final decision is made.
>  @property (nonatomic, getter=_isCookieStoragePartitioningEnabled, setter=_setCookieStoragePartitioningEnabled:) BOOL _cookieStoragePartitioningEnabled WK_API_AVAILABLE(macosx(10.12.3), ios(10.3));

Let's use WK_API_DEPRECATED, return false, and remove the unused boolean on WebProcessPool.

[John Wilander]

Created attachment 346085 [details]
Patch

[John Wilander]

(In reply to Alex Christensen from comment #9)
> Comment on attachment 345946 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=345946&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKProcessPoolPrivate.h:116
> > +// FIXME: Deprecated. Left here until a final decision is made.
> >  @property (nonatomic, getter=_isCookieStoragePartitioningEnabled, setter=_setCookieStoragePartitioningEnabled:) BOOL _cookieStoragePartitioningEnabled WK_API_AVAILABLE(macosx(10.12.3), ios(10.3));
> 
> Let's use WK_API_DEPRECATED, return false, and remove the unused boolean on
> WebProcessPool.

Thanks. Will fix.

[John Wilander]

Created attachment 346102 [details]
Patch

[Alex Christensen]

Created attachment 346128 [details]
Patch

[John Wilander]

Thanks, Alex, for getting me out of the git-svn rebase nightmare.

[John Wilander]

Mac-wk2 test failures are unrelated.

[EWS Watchlist]

Comment on attachment 346128 [details]
Patch

Attachment 346128 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/8705690

New failing tests:
accessibility/smart-invert-reference.html

[EWS Watchlist]

Created attachment 346136 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 346128 [details]
Patch

Attachment 346128 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8707393

New failing tests:
http/tests/security/canvas-remote-read-remote-video-redirect.html
http/tests/security/canvas-remote-read-remote-video-localhost.html

[EWS Watchlist]

Created attachment 346145 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Brent Fulgham]

Comment on attachment 346128 [details]
Patch

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

400KB patches are an anti-pattern! However, this looks reasonable. You improperly deleted some logging routines that we still need, so r- to correct those cases. Otherwise this looks pretty close to ready. I'll look at the test failures next and see if there are any other comments.

> Source/WebCore/ChangeLog:29
> +        reflect the code changes and to shorten their names meanwhile (as 

I don't think you need the word "meanwhile" here.

> Source/WebCore/ChangeLog:46
> +            the Storage Access API. This was previously handled by

handled by what? The suspense is killing me! :-)

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:-1011
> -static void logCookieInformationInternal(const String& label, const void* loggedObject, const WebCore::NetworkStorageSession& networkStorageSession, const WebCore::URL& partition, const WebCore::SameSiteInfo& sameSiteInfo, const WebCore::URL& url, const String& referrer, std::optional<uint64_t> frameID, std::optional<uint64_t> pageID, std::optional<uint64_t> identifier)

I don't think you want to get rid of this logging. You should just remove the partitioning aspect of this. We still would like to log origins with storage access, etc., when running in debug/diagnostic mode.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:-1080
> -        logCookieInformationInternal(label, loggedObject, networkStorageSession, partition, sameSiteInfo, url, referrer, frameID, pageID, identifier);

You should keep this, just not the partition part of the logging.

> Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:-404
> -        LOG(NetworkSession, "%llu %s cookies for redirect URL %s", [m_task taskIdentifier], (requiredStoragePartition.isEmpty() ? "Not partitioning" : "Partitioning"), request.url().string().utf8().data());

You need to keep this logging, just remove the portioning data from the log entry.

> Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:-406
> -        applyCookiePartitioningPolicy(requiredStoragePartition, m_task.get()._storagePartitionIdentifier);

But of course this line can go away.

> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:938
>  bool ResourceLoadStatisticsMemoryStore::wasAccessedAsFirstPartyDueToUserInteraction(const ResourceLoadStatistics& current, const ResourceLoadStatistics& updated) const

I wonder if we even need this method anymore, if there is no partitioning?

[Brent Fulgham]

I don't see how the test failure could be related to this change, since it is a very small pixel difference on some rendering.

[John Wilander]

Thanks, Brent! I'll make sure to work through the logging.

Yes, the patch size is unfortunate but the code changes are almost exclusively code removals and so many parts of the code (and tests) assume partitioning.

[John Wilander]

(In reply to Brent Fulgham from comment #20)
> Comment on attachment 346128 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=346128&action=review
> 
> 400KB patches are an anti-pattern! However, this looks reasonable. You
> improperly deleted some logging routines that we still need, so r- to
> correct those cases. Otherwise this looks pretty close to ready. I'll look
> at the test failures next and see if there are any other comments.
> 
> > Source/WebCore/ChangeLog:29
> > +        reflect the code changes and to shorten their names meanwhile (as 
> 
> I don't think you need the word "meanwhile" here.

Removed.

> > Source/WebCore/ChangeLog:46
> > +            the Storage Access API. This was previously handled by
> 
> handled by what? The suspense is killing me! :-)

Fixed. Good find.

> > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:-1011
> > -static void logCookieInformationInternal(const String& label, const void* loggedObject, const WebCore::NetworkStorageSession& networkStorageSession, const WebCore::URL& partition, const WebCore::SameSiteInfo& sameSiteInfo, const WebCore::URL& url, const String& referrer, std::optional<uint64_t> frameID, std::optional<uint64_t> pageID, std::optional<uint64_t> identifier)
> 
> I don't think you want to get rid of this logging. You should just remove
> the partitioning aspect of this. We still would like to log origins with
> storage access, etc., when running in debug/diagnostic mode.

Brought it back.

> > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:-1080
> > -        logCookieInformationInternal(label, loggedObject, networkStorageSession, partition, sameSiteInfo, url, referrer, frameID, pageID, identifier);
> 
> You should keep this, just not the partition part of the logging.

Got it. You are referring to the third, empty partition case whereas I only thought of the blocked and partitioned cases.

> > Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:-404
> > -        LOG(NetworkSession, "%llu %s cookies for redirect URL %s", [m_task taskIdentifier], (requiredStoragePartition.isEmpty() ? "Not partitioning" : "Partitioning"), request.url().string().utf8().data());
> 
> You need to keep this logging, just remove the portioning data from the log
> entry.

Fixed.

> > Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:-406
> > -        applyCookiePartitioningPolicy(requiredStoragePartition, m_task.get()._storagePartitionIdentifier);
> 
> But of course this line can go away.

Yup.

> > Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:938
> >  bool ResourceLoadStatisticsMemoryStore::wasAccessedAsFirstPartyDueToUserInteraction(const ResourceLoadStatistics& current, const ResourceLoadStatistics& updated) const
> 
> I wonder if we even need this method anymore, if there is no partitioning?

I think we do because user interaction still controls whether a third-party can successfully call the Storage Access API or not. I suggest we remove it in its own patch if you still believe it's not needed.

Thanks again. New patch coming up.

[John Wilander]

Created attachment 346200 [details]
Patch

[Chris Dumez]

Comment on attachment 346200 [details]
Patch

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

Looked at the C++ side and did not see anything obviously wrong except for the comment I made.

> Source/WebCore/platform/network/cf/NetworkStorageSessionCFNet.cpp:278
>          m_topPrivatelyControlledDomainsToBlock.add(domain);

I think the following would work:
m_topPrivatelyControlledDomainsToBlock.add(domains.begin(), domains.end());

> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:1032
> +    RunLoop::main().dispatch([this, store = makeRef(m_store), domainsToBlock = crossThreadCopy(domainsToBlock), completionHandler = WTFMove(completionHandler)] () mutable {

We likely want to capture weakThis = makeWeakPtr(*this) here.

> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:1033
> +        store->callUpdatePrevalentDomainsToBlockCookiesForHandler(domainsToBlock, ShouldClearFirst::No, [this, store = store.copyRef(), completionHandler = WTFMove(completionHandler)]() mutable {

WTFMove() it here.

> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:1034
> +            store->statisticsQueue().dispatch([this, completionHandler = WTFMove(completionHandler)]() mutable {

WTFMove() it here.

> Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp:1036
>  

and return early here if !weakThis, after calling the completionHandler, otherwise we may crash below because |this| could be dead.

[John Wilander]

Thank you, Chris! I will fix all the things you listed.

[John Wilander]

Created attachment 346208 [details]
Patch

[John Wilander]

Comment on attachment 346208 [details]
Patch

Previous patch was green on the bots. Fixed Chris's comments and added Brent, Chris, and Alex as reviewers.

Brent, if you think it looks OK, you can just put it on the queue. Thanks!

[Brent Fulgham]

(In reply to John Wilander from comment #28)
> Comment on attachment 346208 [details]
> Patch
> 
> Previous patch was green on the bots. Fixed Chris's comments and added
> Brent, Chris, and Alex as reviewers.
> 
> Brent, if you think it looks OK, you can just put it on the queue. Thanks!

Looks fine. I cq+'d it.

[WebKit Commit Bot]

Comment on attachment 346208 [details]
Patch

Clearing flags on attachment: 346208

Committed r234440: <https://trac.webkit.org/changeset/234440>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.