Bug 187206

Summary: Resource Load Statistics: Make network process calls only for the process pool that the page belongs to
Product: WebKit Reporter: John Wilander <wilander>
Component: WebKit Misc.Assignee: John Wilander <wilander>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch for landing none

Description John Wilander 2018-06-29 16:54:01 PDT
In the WebKit::WebsiteDataStore infrastructure for ITP and the Storage Access API, instead of iterating over all process pools, we should resolve which process pool the page belongs to and call the network process only for that pool.
Comment 1 Radar WebKit Bug Importer 2018-06-29 16:54:42 PDT
<rdar://problem/41659160>
Comment 2 John Wilander 2018-06-29 17:00:38 PDT
Created attachment 343975 [details]
Patch
Comment 3 Chris Dumez 2018-06-29 17:06:28 PDT
Comment on attachment 343975 [details]
Patch

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

r=me with nits.

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1221
> +    if (auto webPage = WebProcessProxy::webPage(pageID)) {

auto*

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1224
> +    } else

I think I'd rather we do an early return:
auto* webPage = WebProcessProxy::webPage(pageID);
if (!webPage) {
    completionHandler(false);
    return;
}
auto& networkProcess = webPage->process().processPool().ensureNetworkProcess();
networkProcess.hasStorageAccessForFrame(m_sessionID, resourceDomain, firstPartyDomain, frameID, pageID, WTFMove(completionHandler));

Same comments apply below.

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

This is still wrong but I guess we can fix separately since you do not have a pageID here.
Comment 4 John Wilander 2018-06-29 17:13:19 PDT
(In reply to Chris Dumez from comment #3)
> Comment on attachment 343975 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=343975&action=review
> 
> r=me with nits.
> 
> > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1221
> > +    if (auto webPage = WebProcessProxy::webPage(pageID)) {
> 
> auto*

Will fix.

> > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1224
> > +    } else
> 
> I think I'd rather we do an early return:
> auto* webPage = WebProcessProxy::webPage(pageID);
> if (!webPage) {
>     completionHandler(false);
>     return;
> }
> auto& networkProcess =
> webPage->process().processPool().ensureNetworkProcess();
> networkProcess.hasStorageAccessForFrame(m_sessionID, resourceDomain,
> firstPartyDomain, frameID, pageID, WTFMove(completionHandler));

Will fix.

> Same comments apply below.

Will fix.

> > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1231
> > +        processPool->networkProcess()->getAllStorageAccessEntries(m_sessionID, WTFMove(completionHandler));
> 
> This is still wrong but I guess we can fix separately since you do not have
> a pageID here.

I'll add a FIXME comment.

Thanks, Chris!
Comment 5 John Wilander 2018-06-29 17:28:35 PDT
Created attachment 343982 [details]
Patch for landing
Comment 6 WebKit Commit Bot 2018-06-29 18:07:45 PDT
Comment on attachment 343982 [details]
Patch for landing

Clearing flags on attachment: 343982

Committed r233384: <https://trac.webkit.org/changeset/233384>
Comment 7 WebKit Commit Bot 2018-06-29 18:07:46 PDT
All reviewed patches have been landed.  Closing bug.