Summary: | Storage Access API: UI process should update network process about granted access | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | John Wilander <wilander> | ||||||||||||||||
Component: | WebKit2 | Assignee: | John Wilander <wilander> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | achristensen, bfulgham, buildbot, cdumez, commit-queue, rniwa, webkit-bug-importer, wilander | ||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Bug Depends on: | 179857 | ||||||||||||||||||
Bug Blocks: | 176862 | ||||||||||||||||||
Attachments: |
|
Description
John Wilander
2017-09-14 13:03:29 PDT
Created attachment 325942 [details]
Patch
Created attachment 325958 [details]
Patch
Comment on attachment 325958 [details] Patch Attachment 325958 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/5095424 New failing tests: http/tests/storageAccess/request-and-grant-storage-access-cross-origin-sandboxed-iframe-from-prevalent-domain-with-non-recent-user-interaction.html Created attachment 325967 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Created attachment 325970 [details]
Patch
Comment on attachment 325970 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=325970&action=review > Source/WebKit/NetworkProcess/NetworkProcessCreationParameters.h:101 > + bool storageAccessAPIEnabled; We should consider moving these to WebsiteDataStoreParameters > Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:354 > + statistics.mostRecentUserInteractionTime = WallTime::now() - Seconds::fromHours(25); 25 hours should be given a name. > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1343 > + }, [this] (const String& resourceDomain, const String& firstPartyDomain, bool value, WTF::Function<void(bool wasGranted)>&& callback) { These should probably all have protectedThis = makeRef(*this). Also, consider using CompletionHandler instead of Function to ensure it is called only once. Comment on attachment 325970 [details] Patch Attachment 325970 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/5098429 New failing tests: http/tests/resourceLoadStatistics/remove-partitioning-from-redirect.html http/tests/resourceLoadStatistics/add-partitioning-to-redirect.html Created attachment 325995 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
Created attachment 327126 [details]
Patch
I asked if Alex could have a look at the additional changes I did for testing. The test failure on iOS Simulator should be fixed now too. Comment on attachment 327126 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=327126&action=review > Source/WebCore/platform/network/NetworkStorageSession.h:161 > + HashMap<String, HashSet<String>> m_storageAccess; I would give this a better name like m_domainsGrantedStorageAccess. It took a bit of reading to realize that the key is the top domain and the value is the set of domains that have been granted storage access on that domain. > Source/WebCore/platform/network/cf/NetworkStorageSessionCFNet.cpp:301 > + auto accessEntry = m_storageAccess.add(firstPartyDomain, HashSet<String>()); > + accessEntry.iterator->value.add(resourceDomain); HashSet has a constructor that takes a std::initializer_list. Using it could make this one line. > Source/WebKit/NetworkProcess/NetworkProcess.cpp:339 > + parentProcessConnection()->send(Messages::NetworkProcessProxy::StorageAccessRequestResult(true, contextId), 0); > + } else > + parentProcessConnection()->send(Messages::NetworkProcessProxy::StorageAccessRequestResult(false, contextId), 0); Should the value be true or shouldGrantStorage? The else should have an ASSERT_NOT_REACHED in it. We could make one call to send to reduce duplication. Just make a local bool. > Source/WebKit/UIProcess/WebCookieManagerProxy.cpp:292 > +#if PLATFORM(COCOA) > + processPool()->sendToNetworkingProcess(Messages::NetworkProcess::SetStorageAccessAPIEnabled(enabled)); I'm not sure there's anything cocoa-specific about the storage access API enabled bool. > Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:142 > + WebResourceLoadStatisticsStore(const String&, Function<void(const String&)>&& testingCallback, UpdatePrevalentDomainsToPartitionOrBlockCookiesHandler&&, UpdateStorageAccessForPrevalentDomainsHandler&&, RemovePrevalentDomainsHandler&&); There's a growing number of these callback functions. Maybe a better design would be to pass one client. Probably not necessary for this patch, though. Created attachment 327265 [details]
Patch for landing
Thanks for the review, Alex! I fixed everything but the Cocoa check (storage access deals with partitioned cookies which are Cocoa-only as far as I know) and the redesign of the callback parameters. Comment on attachment 327265 [details] Patch for landing Clearing flags on attachment: 327265 Committed r225006: <https://trac.webkit.org/changeset/225006> All reviewed patches have been landed. Closing bug. Re-opened since this is blocked by bug 179857 Chris Dumez solved the issue in https://trac.webkit.org/changeset/225010/webkit. Thanks, Chris! Comment on attachment 327265 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=327265&action=review > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1344 > + }, [this, protectedThis = makeRef(*this)] (const String& resourceDomain, const String& firstPartyDomain, bool value, WTF::Function<void(bool wasGranted)>&& callback) { This is not OK and causes massive leaks. WebsiteDataStore refs m_resourceLoadStatistics and m_resourceLoadStatistics refs the data store via those callbacks. -> ref cycle. |