The UI process needs to tell the network process asynchronously about newly granted storage access. When done, the network process needs to reply so that the UI process can notify web processes.
<rdar://problem/34440612>
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.