Bug 186903

<rdar://problem/41350182>

Created attachment 343297 [details] Patch

Created attachment 343370 [details] Patch

Created attachment 343389 [details] Patch

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).

Created attachment 343520 [details] Patch

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

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.

Created attachment 343539 [details] Patch

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.

Created attachment 343550 [details] Patch

Comment on attachment 343550 [details] Patch r=me if the bots are happy

Thanks, Chris!

Mac-wk2 test flakiness is unrelated.

Comment on attachment 343550 [details] Patch Clearing flags on attachment: 343550 Committed r233180: <https://trac.webkit.org/changeset/233180>

All reviewed patches have been landed. Closing bug.

Filed bug 187035 to fix the WinCairo build error.

(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

(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.

Reopening for follow-up crash fix.

Created attachment 343614 [details] Follow-up fix

Comment on attachment 343614 [details] Follow-up fix Clearing flags on attachment: 343614 Committed r233208: <https://trac.webkit.org/changeset/233208>

All reviewed patches have been landed. Closing bug.

@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

(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.

(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.