Resource Load Statistics: Make WebResourceLoadStatisticsStore::updateCookiePartitioningForDomains() wait for the network process before calling its callback

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[John Wilander]

WebResourceLoadStatisticsStore::updateCookiePartitioningForDomains() should wait for the network process' signal that cookie partitioning and blocking has been updated before calling its callback.

[Radar WebKit Bug Importer]

<rdar://problem/41350182>

[John Wilander]

Created attachment 343297 [details]
Patch

[John Wilander]

Created attachment 343370 [details]
Patch

[John Wilander]

Created attachment 343389 [details]
Patch

[Chris Dumez]

Comment on attachment 343389 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:406
> +void NetworkProcess::updatePrevalentDomainsToPartitionOrBlockCookies(PAL::SessionID sessionID, const Vector<String>& domainsToPartition, const Vector<String>& domainsToBlock, const Vector<String>& domainsToNeitherPartitionNorBlock, bool shouldClearFirst, uint64_t contextId)

We usually call these callbackID, not contextId.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:410
> +    parentProcessConnection()->send(Messages::NetworkProcessProxy::UpdatePartitionOrBlockCookiesDone(contextId), 0);

We would normally call this DidUpdatePartitionOrBlockCookies

> Source/WebKit/NetworkProcess/NetworkProcess.h:137
> +    void updatePrevalentDomainsToPartitionOrBlockCookies(PAL::SessionID, const Vector<String>& domainsToPartition, const Vector<String>& domainsToBlock, const Vector<String>& domainsToNeitherPartitionNorBlock, bool shouldClearFirst, uint64_t contextId);

ditto.

> Source/WebKit/NetworkProcess/NetworkProcess.messages.in:84
> +    UpdatePrevalentDomainsToPartitionOrBlockCookies(PAL::SessionID sessionID, Vector<String> domainsToPartition, Vector<String> domainsToBlock, Vector<String> domainsToNeitherPartitionNorBlock, bool shouldClearFirst, uint64_t contextId)

We usually call these callbackID, not contextId.

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:434
> +static uint64_t nextUpdatePartitionOrBlockCookiesContextId()

Not needed, please rely on the existing generateCallbackID() in this file.

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:447
> +    auto contextId = nextUpdatePartitionOrBlockCookiesContextId();

Please use generateCallbackID() and call this variable callbackID everywhere.

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:453
> +void NetworkProcessProxy::updatePartitionOrBlockCookiesDone(uint64_t contextId)

callbackID

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:455
> +    auto callback = m_updatePartitionOrBlockCookiesCallbackMap.take(contextId);

In NetworkProcessProxy::clearCallbackStates(), please make sure you call your completion handlers (This is called when the network process crashes during a request).

[John Wilander]

Created attachment 343520 [details]
Patch

[John Wilander]

Thanks, Chris! I've fixed all the issues.

[Chris Dumez]

Comment on attachment 343520 [details]
Patch

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

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:1305
> +        m_updatePrevalentDomainsToPartitionOrBlockCookiesHandler(domainsToPartition, domainsToBlock, domainsToNeitherPartitionNorBlock, ShouldClearFirst::No, WTFMove(completionHandler));

We want completionHandler to be called on the background thread here since WebResourceLoadStatisticsStore::updateCookiePartitioning() gets called on the background thread. Therefore, I believe this should be:
m_updatePrevalentDomainsToPartitionOrBlockCookiesHandler(domainsToPartition, domainsToBlock, domainsToNeitherPartitionNorBlock, ShouldClearFirst::No, [completionHandler = WTFMove(completionHandler] {
    m_statisticsQueue->dispatch([completionHandler = WTFMove(completionHandler)]() mutable {
             completionHandler();
     });
});

> Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp:1337
> +        m_updatePrevalentDomainsToPartitionOrBlockCookiesHandler(domainsToPartition, domainsToBlock, domainsToNeitherPartitionNorBlock, shouldClearFirst, WTFMove(completionHandler));

Same issue here, WebResourceLoadStatisticsStore::updateCookiePartitioningForDomains() gets called on the background thread, so completionHandler needs to be called on the background thread.

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1211
> +        processPool->networkProcess()->updatePrevalentDomainsToPartitionOrBlockCookies(m_sessionID, domainsToPartition, domainsToBlock, domainsToNeitherPartitionNorBlock, shouldClearFirst, WTFMove(completionHandler));

processPool->networkProcess() can return null.

[John Wilander]

Created attachment 343539 [details]
Patch

[Chris Dumez]

Comment on attachment 343539 [details]
Patch

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

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1213
> +            process->updatePrevalentDomainsToPartitionOrBlockCookies(m_sessionID, domainsToPartition, domainsToBlock, domainsToNeitherPartitionNorBlock, shouldClearFirst, WTFMove(completionHandler));

This is not correct and will crash if there is more than one process pool because you keep WTFMove() ing the same completionHandler. What you want is to use a WTF::CallbackAggregator to wait for all network processes to complete their requests before calling completionHandler.

[John Wilander]

Created attachment 343550 [details]
Patch

[Chris Dumez]

Comment on attachment 343550 [details]
Patch

r=me if the bots are happy

[John Wilander]

Thanks, Chris!

[John Wilander]

Mac-wk2 test flakiness is unrelated.

[WebKit Commit Bot]

Comment on attachment 343550 [details]
Patch

Clearing flags on attachment: 343550

Committed r233180: <https://trac.webkit.org/changeset/233180>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Fujii Hironori]

Filed bug 187035 to fix the WinCairo build error.

[Dawei Fenton (:realdawei)]

(In reply to WebKit Commit Bot from comment #15)
> Comment on attachment 343550 [details]
> Patch
> 
> Clearing flags on attachment: 343550
> 
> Committed r233180: <https://trac.webkit.org/changeset/233180>

Looks like this revision has caused an API test regression:

Sample output:
https://build.webkit.org/builders/Apple%20High%20Sierra%20Debug%20WK2%20%28Tests%29/builds/3827/steps/run-api-tests/logs/stdio

Crashed

    TestWebKitAPI.ResourceLoadStatistics.GrandfatherCallback
        ASSERTION FAILED: Completion handler should always be called
        !m_function
        /Volumes/Data/slave/highsierra-debug/build/WebKitBuild/Debug/usr/local/include/wtf/CompletionHandler.h(51) : WTF::CompletionHandler<void ()>::~CompletionHandler()
        1   0x10d1dee89 WTFCrash

[Chris Dumez]

(In reply to David Fenton (realdawei) from comment #18)
> (In reply to WebKit Commit Bot from comment #15)
> > Comment on attachment 343550 [details]
> > Patch
> > 
> > Clearing flags on attachment: 343550
> > 
> > Committed r233180: <https://trac.webkit.org/changeset/233180>
> 
> Looks like this revision has caused an API test regression:
> 
> Sample output:
> https://build.webkit.org/builders/
> Apple%20High%20Sierra%20Debug%20WK2%20%28Tests%29/builds/3827/steps/run-api-
> tests/logs/stdio
> 
> Crashed
> 
>     TestWebKitAPI.ResourceLoadStatistics.GrandfatherCallback
>         ASSERTION FAILED: Completion handler should always be called
>         !m_function
>        
> /Volumes/Data/slave/highsierra-debug/build/WebKitBuild/Debug/usr/local/
> include/wtf/CompletionHandler.h(51) : WTF::CompletionHandler<void
> ()>::~CompletionHandler()
>         1   0x10d1dee89 WTFCrash

Will fix.

[Chris Dumez]

Reopening for follow-up crash fix.

[Chris Dumez]

Created attachment 343614 [details]
Follow-up fix

[WebKit Commit Bot]

Comment on attachment 343614 [details]
Follow-up fix

Clearing flags on attachment: 343614

Committed r233208: <https://trac.webkit.org/changeset/233208>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Frédéric Wang (:fredw)]

@Chris: The ASSERTION failure is still happening on GTK. See https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Debug%20%28Tests%29/builds/3277

[Chris Dumez]

(In reply to Frédéric Wang (:fredw) from comment #24)
> @Chris: The ASSERTION failure is still happening on GTK. See
> https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Debug%20%28Tests%29/
> builds/3277

My bet is that this one is GTK specific, John's patch probably only takes care of the #if HAVE(CFNETWORK_STORAGE_PARTITIONING) code path.

[Chris Dumez]

(In reply to Chris Dumez from comment #25)
> (In reply to Frédéric Wang (:fredw) from comment #24)
> > @Chris: The ASSERTION failure is still happening on GTK. See
> > https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Debug%20%28Tests%29/
> > builds/3277
> 
> My bet is that this one is GTK specific, John's patch probably only takes
> care of the #if HAVE(CFNETWORK_STORAGE_PARTITIONING) code path.

Taking care of it via https://bugs.webkit.org/show_bug.cgi?id=187099.