Bug 211520 - ResourceLoadStatistics data summary call should create a web process pool if one doesn't exist
Summary: ResourceLoadStatistics data summary call should create a web process pool if ...
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: Kate Cheney
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-05-06 11:02 PDT by Kate Cheney
Modified: 2020-05-06 13:59 PDT (History)
3 users (show)

See Also:


Attachments
Patch (19.36 KB, patch)
2020-05-06 11:15 PDT, Kate Cheney
no flags Details | Formatted Diff | Diff
Patch (16.62 KB, patch)
2020-05-06 12:24 PDT, Kate Cheney
no flags Details | Formatted Diff | Diff
Patch (15.53 KB, patch)
2020-05-06 12:52 PDT, Kate Cheney
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kate Cheney 2020-05-06 11:02:47 PDT
This call should be available even if a web process pool doesn't already exist.
Comment 1 Kate Cheney 2020-05-06 11:03:01 PDT
<rdar://problem/59869619>
Comment 2 Kate Cheney 2020-05-06 11:15:44 PDT
Created attachment 398639 [details]
Patch
Comment 3 Chris Dumez 2020-05-06 11:42:13 PDT
Comment on attachment 398639 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStorePrivate.h:82
> +- (void)_hasProcessPool:(void (^)(BOOL))completionHandler WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));

This does not need to be asynchronous or take a completion handler in.

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

ensureProcessPools() does NOT create a persistent process pool. As soon as this loop is over, the process pool that was created should get destroyed (unless some other object is ref'ing it).

As a result, I am perplexed by your new hasProcessPool getter and testing.
Comment 4 Kate Cheney 2020-05-06 12:01:02 PDT
(In reply to Chris Dumez from comment #3)
> Comment on attachment 398639 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=398639&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStorePrivate.h:82
> > +- (void)_hasProcessPool:(void (^)(BOOL))completionHandler WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));
> 
> This does not need to be asynchronous or take a completion handler in.
> 
> > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1543
> > +    for (auto& processPool : ensureProcessPools()) {
> 
> ensureProcessPools() does NOT create a persistent process pool. As soon as
> this loop is over, the process pool that was created should get destroyed
> (unless some other object is ref'ing it).
> 
> As a result, I am perplexed by your new hasProcessPool getter and testing.

I am only using the getter to confirm that the test is setup correctly before calling the API. It should have no existing process pools to start with.
Comment 5 Chris Dumez 2020-05-06 12:09:20 PDT
(In reply to katherine_cheney from comment #4)
> (In reply to Chris Dumez from comment #3)
> > Comment on attachment 398639 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=398639&action=review
> > 
> > > Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStorePrivate.h:82
> > > +- (void)_hasProcessPool:(void (^)(BOOL))completionHandler WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));
> > 
> > This does not need to be asynchronous or take a completion handler in.
> > 
> > > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1543
> > > +    for (auto& processPool : ensureProcessPools()) {
> > 
> > ensureProcessPools() does NOT create a persistent process pool. As soon as
> > this loop is over, the process pool that was created should get destroyed
> > (unless some other object is ref'ing it).
> > 
> > As a result, I am perplexed by your new hasProcessPool getter and testing.
> 
> I am only using the getter to confirm that the test is setup correctly
> before calling the API. It should have no existing process pools to start
> with.

I see. Then I think it may be overkill to add a new SPI just to check that your API test did not create a process pool.
Comment 6 Kate Cheney 2020-05-06 12:24:27 PDT
(In reply to Chris Dumez from comment #5)
> (In reply to katherine_cheney from comment #4)
> > (In reply to Chris Dumez from comment #3)
> > > Comment on attachment 398639 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=398639&action=review
> > > 
> > > > Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStorePrivate.h:82
> > > > +- (void)_hasProcessPool:(void (^)(BOOL))completionHandler WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));
> > > 
> > > This does not need to be asynchronous or take a completion handler in.
> > > 
> > > > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1543
> > > > +    for (auto& processPool : ensureProcessPools()) {
> > > 
> > > ensureProcessPools() does NOT create a persistent process pool. As soon as
> > > this loop is over, the process pool that was created should get destroyed
> > > (unless some other object is ref'ing it).
> > > 
> > > As a result, I am perplexed by your new hasProcessPool getter and testing.
> > 
> > I am only using the getter to confirm that the test is setup correctly
> > before calling the API. It should have no existing process pools to start
> > with.
> 
> I see. Then I think it may be overkill to add a new SPI just to check that
> your API test did not create a process pool.

Got it. I was just trying to be thorough :)
Comment 7 Kate Cheney 2020-05-06 12:24:48 PDT
Created attachment 398645 [details]
Patch
Comment 8 Chris Dumez 2020-05-06 12:27:38 PDT
Comment on attachment 398645 [details]
Patch

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

Ok if bots are happy.

> Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStorePrivate.h:-82
> -

Unnecessary change.
Comment 9 Kate Cheney 2020-05-06 12:52:49 PDT
Created attachment 398647 [details]
Patch
Comment 10 Kate Cheney 2020-05-06 12:53:22 PDT
(In reply to Chris Dumez from comment #8)
> Comment on attachment 398645 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=398645&action=review
> 
> Ok if bots are happy.
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStorePrivate.h:-82
> > -
> 
> Unnecessary change.

Thanks! Waiting for bots before I cq+
Comment 11 EWS 2020-05-06 13:59:46 PDT
Committed r261250: <https://trac.webkit.org/changeset/261250>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 398647 [details].