Bug 187206 - Resource Load Statistics: Make network process calls only for the process pool that the page belongs to
Summary: Resource Load Statistics: Make network process calls only for the process poo...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: John Wilander
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-06-29 16:54 PDT by John Wilander
Modified: 2018-06-29 18:07 PDT (History)
3 users (show)

See Also:


Attachments
Patch (7.71 KB, patch)
2018-06-29 17:00 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Patch for landing (8.02 KB, patch)
2018-06-29 17:28 PDT, John Wilander
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.