Bug 185368 – Storage Access API: Add a request roundtrip to check whether prompting is needed

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[John Wilander]

Experimental Storage Access API prompting is added in https://bugs.webkit.org/show_bug.cgi?id=185335. To support it, we should consult WebKit::WebResourceLoadStatisticsStore to see whether prompting is needed.

[Radar WebKit Bug Importer]

<rdar://problem/40011556>

[John Wilander]

Created attachment 339696 [details]
Patch

[John Wilander]

Mac-debug test failures seem unrelated.

[John Wilander]

Created attachment 339786 [details]
Patch

[John Wilander]

Created attachment 339790 [details]
Patch

[John Wilander]

Weird build errors. Added RELEASE_ASSERT_NOT_REACHED() to the two switch-cases over the enum. I believe those are needed for platforms that compile with gcc.

[John Wilander]

Created attachment 339792 [details]
Patch

[John Wilander]

Put all changes behind the appropriate compile-time flag.

[John Wilander]

iOS build are complaining about WebPageProxy.cpp:

The following build commands failed:
	CompileC /Volumes/Data/EWS/WebKit/WebKitBuild/WebKit.build/Release-iphoneos/WebKit.build/Objects-normal/arm64/WebPageProxy.o UIProcess/WebPageProxy.cpp normal arm64 c++ com.apple.compilers.llvm.clang.1_0.compiler
(1 failure)

Failed to run "['Tools/Scripts/build-webkit', '--release', '--sdk=iphoneos', 'ARCHS=arm64']" exit_code: 65

[John Wilander]

GTK and WPE are complaining about WebsiteDataStore.cpp:

Failed to run "['Tools/Scripts/build-webkit', '--release', '--gtk', '--update-gtk', '--makeargs="-j32"']" exit_code: 1
d from DerivedSources/ForwardingHeaders/wtf/text/WTFString.h:29:0,
                 from ../../Source/WebKit/NetworkProcess/NetworkSessionCreationParameters.h:31,
                 from /home/ews/ltilve-gtk-wk2-ews/WebKit/Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:28,
                 from /home/ews/ltilve-gtk-wk2-ews/WebKit/Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:27:
DerivedSources/ForwardingHeaders/wtf/Function.h: In instantiation of ‘Out WTF::Function<Out(In ...)>::CallableWrapper<CallableType>::call(In ...) [with CallableType = WTF::Function<void(bool)>; Out = void; In = {WebKit::StorageAccessStatus}]’:
/home/ews/ltilve-gtk-wk2-ews/WebKit/Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1527:1:   required from here
DerivedSources/ForwardingHeaders/wtf/Function.h:101:77: error: no match for call to ‘(WTF::Function<void(bool)>) (WebKit::StorageAccessStatus)’
         Out call(In... in) final { return m_callable(std::forward<In>(in)...); }
                                                                             ^
DerivedSources/ForwardingHeaders/wtf/Function.h:53:9: note: candidate: Out WTF::Function<Out(In ...)>::operator()(In ...) const [with Out = void; In = {bool}]
     Out operator()(In... in) const
         ^~~~~~~~
DerivedSources/ForwardingHeaders/wtf/Function.h:53:9: note:   no known conversion for argument 1 from ‘WebKit::StorageAccessStatus’ to ‘bool’
DerivedSources/ForwardingHeaders/wtf/Function.h:101:77: error: return-statement with a value, in function returning 'void' [-fpermissive]
         Out call(In... in) final { return m_callable(std::forward<In>(in)...); }

[youenn fablet]

Your patch is adding some #if HAVE(CFNETWORK_STORAGE_PARTITIONING) to declare some types and include some headers in WebPageProxy.cpp and WebResourceLoadStatisticsStore.h.
There is a need for the code using those declarations to also be guarded in the same way, or drop these #if if possible.

[John Wilander]

(In reply to youenn fablet from comment #11)
> Your patch is adding some #if HAVE(CFNETWORK_STORAGE_PARTITIONING) to
> declare some types and include some headers in WebPageProxy.cpp and
> WebResourceLoadStatisticsStore.h.
> There is a need for the code using those declarations to also be guarded in
> the same way, or drop these #if if possible.

I’ve worked through that and can’t see any such missing checks in the current patch. Can you spot any? Maybe I’m blind. I am pretty tired. :)

[youenn fablet]

Comment on attachment 339792 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=339792&action=review

> Source/WebKit/UIProcess/WebPageProxy.cpp:7488
>      auto completionHandler = [this, protectedThis = makeRef(*this), webProcessContextId] (bool access) {

I wonder whether this completion handler might sometimes be released in a background thread which might not be right.

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:370
> +    m_statisticsQueue->dispatch([this, protectedThis = makeRef(*this), subFramePrimaryDomain = crossThreadCopy(subFramePrimaryDomain), topFramePrimaryDomain = crossThreadCopy(topFramePrimaryDomain), frameID, pageID, promptStatus, callback = WTFMove(callback)] () mutable {

WebResourceLoadStatisticsStore has maps, we probably shouldn't take a ref and pass it to a background thread as there might be a risk that it gets deallocated in the background thread.
Ditto for the callback. We should probably use a weak ref and store the callback in WebResourceLoadStatisticsStore.
Or we should make WebResourceLoadStatisticsStore use DestructionThread::MainThread.

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:404
> +                });

Might be better to just call callOnMainThread once and capture the callback value in the lambda.

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:78
> +    static Ref<WebResourceLoadStatisticsStore> create(const String& resourceLoadStatisticsDirectory, Function<void (const String&)>&& testingCallback, bool isEphemeral, UpdatePrevalentDomainsToPartitionOrBlockCookiesHandler&& updatePrevalentDomainsToPartitionOrBlockCookiesHandler = [](const WTF::Vector<String>&, const WTF::Vector<String>&, const WTF::Vector<String>&, ShouldClearFirst) { }, HasStorageAccessForFrameHandler&& hasStorageAccessForFrameHandler = [](const String&, const String&, uint64_t, uint64_t, WTF::Function<void(bool)>&&) { }, GrantStorageAccessHandler&& grantStorageAccessHandler = [](const String&, const String&, std::optional<uint64_t>, uint64_t, WTF::Function<void(StorageAccessStatus)>&&) { }, RemoveAllStorageAccessHandler&& removeAllStorageAccessHandler = []() { }, RemovePrevalentDomainsHandler&& removeDomainsHandler = [] (const WTF::Vector<String>&) { })

This one is using both StorageAccessStatus and StorageAccessPromptStatus and is not guarded.

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h:97
> +    void requestStorageAccess(String&& subFrameHost, String&& topFrameHost, uint64_t frameID, uint64_t pageID, StorageAccessPromptStatus, WTF::CompletionHandler<void(StorageAccessStatus)>&& callback);

No need for WTF::

[Alex Christensen]

Comment on attachment 339792 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=339792&action=review

>> Source/WebKit/UIProcess/WebPageProxy.cpp:7488
>>      auto completionHandler = [this, protectedThis = makeRef(*this), webProcessContextId] (bool access) {
> 
> I wonder whether this completion handler might sometimes be released in a background thread which might not be right.

Not after r231465

> Source/WebKit/UIProcess/WebPageProxy.cpp:7492
> +    auto promptStatus = promptEnabled ? StorageAccessPromptStatus::UserNotPrompted : StorageAccessPromptStatus::UserGrantedAccess;

I think it's incorrect to say the user granted access at this point.  It looks like we need a third state for PromptingDisabled.

> Source/WebKit/UIProcess/WebPageProxy.cpp:7510
> +                    m_websiteDataStore->requestStorageAccess(WTFMove(subFrameHost), WTFMove(topFrameHost), frameID, m_pageID, StorageAccessPromptStatus::UserGrantedAccess, [this, protectedThis = makeRef(*this), completionHandler = WTFMove(completionHandler)] (StorageAccessStatus status) mutable {

iOS correctly indicates that this and protectedThis are unused here.  The completionHandler will keep this alive.

> Source/WebKit/UIProcess/WebPageProxy.cpp:7514
> +                            ASSERT_NOT_REACHED();

I still think this is an indicator that we need better abstractions in our protocol here.

[Alex Christensen]

Yeah, I don't know what the linux bots are complaining about.  Carlos?

[youenn fablet]

(In reply to Alex Christensen from comment #15)
> Yeah, I don't know what the linux bots are complaining about.  Carlos?

I believe GrantStorageAccessHandler default parameter value is incorrect in line 78 of the patch.

[John Wilander]

Thanks, both of you! I'm too tired to see these things right now and the build errors were just too vague. New patch coming up.

[John Wilander]

Created attachment 339800 [details]
Patch

[John Wilander]

(In reply to Alex Christensen from comment #14)
> > Source/WebKit/UIProcess/WebPageProxy.cpp:7492
> > +    auto promptStatus = promptEnabled ? StorageAccessPromptStatus::UserNotPrompted : StorageAccessPromptStatus::UserGrantedAccess;
> 
> I think it's incorrect to say the user granted access at this point.  It
> looks like we need a third state for PromptingDisabled.

The implication here is that in shipping we consider the user's click or tap in the requester's iframe as the grant access signal. So as long as prompts are disabled, WebPageProxy will tell WebResourceLoadStatisticsStore that "I got what you need from the user. Go ahead and grant access."

[youenn fablet]

Comment on attachment 339800 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=339800&action=review

tire

> Source/WebKit/UIProcess/WebPageProxy.cpp:7502
> +                ASSERT_NOT_REACHED();

This ASSERT_NOT_REACHED is not clear to me.
Instead of doing this if (!promptEnabled) check, we could just do ASSERT(promptEnabled).
To make things clearer, we could also rename StorageAccessStatus::DoesNotHaveAccess to StorageAccessStatus::RequiresPrompt.

> Source/WebKit/UIProcess/WebPageProxy.cpp:7507
> +            String topFrameHostCopy = topFrameHost;

Might not need these extra parameters. Instead we could pass String { } to requestStorageAccessConfirm.
Given the size of this method, I would also be tempted to handle StorageAccessStatus::DoesNotHaveAccess in its own method.

> Source/WebKit/UIProcess/WebPageProxy.cpp:7510
> +                    m_websiteDataStore->requestStorageAccess(WTFMove(subFrameHost), WTFMove(topFrameHost), frameID, m_pageID, StorageAccessPromptStatus::UserGrantedAccess, [completionHandler = WTFMove(completionHandler)] (StorageAccessStatus status) mutable {

So we are using requestStorageAccess for two things:
1. to get the access state as done above
2. to set the access state as done below

It might be clearer to add a new method for 2.
The completionHandler would then either take no parameter or an std::optional<Error> in case there might be errors.

> Source/WebKit/UIProcess/WebPageProxy.cpp:7521
> +                        RELEASE_ASSERT_NOT_REACHED();

Probably not needed.

> Source/WebKit/UIProcess/WebPageProxy.cpp:7532
> +        RELEASE_ASSERT_NOT_REACHED();

Probably not needed.

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:358
> +void WebResourceLoadStatisticsStore::requestStorageAccess(String&& subFrameHost, String&& topFrameHost, uint64_t frameID, uint64_t pageID, StorageAccessPromptStatus promptStatus, WTF::CompletionHandler<void(StorageAccessStatus)>&& callback)

No need for WTF::

[Alex Christensen]

Comment on attachment 339800 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=339800&action=review

>> Source/WebKit/UIProcess/WebPageProxy.cpp:7510
>> +                    m_websiteDataStore->requestStorageAccess(WTFMove(subFrameHost), WTFMove(topFrameHost), frameID, m_pageID, StorageAccessPromptStatus::UserGrantedAccess, [completionHandler = WTFMove(completionHandler)] (StorageAccessStatus status) mutable {
> 
> So we are using requestStorageAccess for two things:
> 1. to get the access state as done above
> 2. to set the access state as done below
> 
> It might be clearer to add a new method for 2.
> The completionHandler would then either take no parameter or an std::optional<Error> in case there might be errors.

We can make a requestStorageAccess and grantStorageAccess, and grant would be used by request in the case where it has access.

[John Wilander]

Created attachment 339836 [details]
Patch

[Alex Christensen]

Created attachment 339840 [details]
Patch

[youenn fablet]

Comment on attachment 339840 [details]
Patch

Some additional comments below.
If pressed on time, please take the time to address these as follow-ups if needed.

View in context: https://bugs.webkit.org/attachment.cgi?id=339840&action=review

> Source/WebKit/UIProcess/WebPageProxy.cpp:7488
> +    CompletionHandler<void(bool)> completionHandler = [this, protectedThis = makeRef(*this), webProcessContextId] (bool access) {

This handler is refing WebPageProxy which is unneeded AFAICS since it is captured in a lambda that is also refing this.
Also, it could be defined later on, inside the lambda to requestStorageAccess.

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:401
> +                    callback(StorageAccessStatus::CannotRequestAccess);

Could use a ternary instead.

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:435
> +    m_statisticsQueue->dispatch([this, protectedThis = makeRef(*this), subFrameHost = crossThreadCopy(subFrameHost), topFrameHost = crossThreadCopy(topFrameHost), frameID, pageID, userWasPrompted, callback = WTFMove(callback)] () mutable {

Is it fine to destroy WebResourceLoadStatisticsStore on a background thread?
Is it fine to have callback being destroyed in a background thread?

It might be safer to use a weak pointer/map-of-callbacks approach.

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:437
> +            callOnMainThread([callback = WTFMove(callback), wasGrantedAccess] () mutable {

Since this is WebKit2 code, we usually use RunLoop.

[John Wilander]

Committed r231501: <https://trac.webkit.org/changeset/231501>