RESOLVED FIXED Bug 186903
Resource Load Statistics: Make WebResourceLoadStatisticsStore::updateCookiePartitioningForDomains() wait for the network process before calling its callback
https://bugs.webkit.org/show_bug.cgi?id=186903
Summary Resource Load Statistics: Make WebResourceLoadStatisticsStore::updateCookiePa...
John Wilander
Reported 2018-06-21 17:12:33 PDT
WebResourceLoadStatisticsStore::updateCookiePartitioningForDomains() should wait for the network process' signal that cookie partitioning and blocking has been updated before calling its callback.
Attachments
Patch (27.78 KB, patch)
2018-06-21 17:23 PDT, John Wilander
no flags
Patch (35.09 KB, patch)
2018-06-22 14:43 PDT, John Wilander
no flags
Patch (34.49 KB, patch)
2018-06-22 16:13 PDT, John Wilander
no flags
Patch (34.95 KB, patch)
2018-06-25 11:30 PDT, John Wilander
no flags
Patch (35.55 KB, patch)
2018-06-25 14:29 PDT, John Wilander
no flags
Patch (35.87 KB, patch)
2018-06-25 15:36 PDT, John Wilander
no flags
Follow-up fix (2.26 KB, patch)
2018-06-26 10:06 PDT, Chris Dumez
no flags
Radar WebKit Bug Importer
Comment 1 2018-06-21 17:13:02 PDT
John Wilander
Comment 2 2018-06-21 17:23:53 PDT
John Wilander
Comment 3 2018-06-22 14:43:52 PDT
John Wilander
Comment 4 2018-06-22 16:13:17 PDT
Chris Dumez
Comment 5 2018-06-25 11:05:33 PDT
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
Comment 6 2018-06-25 11:30:07 PDT
John Wilander
Comment 7 2018-06-25 11:30:32 PDT
Thanks, Chris! I've fixed all the issues.
Chris Dumez
Comment 8 2018-06-25 11:53:00 PDT
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
Comment 9 2018-06-25 14:29:42 PDT
Chris Dumez
Comment 10 2018-06-25 14:33:34 PDT
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
Comment 11 2018-06-25 15:36:14 PDT
Chris Dumez
Comment 12 2018-06-25 15:41:34 PDT
Comment on attachment 343550 [details] Patch r=me if the bots are happy
John Wilander
Comment 13 2018-06-25 15:44:18 PDT
Thanks, Chris!
John Wilander
Comment 14 2018-06-25 16:02:29 PDT
Mac-wk2 test flakiness is unrelated.
WebKit Commit Bot
Comment 15 2018-06-25 16:37:45 PDT
Comment on attachment 343550 [details] Patch Clearing flags on attachment: 343550 Committed r233180: <https://trac.webkit.org/changeset/233180>
WebKit Commit Bot
Comment 16 2018-06-25 16:37:47 PDT
All reviewed patches have been landed. Closing bug.
Fujii Hironori
Comment 17 2018-06-26 01:50:37 PDT
Filed bug 187035 to fix the WinCairo build error.
Dawei Fenton (:realdawei)
Comment 18 2018-06-26 09:36:07 PDT
(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
Comment 19 2018-06-26 10:00:10 PDT
(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
Comment 20 2018-06-26 10:04:42 PDT
Reopening for follow-up crash fix.
Chris Dumez
Comment 21 2018-06-26 10:06:18 PDT
Created attachment 343614 [details] Follow-up fix
WebKit Commit Bot
Comment 22 2018-06-26 11:26:37 PDT
Comment on attachment 343614 [details] Follow-up fix Clearing flags on attachment: 343614 Committed r233208: <https://trac.webkit.org/changeset/233208>
WebKit Commit Bot
Comment 23 2018-06-26 11:26:39 PDT
All reviewed patches have been landed. Closing bug.
Frédéric Wang (:fredw)
Comment 24 2018-06-27 04:54:05 PDT
@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
Comment 25 2018-06-27 08:47:59 PDT
(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
Comment 26 2018-06-27 08:56:27 PDT
(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.
Note You need to log in before you can comment on or make changes to this bug.