Bug 188109 - Resource Load Statistics: Remove partitioned cookies for reduced complexity, lower memory footprint, and ability to support more platforms
Summary: Resource Load Statistics: Remove partitioned cookies for reduced complexity, ...
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: John Wilander
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-07-27 12:21 PDT by John Wilander
Modified: 2018-07-31 16:23 PDT (History)
8 users (show)

See Also:


Attachments
Patch (430.96 KB, patch)
2018-07-27 13:27 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-sierra (2.30 MB, application/zip)
2018-07-27 14:37 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-sierra (3.28 MB, application/zip)
2018-07-27 14:58 PDT, Build Bot
no flags Details
Patch (431.31 KB, patch)
2018-07-30 12:33 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Patch (430.92 KB, patch)
2018-07-30 14:38 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Patch (431.34 KB, patch)
2018-07-30 17:21 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-sierra-wk2 (3.07 MB, application/zip)
2018-07-30 18:41 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews206 for win-future (13.13 MB, application/zip)
2018-07-31 00:17 PDT, Build Bot
no flags Details
Patch (429.53 KB, patch)
2018-07-31 13:52 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Patch (429.86 KB, patch)
2018-07-31 15:14 PDT, John Wilander
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Wilander 2018-07-27 12:21:05 PDT
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.
Comment 1 John Wilander 2018-07-27 12:21:22 PDT
<rdar://problem/42664391>
Comment 2 John Wilander 2018-07-27 13:27:43 PDT
Created attachment 345946 [details]
Patch
Comment 3 Build Bot 2018-07-27 14:37:49 PDT
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
Comment 4 Build Bot 2018-07-27 14:37:51 PDT
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
Comment 5 John Wilander 2018-07-27 14:44:32 PDT
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.
Comment 6 Build Bot 2018-07-27 14:58:42 PDT
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
Comment 7 Build Bot 2018-07-27 14:58:44 PDT
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
Comment 8 John Wilander 2018-07-30 11:52:11 PDT
After analyzing the code paths and discussing with Dan Bates, I believe I've found the reason for the test failures on Sierra.
Comment 9 Alex Christensen 2018-07-30 11:58:12 PDT
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.
Comment 10 John Wilander 2018-07-30 12:33:04 PDT
Created attachment 346085 [details]
Patch
Comment 11 John Wilander 2018-07-30 12:33:42 PDT
(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.
Comment 12 John Wilander 2018-07-30 14:38:53 PDT
Created attachment 346102 [details]
Patch
Comment 13 Alex Christensen 2018-07-30 17:21:52 PDT
Created attachment 346128 [details]
Patch
Comment 14 John Wilander 2018-07-30 17:59:00 PDT
Thanks, Alex, for getting me out of the git-svn rebase nightmare.
Comment 15 John Wilander 2018-07-30 18:07:50 PDT
Mac-wk2 test failures are unrelated.
Comment 16 Build Bot 2018-07-30 18:41:01 PDT
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
Comment 17 Build Bot 2018-07-30 18:41:03 PDT
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
Comment 18 Build Bot 2018-07-31 00:17:15 PDT
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
Comment 19 Build Bot 2018-07-31 00:17:27 PDT
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
Comment 20 Brent Fulgham 2018-07-31 09:28:17 PDT
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?
Comment 21 Brent Fulgham 2018-07-31 09:30:22 PDT
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.
Comment 22 John Wilander 2018-07-31 12:43:25 PDT
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.
Comment 23 John Wilander 2018-07-31 13:36:08 PDT
(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.
Comment 24 John Wilander 2018-07-31 13:52:37 PDT
Created attachment 346200 [details]
Patch
Comment 25 Chris Dumez 2018-07-31 14:47:04 PDT
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.
Comment 26 John Wilander 2018-07-31 14:58:05 PDT
Thank you, Chris! I will fix all the things you listed.
Comment 27 John Wilander 2018-07-31 15:14:16 PDT
Created attachment 346208 [details]
Patch
Comment 28 John Wilander 2018-07-31 15:15:37 PDT
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!
Comment 29 Brent Fulgham 2018-07-31 15:55:52 PDT
(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.
Comment 30 WebKit Commit Bot 2018-07-31 16:23:52 PDT
Comment on attachment 346208 [details]
Patch

Clearing flags on attachment: 346208

Committed r234440: <https://trac.webkit.org/changeset/234440>
Comment 31 WebKit Commit Bot 2018-07-31 16:23:54 PDT
All reviewed patches have been landed.  Closing bug.