WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(35.09 KB, patch)
2018-06-22 14:43 PDT
,
John Wilander
no flags
Details
Formatted Diff
Diff
Patch
(34.49 KB, patch)
2018-06-22 16:13 PDT
,
John Wilander
no flags
Details
Formatted Diff
Diff
Patch
(34.95 KB, patch)
2018-06-25 11:30 PDT
,
John Wilander
no flags
Details
Formatted Diff
Diff
Patch
(35.55 KB, patch)
2018-06-25 14:29 PDT
,
John Wilander
no flags
Details
Formatted Diff
Diff
Patch
(35.87 KB, patch)
2018-06-25 15:36 PDT
,
John Wilander
no flags
Details
Formatted Diff
Diff
Follow-up fix
(2.26 KB, patch)
2018-06-26 10:06 PDT
,
Chris Dumez
no flags
Details
Formatted Diff
Diff
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2018-06-21 17:13:02 PDT
<
rdar://problem/41350182
>
John Wilander
Comment 2
2018-06-21 17:23:53 PDT
Created
attachment 343297
[details]
Patch
John Wilander
Comment 3
2018-06-22 14:43:52 PDT
Created
attachment 343370
[details]
Patch
John Wilander
Comment 4
2018-06-22 16:13:17 PDT
Created
attachment 343389
[details]
Patch
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
Created
attachment 343520
[details]
Patch
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
Created
attachment 343539
[details]
Patch
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
Created
attachment 343550
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug