Summary: | Resource Load Statistics: Remove partitioned cookies for reduced complexity, lower memory footprint, and ability to support more platforms | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | John Wilander <wilander> | ||||||||||||||||||||||
Component: | WebKit Misc. | Assignee: | John Wilander <wilander> | ||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||
Severity: | Normal | CC: | achristensen, bfulgham, cdumez, commit-queue, dbates, ews-watchlist, japhet, rniwa | ||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||
Attachments: |
|
Description
John Wilander
2018-07-27 12:21:05 PDT
Created attachment 345946 [details]
Patch
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 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
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 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 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
After analyzing the code paths and discussing with Dan Bates, I believe I've found the reason for the test failures on Sierra. 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. Created attachment 346085 [details]
Patch
(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. Created attachment 346102 [details]
Patch
Created attachment 346128 [details]
Patch
Thanks, Alex, for getting me out of the git-svn rebase nightmare. Mac-wk2 test failures are unrelated. 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 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 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 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 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? 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. 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. (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. Created attachment 346200 [details]
Patch
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. Thank you, Chris! I will fix all the things you listed. Created attachment 346208 [details]
Patch
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!
(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 on attachment 346208 [details] Patch Clearing flags on attachment: 346208 Committed r234440: <https://trac.webkit.org/changeset/234440> All reviewed patches have been landed. Closing bug. |