Bug 176943 – Storage Access API: UI process should update network process about granted access

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[John Wilander]

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.

[Radar WebKit Bug Importer]

<rdar://problem/34440612>

[John Wilander]

Created attachment 325942 [details]
Patch

[John Wilander]

Created attachment 325958 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[John Wilander]

Created attachment 325970 [details]
Patch

[Alex Christensen]

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.

[Build Bot]

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

[Build Bot]

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

[John Wilander]

Created attachment 327126 [details]
Patch

[John Wilander]

I asked if Alex could have a look at the additional changes I did for testing.

[John Wilander]

The test failure on iOS Simulator should be fixed now too.

[Alex Christensen]

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.

[John Wilander]

Created attachment 327265 [details]
Patch for landing

[John Wilander]

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.

[WebKit Commit Bot]

Comment on attachment 327265 [details]
Patch for landing

Clearing flags on attachment: 327265

Committed r225006: <https://trac.webkit.org/changeset/225006>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 179857

[John Wilander]

Chris Dumez solved the issue in https://trac.webkit.org/changeset/225010/webkit. Thanks, Chris!

[Chris Dumez]

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.