Bug 183669 – Resource Load Statistics: Clean up network process calls in WebsiteDataStore

• Status: RESOLVED
• Resolution: CONFIGURATION CHANGED
• Component: WebKit Misc.

[John Wilander]

The recently added WebKit::WebsiteDataStore::removeAllStorageAccessHandler() doesn't have a callback so it can leverage the existing sendToNetworkingProcess() method. The functions that do have callbacks should do a null check before calling functions on WebKit::NetworkProcessProxy.

[Radar WebKit Bug Importer]

<rdar://problem/38508429>

[John Wilander]

Created attachment 335867 [details]
Patch

[Alex Christensen]

Comment on attachment 335867 [details]
Patch

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

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1201
> +    for (auto& processPool : processPools()) {

This looks like it only works with one processPool.  The callback is moved into the first one, so if there is another one it will move a completion handler in an undefined state, probably nullptr.  We should instead make a Vector<Ref<WebProcessPool>> and iterate it recursively, moving it into the next lambda each time until it's empty, then calling the completion handler in the last one.  If there's no networkProcess, recursively continue or call the completion handler.  This will ensure the completion handler is called once always and only once.

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1211
> +            networkProcess->getAllStorageAccessEntries(m_sessionID, WTFMove(callback));

ditto

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1219
> +            networkProcess->grantStorageAccess(m_sessionID, resourceDomain, firstPartyDomain, frameID, pageID, WTFMove(callback));

ditto

[John Wilander]

(In reply to Alex Christensen from comment #3)
> Comment on attachment 335867 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=335867&action=review
> 
> > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1201
> > +    for (auto& processPool : processPools()) {
> 
> This looks like it only works with one processPool.  The callback is moved
> into the first one, so if there is another one it will move a completion
> handler in an undefined state, probably nullptr.  We should instead make a
> Vector<Ref<WebProcessPool>> and iterate it recursively, moving it into the
> next lambda each time until it's empty, then calling the completion handler
> in the last one.  If there's no networkProcess, recursively continue or call
> the completion handler.  This will ensure the completion handler is called
> once always and only once.
> 
> > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1211
> > +            networkProcess->getAllStorageAccessEntries(m_sessionID, WTFMove(callback));
> 
> ditto
> 
> > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1219
> > +            networkProcess->grantStorageAccess(m_sessionID, resourceDomain, firstPartyDomain, frameID, pageID, WTFMove(callback));
> 
> ditto

True. I believe this was originally called just once. I'll have a look.